Re: [Qemu-ppc] [Qemu-devel] [PATCH] spapr-pci: fix config space access to support bridges

[Andreas Färber]

Am 09.08.2013 10:50, schrieb Alexey Kardashevskiy:
> spapr-pci config space accessors use find_dev() to find a PCI device.
> However find_dev() only searched on a primary bus and did not do
> recursive search through secondary buses so config space access was not
> possible for devices other that on a primary bus.
> 
> This fixed find_dev() by using the PCI API pci_find_device() function.
> This effectively enabled pci bridges on spapr.
> 
> While we are here, let's add some config space access traces.

Suggest to do that in a separate patch. The more things you can keep
apart, the more likely to get Reviewed-bys. :)

> 
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
>  hw/ppc/spapr_pci.c | 14 +++-----------
>  trace-events       |  2 ++
>  2 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 1ca35a0..2c8e55d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -65,22 +65,13 @@ static PCIDevice *find_dev(sPAPREnvironment *spapr, 
> uint64_t buid,
>  {
>      sPAPRPHBState *sphb = find_phb(spapr, buid);
>      PCIHostState *phb = PCI_HOST_BRIDGE(sphb);
> -    BusState *bus = BUS(phb->bus);
> -    BusChild *kid;
>      int devfn = (config_addr >> 8) & 0xFF;
>  
>      if (!phb) {
>          return NULL;
>      }
>  
> -    QTAILQ_FOREACH(kid, &bus->children, sibling) {
> -        PCIDevice *dev = (PCIDevice *)kid->child;
> -        if (dev->devfn == devfn) {
> -            return dev;
> -        }
> -    }
> -
> -    return NULL;
> +    return pci_find_device(phb->bus, (config_addr>>16) & 0xff, devfn);

Spaces around operator missing, cf. devfn above.

>  }
>  
>  static uint32_t rtas_pci_cfgaddr(uint32_t arg)
> @@ -114,9 +105,9 @@ static void finish_read_pci_config(sPAPREnvironment 
> *spapr, uint64_t buid,
>  
>      val = pci_host_config_read_common(pci_dev, addr,
>                                        pci_config_size(pci_dev), size);
> -

Whitespace change intentional?

>      rtas_st(rets, 0, 0);
>      rtas_st(rets, 1, val);
> +    trace_spapr_pci_cfg_read(pci_dev->name, addr, val);

These tracepoints are a bit odd: They do not seem to trace the actual
PCI config space access but some RTAS call to do so? Then please reflect
that in the name or move it closer to the actual access (keep in mind
that additional tracepoints may be enabled!) or best add tracepoints
somewhere inside pci_host_config_read_common() for everyone's benefit
since you do not seem to be tracing the RTAS-specific bits such as the 0
stored @ 0.

>  }
>  
>  static void rtas_ibm_read_pci_config(PowerPCCPU *cpu, sPAPREnvironment 
> *spapr,
> @@ -183,6 +174,7 @@ static void finish_write_pci_config(sPAPREnvironment 
> *spapr, uint64_t buid,
>                                   val, size);
>  
>      rtas_st(rets, 0, 0);
> +    trace_spapr_pci_cfg_write(pci_dev->name, addr, val);

Dito.

>  }
>  
>  static void rtas_ibm_write_pci_config(PowerPCCPU *cpu, sPAPREnvironment 
> *spapr,
> diff --git a/trace-events b/trace-events
> index 3856b5c..f99a8aa 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1119,6 +1119,8 @@ spapr_pci_rtas_ibm_change_msi(unsigned func, unsigned 
> req) "func %u, requested %
>  spapr_pci_rtas_ibm_query_interrupt_source_number(unsigned ioa, unsigned 
> intr) "queries for #%u, IRQ%u"
>  spapr_pci_msi_write(uint64_t addr, uint64_t data, uint32_t dt_irq) 
> "@%"PRIx64"<=%"PRIx64" IRQ %u"
>  spapr_pci_lsi_set(const char *busname, int pin, uint32_t irq) "%s PIN%d IRQ 
> %u"
> +spapr_pci_cfg_read(const char *dev, unsigned offs, unsigned val) "%s @0x%x 
> 0x%x"
> +spapr_pci_cfg_write(const char *dev, unsigned offs, unsigned val) "%s @0x%x 
> 0x%x"
>  
>  # hw/ppc/xics.c
>  xics_icp_check_ipi(int server, uint8_t mfrr) "CPU %d can take IPI mfrr=%#x"

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg