[Qemu-devel] Re: [PATCH 25/25] pci/monitor: print out bridge's filtering values and so on.

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Oct 03, 2009 at 05:16:17AM +0900, Isaku Yamahata wrote:
> make pci_info_device() print out bridge's filtering value like
> io base/limit, subbus and subordinate bus.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> ---
>  hw/pci.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 40d7090..76953fe 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1064,7 +1064,50 @@ static void pci_info_device(PCIBus *bus, PCIDevice *d)
>                         d->config[PCI_INTERRUPT_LINE]);
>      }
>      if (class == 0x0604) {
> +        int shift;
> +        uint64_t base;
> +        uint64_t limit;

just use unsigned long long and you won't need PRIx64: just %llx.

>          monitor_printf(mon, "      BUS %d.\n", d->config[0x19]);
> +        monitor_printf(mon, "      SECONDARY BUS %d.\n",
> +                       d->config[PCI_SECONDARY_BUS]);
> +        monitor_printf(mon, "      SUBORDINATE BUS %d.\n",
> +                       d->config[PCI_SUBORDINATE_BUS]);
> +

Haven't yes looked at surrounding code: is it all shouting in upper case
like this?

> +        if (d->config[PCI_IO_BASE] & PCI_IO_RANGE_TYPE_32) {
> +            shift = 16;
> +        } else {
> +            shift = 8;
> +        }
> +        base = ((uint32_t)d->config[PCI_IO_BASE] & ~PCI_IO_RANGE_TYPE_MASK)
> +            << shift;
> +        base |= pci_get_word(d->config + PCI_IO_BASE_UPPER16) << 16;
> +        limit = ((uint32_t)d->config[PCI_IO_LIMIT] & ~PCI_IO_RANGE_TYPE_MASK)
> +            << shift;
> +        limit |= pci_get_word(d->config + PCI_IO_LIMIT_UPPER16) << 16;

Can this just use the pci_get_quad functions?

> +        limit |= 0xfff;

what's this?

> +        monitor_printf(mon, "      IO range [0x%04"PRIx64", 0x%04"PRIx64"]\n",
> +                       base, limit);
> +
> +        shift = 16;
> +        base = (pci_get_word(d->config + PCI_MEMORY_BASE) &
> +                PCI_MEMORY_RANGE_MASK) << shift;
> +        limit = (pci_get_word(d->config + PCI_MEMORY_LIMIT) &
> +                 PCI_MEMORY_RANGE_MASK) << shift;
> +        limit |= 0xfffff;
> +        monitor_printf(mon,
> +                       "      MEM range [0x%08"PRIx64", 0x%08"PRIx64"]\n",
> +                       base, limit);
> +        shift = 16;
> +        base = ((uint64_t)pci_get_word(d->config + PCI_PREF_MEMORY_BASE) &
> +                PCI_PREF_RANGE_MASK) << shift;
> +        limit = ((uint64_t)pci_get_word(d->config + PCI_PREF_MEMORY_LIMIT) &
> +                 PCI_PREF_RANGE_MASK) << shift;
> +        base |= (uint64_t)pci_get_long(d->config + PCI_PREF_BASE_UPPER32) << 32;
> +        limit |= (uint64_t)pci_get_long(d->config + PCI_PREF_LIMIT_UPPER32) << 32;
> +        limit |= 0xfffff;
> +        monitor_printf(mon,
> +                       "      pref MEM range [0x%08"PRIx64", 0x%08"PRIx64"]\n",

Why mix case? User likely won't know what's pref MEM.
Let's use plain English?

> +                       base, limit);
>      }
>      for(i = 0;i < PCI_NUM_REGIONS; i++) {
>          r = &d->io_regions[i];
> -- 
> 1.6.0.2
> 
>