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

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3110 bytes --]

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 <stefanha@redhat.com>
>>>> ---
>>>>  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?

 Thanks,
  Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]