[1/1] qmp, hmp: add PCI subsystem id and vendor id to PCI info
diff mbox series

Message ID 20180918095852.28422-1-den@openvz.org
State New
Headers show
Series
  • [1/1] qmp, hmp: add PCI subsystem id and vendor id to PCI info
Related show

Commit Message

Denis V. Lunev Sept. 18, 2018, 9:58 a.m. UTC
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 <den@openvz.org>
CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
---
 hmp.c            | 2 ++
 hw/pci/pci.c     | 3 +++
 qapi-schema.json | 7 ++++++-
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Dr. David Alan Gilbert Sept. 18, 2018, 10:33 a.m. UTC | #1
* Denis V. Lunev (den@openvz.org) wrote:
> 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 <den@openvz.org>
> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>


I'll take it via HMP.

Dave

> ---
>  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'} }
>  
>  ##
>  # @PciDeviceInfo:
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Eric Blake Sept. 18, 2018, 3:55 p.m. UTC | #2
On 9/18/18 4:58 AM, Denis V. Lunev wrote:
> This is a long story. RedHat has relicensed Windows KVM device drivers

s/RedHat/Red Hat/

> 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 <den@openvz.org>
> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>
Dr. David Alan Gilbert Sept. 25, 2018, 10:47 a.m. UTC | #3
* Eric Blake (eblake@redhat.com) wrote:
> On 9/18/18 4:58 AM, Denis V. Lunev wrote:
> > This is a long story. RedHat has relicensed Windows KVM device drivers
> 
> s/RedHat/Red Hat/

Space added during queuing.

Dave

> > 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 <den@openvz.org>
> > CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > ---
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Markus Armbruster Sept. 25, 2018, 4:58 p.m. UTC | #4
This is now commit 5383a705207.  Sorry for being late with my comments.

"Denis V. Lunev" <den@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 <den@openvz.org>
> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@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".

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.

>  
>  ##
>  # @PciDeviceInfo:
Dr. David Alan Gilbert Sept. 28, 2018, 4:23 p.m. UTC | #5
* Markus Armbruster (armbru@redhat.com) wrote:
> This is now commit 5383a705207.  Sorry for being late with my comments.
> 
> "Denis V. Lunev" <den@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 <den@openvz.org>
> > CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: Markus Armbruster <armbru@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.

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

Dave

> >  
> >  ##
> >  # @PciDeviceInfo:
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Denis V. Lunev Sept. 28, 2018, 4:34 p.m. UTC | #6
On 09/28/2018 07:23 PM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> This is now commit 5383a705207.  Sorry for being late with my comments.
>>
>> "Denis V. Lunev" <den@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 <den@openvz.org>
>>> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> CC: Markus Armbruster <armbru@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 / dgilbert@redhat.com / Manchester, UK
Denis V. Lunev Sept. 28, 2018, 4:55 p.m. UTC | #7
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 (armbru@redhat.com) wrote:
>>> This is now commit 5383a705207.  Sorry for being late with my comments.
>>>
>>> "Denis V. Lunev" <den@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 <den@openvz.org>
>>>> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>>>> CC: Eric Blake <eblake@redhat.com>
>>>> CC: Markus Armbruster <armbru@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

Patch
diff mbox series

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'} }
 
 ##
 # @PciDeviceInfo: