| Message ID | 20190806214925.7534-6-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/7] xen/arm: extend device_tree_for_each_node | expand |
Hi Stefano, Stefano Stabellini writes: > Don't allow reserved-memory regions to be remapped into any guests, > until reserved-memory regions are properly supported in Xen. For now, > do not call iomem_permit_access for them. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > --- > > Changes in v4: > - compare the parent name with reserved-memory > - use dt_node_cmp > > Changes in v3: > - new patch > --- > xen/arch/arm/domain_build.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 4c8404155a..267e0549e2 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev, > struct map_range_data *mr_data = data; > struct domain *d = mr_data->d; > bool need_mapping = !dt_device_for_passthrough(dev); > + const struct dt_device_node *parent = dt_get_parent(dev); > int res; > > - res = iomem_permit_access(d, paddr_to_pfn(addr), > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > - if ( res ) > + /* > + * Don't give iomem permissions for reserved-memory ranges until > + * reserved-memory support is complete. > + */ > + if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 ) Am I missing something, or you are permitting access only if it from a "reserved-memory" node? This contradicts with patch description. > { > - printk(XENLOG_ERR "Unable to permit to dom%d access to" > - " 0x%"PRIx64" - 0x%"PRIx64"\n", > - d->domain_id, > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > - return res; > + res = iomem_permit_access(d, paddr_to_pfn(addr), > + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to permit to dom%d access to" > + " 0x%"PRIx64" - 0x%"PRIx64"\n", > + d->domain_id, > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > + return res; > + } > } > > if ( need_mapping ) So, this region cold be mapped, but without the access? -- Volodymyr Babchuk at EPAM
On Thu, 8 Aug 2019, Volodymyr Babchuk wrote: > Hi Stefano, > > Stefano Stabellini writes: > > > Don't allow reserved-memory regions to be remapped into any guests, > > until reserved-memory regions are properly supported in Xen. For now, > > do not call iomem_permit_access for them. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > > > Changes in v4: > > - compare the parent name with reserved-memory > > - use dt_node_cmp > > > > Changes in v3: > > - new patch > > --- > > xen/arch/arm/domain_build.c | 24 ++++++++++++++++-------- > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 4c8404155a..267e0549e2 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev, > > struct map_range_data *mr_data = data; > > struct domain *d = mr_data->d; > > bool need_mapping = !dt_device_for_passthrough(dev); > > + const struct dt_device_node *parent = dt_get_parent(dev); > > int res; > > > > - res = iomem_permit_access(d, paddr_to_pfn(addr), > > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > > - if ( res ) > > + /* > > + * Don't give iomem permissions for reserved-memory ranges until > > + * reserved-memory support is complete. > > + */ > > + if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 ) > Am I missing something, or you are permitting access only if it from a > "reserved-memory" node? This contradicts with patch description. Well spotted! I inverted the condition by mistake. > > { > > - printk(XENLOG_ERR "Unable to permit to dom%d access to" > > - " 0x%"PRIx64" - 0x%"PRIx64"\n", > > - d->domain_id, > > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > > - return res; > > + res = iomem_permit_access(d, paddr_to_pfn(addr), > > + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > > + if ( res ) > > + { > > + printk(XENLOG_ERR "Unable to permit to dom%d access to" > > + " 0x%"PRIx64" - 0x%"PRIx64"\n", > > + d->domain_id, > > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > > + return res; > > + } > > } > > > > if ( need_mapping ) > So, this region cold be mapped, but without the access? I'll change it to return early from the function for reserved-memory regions.
Hi, On 09/08/2019 23:56, Stefano Stabellini wrote: > On Thu, 8 Aug 2019, Volodymyr Babchuk wrote: >> Hi Stefano, >> >> Stefano Stabellini writes: >> >>> Don't allow reserved-memory regions to be remapped into any guests, >>> until reserved-memory regions are properly supported in Xen. For now, >>> do not call iomem_permit_access for them. >>> >>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> >>> --- >>> >>> Changes in v4: >>> - compare the parent name with reserved-memory >>> - use dt_node_cmp >>> >>> Changes in v3: >>> - new patch >>> --- >>> xen/arch/arm/domain_build.c | 24 ++++++++++++++++-------- >>> 1 file changed, 16 insertions(+), 8 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 4c8404155a..267e0549e2 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev, >>> struct map_range_data *mr_data = data; >>> struct domain *d = mr_data->d; >>> bool need_mapping = !dt_device_for_passthrough(dev); >>> + const struct dt_device_node *parent = dt_get_parent(dev); >>> int res; >>> >>> - res = iomem_permit_access(d, paddr_to_pfn(addr), >>> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); >>> - if ( res ) >>> + /* >>> + * Don't give iomem permissions for reserved-memory ranges until >>> + * reserved-memory support is complete. >>> + */ >>> + if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 ) >> Am I missing something, or you are permitting access only if it from a >> "reserved-memory" node? This contradicts with patch description. > > Well spotted! I inverted the condition by mistake. > > >>> { >>> - printk(XENLOG_ERR "Unable to permit to dom%d access to" >>> - " 0x%"PRIx64" - 0x%"PRIx64"\n", >>> - d->domain_id, >>> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); >>> - return res; >>> + res = iomem_permit_access(d, paddr_to_pfn(addr), >>> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); >>> + if ( res ) >>> + { >>> + printk(XENLOG_ERR "Unable to permit to dom%d access to" >>> + " 0x%"PRIx64" - 0x%"PRIx64"\n", >>> + d->domain_id, >>> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); >>> + return res; >>> + } >>> } >>> >>> if ( need_mapping ) >> So, this region cold be mapped, but without the access? IOMEM access and mapping are two different things. The former gives a domain control over managing the region (i.e mapping, unmapping, giving access to another domain). The latter will map the region in the P2M so the domain can read/write into it. > > I'll change it to return early from the function for reserved-memory > regions. I am not sure to understand you suggestion here... You still need to have reserved-regions mapped into the hardware domain. The only thing we want to prevent is the domain to manage the region. Cheers,
On Mon, 12 Aug 2019, Julien Grall wrote: > On 09/08/2019 23:56, Stefano Stabellini wrote: > > On Thu, 8 Aug 2019, Volodymyr Babchuk wrote: > > > Hi Stefano, > > > > > > Stefano Stabellini writes: > > > > > > > Don't allow reserved-memory regions to be remapped into any guests, > > > > until reserved-memory regions are properly supported in Xen. For now, > > > > do not call iomem_permit_access for them. > > > > > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > > > --- > > > > > > > > Changes in v4: > > > > - compare the parent name with reserved-memory > > > > - use dt_node_cmp > > > > > > > > Changes in v3: > > > > - new patch > > > > --- > > > > xen/arch/arm/domain_build.c | 24 ++++++++++++++++-------- > > > > 1 file changed, 16 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > > > index 4c8404155a..267e0549e2 100644 > > > > --- a/xen/arch/arm/domain_build.c > > > > +++ b/xen/arch/arm/domain_build.c > > > > @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const > > > > struct dt_device_node *dev, > > > > struct map_range_data *mr_data = data; > > > > struct domain *d = mr_data->d; > > > > bool need_mapping = !dt_device_for_passthrough(dev); > > > > + const struct dt_device_node *parent = dt_get_parent(dev); > > > > int res; > > > > > > > > - res = iomem_permit_access(d, paddr_to_pfn(addr), > > > > - paddr_to_pfn(PAGE_ALIGN(addr + len - > > > > 1))); > > > > - if ( res ) > > > > + /* > > > > + * Don't give iomem permissions for reserved-memory ranges until > > > > + * reserved-memory support is complete. > > > > + */ > > > > + if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 ) > > > Am I missing something, or you are permitting access only if it from a > > > "reserved-memory" node? This contradicts with patch description. > > > > Well spotted! I inverted the condition by mistake. > > > > > > > > { > > > > - printk(XENLOG_ERR "Unable to permit to dom%d access to" > > > > - " 0x%"PRIx64" - 0x%"PRIx64"\n", > > > > - d->domain_id, > > > > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > > > > - return res; > > > > + res = iomem_permit_access(d, paddr_to_pfn(addr), > > > > + paddr_to_pfn(PAGE_ALIGN(addr + len - > > > > 1))); > > > > + if ( res ) > > > > + { > > > > + printk(XENLOG_ERR "Unable to permit to dom%d access to" > > > > + " 0x%"PRIx64" - 0x%"PRIx64"\n", > > > > + d->domain_id, > > > > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > > > > + return res; > > > > + } > > > > } > > > > > > > > if ( need_mapping ) > > > So, this region cold be mapped, but without the access? > > IOMEM access and mapping are two different things. The former gives a domain > control over managing the region (i.e mapping, unmapping, giving access to > another domain). The latter will map the region in the P2M so the domain can > read/write into it. > > > > > I'll change it to return early from the function for reserved-memory > > regions. > > I am not sure to understand you suggestion here... You still need to have > reserved-regions mapped into the hardware domain. The only thing we want to > prevent is the domain to manage the region. I forgot that giving iomem permission to dom0 automatically means that the toolstack can give iomem permission to a domU for the same region.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 4c8404155a..267e0549e2 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev, struct map_range_data *mr_data = data; struct domain *d = mr_data->d; bool need_mapping = !dt_device_for_passthrough(dev); + const struct dt_device_node *parent = dt_get_parent(dev); int res; - res = iomem_permit_access(d, paddr_to_pfn(addr), - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); - if ( res ) + /* + * Don't give iomem permissions for reserved-memory ranges until + * reserved-memory support is complete. + */ + if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 ) { - printk(XENLOG_ERR "Unable to permit to dom%d access to" - " 0x%"PRIx64" - 0x%"PRIx64"\n", - d->domain_id, - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); - return res; + res = iomem_permit_access(d, paddr_to_pfn(addr), + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); + if ( res ) + { + printk(XENLOG_ERR "Unable to permit to dom%d access to" + " 0x%"PRIx64" - 0x%"PRIx64"\n", + d->domain_id, + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); + return res; + } } if ( need_mapping )
Don't allow reserved-memory regions to be remapped into any guests, until reserved-memory regions are properly supported in Xen. For now, do not call iomem_permit_access for them. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Changes in v4: - compare the parent name with reserved-memory - use dt_node_cmp Changes in v3: - new patch --- xen/arch/arm/domain_build.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)