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