"Denis V. Lunev" <d...@openvz.org> writes:
> According to PCI specification subsystem id and subsystem vendor id are
> optinal and could be abscent in Type1 header and can be found on
> different offsets within Type0 and Type2 headers.
Well, they *are* absent in Type1 headers. Perhaps:
According to PCI specification, subsystem id and subsystem vendor id
are present only in type 0 and type 2 headers (at different offsets),
but not in type 1 headers.
> Thus we should make this data optional in struct PciDeviceId and skip
> reporting them via HMP if the information is not available.
>
> Additional (wrong information) about PCI bridges (Type1 devices) has been
> added in 5383a705 and fortunately not released. This patch fixes that
> problem. The problem was spotted by Markus.
A machine-readable way to phrase the last sentence is
Reported-by: Markus Armbruster <arm...@redhat.com>
> 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 | 6 ++++--
> hw/pci/pci.c | 13 ++++++++++---
> qapi/misc.json | 4 ++--
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 3a9f797677..55633d29a3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -824,8 +824,10 @@ 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->id->has_subsystem_vendor && dev->id->has_subsystem) {
> + 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 51d0dec466..b937f0dc0a 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1737,9 +1737,6 @@ 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 : "");
>
> @@ -1752,6 +1749,16 @@ static PciDeviceInfo *qmp_query_pci_device(PCIDevice
> *dev, PCIBus *bus,
> if (type == PCI_HEADER_TYPE_BRIDGE) {
> info->has_pci_bridge = true;
> info->pci_bridge = qmp_query_pci_bridge(dev, bus, bus_num);
> + } else if (type == PCI_HEADER_TYPE_NORMAL) {
> + info->id->has_subsystem = info->id->has_subsystem_vendor = true;
> + info->id->subsystem = pci_get_word(dev->config + PCI_SUBSYSTEM_ID);
> + info->id->subsystem_vendor =
> + pci_get_word(dev->config + PCI_SUBSYSTEM_VENDOR_ID);
> + } else if (type == PCI_HEADER_TYPE_CARDBUS) {
> + info->id->has_subsystem = info->id->has_subsystem_vendor = true;
> + info->id->subsystem = pci_get_word(dev->config +
> PCI_CB_SUBSYSTEM_ID);
> + info->id->subsystem_vendor =
> + pci_get_word(dev->config + PCI_CB_SUBSYSTEM_VENDOR_ID);
> }
>
> return info;
> diff --git a/qapi/misc.json b/qapi/misc.json
> index ada9af5add..95a6ed022d 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -839,8 +839,8 @@
> # Since: 2.4
> ##
> { 'struct': 'PciDeviceId',
> - 'data': {'device': 'int', 'vendor': 'int', 'subsystem': 'int',
> - 'subsystem-vendor': 'int'} }
> + 'data': {'device': 'int', 'vendor': 'int', '*subsystem': 'int',
> + '*subsystem-vendor': 'int'} }
'uint16' would match PCI, but I since we didn't for @device and @vendor,
we probably shouldn't for @subsystem and @subsystem-vendor.
>
> ##
> # @PciDeviceInfo:
With Eric's spelling corrections or my rephrasing of the commit message:
Reviewed-by: Markus Armbruster <arm...@redhat.com>
David, would you like to pick this up?
