diff mbox series

[PATCH-for-9.1?] hw/pci/pci-hmp-cmds: Avoid displaying bogus size in 'info pci'

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

Commit Message

Philippe Mathieu-Daudé Aug. 1, 2024, 10:07 a.m. UTC
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(-)

Comments

Michael S. Tsirkin Aug. 1, 2024, 10:27 a.m. UTC | #1
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
Philippe Mathieu-Daudé Aug. 1, 2024, 12:36 p.m. UTC | #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/?
Philippe Mathieu-Daudé Aug. 1, 2024, 12:38 p.m. UTC | #3
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.
Michael S. Tsirkin Aug. 1, 2024, 12:41 p.m. UTC | #4
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 mbox series

Patch

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" : "");
+            }
         }
     }