Re: [PATCH v3 4/6] xen/arm: zynqmp: eemi access control

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 10/16/2018 03:39 AM, Stefano Stabellini wrote:
> On 15/10/2018 08:25, Julien Grall wrote:
>> Hi,
>>
>> On 10/10/2018 11:35 PM, Stefano Stabellini wrote:
>>> On Tue, 28 Aug 2018, Julien Grall wrote:
>>>> On 11/08/18 01:01, Stefano Stabellini wrote:
>>>>>     #include <xen/iocap.h>
>>>>>     #include <xen/sched.h>
>>>>>     #include <xen/types.h>
>>>>> @@ -23,6 +91,721 @@
>>>>>     #include <asm/regs.h>
>>>>>     #include <asm/platforms/xilinx-zynqmp-eemi.h>
>>>>>     +struct pm_access
>>>>> +{
>>>>> +    paddr_t addr;
>>>>
>>>> It seems that the address will always page-aligned. So could we store a
>>>> frame
>>>> using mfn_t?
>>>
>>> Yes we could store just the frame. I started to make the change
>>> suggested, and all the required corresponding changes to the
>>> initializations below, for instance:
>>>
>>>     [NODE_RPU] = { MM_RPU },
>>>
>>> needs to become:
>>>
>>>     [NODE_RPU] = { _mfn(MM_RPU) },
>>>
>>> but when I tried to do that, I get a compilation error:
>>>
>>>     xilinx-zynqmp-eemi.c:106:20: error: initializer element is not constant
>>>          [NODE_RPU] = { _mfn(MM_RPU) },
>>>
>>> Unfortunately it is not possible to use mfn_t in static initializations,
>>> because it is a static inline. I could do:
>>>
>>
>> This is a bug in GCC older than 5.0.
>>
>>>     [NODE_RPU] = { { MM_RPU } },
>>>
>>> which would work for DEBUG builds but wouldn't work for non-DEBUG
>>> builds.
>>>
>>> I'll keep paddr_t for the next version of the series.
>>
>> What is the issue with that on non-debug build? We are using this
>> construction in another place without any compiler issue.
> 
> I modified the code as suggested again and tried with newer GCCs (both
> 6.3.1 and 7.3.1) but I still get the same error:
> 
> xilinx-zynqmp-eemi.c:105:20: error: initializer element is not constant
>       [NODE_RPU] = { _mfn(MM_RPU) },

I actually misremembered the problem. _mfn is using a static inline 
helper which is not considered as a constant.

The correct solution would be (mfn_t){ MM_RPU } but GCC 5.0 (and older) 
does not parse correctly the type cast. So we workaround by dropping the 
cast for the initializer.

On your previous e-mail you said that { { MM_RPU } } would not work for 
debug. One solution would be to introduce _mfn_initializer(...) that 
would be implemented differently for debug and non-debug.

I think such helper would be useful in other context as well.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel