Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: add PCI subsystem id and vendor id to PCI info

[Denis V. Lunev]

On 09/28/2018 07:23 PM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (address@hidden) wrote:
>> This is now commit 5383a705207.  Sorry for being late with my comments.
>>
>> "Denis V. Lunev" <address@hidden> writes:
>>
>>> This is a long story. RedHat has relicensed Windows KVM device drivers
>>> in 2018 and there was an agreement that to avoid WHQL driver conflict
>>> software manufacturers should set proper PCI subsystem vendor ID in
>>> their distributions. Thus PCI subsystem vendor id becomes actively used.
>>>
>>> The problem is that this field is applied by us via hardware compats.
>>> Thus technically it could be lost.
>>>
>>> This patch adds PCI susbsystem id and vendor id to exportable parameters
>>> for validation.
>>>
>>> Signed-off-by: Denis V. Lunev <address@hidden>
>>> CC: "Dr. David Alan Gilbert" <address@hidden>
>>> CC: Eric Blake <address@hidden>
>>> CC: Markus Armbruster <address@hidden>
>>> ---
>>>  hmp.c            | 2 ++
>>>  hw/pci/pci.c     | 3 +++
>>>  qapi-schema.json | 7 ++++++-
>>>  3 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hmp.c b/hmp.c
>>> index 2874bcd789..8fb0957cfd 100644
>>> --- a/hmp.c
>>> +++ b/hmp.c
>>> @@ -803,6 +803,8 @@ static void hmp_info_pci_device(Monitor *mon, const 
>>> PciDeviceInfo *dev)
>>>  
>>>      monitor_printf(mon, ": PCI device %04" PRIx64 ":%04" PRIx64 "\n",
>>>                     dev->id->vendor, dev->id->device);
>>> +    monitor_printf(mon, "      PCI subsystem %04" PRIx64 ":%04" PRIx64 
>>> "\n",
>>> +                   dev->id->subsystem_vendor, dev->id->subsystem);
>>>  
>>>      if (dev->has_irq) {
>>>          monitor_printf(mon, "      IRQ %" PRId64 ".\n", dev->irq);
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index f0c98cd0ae..be70dd425c 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -1724,6 +1724,9 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice 
>>> *dev, PCIBus *bus,
>>>      info->id = g_new0(PciDeviceId, 1);
>>>      info->id->vendor = pci_get_word(dev->config + PCI_VENDOR_ID);
>>>      info->id->device = pci_get_word(dev->config + PCI_DEVICE_ID);
>>> +    info->id->subsystem = pci_get_word(dev->config + PCI_SUBSYSTEM_ID);
>>> +    info->id->subsystem_vendor =
>>> +        pci_get_word(dev->config + PCI_SUBSYSTEM_VENDOR_ID);
>>>      info->regions = qmp_query_pci_regions(dev);
>>>      info->qdev_id = g_strdup(dev->qdev.id ? dev->qdev.id : "");
>>>  
>>> diff --git a/qapi/misc.json b/qapi/misc.json
>>> index dfef6faf81..1704a8d437 100644
>>> --- a/qapi/misc.json
>>> +++ b/qapi/misc.json
>>> @@ -2162,10 +2162,15 @@
>>>  #
>>>  # @vendor: the PCI vendor id
>>>  #
>>> +# @subsystem: the PCI subsystem id (since 3.1)
>>> +#
>>> +# @subsystem-vendor: the PCI subsystem vendor id (since 3.1)
>>> +#
>>>  # Since: 2.4
>>>  ##
>>>  { 'struct': 'PciDeviceId',
>>> -  'data': {'device': 'int', 'vendor': 'int'} }
>>> +  'data': {'device': 'int', 'vendor': 'int', 'subsystem': 'int',
>>> +            'subsystem-vendor': 'int'} }
>> Uh...
>>
>> Device ID and Vendor ID are in "Type 0/1 Common Configuration Space",
>> i.e. any PCI Device has them.
>>
>> Subsystem ID and Subsystem Vendor ID are in "Type 0 Configuration Space
>> Header".  Devices using a "Type 1 Configuration Space Header"
>> (PCI-to-PCI bridges) have something else there: "Prefetchable Limit
>> Upper 32 Bits".
> Ewww - thanks for spotting it.
interesting

>> In short, you add these IDs to PciDeviceId for all PCI devices, even
>> though some PCI devices don't have them.
>>
>> I suspect qmp_query_pci_device() will happily interpret the
>> "Prefetchable Limit Upper 32 Bits" as Subsystem ID and Subsystem Vendor
>> ID.
>>
>> Quotes are from "PCI Express Base Specification Revision 3.0".
>>
>> To spice things up, the Type 2 Configuration Space Header
>> (PCI-to-CardBus bridges) does have Subsystem ID and Subsystem Vendor ID,
>> but at different offsets.
> So it looks like we need to:
>   a) Modify the json definition to make those two fields optional
>   b) Modify the hmp code that prints it to only do it when they're there
>   c) Modify the pci code to look up the class and then find what it's
> really got.
Yes, really. I have to make some my home work.

Thank you,
    Den


> Dave
>
>>>  
>>>  ##
>>>  # @PciDeviceInfo:
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK