On Tue, 12 Jan 2021 11:27:20 +0800
Jason Wang <> wrote:
> On 2021/1/12 äå2:16, Cornelia Huck wrote:
> > On Wed, 30 Dec 2020 16:15:47 +0800
> > Jason Wang <> wrote:
> >
> >> On 2020/12/29 äå9:35, Halil Pasic wrote:
> >>> On Mon, 28 Dec 2020 15:01:57 +0800
> >>> Jason Wang <> 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.
