From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gAYm9-0002G9-C4 for qemu-devel@nongnu.org; Thu, 11 Oct 2018 07:05:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gAYm5-0001vD-BQ for qemu-devel@nongnu.org; Thu, 11 Oct 2018 07:05:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49014) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1gAYm4-0001us-Vq for qemu-devel@nongnu.org; Thu, 11 Oct 2018 07:05:01 -0400 Date: Thu, 11 Oct 2018 12:04:55 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20181011110454.GG2483@work-vm> References: <20181002135538.12113-1-den@openvz.org> <875zybxorn.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <875zybxorn.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Denis V. Lunev" , qemu-devel@nongnu.org * Markus Armbruster (armbru@redhat.com) wrote: > "Denis V. Lunev" 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 > > > Signed-off-by: Denis V. Lunev > > CC: "Dr. David Alan Gilbert" > > CC: Eric Blake > > CC: Markus Armbruster > > --- > > 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 > > David, would you like to pick this up? Yes, I can do. Dave -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK