[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 Wed, 30 Dec 2020 16:15:47 +0800 Jason Wang <firstname.lastname@example.org> wrote: > On 2020/12/29 äå9:35, Halil Pasic wrote: > > On Mon, 28 Dec 2020 15:01:57 +0800 > > Jason Wang <email@example.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?