[Top][All Lists]
[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: |
Alexander Graf |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] Adding BAR0 for e500 PCI controller |
Date: |
Mon, 8 Oct 2012 21:04:43 +0200 |
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 <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).
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
[Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region, Bharat Bhushan, 2012/10/08
- Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region, Andreas Färber, 2012/10/08
- Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region, Avi Kivity, 2012/10/09
- Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region, Bhushan Bharat-R65777, 2012/10/09
- Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region, Avi Kivity, 2012/10/09
- Re: [Qemu-devel] [PATCH 2/3] e500: Adding CCSR memory region, Bhushan Bharat-R65777, 2012/10/09