Re: [PATCH v2 7/7] libqos: add VIRTIO PCI 1.0 support

[Stefan Hajnoczi]

On Fri, Oct 18, 2019 at 08:48:23AM +0200, Thomas Huth wrote:
> On 17/10/2019 18.18, Thomas Huth wrote:
> > On 17/10/2019 18.07, Stefan Hajnoczi wrote:
> >> On Thu, Oct 17, 2019 at 04:52:54PM +0200, Thomas Huth wrote:
> >>> On 11/10/2019 10.56, Stefan Hajnoczi wrote:
> >>>> Implement the VIRTIO 1.0 virtio-pci interface.  The main change here is
> >>>> that the register layout is no longer a fixed layout in BAR 0.  Instead
> >>>> we have to iterate of PCI Capabilities to find descriptions of where
> >>>> various registers are located.  The vring registers are also more
> >>>> fine-grained, allowing for more flexible vring layouts, but we don't
> >>>> take advantage of that.
> >>>>
> >>>> Note that test cases do not negotiate VIRTIO_F_VERSION_1 yet and are
> >>>> therefore not running in VIRTIO 1.0 mode.
> >>>>
> >>>> Signed-off-by: Stefan Hajnoczi <address@hidden>
> >>>> ---
> >>>>  tests/Makefile.include           |   1 +
> >>>>  tests/libqos/virtio-pci-modern.h |  17 ++
> >>>>  tests/libqos/virtio-pci.h        |  10 +
> >>>>  tests/libqos/virtio-pci-modern.c | 412 +++++++++++++++++++++++++++++++
> >>>>  tests/libqos/virtio-pci.c        |   6 +-
> >>>>  5 files changed, 445 insertions(+), 1 deletion(-)
> >>>>  create mode 100644 tests/libqos/virtio-pci-modern.h
> >>>>  create mode 100644 tests/libqos/virtio-pci-modern.c
> >>> [...]
> >>>> +static bool probe_device_type(QVirtioPCIDevice *dev)
> >>>> +{
> >>>> +    uint16_t vendor_id;
> >>>> +    uint16_t device_id;
> >>>> +
> >>>> +    /* "Drivers MUST match devices with the PCI Vendor ID 0x1AF4" */
> >>>> +    vendor_id = qpci_config_readw(dev->pdev, PCI_VENDOR_ID);
> >>>> +    if (vendor_id != 0x1af4) {
> >>>> +        return false;
> >>>> +    }
> >>>> +
> >>>> +    /*
> >>>> +     * "Any PCI device with ... PCI Device ID 0x1000 through 0x107F 
> >>>> inclusive
> >>>> +     * is a virtio device"
> >>>> +     */
> >>>> +    device_id = qpci_config_readw(dev->pdev, PCI_DEVICE_ID);
> >>>> +    if (device_id < 0x1000 || device_id > 0x107f) {
> >>>> +        return false;
> >>>> +    }
> >>>> +
> >>>> +    /*
> >>>> +     * "Devices MAY utilize a Transitional PCI Device ID range, 0x1000 
> >>>> to
> >>>> +     * 0x103F depending on the device type"
> >>>> +     */
> >>>> +    if (device_id < 0x1040) {
> >>>> +        /*
> >>>> +         * "Transitional devices MUST have the PCI Subsystem Device ID 
> >>>> matching
> >>>> +         * the Virtio Device ID"
> >>>> +         */
> >>>> +        dev->vdev.device_type = qpci_config_readw(dev->pdev, 
> >>>> PCI_SUBSYSTEM_ID);
> >>>
> >>> Shouldn't you return "false" here in case the device_type is 0 ? Which
> >>> likely means that it is a legacy or broken device ...?
> >>
> >> The real decision whether to use this PCI device or not happens in
> >> probe_device_layout().  If it's broken or a legacy device then that
> >> function will fail.
> > 
> > Ok, fair.
> > 
> > I've added the patches to my qtest-next branch:
> > 
> > https://gitlab.com/huth/qemu/tree/qtest-next
> 
>  Hi Stephan,
> 
> looks like this is breaking the virtio-blk-test in certain configurations:
> 
>  https://gitlab.com/huth/qemu/-/jobs/324085741
> 
> and:
> 
>  https://cirrus-ci.com/task/4511314474434560
> 
> Could you please have a look?

On reading the VIRTIO specification again, I think my idea of supporting
the VIRTIO 1.0 PCI interface without actually negotiating the
VIRTIO_F_VERSION_1 feature bit is non-compliant:

  2.2.3 Legacy Interface: A Note on Feature Bits

  Transitional Drivers MUST detect Legacy Devices by detecting that the feature 
bit VIRTIO_F_VERSION_1 is not offered. [...]

  In this case device is used through the legacy interface.

Please drop this patch series for now.  Additional patches are required
to implement VIRTIO_F_VERSION_1 and then the endianness issue will go
away.  I will send a v2.

Stefan

signature.asc
Description: PGP signature