| Message ID | 20240801100742.50312-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH-for-9.1?] hw/pci/pci-hmp-cmds: Avoid displaying bogus size in 'info pci' | expand |
On Thu, Aug 01, 2024 at 12:07:42PM +0200, Philippe Mathieu-Daudé wrote: > When BAR aren't configured, we get: > > (qemu) info pci > Bus 0, device 0, function 0: > Host bridge: PCI device dead:beef > ... > BAR4: 32 bit memory at 0xffffffffffffffff [0x00000ffe]. > BAR5: I/O at 0xffffffffffffffff [0x0ffe]. > > Improve logging to not display bogus sizes: > > BAR4: 32 bit memory (not configured) > BAR5: I/O (not configured) > > Remove trailing dot which is not used in other commands format. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/pci/pci-hmp-cmds.c | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > diff --git a/hw/pci/pci-hmp-cmds.c b/hw/pci/pci-hmp-cmds.c > index b09fce9377..8421c3f74a 100644 > --- a/hw/pci/pci-hmp-cmds.c > +++ b/hw/pci/pci-hmp-cmds.c > @@ -83,15 +83,25 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev) > monitor_printf(mon, " BAR%" PRId64 ": ", region->value->bar); > > if (!strcmp(region->value->type, "io")) { > - monitor_printf(mon, "I/O at 0x%04" PRIx64 > - " [0x%04" PRIx64 "].\n", > - addr, addr + size - 1); > + if (addr != UINT64_MAX) { > + monitor_printf(mon, "I/O at 0x%04" PRIx64 > + " [0x%04" PRIx64 "]\n", > + addr, addr + size - 1); > + } else { > + monitor_printf(mon, "I/O (not configured)\n"); > + } > } else { > - monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 > - " [0x%08" PRIx64 "].\n", > - region->value->mem_type_64 ? 64 : 32, > - region->value->prefetch ? " prefetchable" : "", > - addr, addr + size - 1); > + if (addr != UINT64_MAX) { > + monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 > + " [0x%08" PRIx64 "]\n", > + region->value->mem_type_64 ? 64 : 32, > + region->value->prefetch ? " prefetchable" : "", > + addr, addr + size - 1); > + } else { > + monitor_printf(mon, "%d bit%s memory (not configured)\n", > + region->value->mem_type_64 ? 64 : 32, > + region->value->prefetch ? " prefetchable" : ""); > + } > } > } what makes bar unconfigured is that memory space is disabled, not that it has a special value. > > -- > 2.45.2
On 1/8/24 12:27, Michael S. Tsirkin wrote: > On Thu, Aug 01, 2024 at 12:07:42PM +0200, Philippe Mathieu-Daudé wrote: >> When BAR aren't configured, we get: >> >> (qemu) info pci >> Bus 0, device 0, function 0: >> Host bridge: PCI device dead:beef >> ... >> BAR4: 32 bit memory at 0xffffffffffffffff [0x00000ffe]. >> BAR5: I/O at 0xffffffffffffffff [0x0ffe]. >> >> Improve logging to not display bogus sizes: >> >> BAR4: 32 bit memory (not configured) >> BAR5: I/O (not configured) >> >> Remove trailing dot which is not used in other commands format. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/pci/pci-hmp-cmds.c | 26 ++++++++++++++++++-------- >> 1 file changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/hw/pci/pci-hmp-cmds.c b/hw/pci/pci-hmp-cmds.c >> index b09fce9377..8421c3f74a 100644 >> --- a/hw/pci/pci-hmp-cmds.c >> +++ b/hw/pci/pci-hmp-cmds.c >> @@ -83,15 +83,25 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev) >> monitor_printf(mon, " BAR%" PRId64 ": ", region->value->bar); >> >> if (!strcmp(region->value->type, "io")) { >> - monitor_printf(mon, "I/O at 0x%04" PRIx64 >> - " [0x%04" PRIx64 "].\n", >> - addr, addr + size - 1); >> + if (addr != UINT64_MAX) { >> + monitor_printf(mon, "I/O at 0x%04" PRIx64 >> + " [0x%04" PRIx64 "]\n", >> + addr, addr + size - 1); >> + } else { >> + monitor_printf(mon, "I/O (not configured)\n"); >> + } >> } else { >> - monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 >> - " [0x%08" PRIx64 "].\n", >> - region->value->mem_type_64 ? 64 : 32, >> - region->value->prefetch ? " prefetchable" : "", >> - addr, addr + size - 1); >> + if (addr != UINT64_MAX) { >> + monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 >> + " [0x%08" PRIx64 "]\n", >> + region->value->mem_type_64 ? 64 : 32, >> + region->value->prefetch ? " prefetchable" : "", >> + addr, addr + size - 1); >> + } else { >> + monitor_printf(mon, "%d bit%s memory (not configured)\n", >> + region->value->mem_type_64 ? 64 : 32, >> + region->value->prefetch ? " prefetchable" : ""); >> + } >> } >> } > > what makes bar unconfigured is that memory space is disabled, > not that it has a special value. I tried to add a PciMemoryRegion::enabled field then realized unmapped regions are advertised using addr = PCI_BAR_UNMAPPED (which is UINT64_MAX): typedef struct PCIIORegion { pcibus_t addr; /* current PCI mapping address. -1 means not mapped */ #define PCI_BAR_UNMAPPED (~(pcibus_t)0) OK if I respin this patch with s/UINT64_MAX/PCI_BAR_UNMAPPED/?
On 1/8/24 14:36, Philippe Mathieu-Daudé wrote: > On 1/8/24 12:27, Michael S. Tsirkin wrote: >> On Thu, Aug 01, 2024 at 12:07:42PM +0200, Philippe Mathieu-Daudé wrote: >>> When BAR aren't configured, we get: >>> >>> (qemu) info pci >>> Bus 0, device 0, function 0: >>> Host bridge: PCI device dead:beef >>> ... >>> BAR4: 32 bit memory at 0xffffffffffffffff [0x00000ffe]. >>> BAR5: I/O at 0xffffffffffffffff [0x0ffe]. >>> >>> Improve logging to not display bogus sizes: >>> >>> BAR4: 32 bit memory (not configured) >>> BAR5: I/O (not configured) >>> >>> Remove trailing dot which is not used in other commands format. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/pci/pci-hmp-cmds.c | 26 ++++++++++++++++++-------- >>> 1 file changed, 18 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/pci/pci-hmp-cmds.c b/hw/pci/pci-hmp-cmds.c >>> index b09fce9377..8421c3f74a 100644 >>> --- a/hw/pci/pci-hmp-cmds.c >>> +++ b/hw/pci/pci-hmp-cmds.c >>> @@ -83,15 +83,25 @@ static void hmp_info_pci_device(Monitor *mon, >>> const PciDeviceInfo *dev) >>> monitor_printf(mon, " BAR%" PRId64 ": ", >>> region->value->bar); >>> if (!strcmp(region->value->type, "io")) { >>> - monitor_printf(mon, "I/O at 0x%04" PRIx64 >>> - " [0x%04" PRIx64 "].\n", >>> - addr, addr + size - 1); >>> + if (addr != UINT64_MAX) { >>> + monitor_printf(mon, "I/O at 0x%04" PRIx64 >>> + " [0x%04" PRIx64 "]\n", >>> + addr, addr + size - 1); >>> + } else { >>> + monitor_printf(mon, "I/O (not configured)\n"); >>> + } >>> } else { >>> - monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 >>> - " [0x%08" PRIx64 "].\n", >>> - region->value->mem_type_64 ? 64 : 32, >>> - region->value->prefetch ? " prefetchable" >>> : "", >>> - addr, addr + size - 1); >>> + if (addr != UINT64_MAX) { >>> + monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 >>> + " [0x%08" PRIx64 "]\n", >>> + region->value->mem_type_64 ? 64 : 32, >>> + region->value->prefetch ? " >>> prefetchable" : "", >>> + addr, addr + size - 1); >>> + } else { >>> + monitor_printf(mon, "%d bit%s memory (not >>> configured)\n", >>> + region->value->mem_type_64 ? 64 : 32, >>> + region->value->prefetch ? " >>> prefetchable" : ""); >>> + } >>> } >>> } >> >> what makes bar unconfigured is that memory space is disabled, >> not that it has a special value. > > I tried to add a PciMemoryRegion::enabled field then realized > unmapped regions are advertised using addr = PCI_BAR_UNMAPPED > (which is UINT64_MAX): > > typedef struct PCIIORegion { > pcibus_t addr; /* current PCI mapping address. -1 means not mapped */ > #define PCI_BAR_UNMAPPED (~(pcibus_t)0) > > OK if I respin this patch with s/UINT64_MAX/PCI_BAR_UNMAPPED/? and s/configured/mapped/ in printf.
On Thu, Aug 01, 2024 at 02:36:11PM +0200, Philippe Mathieu-Daudé wrote: > On 1/8/24 12:27, Michael S. Tsirkin wrote: > > On Thu, Aug 01, 2024 at 12:07:42PM +0200, Philippe Mathieu-Daudé wrote: > > > When BAR aren't configured, we get: > > > > > > (qemu) info pci > > > Bus 0, device 0, function 0: > > > Host bridge: PCI device dead:beef > > > ... > > > BAR4: 32 bit memory at 0xffffffffffffffff [0x00000ffe]. > > > BAR5: I/O at 0xffffffffffffffff [0x0ffe]. > > > > > > Improve logging to not display bogus sizes: > > > > > > BAR4: 32 bit memory (not configured) > > > BAR5: I/O (not configured) > > > > > > Remove trailing dot which is not used in other commands format. > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > --- > > > hw/pci/pci-hmp-cmds.c | 26 ++++++++++++++++++-------- > > > 1 file changed, 18 insertions(+), 8 deletions(-) > > > > > > diff --git a/hw/pci/pci-hmp-cmds.c b/hw/pci/pci-hmp-cmds.c > > > index b09fce9377..8421c3f74a 100644 > > > --- a/hw/pci/pci-hmp-cmds.c > > > +++ b/hw/pci/pci-hmp-cmds.c > > > @@ -83,15 +83,25 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev) > > > monitor_printf(mon, " BAR%" PRId64 ": ", region->value->bar); > > > if (!strcmp(region->value->type, "io")) { > > > - monitor_printf(mon, "I/O at 0x%04" PRIx64 > > > - " [0x%04" PRIx64 "].\n", > > > - addr, addr + size - 1); > > > + if (addr != UINT64_MAX) { > > > + monitor_printf(mon, "I/O at 0x%04" PRIx64 > > > + " [0x%04" PRIx64 "]\n", > > > + addr, addr + size - 1); > > > + } else { > > > + monitor_printf(mon, "I/O (not configured)\n"); > > > + } > > > } else { > > > - monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 > > > - " [0x%08" PRIx64 "].\n", > > > - region->value->mem_type_64 ? 64 : 32, > > > - region->value->prefetch ? " prefetchable" : "", > > > - addr, addr + size - 1); > > > + if (addr != UINT64_MAX) { > > > + monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 > > > + " [0x%08" PRIx64 "]\n", > > > + region->value->mem_type_64 ? 64 : 32, > > > + region->value->prefetch ? " prefetchable" : "", > > > + addr, addr + size - 1); > > > + } else { > > > + monitor_printf(mon, "%d bit%s memory (not configured)\n", > > > + region->value->mem_type_64 ? 64 : 32, > > > + region->value->prefetch ? " prefetchable" : ""); > > > + } > > > } > > > } > > > > what makes bar unconfigured is that memory space is disabled, > > not that it has a special value. > > I tried to add a PciMemoryRegion::enabled field then realized > unmapped regions are advertised using addr = PCI_BAR_UNMAPPED > (which is UINT64_MAX): > > typedef struct PCIIORegion { > pcibus_t addr; /* current PCI mapping address. -1 means not mapped */ > #define PCI_BAR_UNMAPPED (~(pcibus_t)0) > > OK if I respin this patch with s/UINT64_MAX/PCI_BAR_UNMAPPED/? ok
diff --git a/hw/pci/pci-hmp-cmds.c b/hw/pci/pci-hmp-cmds.c index b09fce9377..8421c3f74a 100644 --- a/hw/pci/pci-hmp-cmds.c +++ b/hw/pci/pci-hmp-cmds.c @@ -83,15 +83,25 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev) monitor_printf(mon, " BAR%" PRId64 ": ", region->value->bar); if (!strcmp(region->value->type, "io")) { - monitor_printf(mon, "I/O at 0x%04" PRIx64 - " [0x%04" PRIx64 "].\n", - addr, addr + size - 1); + if (addr != UINT64_MAX) { + monitor_printf(mon, "I/O at 0x%04" PRIx64 + " [0x%04" PRIx64 "]\n", + addr, addr + size - 1); + } else { + monitor_printf(mon, "I/O (not configured)\n"); + } } else { - monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 - " [0x%08" PRIx64 "].\n", - region->value->mem_type_64 ? 64 : 32, - region->value->prefetch ? " prefetchable" : "", - addr, addr + size - 1); + if (addr != UINT64_MAX) { + monitor_printf(mon, "%d bit%s memory at 0x%08" PRIx64 + " [0x%08" PRIx64 "]\n", + region->value->mem_type_64 ? 64 : 32, + region->value->prefetch ? " prefetchable" : "", + addr, addr + size - 1); + } else { + monitor_printf(mon, "%d bit%s memory (not configured)\n", + region->value->mem_type_64 ? 64 : 32, + region->value->prefetch ? " prefetchable" : ""); + } } }
When BAR aren't configured, we get: (qemu) info pci Bus 0, device 0, function 0: Host bridge: PCI device dead:beef ... BAR4: 32 bit memory at 0xffffffffffffffff [0x00000ffe]. BAR5: I/O at 0xffffffffffffffff [0x0ffe]. Improve logging to not display bogus sizes: BAR4: 32 bit memory (not configured) BAR5: I/O (not configured) Remove trailing dot which is not used in other commands format. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/pci/pci-hmp-cmds.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)