| Message ID | 1484048223-27983-2-git-send-email-edgar.iglesias@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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.
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
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
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,
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
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;