virtio-comment

 View Only
  • 1.  device reset should entail QueuePFN reset (for all queues) on virtio-mmio

    Posted 10-24-2013 13:04
    Cross-posting from the Linux Virtualization list [1] with minor edits:


    "Appendix X: virtio-mmio" in the virtio spec says

    * 0x040 | RW | QueuePFN
    [...] When the Guest stops using the queue it must write zero
    (0x0) to this register.
    [...]

    and

    Virtqueue Configuration

    [...]
    2. Check if the queue is not already in use: read QueuePFN
    register, returned value should be zero (0x0).
    [...]

    I think this is suboptimal per se, because a guest that crashes and
    reboots (while the emulator survives) will not be able to use the device
    after said reboot (it has never re-set QueuePFN to zero).

    But, more importantly: I think that resetting the device (by writing 0
    to its status register) should include (ie. *guarantee*) the effects of
    setting QueuePFN to zero for all imaginable queues of the device.

    This way, a defensive guest that starts up by resetting the device (*)
    after identifying it via MagicValue / Version / DeviceID / VendorID
    would be able to use the device regardless of the device's prior
    QueuePFN setting(s).

    (*) Resetting the device is the first step in "2.2.1 Device
    Initialization Sequence". It "is not required on initial start up", but
    as a guest driver can never be sure whether the startup in question is
    the initial one, a defensive driver will always start with device reet.


    The question arises because Olivier Martin posted a series to edk2-devel
    [2] that adds virtio-mmio support to TianoCore, and Mark Salter tested
    it [3] on an AArch64 foundation model with a Linux guest, and found
    problems. Namely, the UEFI firmware can drive the virtio devices via
    virtio-mmio, but the Linux kernel booted from it can not. The reason is
    the missing zeroing of QueuePFN across ExitBootServices(). (I'm just
    paraphrasing the analysis.)

    I think
    - that resetting the device (via its status register) should make the
    host forget *all* prior configuration, including the QueuePFNs,
    - and that the Linux driver should reset the device as first step.

    So:
    - What's the motivation for the "acquire/release" semantics of QueuePFN?
    - Am I right that device reset should force a QueuePFN reset?

    Thanks!
    Laszlo

    [1] http://thread.gmane.org/gmane.linux.kernel.virtualization/21022
    [2] http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4557
    [3] http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4373/focus=4411



  • 2.  Re: [virtio-comment] device reset should entail QueuePFN reset (for all queues) on virtio-mmio

    Posted 10-25-2013 09:09
    Il 24/10/2013 14:03, Laszlo Ersek ha scritto:
    > I think
    > - that resetting the device (via its status register) should make the
    > host forget *all* prior configuration, including the QueuePFNs,
    > - and that the Linux driver should reset the device as first step.

    I agree.

    > So:
    > - What's the motivation for the "acquire/release" semantics of QueuePFN?

    I don't know, but...

    > - Am I right that device reset should force a QueuePFN reset?

    ... I agree about this.

    What QEMU currently does, is that value zero actually resets the device,
    including _all the queues_ and setting the status to 0. Is your bug
    reporter using another virtio implementation?

    I'm not sure this is the right thing to do, but the device reset should
    definitely include a full vq reset, which in turn includes zeroing QueuePFN.

    Paolo



  • 3.  Re: [virtio-comment] device reset should entail QueuePFN reset (for all queues) on virtio-mmio

    Posted 10-25-2013 12:53
    On 10/25/13 11:08, Paolo Bonzini wrote:
    > What QEMU currently does, is that value zero actually resets the
    > device, including _all the queues_ and setting the status to 0. Is
    > your bug reporter using another virtio implementation?

    Yes, the "AArch64 foundation model":

    http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4373/focus=4411

    I find that name a bit confusing (when I had heard it first I thought it
    was real hardware), but in reality it is an emulator:

    http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4217/focus=4257
    http://www.arm.com/products/tools/models/fast-models/foundation-model.php

    We did consider the host side as culprit:

    http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4373/focus=4435

    But then Mark pointed out that the code doesn't do what the virtio spec
    seems to mandate (that is, zero out the QeueuePFNs explicitly,
    regardless of whether the device is wholly reset or not).

    > I'm not sure this is the right thing to do, but the device reset
    > should definitely include a full vq reset, which in turn includes
    > zeroing QueuePFN.

    Indeed!

    Thanks!
    Laszlo



  • 4.  Re: [virtio-comment] device reset should entail QueuePFN reset (for all queues) on virtio-mmio

    Posted 10-29-2013 05:30
    Laszlo Ersek <lersek@redhat.com> writes:
    > Cross-posting from the Linux Virtualization list [1] with minor edits:
    >
    >
    > "Appendix X: virtio-mmio" in the virtio spec says
    >
    > * 0x040 | RW | QueuePFN
    > [...] When the Guest stops using the queue it must write zero
    > (0x0) to this register.
    > [...]
    >
    > and
    >
    > Virtqueue Configuration
    >
    > [...]
    > 2. Check if the queue is not already in use: read QueuePFN
    > register, returned value should be zero (0x0).
    > [...]
    >
    > I think this is suboptimal per se, because a guest that crashes and
    > reboots (while the emulator survives) will not be able to use the device
    > after said reboot (it has never re-set QueuePFN to zero).
    >
    > But, more importantly: I think that resetting the device (by writing 0
    > to its status register) should include (ie. *guarantee*) the effects of
    > setting QueuePFN to zero for all imaginable queues of the device.
    >
    > This way, a defensive guest that starts up by resetting the device (*)
    > after identifying it via MagicValue / Version / DeviceID / VendorID
    > would be able to use the device regardless of the device's prior
    > QueuePFN setting(s).
    >
    > (*) Resetting the device is the first step in "2.2.1 Device
    > Initialization Sequence". It "is not required on initial start up", but
    > as a guest driver can never be sure whether the startup in question is
    > the initial one, a defensive driver will always start with device reet.
    >
    >
    > The question arises because Olivier Martin posted a series to edk2-devel
    > [2] that adds virtio-mmio support to TianoCore, and Mark Salter tested
    > it [3] on an AArch64 foundation model with a Linux guest, and found
    > problems. Namely, the UEFI firmware can drive the virtio devices via
    > virtio-mmio, but the Linux kernel booted from it can not. The reason is
    > the missing zeroing of QueuePFN across ExitBootServices(). (I'm just
    > paraphrasing the analysis.)
    >
    > I think
    > - that resetting the device (via its status register) should make the
    > host forget *all* prior configuration, including the QueuePFNs,
    > - and that the Linux driver should reset the device as first step.
    >
    > So:
    > - What's the motivation for the "acquire/release" semantics of QueuePFN?
    > - Am I right that device reset should force a QueuePFN reset?

    Hi Laszlo,

    The latching behaviour is basically a debugging helper, but
    clearly device reset should set it to 0.

    For the v2 layout proposed for the spec, we have this:

    +* 0x03c | RW | QueueReady
    + Virtual queue ready bit.
    + Writing one (0x1) to this register notifies the Host that the
    + virtual queue is ready to be used. Reading from this register
    + returns the last value written to it. Both read and write
    + accesses apply to the queue selected by writing to QueueSel.
    + When the Guest wants to stop using the queue it must write
    + zero (0x0) to this register and read the value back to
    + ensure synchronisation.

    I suggest we add:

    This register will be 0 on reset.

    Cheers,
    Rusty.




  • 5.  Re: [virtio-comment] device reset should entail QueuePFN reset (for all queues) on virtio-mmio

    Posted 10-29-2013 10:42
    On 10/29/13 06:29, Rusty Russell wrote:
    > Laszlo Ersek <lersek@redhat.com> writes:

    >> So:
    >> - What's the motivation for the "acquire/release" semantics of QueuePFN?
    >> - Am I right that device reset should force a QueuePFN reset?
    >
    > Hi Laszlo,
    >
    > The latching behaviour is basically a debugging helper, but
    > clearly device reset should set it to 0.
    >
    > For the v2 layout proposed for the spec, we have this:

    Is there a public git repo for v2?

    >
    > +* 0x03c | RW | QueueReady
    > + Virtual queue ready bit.
    > + Writing one (0x1) to this register notifies the Host that the
    > + virtual queue is ready to be used. Reading from this register
    > + returns the last value written to it. Both read and write
    > + accesses apply to the queue selected by writing to QueueSel.
    > + When the Guest wants to stop using the queue it must write
    > + zero (0x0) to this register and read the value back to
    > + ensure synchronisation.
    >
    > I suggest we add:
    >
    > This register will be 0 on reset.

    ... "for all valid QueueSel values"?

    Thanks!
    Laszlo



  • 6.  Re: [virtio-comment] device reset should entail QueuePFN reset (for all queues) on virtio-mmio

    Posted 10-29-2013 12:52
    Laszlo Ersek <lersek@redhat.com> writes:
    > On 10/29/13 06:29, Rusty Russell wrote:
    >> Laszlo Ersek <lersek@redhat.com> writes:
    >
    >>> So:
    >>> - What's the motivation for the "acquire/release" semantics of QueuePFN?
    >>> - Am I right that device reset should force a QueuePFN reset?
    >>
    >> Hi Laszlo,
    >>
    >> The latching behaviour is basically a debugging helper, but
    >> clearly device reset should set it to 0.
    >>
    >> For the v2 layout proposed for the spec, we have this:
    >
    > Is there a public git repo for v2?

    The official one is the SVN repo:

    https://tools.oasis-open.org/version-control/browse/wsvn/virtio/?sc=0

    Works fine with git svn.

    >> I suggest we add:
    >>
    >> This register will be 0 on reset.
    >
    > ... "for all valid QueueSel values"?

    Yes, let's be explicit.

    Cheers,
    Rusty.