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:34 PM, Denis V. Lunev wrote:
> 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.

and, adding my $.02 here, technically we can have subsystem id and
subsystem vendor id on cardbus device (type 2) with different offsets.

Thus I should add a couple more clauses here.

Den