* [PATCH v1 0/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
@ 2017-01-10 11:37 Edgar E. Iglesias
2017-01-10 11:37 ` [PATCH v1 1/1] " Edgar E. Iglesias
0 siblings, 1 reply; 8+ messages in thread
From: Edgar E. Iglesias @ 2017-01-10 11:37 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
2017-01-10 11:37 [PATCH v1 0/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c Edgar E. Iglesias
@ 2017-01-10 11:37 ` Edgar E. Iglesias
2017-01-19 12:40 ` Julien Grall
0 siblings, 1 reply; 8+ messages in thread
From: Edgar E. Iglesias @ 2017-01-10 11:37 UTC (permalink / raw)
To: xen-devel; +Cc: edgar.iglesias, julien.grall, sstabellini
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
2017-01-10 11:37 ` [PATCH v1 1/1] " Edgar E. Iglesias
@ 2017-01-19 12:40 ` Julien Grall
2017-01-19 18:46 ` Stefano Stabellini
2017-01-26 12:52 ` Edgar E. Iglesias
0 siblings, 2 replies; 8+ messages in thread
From: Julien Grall @ 2017-01-19 12:40 UTC (permalink / raw)
To: Edgar E. Iglesias, xen-devel; +Cc: edgar.iglesias, sstabellini
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
2017-01-19 12:40 ` Julien Grall
@ 2017-01-19 18:46 ` Stefano Stabellini
2017-01-26 12:52 ` Edgar E. Iglesias
2017-01-26 12:52 ` Edgar E. Iglesias
1 sibling, 1 reply; 8+ messages in thread
From: Stefano Stabellini @ 2017-01-19 18:46 UTC (permalink / raw)
To: Julien Grall; +Cc: edgar.iglesias, Edgar E. Iglesias, sstabellini, xen-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
2017-01-19 12:40 ` Julien Grall
2017-01-19 18:46 ` Stefano Stabellini
@ 2017-01-26 12:52 ` Edgar E. Iglesias
2017-01-26 12:58 ` Julien Grall
1 sibling, 1 reply; 8+ messages in thread
From: Edgar E. Iglesias @ 2017-01-26 12:52 UTC (permalink / raw)
To: Julien Grall; +Cc: edgar.iglesias, sstabellini, xen-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
2017-01-19 18:46 ` Stefano Stabellini
@ 2017-01-26 12:52 ` Edgar E. Iglesias
0 siblings, 0 replies; 8+ messages in thread
From: Edgar E. Iglesias @ 2017-01-26 12:52 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: edgar.iglesias, Julien Grall, xen-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
2017-01-26 12:52 ` Edgar E. Iglesias
@ 2017-01-26 12:58 ` Julien Grall
2017-01-26 13:18 ` Edgar E. Iglesias
0 siblings, 1 reply; 8+ messages in thread
From: Julien Grall @ 2017-01-26 12:58 UTC (permalink / raw)
To: Edgar E. Iglesias; +Cc: edgar.iglesias, sstabellini, xen-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c
2017-01-26 12:58 ` Julien Grall
@ 2017-01-26 13:18 ` Edgar E. Iglesias
0 siblings, 0 replies; 8+ messages in thread
From: Edgar E. Iglesias @ 2017-01-26 13:18 UTC (permalink / raw)
To: Julien Grall; +Cc: edgar.iglesias, sstabellini, xen-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-01-26 13:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-10 11:37 [PATCH v1 0/1] xen/arm: Relax hw domain mapping attributes to p2m_mmio_direct_c Edgar E. Iglesias
2017-01-10 11:37 ` [PATCH v1 1/1] " Edgar E. Iglesias
2017-01-19 12:40 ` Julien Grall
2017-01-19 18:46 ` Stefano Stabellini
2017-01-26 12:52 ` Edgar E. Iglesias
2017-01-26 12:52 ` Edgar E. Iglesias
2017-01-26 12:58 ` Julien Grall
2017-01-26 13:18 ` Edgar E. Iglesias
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.