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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.