OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


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


On Tue, 12 Jan 2021 11:27:20 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/1/12 äå2:16, Cornelia Huck wrote:
> > 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.)  
> 
> 
> Then we need some clarifications in the spec. It would be easily imply 
> that reset is part of device status after reading "Basic Facilities of a 
> Virtio Device".

I can try to come up with a patch.

> 
> 
> >  
> >>  
> >>> 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."  
> 
> 
> Yes, that's kind of conflict but it was how PCI works now ...

Nod.

> 
> 
> > - "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.)  
> 
> 
> Not a native speaker but we probably need to define "set" and "clear" 
> mean, E.g:
> 
> Clear a bit from driver means write with that bit cleared. Set a bit 
> from driver means write with that bit set.

Agreed.

> Clear a bit from device means read with that be cleared. Set a bit from 
> device means read with that bit set.

I always saw the status as an actual part of the device. I.e., the
device setting or clearing a bit means that the device is modifying the
status in the device. The driver doing a read will get the status as it
currently is within the device. IOW, the device "controls" the status,
and the driver can submit changes to the status.

[Maybe that's closer to how ccw operates with its commands for reading
or writing the status. Are pci/mmio envisioning the status more like
some kind of shared area?]

> 
> So a question here is how to trigger the device set as we discussed before.
> 
> 
> >
> > Also, treating CCW_CMD_WRITE_STATUS with 0 as a reset request would be
> > incompatible with older devices, wouldn't it?  
> 
> 
> Probably, but we can make both methods work. Note that I'm not 
> suggesting doing something like this. Just to point out it may work. And 
> there's something not clear in the spec.

I'm still not convinced that's the way to go. But first, we should be
clear on how the status really is supposed to work.



[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]