On 08.10.2012, at 20:00, Andreas Färber wrote: > 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 <[email protected]> > > 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).
Blank line? White line? > >> + 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. I'd assume he tried to work around a bug I fixed in between. But this change definitely is not intentional. Alex
