| Message ID | 20200415010255.10081-12-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] xen: introduce xen_dom_flags | expand |
On 15/04/2020 02:02, Stefano Stabellini wrote: > iomem_permit_access should be called for MMIO regions of devices > assigned to a domain. Currently it is not called for MMIO regions of > passthrough devices of Dom0less guests. This patch fixes it. You can already have cases today where the MMIO regions are mapped to the guest but the guest doesn't have permission on them. Can you explain why this is a problem here? Cheers, > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > xen/arch/arm/domain_build.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index d0fc497d07..d3d42eef5d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1846,6 +1846,17 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, > return -EINVAL; > } > > + res = iomem_permit_access(kinfo->d, paddr_to_pfn(mstart), > + paddr_to_pfn(PAGE_ALIGN(mstart + size - 1))); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to permit to dom%d access to" > + " 0x%"PRIx64" - 0x%"PRIx64"\n", > + kinfo->d->domain_id, > + mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1); > + return res; > + } > + > res = map_regions_p2mt(kinfo->d, > gaddr_to_gfn(gstart), > PFN_DOWN(size), >
On Wed, 15 Apr 2020, Julien Grall wrote: > On 15/04/2020 02:02, Stefano Stabellini wrote: > > iomem_permit_access should be called for MMIO regions of devices > > assigned to a domain. Currently it is not called for MMIO regions of > > passthrough devices of Dom0less guests. This patch fixes it. > > You can already have cases today where the MMIO regions are mapped to the > guest but the guest doesn't have permission on them. > > Can you explain why this is a problem here? I don't remember the original problem that prompted me into doing this investigation. It came up while I was developing and testing this series: I noticed the discrepancy compared to the regular (not dom0less) device assignment code path (tools/libxl/libxl_create.c:domcreate_launch_dm). Now I removed this patch from the series, went back to test it, and it still works fine. Oops, it is not needed after all :-) I think it makes sense to remove this patch from this series, I'll do that. But doesn't it make sense to give domU permission if it is going to get the memory mapped? But admittedly I can't think of something that would break because of the lack of the iomem_permit_access call in this code path. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > --- > > xen/arch/arm/domain_build.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index d0fc497d07..d3d42eef5d 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -1846,6 +1846,17 @@ static int __init handle_passthrough_prop(struct > > kernel_info *kinfo, > > return -EINVAL; > > } > > + res = iomem_permit_access(kinfo->d, paddr_to_pfn(mstart), > > + paddr_to_pfn(PAGE_ALIGN(mstart + size - > > 1))); > > + if ( res ) > > + { > > + printk(XENLOG_ERR "Unable to permit to dom%d access to" > > + " 0x%"PRIx64" - 0x%"PRIx64"\n", > > + kinfo->d->domain_id, > > + mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1); > > + return res; > > + } > > + > > res = map_regions_p2mt(kinfo->d, > > gaddr_to_gfn(gstart), > > PFN_DOWN(size), > > > > -- > Julien Grall >
Hi Stefano, On 29/04/2020 21:47, Stefano Stabellini wrote: > On Wed, 15 Apr 2020, Julien Grall wrote: > But doesn't it make sense to give domU permission if it is going to get > the memory mapped? But admittedly I can't think of something that would > break because of the lack of the iomem_permit_access call in this code > path. On Arm, the permissions are only useful if you plan you DomU to delegate the regions to another domain. As your domain is not even aware it is running on Xen (we don't expose 'xen' node in the DT), it makes little sense to add the permission. Even today, you can map IOMEM to a DomU and then revert the permission right after. They IOMEM will still be mapped in the guest and it will act normaly. Cheers,
Hi, On 30/04/2020 14:01, Julien Grall wrote: > On 29/04/2020 21:47, Stefano Stabellini wrote: >> On Wed, 15 Apr 2020, Julien Grall wrote: But doesn't it make sense to >> give domU permission if it is going to get >> the memory mapped? But admittedly I can't think of something that would >> break because of the lack of the iomem_permit_access call in this code >> path. > > On Arm, the permissions are only useful if you plan you DomU to delegate > the regions to another domain. As your domain is not even aware it is > running on Xen (we don't expose 'xen' node in the DT), it makes little > sense to add the permission. I actually found one use when helping a user last week. You can dump the list of MMIO regions assigned to a guest from Xen Console. This will use d->iomem_caps that is modified via iomem_permit_access(). Without it, there is no easy way to confirm the list of MMIO regions assigned to a guest. Although... > Even today, you can map IOMEM to a DomU and then revert the permission > right after. They IOMEM will still be mapped in the guest and it will > act normaly. ... this would not help the case where permissions are reverted. But I am assuming this shouldn't happen for Dom0less. Stefano, I am not sure what's your plan for the series itself for Xen 4.14. I think this patch could go in now. Any thoughts? Cheers,
On Sun, 24 May 2020, Julien Grall wrote: > On 30/04/2020 14:01, Julien Grall wrote: > > On 29/04/2020 21:47, Stefano Stabellini wrote: > > > On Wed, 15 Apr 2020, Julien Grall wrote: But doesn't it make sense to give > > > domU permission if it is going to get > > > the memory mapped? But admittedly I can't think of something that would > > > break because of the lack of the iomem_permit_access call in this code > > > path. > > > > On Arm, the permissions are only useful if you plan you DomU to delegate the > > regions to another domain. As your domain is not even aware it is running on > > Xen (we don't expose 'xen' node in the DT), it makes little sense to add the > > permission. > > I actually found one use when helping a user last week. You can dump the list > of MMIO regions assigned to a guest from Xen Console. > > This will use d->iomem_caps that is modified via iomem_permit_access(). > Without it, there is no easy way to confirm the list of MMIO regions assigned > to a guest. Although... > > > Even today, you can map IOMEM to a DomU and then revert the permission right > > after. They IOMEM will still be mapped in the guest and it will act normaly. > > ... this would not help the case where permissions are reverted. But I am > assuming this shouldn't happen for Dom0less. Thank you for looking into this > Stefano, I am not sure what's your plan for the series itself for Xen 4.14. I > think this patch could go in now. Any thoughts? For the series: I have addresses all comments in my working tree except for the ones on memory allocation (the thread "xen: introduce reserve_heap_pages"). It looks like that part requires a complete rewrite, and it seems that the new code is not trivial to write. So I am thinking of not targeting 4.14. What do you think? Do you think the new code should be "easy" enough that I could target 4.14? For this patch: it is fine to go in now, doesn't have to wait for the series.
Hi Stefano, On 26/05/2020 17:46, Stefano Stabellini wrote: > On Sun, 24 May 2020, Julien Grall wrote: >> On 30/04/2020 14:01, Julien Grall wrote: >>> On 29/04/2020 21:47, Stefano Stabellini wrote: >>>> On Wed, 15 Apr 2020, Julien Grall wrote: But doesn't it make sense to give >>>> domU permission if it is going to get >>>> the memory mapped? But admittedly I can't think of something that would >>>> break because of the lack of the iomem_permit_access call in this code >>>> path. >>> >>> On Arm, the permissions are only useful if you plan you DomU to delegate the >>> regions to another domain. As your domain is not even aware it is running on >>> Xen (we don't expose 'xen' node in the DT), it makes little sense to add the >>> permission. >> >> I actually found one use when helping a user last week. You can dump the list >> of MMIO regions assigned to a guest from Xen Console. >> >> This will use d->iomem_caps that is modified via iomem_permit_access(). >> Without it, there is no easy way to confirm the list of MMIO regions assigned >> to a guest. Although... >> >>> Even today, you can map IOMEM to a DomU and then revert the permission right >>> after. They IOMEM will still be mapped in the guest and it will act normaly. >> >> ... this would not help the case where permissions are reverted. But I am >> assuming this shouldn't happen for Dom0less. > > Thank you for looking into this > > >> Stefano, I am not sure what's your plan for the series itself for Xen 4.14. I >> think this patch could go in now. Any thoughts? > > For the series: I have addresses all comments in my working tree except > for the ones on memory allocation (the thread "xen: introduce > reserve_heap_pages"). It looks like that part requires a complete > rewrite, and it seems that the new code is not trivial to write. So I am > thinking of not targeting 4.14. What do you think? Do you think the new > code should be "easy" enough that I could target 4.14? It may be a stretch with the code freeze on Friday. I would suggest to send it when it is ready and we can either include in Xen 4.14 or as soon as the tree re-open. > > For this patch: it is fine to go in now, doesn't have to wait for the > series. Feel free to add my ack on the patch. Cheers,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d0fc497d07..d3d42eef5d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1846,6 +1846,17 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, return -EINVAL; } + res = iomem_permit_access(kinfo->d, paddr_to_pfn(mstart), + paddr_to_pfn(PAGE_ALIGN(mstart + size - 1))); + if ( res ) + { + printk(XENLOG_ERR "Unable to permit to dom%d access to" + " 0x%"PRIx64" - 0x%"PRIx64"\n", + kinfo->d->domain_id, + mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1); + return res; + } + res = map_regions_p2mt(kinfo->d, gaddr_to_gfn(gstart), PFN_DOWN(size),
iomem_permit_access should be called for MMIO regions of devices assigned to a domain. Currently it is not called for MMIO regions of passthrough devices of Dom0less guests. This patch fixes it. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> --- xen/arch/arm/domain_build.c | 11 +++++++++++ 1 file changed, 11 insertions(+)