On 09/28/2018 07:23 PM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (arm...@redhat.com) wrote:
>> This is now commit 5383a705207. Sorry for being late with my comments.
>>
>> "Denis V. Lunev" <d...@openvz.org> 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 <d...@openvz.org>
>>> CC: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
>>> CC: Eric Blake <ebl...@redhat.com>
>>> CC: Markus Armbruster <arm...@redhat.com>
>>> ---
>>> 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 / dgilb...@redhat.com / Manchester, UK
