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

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

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

This relaxes the hw domains S2 mapping attributes to p2m_mmio_direct_c.

Eventually, I think we should allow guests to execute code from on chip
memories. We could follow-up with a patch that allows execution for
p2m_mmio_direct_c and p2m_mmio_direct_nc. Does that sound OK?

Cheers,
Edgar

Edgar E. Iglesias (1):
  xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c

 xen/arch/arm/domain_build.c | 63 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 50 deletions(-)

-- 
2.7.4


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

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

[Edgar E. Iglesias]

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.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 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;
-    struct domain *d = mr_data->d;
+    struct domain *d = data;
     bool_t need_mapping = !dt_device_for_passthrough(dev);
     int res;
 
@@ -1027,7 +1012,7 @@ static int map_range_to_domain(const struct dt_device_node *dev,
                                _gfn(paddr_to_pfn(addr)),
                                DIV_ROUND_UP(len, PAGE_SIZE),
                                _mfn(paddr_to_pfn(addr)),
-                               mr_data->p2mt);
+                               p2m_mmio_direct_c);
 
         if ( res < 0 )
         {
@@ -1039,8 +1024,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
         }
     }
 
-    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n",
-               addr, addr + len, mr_data->p2mt);
+    dt_dprintk("  - MMIO: %010"PRIx64" - %010"PRIx64"\n",
+               addr, addr + len);
 
     return 0;
 }
@@ -1051,10 +1036,8 @@ static int map_range_to_domain(const struct dt_device_node *dev,
  * the child resources available to domain 0.
  */
 static int map_device_children(struct domain *d,
-                               const struct dt_device_node *dev,
-                               p2m_type_t p2mt)
+                               const struct dt_device_node *dev)
 {
-    struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
     int ret;
 
     if ( dt_device_type_is_equal(dev, "pci") )
@@ -1066,7 +1049,7 @@ static int map_device_children(struct domain *d,
         if ( ret < 0 )
             return ret;
 
-        ret = dt_for_each_range(dev, &map_range_to_domain, &mr_data);
+        ret = dt_for_each_range(dev, &map_range_to_domain, d);
         if ( ret < 0 )
             return ret;
     }
@@ -1082,8 +1065,7 @@ static int map_device_children(struct domain *d,
  *  - Assign the device to the guest if it's protected by an IOMMU
  *  - Map the IRQs and iomem regions to DOM0
  */
-static int handle_device(struct domain *d, struct dt_device_node *dev,
-                         p2m_type_t p2mt)
+static int handle_device(struct domain *d, struct dt_device_node *dev)
 {
     unsigned int nirq;
     unsigned int naddr;
@@ -1149,7 +1131,6 @@ static int handle_device(struct domain *d, struct dt_device_node *dev,
     /* Give permission and map MMIOs */
     for ( i = 0; i < naddr; i++ )
     {
-        struct map_range_data mr_data = { .d = d, .p2mt = p2mt };
         res = dt_device_get_address(dev, i, &addr, &size);
         if ( res )
         {
@@ -1158,36 +1139,20 @@ static int handle_device(struct domain *d, struct dt_device_node *dev,
             return res;
         }
 
-        res = map_range_to_domain(dev, addr, size, &mr_data);
+        res = map_range_to_domain(dev, addr, size, d);
         if ( res )
             return res;
     }
 
-    res = map_device_children(d, dev, p2mt);
+    res = map_device_children(d, dev);
     if ( res )
         return res;
 
     return 0;
 }
 
-static p2m_type_t lookup_map_attr(struct dt_device_node *node,
-                                  p2m_type_t parent_p2mt)
-{
-    const struct dt_device_match *r;
-
-    /* Search and if nothing matches, use the parent's attributes.  */
-    r = dt_match_node(dev_map_attrs, node);
-
-    /*
-     * If this node does not dictate specific mapping attributes,
-     * it inherits its parent's attributes.
-     */
-    return r ? (uintptr_t) r->data : parent_p2mt;
-}
-
 static int handle_node(struct domain *d, struct kernel_info *kinfo,
-                       struct dt_device_node *node,
-                       p2m_type_t p2mt)
+                       struct dt_device_node *node)
 {
     static const struct dt_device_match skip_matches[] __initconst =
     {
@@ -1275,8 +1240,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
                "WARNING: Path %s is reserved, skip the node as we may re-use the path.\n",
                path);
 
-    p2mt = lookup_map_attr(node, p2mt);
-    res = handle_device(d, node, p2mt);
+    res = handle_device(d, node);
     if ( res)
         return res;
 
@@ -1298,7 +1262,7 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
 
     for ( child = node->child; child != NULL; child = child->sibling )
     {
-        res = handle_node(d, kinfo, child, p2mt);
+        res = handle_node(d, kinfo, child);
         if ( res )
             return res;
     }
@@ -1330,7 +1294,6 @@ static int handle_node(struct domain *d, struct kernel_info *kinfo,
 
 static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 {
-    const p2m_type_t default_p2mt = p2m_mmio_direct_dev;
     const void *fdt;
     int new_size;
     int ret;
@@ -1350,7 +1313,7 @@ static int prepare_dtb(struct domain *d, struct kernel_info *kinfo)
 
     fdt_finish_reservemap(kinfo->fdt);
 
-    ret = handle_node(d, kinfo, dt_host, default_p2mt);
+    ret = handle_node(d, kinfo, dt_host);
     if ( ret )
         goto err;
 
-- 
2.7.4


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

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

[Julien Grall]

Hi Edgar,

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/

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

A such relaxation would probably be necessary for the ACPI case too (see 
map_dev_mmio_region).

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.

>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

On Thu, 19 Jan 2017, Julien Grall wrote:
> Hi Edgar,
> 
> 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/
> 
> 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).
> 
> A such relaxation would probably be necessary for the ACPI case too (see
> map_dev_mmio_region).
> 
> 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.
> 
> > 
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  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?

I would keep the plumbings too (9eed361a), but I am fine either way.

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

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

[Edgar E. Iglesias]

On Thu, Jan 19, 2017 at 10:46:03AM -0800, Stefano Stabellini wrote:
> On Thu, 19 Jan 2017, Julien Grall wrote:
> > Hi Edgar,
> > 
> > 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/
> > 
> > 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).
> > 
> > A such relaxation would probably be necessary for the ACPI case too (see
> > map_dev_mmio_region).
> > 
> > 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.
> > 
> > > 
> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > > ---
> > >  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?
> 
> I would keep the plumbings too (9eed361a), but I am fine either way.


Thanks, I've changed the patch to keep the plumbing from 9eed361a.

Cheers,
Edgar

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

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

[Edgar E. Iglesias]

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@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
    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@xilinx.com>
> >---
> > 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

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

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

[Edgar E. Iglesias]

On Thu, Jan 26, 2017 at 12:58:52PM +0000, Julien Grall wrote:
> Hi Edgar,

Hi Julien,

> 
> 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.

Sorry, saw this a bit late but the series has one patch fixing DT and
another fixing ACPI. If you prefer I can squash them in a v3 or perhaps
Stefano can squash it as he commits them.


> 
> >    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.

Thanks!

> >
> >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?

Yes, I've updated the comment.

Cheers,
Edgar

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