Re: [virtio-comment] comment on pci section

[Rusty Russell <rusty@au1.ibm.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Jan 30, 2014 at 10:11:14AM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > On Wed, Jan 29, 2014 at 04:38:58PM +1030, Rusty Russell wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >> > Two comments received from a co-worker:
>> >> >
>> >> > 3.1 "The device MUST NOT consume buffers before DRIVER_OK ..." - I
>> >> > personally
>> >> > would like to see also something like:  "device must not generate
>> >> > interrupts if
>> >> > DRIVER_OK is not set” to better handle power management transitions.
>> >> 
>> >> That addition seems to make sense, but I don't understand the reference
>> >> to power management transitions?
>> >
>> > Well - ATM we do things like suspend by device reset and re-initialization,
>> > right? I'm guessing during e.g. resume it's important not to get
>> > interrupts too early. I'll check.
>> >
>> > Hmm actually we have a chapter about initialization but not cleanup.
>> > Might be a good addition.
>> 
>> Maybe, but it would only need to say to reset the device and you can
>> remove buffers safely.
>> 
>> Looking at the current Linux block driver, it drives the device before
>> setting DRIVER_OK (add_disk will do this to scan partitions).  Should we
>> make the rule that we can driver the device after FEATURES_OK instead?
>> In which case, perhaps we should just combine DRIVER_OK and FEATURES_OK
>> into one bit.
>
> Hmm we discussed this already.
> There are two distinct points:
>
> 1. driver has finalized device features (FEATURES_OK)
>
> after this point we start configuring the device:
> VQ setup etc
>
> 2. driver adds buffers, device consumes them (DRIVER_OK)

Yes, I think I agree.  On a similar note, we should make it clear that
device-specific configuration fields *can* be read before FEATURES_OK.
ie. if a device has a configuration field 'foo' which is only present if
FEATURE_FOO, and it offers FEATURE_FOO, the driver can read 'foo'
before deciding whether to accept FEATURE_FOO.

>> > Also, I note that QEMU implemented master abort bit by
>> > clearing DRIVER_OK - AFAIK useful to stop device in a
>> > standard way if driver malfunctions.
>> >
>> > Also with QEMU you can clear DRIVER_OK and this stops the device. I
>> > wonder if someone uses this.
>> 
>> The host should not change status bits, they should use a different
>> field.  This isn't explicitly stated in the spec, but it's a bit weird.
>
> Well - device already clears FEATURES_OK on configuration failure.
> It seems reasonable to clear DRIVER_OK to internal device failure, no?

The device refuses to accept the FEATURES_OK write, which is subtly
different from clearing the bit IMHO.

The status bits are for the driver to advertise its status, not the
device.

>> >> > 4.1.3.1.1.
>> >> > Must clarify that all devices MUST support all cfg_type values 1-5.
>> >> > This way BIOS can limit type 5 exclusively and drivers don't
>> >> > need to add support for type 5.
>> >> 
>> >> The word "support" here is unclear to me.  Are you saying that a device
>> >> MUST offer a VIRTIO_PCI_CAP_PCI_CFG value?  Something like:
>> >> 
>> >>         Drivers MUST offer at least one capability of each type.
>> >
>> > Exactly. Except
>> 
>> ?
>
> Indeed.
> Except I'm a bit worried that we'll add some optional
> capability in the future and this will become wrong.
> One way to address this is to add a comment in latex source
> near the capability list:
>
> %% WARNING: if optional capabilities are added,
> %% remember to fix text below.

Very good point.  How about we annotate each one instead...

Hmm, actually, I started hacking on this, and now have a significant
rework of this section (which I think makes more sense).  Will post in a
moment.

BTW, the struct virtio_pci_cfg_cap seems weird: why do you trigger on
offset read/write, rather than triggering on the actual pci_cfg_data?
Or am I misreading the spec?

Thanks,
Rusty.