Re: [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c

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

Hi Edgar,

On 26/01/2017 12:52, Edgar E. Iglesias wrote:
> On Thu, Jan 19, 2017 at 12:40:45PM +0000, Julien Grall wrote:
>>
>> On 10/01/2017 11:37, Edgar E. Iglesias wrote:
>>> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>>>
>>> Relax the hardware domains mapping attributes to p2m_mmio_direct_c.
>>> This will allow the hardware domain to fully control the
>>> attribtues via its S1 mappings.
>>
>> s/attribtues/attributes/
>
> Fixed for v2.
>
>
>>
>> I would like some rationale in the commit message to explain why it is fine
>> to do this relaxation (e.g the hardware domain is a trusted domain).
>
> I've added the following for v2:
>     Since the hardware domain is a trusted domain, we extend the
>     trust to include making final decisions on what attributes to
>     use when mapping memory regions.
>
>     For device-tree configured hardware domains, this patch relaxes

I would drop the "For device-tree configured hardware domains" as you 
will also fix ACPI. The rest looks good to me.

>     the hardware domains mapping attributes to p2m_mmio_direct_c.
>     This will allow the hardware domain to control the attributes
>     via its S1 mappings.
>
>>
>> A such relaxation would probably be necessary for the ACPI case too (see
>> map_dev_mmio_region).
>
> I don't have testcases for ACPI but I'll try to fix it.
>
> Please correct me if I'm wrong. IIUC, when using ACPI, we map in a few
> selected devices (UART, GIC, SMMU, RAM) to dom0 but leave the rest unmapped.
> Dom0 then parses ACPI tables and issues hypervisor calls to map individual
> devices (XENMEM_add_to_physmap with XENMAPSPACE_dev_mmio).

That is correct.

>
> Since XENMEM_add_to_physmap with XENMAPSPACE_dev_mmio is only used
> for dom0 mappings, I think this relaxation would be safe:
> +++ b/xen/arch/arm/p2m.c
> @@ -1185,7 +1185,7 @@ int map_dev_mmio_region(struct domain *d,
>      if ( !(nr && iomem_access_permitted(d, mfn_x(mfn), mfn_x(mfn) + nr - 1)) )
>          return 0;
>
> -    res = map_mmio_regions(d, gfn, nr, mfn);
> +    res = p2m_insert_mapping(d, gfn, nr, mfn, p2m_mmio_direct_c);
>      if ( res < 0 )
>      {

This change looks good to me. I will give a try when the new version 
will be sent.

>
> Anyway, I'll send the v2 series out and we can discuss from there.

Can you also please modify the comment on "XENMEMSPACE_dev_mmio" in 
xen/include/public/memory.h regarding the memory attribute used to map?

Cheers,

-- 
Julien Grall

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