Re: [virtio-comment] comment on pci section

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

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)


It's not clean to setup VQs after point 2 (VQs need to be setup to
consume buffers), and we can't do this before point 1 (# of vqs depends
on features).

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

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

> Cheers,
> Rusty.