* [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional
@ 2018-10-02 13:55 Denis V. Lunev
2018-10-02 14:27 ` Eric Blake
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Denis V. Lunev @ 2018-10-02 13:55 UTC (permalink / raw)
To: den; +Cc: qemu-devel, Dr. David Alan Gilbert, Eric Blake, Markus Armbruster
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.
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.
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 | 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'} }
##
# @PciDeviceInfo:
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional
2018-10-02 13:55 [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional Denis V. Lunev
@ 2018-10-02 14:27 ` Eric Blake
2018-10-02 16:03 ` Dr. David Alan Gilbert
2018-10-09 12:55 ` Markus Armbruster
2 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2018-10-02 14:27 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, Dr. David Alan Gilbert, Markus Armbruster
On 10/2/18 8:55 AM, Denis V. Lunev wrote:
> According to PCI specification subsystem id and subsystem vendor id are
> optinal and could be abscent in Type1 header and can be found on
s/optinal/optional/
s/abscent/absent/
> different offsets within Type0 and Type2 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.
>
> 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 | 6 ++++--
> hw/pci/pci.c | 13 ++++++++++---
> qapi/misc.json | 4 ++--
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional
2018-10-02 13:55 [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional Denis V. Lunev
2018-10-02 14:27 ` Eric Blake
@ 2018-10-02 16:03 ` Dr. David Alan Gilbert
2018-10-09 12:55 ` Markus Armbruster
2 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-02 16:03 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, Eric Blake, Markus Armbruster
* Denis V. Lunev (den@openvz.org) wrote:
> 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.
>
> 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.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
Looks good to me, thanks
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@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'} }
>
> ##
> # @PciDeviceInfo:
> --
> 2.17.1
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional
2018-10-02 13:55 [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional Denis V. Lunev
2018-10-02 14:27 ` Eric Blake
2018-10-02 16:03 ` Dr. David Alan Gilbert
@ 2018-10-09 12:55 ` Markus Armbruster
2018-10-11 11:04 ` Dr. David Alan Gilbert
2018-10-11 18:03 ` Dr. David Alan Gilbert
2 siblings, 2 replies; 6+ messages in thread
From: Markus Armbruster @ 2018-10-09 12:55 UTC (permalink / raw)
To: Denis V. Lunev; +Cc: qemu-devel, Dr. David Alan Gilbert
"Denis V. Lunev" <den@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 <armbru@redhat.com>
> 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 | 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 <armbru@redhat.com>
David, would you like to pick this up?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional
2018-10-09 12:55 ` Markus Armbruster
@ 2018-10-11 11:04 ` Dr. David Alan Gilbert
2018-10-11 18:03 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 11:04 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Denis V. Lunev, qemu-devel
* Markus Armbruster (armbru@redhat.com) wrote:
> "Denis V. Lunev" <den@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 <armbru@redhat.com>
>
> > 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 | 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 <armbru@redhat.com>
>
> David, would you like to pick this up?
Yes, I can do.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional
2018-10-09 12:55 ` Markus Armbruster
2018-10-11 11:04 ` Dr. David Alan Gilbert
@ 2018-10-11 18:03 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 18:03 UTC (permalink / raw)
To: Markus Armbruster; +Cc: Denis V. Lunev, qemu-devel
* Markus Armbruster (armbru@redhat.com) wrote:
> "Denis V. Lunev" <den@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 <armbru@redhat.com>
>
> > 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 | 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 <armbru@redhat.com>
>
> David, would you like to pick this up?
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-11 18:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-02 13:55 [Qemu-devel] [PATCH 1/1] qmp, hmp: make subsystem/system-vendor identities optional Denis V. Lunev
2018-10-02 14:27 ` Eric Blake
2018-10-02 16:03 ` Dr. David Alan Gilbert
2018-10-09 12:55 ` Markus Armbruster
2018-10-11 11:04 ` Dr. David Alan Gilbert
2018-10-11 18:03 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.