[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]