qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Andreas Färber
Subject: Re: [Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller
Date: Mon, 08 Oct 2012 20:00:29 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120825 Thunderbird/15.0

Am 08.10.2012 18:46, 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
> 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
> 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 is starting to look really good modeling-wise. :) Some minor issues
below.

> ---
>  hw/ppc/e500-ccsr.h |   13 +++++++++++++
>  hw/ppc/e500.c      |   38 +++++++++++++++++++++++++++++++++++---
>  hw/ppce500_pci.c   |   22 +++++++++++++++++++++-
>  3 files changed, 69 insertions(+), 4 deletions(-)
>  create mode 100644 hw/ppc/e500-ccsr.h
> 
> diff --git a/hw/ppc/e500-ccsr.h b/hw/ppc/e500-ccsr.h
> new file mode 100644
> index 0000000..867bdb0
> --- /dev/null
> +++ b/hw/ppc/e500-ccsr.h
> @@ -0,0 +1,13 @@
> +#ifndef E500_CCSR_H
> +#define E500_CCSR_H 
> +
> +#include "../sysbus.h"
> +
> +typedef struct PPCE500CCSRState {
> +    SysBusDevice parent;

I would suggest to insert a while line here to separate the parent from
the other field(s).

> +    MemoryRegion ccsr_space;
> +} PPCE500CCSRState;
> +
> +#define TYPE_CCSR "e500-ccsr"
> +#define CCSR(obj) OBJECT_CHECK(PPCE500CCSRState, (obj), TYPE_CCSR)

While line please, since #endif corresponds to whole file.

> +#endif
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index b3e6a1e..ffcacd5 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -17,6 +17,7 @@
>  #include "config.h"
>  #include "qemu-common.h"
>  #include "e500.h"
> +#include "e500-ccsr.h"
>  #include "net.h"
>  #include "hw/hw.h"
>  #include "hw/pc.h"

> @@ -36,7 +37,7 @@
>  
>  #define BINARY_DEVICE_TREE_FILE    "mpc8544ds.dtb"
>  #define UIMAGE_LOAD_BASE           0
> -#define DTC_LOAD_PAD               0x1800000
> +#define DTC_LOAD_PAD               0x500000
>  #define DTC_PAD_MASK               0xFFFFF
>  #define INITRD_LOAD_PAD            0x2000000
>  #define INITRD_PAD_MASK            0xFFFFFF

Was this change intentional? I don't see it being used here, and commit
message doesn't seem to mention it.

> @@ -424,6 +425,7 @@ void ppce500_init(PPCE500Params *params)
>      DeviceState *dev;
>      CPUPPCState *firstenv = NULL;
>      MemoryRegion *ccsr;
> +    PPCE500CCSRState *pci_ccsr;
>      SysBusDevice *s;
>  
>      /* Setup CPUs */
> @@ -481,8 +483,13 @@ void ppce500_init(PPCE500Params *params)
>      vmstate_register_ram_global(ram);
>      memory_region_add_subregion(address_space_mem, 0, ram);
>  
> -    ccsr = g_malloc0(sizeof(MemoryRegion));
> -    memory_region_init(ccsr, "e500-cssr", MPC8544_CCSRBAR_SIZE);
> +    dev = qdev_create(NULL, "e500-ccsr");
> +    object_property_add_child(qdev_get_machine(), "e500-ccsr",
> +                              OBJECT(dev), NULL);

Note: According to our QOM ABI guarantees this means that we cannot
place any differently typed property of that name onto that machine, but
I believe this is okay with that naming.

(My plan for the future would be an "e500" SoC object in e500.c either
with a "ccsr" child or exposing the MemoryRegion directly through its
struct.)

> +    qdev_init_nofail(dev);
> +    pci_ccsr = CCSR(dev);
> +    ccsr = &pci_ccsr->ccsr_space;
> +    memory_region_init(ccsr, "e500-ccsr", MPC8544_CCSRBAR_SIZE);

Since ccsr_space is in the SysBusDevice now, it would nice to move the
initialization into the new initfn below, e.g.:

