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

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

Hi Stefano,

On 17/10/2018 15:03, Stefano Stabellini wrote:
> On Tue, 16 Oct 2018, Julien Grall wrote:
>> Hi,
>>
>> Sorry I forgot to answer to the rest of the e-mail.
>>
>> On 10/16/2018 03:39 AM, Stefano Stabellini wrote:
>>> On 15/10/2018 08:25, Julien Grall wrote:
>>>>>>> +    bool hwdom_access;    /* HW domain gets access regardless.  */
>>>>>>> +};
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * This table maps a node into a memory address.
>>>>>>> + * If a guest has access to the address, it has enough control
>>>>>>> + * over the node to grant it access to EEMI calls for that node.
>>>>>>> + */
>>>>>>> +static const struct pm_access pm_node_access[] = {
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * This table maps reset line IDs into a memory address.
>>>>>>> + * If a guest has access to the address, it has enough control
>>>>>>> + * over the affected node to grant it access to EEMI calls for
>>>>>>> + * resetting that node.
>>>>>>> + */
>>>>>>> +#define XILPM_RESET_IDX(n) (n - XILPM_RESET_PCIE_CFG)
>>>>>>> +static const struct pm_access pm_reset_access[] = {
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * This table maps reset line IDs into a memory address.
>>>>>>> + * If a guest has access to the address, it has enough control
>>>>>>> + * over the affected node to grant it access to EEMI calls for
>>>>>>> + * resetting that node.
>>>>>>> + */
>>>>>>> +static const struct {
>>>>>>> +    paddr_t start;
>>>>>>> +    paddr_t size;
>>>>>>> +    uint32_t mask;   /* Zero means no mask, i.e all bits.  */
>>>>>>> +    enum pm_node_id node;
>>>>>>> +    bool hwdom_access;
>>>>>>> +    bool readonly;
>>>>>>> +} pm_mmio_access[] = {
>>>>>>
>>>>>> Those 3 arrays contains a lot of hardcoded value. Can't any of this be
>>>>>> detected from the device-tree?
>>>>>
>>>>> No, the information is not available on device tree unfortunately. >
>>>>>
>>>>>> I would be interested to know how this is going to work with upstream
>>>>>> Linux.
>>>>>> Do you hardcode all the values there as well?
>>>>>
>>>>> Yes: the IDs are specified on an header file, see
>>>>> include/linux/firmware/xlnx-zynqmp.h on the zynq/firmware branch of the
>>>>> arm-soc tree. In addition to the IDs, we also have the memory addresses
>>>>> in Xen to do the permission checks.
>>>>
>>>> I am afraid this is not linux upstream. Can you point to the code in
>>>> Linus's
>>>> git or explain the state of the review?
>>>
>>> It hasn't been pulled into Linux yet, I was told it has already been
>>> reviewed and is queued in arm-soc for upstreaming at the next merge
>>> window, which should be imminent.
>>
>> Looking at that branch, I can see some DT bindings at least for the clock. I
>> also don't see any hardcoded value for device so far in that series. Is it
>> going to be sent separately?
> 
> If you look at include/linux/firmware/xlnx-zynqmp.h, you'll see some
> hardcode values, specifically enum pm_api_id matches numerically the
> enum by the same name this series introduces in
> xen/include/asm-arm/platforms/xilinx-zynqmp-eemi.h

I don't think we are talking the same things. I am talking about 
pm_node_id/pm_node_reset (not pm_api_id). I don't see such code in Linux 
at the moment and a bit surprised that no DT bindings will be used to 
link the value with a device.

So my question stands, how Linux will use pm_node_id/pm_node_reset? Can 
you point to code if that exists.

Cheers,

-- 
Julien Grall

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