Re: [Qemu-devel] [PATCH 2/2 v2] Adding BAR0 for e500 PCI controller

[Andreas Färber]

Am 09.10.2012 20:19, schrieb Bharat Bhushan:
> PCI Root complex have TYPE-1 configuration header while PCI endpoint
> have type-0 configuration header. The type-1 configuration header have
> a BAR (BAR0). In Freescale PCI controller BAR0 is used for mapping pci
> address space to CCSR address space. This can used for 2 purposes: 1)
> for MSI interrupt generation 2) Allow CCSR registers access when configured
> as PCI endpoint, which I am not sure is a use case with QEMU-KVM guest.
> 
> What I observed is that when guest read the size of BAR0 of host controller
> configuration header (TYPE1 header) then it always reads it as 0. When
> looking into the QEMU hw/ppce500_pci.c, I do not find the PCI controller
> device registering BAR0. I do not find any other controller also doing so
> may they do not use BAR0.
> 
> There are two issues when BAR0 is not there (which I can think of):
> 1) There should be BAR0 emulated for PCI Root comaplex (TYPE1 header) and

"complex"

> when reading the size of BAR0, it should give size as per real h/w.
> 
> 2) Do we need this BAR0 inbound address translation?
>         When BAR0 is of non-zero size then it will be configured for PCI
> address space to local address(CCSR) space translation on inbound access.
> The primary use case is for MSI interrupt generation. The device is
> configured with a address offsets in PCI address space, which will be

"with address offsets" or "with an address offset"

> translated to MSI interrupt generation MPIC registers. Currently I do
> not understand the MSI interrupt generation mechanism in QEMU and also
> IIRC we do not use QEMU MSI interrupt mechanism on e500 guest machines.
> But this BAR0 will be used when using MSI on e500.
> 
> I can see one more issue, There are ATMUs emulated in hw/ppce500_pci.c,
> but i do not see these being used for address translation.
> So far that works because pci address space and local address space are 1:1
> mapped. BAR0 inbound translation + ATMU translation will complete the address
> translation of inbound traffic.
> 
> Signed-off-by: Bharat Bhushan <address@hidden>

This looks perfect except for typos above and one line of code that
maybe Alex can fix.

Reviewed-by: Andreas Färber <address@hidden>

> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..58dbc1a 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c

> @@ -307,6 +321,19 @@ static const VMStateDescription vmstate_ppce500_pci = {
>  
>  #include "exec-memory.h"
>  
> +static int e500_pcihost_bridge_initfn(PCIDevice *d)
> +{
> +    PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
> +    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
> +                                  "/e500-ccsr"));
> +

> +    b->bar0 = ccsr->ccsr_space;

This copy-assignment is getting overwritten by the alias init in the
next line, so it would seem cleaner to drop this line now.

Andreas

> +    memory_region_init_alias(&b->bar0, "e500-pci-bar0", &ccsr->ccsr_space,
> +                             0, int128_get64(ccsr->ccsr_space.size));
> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
> +    return 0;
> +}
> +
>  static int e500_pcihost_initfn(SysBusDevice *dev)
>  {
>      PCIHostState *h;


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