Re: [virtio-comment] [PATCH RFC] virtio: introduce VIRTIO_F_DEVICE_STOP

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 30 Dec 2020 16:15:47 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2020/12/29 äå9:35, Halil Pasic wrote:
> > On Mon, 28 Dec 2020 15:01:57 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >  
> >> Some part of the virtio has enforced an asynchronous interface during reset:
> >>
> >> For MMIO the spec said:
> >>
> >> """
> >>
> >> To stop using the queue the driver MUST write zero (0x0) to this
> >> QueueReady and MUST read the value back to ensure synchronization.
> >>
> >> """  
> > I read the MMIO quote like a single read is sufficient to ensure
> > synchronization. I.e. it does not require a loop which waits for
> > the read to yield the expected value.
> >  
> >> For PCI it said:
> >>
> >> """
> >>
> >> After writing 0 to device_status, the driver MUST wait for a read of
> >> device_status to return 0 before reinitializing the device.
> >>
> >> """  
> > Yes, this does sound like a loop. And that's what Linux does. But this
> > is transport (PCI) specific. On a spec level, a reset is a distinct
> > operation from setting device status (to 0).  
> 
> 
> I'm not sure or it looks unclear to me for this point.
> 
> E.g the device reset is mentioned in "Device Status Field" belongs to 
> "Basic Facilities of a Virtio Device". But there's no "Device Reset" in 
> "Basic Facilities of a Virtio Device".

I think it should be, just to make clear what a driver-initiated reset
of a device actually resets (and that the method for doing so is
transport-specific.)

> 
> 
> > It just happens to be
> > mapped to the PCI transport as setting the status to 0.  
> 
> 
> MMIO did the same. And it makes sense since using a single API to 
> configure/change device status looks simpler.
> 
> 
> >
> > For the channel IO transport it is mapped via CCW_CMD_VDEV_RESET while
> > setting the status is mapped via CCW_CMD_WRITE_STATUS.  
> 
> 
> Yes, but I think actually there's no limitation if we want to tread 0 as 
> a reset command for CCW_CMD_WRITE_STATUS.

I'm not a fan of the "driver writes 0 to status to initiate a device
reset" method, but we are stuck with it. I think it's actually not
working well with two other requirements in the spec:

- "The driver MUST NOT clear a device status bit."
- "The device MUST initialize device status to 0 upon reset." (This
  suggests to me that a zero status is something set by the device as
  the result of a reset request by the driver, and _not_ set by the
  driver.)

Also, treating CCW_CMD_WRITE_STATUS with 0 as a reset request would be
incompatible with older devices, wouldn't it?