Re: [Qemu-devel] [PATCH v2 03/34] tests/qgraph: pci-pc driver and interface nodes

[Laurent Vivier]

On 09/08/2018 14:17, Emanuele wrote:
> 
>>> +    return TRUE;
>>> +}
>>> +
>>> +QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
>>> +{
>>> +    QPCIDevice *dev;
>>> +
>>> +    dev = g_malloc0(sizeof(*dev));
>>> +
>>> +    if (!qpci_device_set(dev, bus, devfn)) {
>>>           g_free(dev);
>>>           return NULL;
>>>       }
>>> @@ -66,6 +83,21 @@ QPCIDevice *qpci_device_find(QPCIBus *bus, int devfn)
>>>       return dev;
>>>   }
>>>   +void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress
>>> *addr)
>>> +{
>>> +    uint16_t vendor_id, device_id;
>>> +
>>> +    if (!qpci_device_set(dev, bus, addr->devfn)) {
>>> +        printf("PCI Device not found\n");
>>> +        abort();
>>> +    }
>> so, here, you should use qpci_device_set() and qpci_device_available()...
> I changed qpci_device_set to just set the fields, while
> qpci_device_available simply is doing
> {
>     return qpci_config_readw(...) != 0xFFFF
> }
> Now both qpci_device_init and qpci_device_find call qpci_device_set, and
> check that  qpci_device_available is not false, otherwise it will
> trigger g_assert_not_reached().
>>
>>> +
>>> +    vendor_id = qpci_config_readw(dev, PCI_VENDOR_ID);
>> or you can only check vendor_id to see it is 0xffff or not...
>>
>>> +    device_id = qpci_config_readw(dev, PCI_DEVICE_ID);
>>> +    g_assert(vendor_id == addr->vendor_id);
>> that should be in fact detected by this g_assert().
> since this case is covered by qpci_device_available, I don't think
> there's the need to insert the assertion.

What I mean is you don't need the qpci_device_available().

The 0xffff case is detected by "g_assert(vendor_id == addr->vendor_id)".

The functions could be:

static void qpci_device_set(QPCIDevice *dev, QPCIBus *bus, int devfn)
{
     dev->bus = bus;
     dev->devfn = devfn;
}

void qpci_device_init(QPCIDevice *dev, QPCIBus *bus, QPCIAddress *addr)
{
    uint16_t vendor_id, device_id;

    qpci_device_set(dev, bus, addr->devfn);

    vendor_id = qpci_config_readw(dev, PCI_VENDOR_ID);
    device_id = qpci_config_readw(dev, PCI_DEVICE_ID);
    g_assert(vendor_id == addr->vendor_id);
    g_assert(device_id == addr->device_id);
}

Thanks,
Laurent