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

["Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxx>]

On Thu, Jan 19, 2017 at 12:40:45PM +0000, Julien Grall wrote:
> Hi Edgar,

Hi Julien,

> 
> On 10/01/2017 11:37, Edgar E. Iglesias wrote:
> >From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxxxxx>
> >
> >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
    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).

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 )
     {

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


> Also, this is a revert of patch 1e75ed8 and 9eed361 + relaxation, I would
> either mention it in the commit message. Or send separate patch to revert
> both of them. Either way will be fine by me.


I've changed v2 to keep the plumbing but revert 9eed361.

Thanks,
Edgar


> 
> >
> >Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxxxxx>
> >---
> > xen/arch/arm/domain_build.c | 63 
> > ++++++++++-----------------------------------
> > 1 file changed, 13 insertions(+), 50 deletions(-)
> >
> >diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >index 07b868d..a3521c7 100644
> >--- a/xen/arch/arm/domain_build.c
> >+++ b/xen/arch/arm/domain_build.c
> >@@ -48,20 +48,6 @@ struct map_range_data
> >     p2m_type_t p2mt;
> > };
> >
> >-static const struct dt_device_match dev_map_attrs[] __initconst =
> >-{
> >-    {
> >-        __DT_MATCH_COMPATIBLE("mmio-sram"),
> >-        __DT_MATCH_PROP("no-memory-wc"),
> >-        .data = (void *) (uintptr_t) p2m_mmio_direct_dev,
> >-    },
> >-    {
> >-        __DT_MATCH_COMPATIBLE("mmio-sram"),
> >-        .data = (void *) (uintptr_t) p2m_mmio_direct_nc,
> >-    },
> >-    { /* sentinel */ },
> >-};
> >-
> > //#define DEBUG_11_ALLOCATION
> > #ifdef DEBUG_11_ALLOCATION
> > # define D11PRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args)
> >@@ -1005,8 +991,7 @@ static int map_range_to_domain(const struct 
> >dt_device_node *dev,
> >                                u64 addr, u64 len,
> >                                void *data)
> > {
> >-    struct map_range_data *mr_data = data;
> 
> I would actually prefer to keep the plumbing and only remove dev_map_attrs.
> Stefano do you have any opinions?
> 
> If we are going to remove the plumbing, you would need to remove
> map_range_data which has been introduced by the plumbing patch.
> 
> Cheers,
> 
> -- 
> Julien Grall

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