Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional

[Dr. David Alan Gilbert]

* Markus Armbruster (address@hidden) wrote:
> "Denis V. Lunev" <address@hidden> 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 <address@hidden>
> 
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > CC: "Dr. David Alan Gilbert" <address@hidden>
> > CC: Eric Blake <address@hidden>
> > CC: Markus Armbruster <address@hidden>
> > ---
> >  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 <address@hidden>
> 
> David, would you like to pick this up?

Yes, I can do.

Dave

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK