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 (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.
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