>      memory_region_add_subregion(address_space_mem, MPC8544_CCSRBAR_BASE, 
> ccsr);
>  
>      /* MPIC */
> @@ -594,3 +601,28 @@ void ppce500_init(PPCE500Params *params)
>          kvmppc_init();
>      }
>  }
> +
> +static int e500_ccsr_initfn(SysBusDevice *dev)
> +{

PPCE500CCSRState *ccsr = CCSR(dev);
memory_region_init(&ccsr->ccsr_space, "e500-ccsr", MPC8544_CCSRBAR_SIZE);

> +    return 0;
> +}
> +
> +static void e500_ccsr_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    k->init = e500_ccsr_initfn;
> +}
> +
> +static const TypeInfo e500_ccsr_info = {
> +    .name          = TYPE_CCSR,
> +    .parent        =  TYPE_SYS_BUS_DEVICE,

Duplicate white space after assignment.

> +    .instance_size = sizeof(PPCE500CCSRState),
> +    .class_init    = e500_ccsr_class_init,
> +};
> +
> +static void e500_ccsr_register_types(void)
> +{
> +    type_register_static(&e500_ccsr_info);
> +}
> +
> +type_init(e500_ccsr_register_types)

Usually the naming follows the file name, i.e. e500_register_types. Then
we can nicely add non-CCSR types there later.

> diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> index 92b1dc0..869ed92 100644
> --- a/hw/ppce500_pci.c
> +++ b/hw/ppce500_pci.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include "hw.h"
> +#include "hw/ppc/e500-ccsr.h"
>  #include "pci.h"
>  #include "pci_host.h"
>  #include "bswap.h"
> @@ -89,6 +90,12 @@ struct PPCE500PCIState {
>      MemoryRegion iomem;
>  };
>  
> +struct PPCE500PCI_Bridge_State {
> +    PCIDevice p;

Suggest readable "parent" and optionally separating white line.

> +    MemoryRegion bar0;
> +};
> +
> +typedef struct PPCE500PCI_Bridge_State PPCE500PCI_Bridge_State;

Coding Style asks for CamelCase in both cases, i.e. PPCE500PCIBridgeState.

>  typedef struct PPCE500PCIState PPCE500PCIState;
>  
>  static uint64_t pci_reg_read4(void *opaque, target_phys_addr_t addr,
> @@ -307,6 +314,18 @@ static const VMStateDescription vmstate_ppce500_pci = {
>  
>  #include "exec-memory.h"
>  
> +static int e500_pcihost_bridge_initfn(PCIDevice *d)
> +{
> +    PPCE500PCI_Bridge_State *b = DO_UPCAST(PPCE500PCI_Bridge_State, p, d);

Please introduce your own QOM cast macro for this type, like you did
CCSR(). Then the field names become irrelevant.

> +    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(), 
> "/e500-ccsr"));
> +
> +    b->bar0 = ccsr->ccsr_space;
> +    pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
> +    memory_region_init_alias(&b->bar0, "e500-pci-bar0", &ccsr->ccsr_space,
> +                             0, int128_get64(ccsr->ccsr_space.size));

This looks fishy: First you value-assign the CCSR MemoryRegion to bar0,
then you newly init it as an alias.
Not sure how that fits with bar registration, whether that may need to
be reordered to after the initialization and the copy-initialization
dropped?

Regards,
Andreas

> +    return 0;
> +}
> +
>  static int e500_pcihost_initfn(SysBusDevice *dev)
>  {
>      PCIHostState *h;
> @@ -350,6 +369,7 @@ static void e500_host_bridge_class_init(ObjectClass 
> *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
> +    k->init = e500_pcihost_bridge_initfn;
>      k->vendor_id = PCI_VENDOR_ID_FREESCALE;
>      k->device_id = PCI_DEVICE_ID_MPC8533E;
>      k->class_id = PCI_CLASS_PROCESSOR_POWERPC;
> @@ -359,7 +379,7 @@ static void e500_host_bridge_class_init(ObjectClass 
> *klass, void *data)
>  static const TypeInfo e500_host_bridge_info = {
>      .name          = "e500-host-bridge",
>      .parent        = TYPE_PCI_DEVICE,
> -    .instance_size = sizeof(PCIDevice),
> +    .instance_size = sizeof(PPCE500PCI_Bridge_State),
>      .class_init    = e500_host_bridge_class_init,
>  };
>  
> 


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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]