* Markus Armbruster (arm...@redhat.com) wrote:
> "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.
Queued with that text.
> > 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?
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
