Re: [Xen-devel] [PATCH ARM v4 11/12] mini-os: get GIC addresses from FDT

[Thomas Leonard <talex5@xxxxxxxxx>]

On 19 June 2014 11:58, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Thomas,
>
> On 06/19/2014 09:50 AM, Thomas Leonard wrote:
>> On 18 June 2014 18:25, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>>> Hi Thomas,
>>>
>>> On 06/18/2014 04:08 PM, Thomas Leonard wrote:
>>>>  //#define VGIC_DEBUG
>>>>  #ifdef VGIC_DEBUG
>>>> @@ -168,9 +169,38 @@ static void gic_handler(void) {
>>>>  }
>>>>
>>>>  void gic_init(void) {
>>>> -    // FIXME Get from dt!
>>>> -    gic.gicd_base = (char *)0x2c001000ULL;
>>>> -    gic.gicc_base = (char *)0x2c002000ULL;
>>>> +    gic.gicd_base = NULL;
>>>
>>> Any reason to not fold this patch in patch #7? Or better move the gic
>>> code in a separate patch?
>>
>> It was previously requested that I split the FDT patch from the main
>> ARM one. This patch depends on libfdt being present, so it has to go
>> after that.
>
> Please make sure that this "standalone patch" works correctly on Xen
> unstable...

OK, I can make it use GUEST_GICD_BASE/GUEST_GICC_BASE (with a FIXME)
so that the first commit works on unstable.

I tried installing the unstable version (staging branch) of Xen to
test it, but for some reason it didn't work:

# LD_LIBRARY_PATH=/opt/xen-4.5-unstable/lib/ /opt/xen-4.5-unstable/sbin/xl dmesg
xc: error: Could not obtain handle on privileged command interface (2
= No such file or directory): Internal error
libxl: error: libxl.c:99:libxl_ctx_alloc: cannot open libxc handle: No
such file or directory
cannot init xl context

It tries to open "/proc/xen/privcmd", which doesn't exist. Maybe I
need a newer Linux kernel? That could be tricky, as I'm using a
special fork for this board.

>> Moving all the GIC code to this patch would mean that the original
>> patch wouldn't work on its own.
>
> I'm not sure why you want to have a single big patch to support ARM...
> AFAIU, there is some patch requirements to work correctly.

If it isn't necessary that the first commit works, then I can split
things up further (e.g. all the include files first, then just
arm32.S, etc), but I'm not sure how much that would help. Let me know
what's easiest for reviewing.

>>> You made assumption of the layout of the device tree provided by Xen:
>>>         - #address-cells == #size-cells == 2
>>>         - regs contains a valid physical address, i.e the device is not 
>>> under a bus
>>>
>>> This can be changed by the toolstack in the future and will likely break
>>> mini-os.
>>
>> Is that likely? Seems like using BUG here is the right thing to do
>> until that happens.
>
> You need at least to add a comment about the (len != 32). It's confusing
> for people that doesn't know how the device tree has been created.
>
> I still think that a layer for IRQ and MMIO translation would be helpful
> for mini-os. So you will directly get the range (base + size), and won't
> duplicate checking code on every part of mini-os.
>
>> That would be very helpful - thanks!
>
> You can join #xenarm for any question if you are not already there.
>
> Regards,
>
> --
> Julien Grall



-- 
Dr Thomas Leonard        http://0install.net/
GPG: 9242 9807 C985 3C07 44A6  8B9A AE07 8280 59A5 3CC1
GPG: DA98 25AE CAD0 8975 7CDA  BD8E 0713 3F96 CA74 D8BA

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel