Re: [virtio-comment] comment on pci section

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jan 31, 2014 at 01:10:30PM +1030, Rusty Russell wrote:
> "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.


I'd like to explain.
What we are trying to implement is PCI bus master bit.
Rules:
- It must be set before DRIVER_OK.
- If it set/cleared any number of time before DRIVER_OK, device will be
  livea after DRIVER_OK
- If it is cleared when driver sets DRIVER_OK, or if
  it is cleared after driver sets DRIVER_OK, device stops
  being live, you must reset to make it live again


> >> >> > 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.

This is not what was intended.
I see what gives you this impression:

After this field is written by driver, the first length bytes in
pci_cfg_data are written at the selected offset
in the selected BAR.
When this field is read by driver, length bytes at the selected offset
in the selected BAR are read into
pci_cfg_data.

"this" should be replaced by pci_cfg_data.