| Message ID | 20190507151458.29350-10-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm. | expand |
On Tue, 7 May 2019, Julien Grall wrote: > While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty > bogus as we directly return the MFN passed in parameter. > > Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There > are only 3 callers: > - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU > page-tables when the P2M is not shared with the IOMMU. No issues so > far as Arm does not yet support non-shared P2M case. > - memory_exchange: Arm cannot not use it because steal_page is not > implemented. > - getdomaininfo: Toolstack may map the shared page. It looks like > this is mostly used for mapping the P2M of PV guest. Therefore the > issue might be minor. > > Implementing the M2P on Arm is not planned. The M2P would require significant > amount of VA address (very tough on 32-bit) that can hardly be justified with > the current use of mfn_to_gmfn. > - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the > IOMMU page-tables is delayed until the first device is assigned. > In the embedded case, we will known in most of the times what > devices are assigned during the domain creation. So it is possible > to take to enable the IOMMU from start. See [1] for the patch. > - memory_exchange: This does not work and I haven't seen any > request for it so far. > - getdomaininfo: The structure on Arm does not seem to contain a lot > of useful information on Arm. It is unclear whether we want to > allow the toolstack mapping it on Arm. > > This patch introduces a config option HAS_M2P to tell whether an > architecture implements the M2P. > - iommu_hwdom_init: For now, we require the M2P support when the IOMMU > is not sharing the P2M. > - memory_exchange: The hypercall is marked as not supported when the > M2P does not exist. > - getdomaininfo: A new helper is introduced to wrap the call to > mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping > will fail. > > [1] https://patchwork.kernel.org/patch/9719913/ > > Signed-off-by Julien Grall <julien.grall@arm.com> > > --- > > Cc: oleksandr_tyshchenko@epam.com > Cc: andrii_anisov@epam.com > > Changes in v2: > - Add a warning in public headers > - Constify local variable in domain_shared_info_gfn > - Invert the naming (_d / d) in domain_shared_info_gfn > - Use -EOPNOTSUPP rather than -ENOSYS > - Rework how the memory_exchange hypercall is removed from Arm > --- > xen/arch/x86/Kconfig | 1 + > xen/common/Kconfig | 3 +++ > xen/common/domctl.c | 2 +- > xen/common/memory.c | 4 ++++ > xen/drivers/passthrough/iommu.c | 6 +++++- > xen/include/asm-arm/domain.h | 5 +++++ > xen/include/public/domctl.h | 4 ++++ > xen/include/xen/domain.h | 8 ++++++++ > 8 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 4b8b07b549..52922a87e7 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -16,6 +16,7 @@ config X86 > select HAS_IOPORTS > select HAS_KEXEC > select MEM_ACCESS_ALWAYS_ON > + select HAS_M2P > select HAS_MEM_PAGING > select HAS_MEM_SHARING > select HAS_NS16550 > diff --git a/xen/common/Kconfig b/xen/common/Kconfig > index c838506241..df871adc8f 100644 > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -63,6 +63,9 @@ config HAS_GDBSX > config HAS_IOPORTS > bool > > +config HAS_M2P > + bool > + > config NEEDS_LIBELF > bool > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index bade9a63b1..29940fdea5 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info) > info->outstanding_pages = d->outstanding_pages; > info->shr_pages = atomic_read(&d->shr_pages); > info->paged_pages = atomic_read(&d->paged_pages); > - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); > + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); > BUG_ON(SHARED_M2P(info->shared_info_frame)); > > info->cpupool = d->cpupool ? d->cpupool->cpupool_id : CPUPOOLID_NONE; > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 86567e6117..d6a580da31 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -512,6 +512,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags) > > static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) > { > +#ifdef CONFIG_M2P > struct xen_memory_exchange exch; > PAGE_LIST_HEAD(in_chunk_list); > PAGE_LIST_HEAD(out_chunk_list); > @@ -806,6 +807,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) > if ( __copy_field_to_guest(arg, &exch, nr_exchanged) ) > rc = -EFAULT; > return rc; > +#else /* !CONFIG_M2P */ > + return -EOPNOTSUPP; > +#endif > } > > int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index a6697d58fb..dbb64b13bc 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); > if ( need_iommu_pt_sync(d) ) > { > + int rc = 0; > +#ifdef CONFIG_HAS_M2P > struct page_info *page; > unsigned int i = 0, flush_flags = 0; > - int rc = 0; > > page_list_for_each ( page, &d->page_list ) > { > @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > /* Use while-break to avoid compiler warning */ > while ( iommu_iotlb_flush_all(d, flush_flags) ) > break; > +#else > + rc = -EOPNOTSUPP; > +#endif > > if ( rc ) > printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 312fec8932..d61b0188da 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc) > > static inline void arch_vcpu_block(struct vcpu *v) {} > > +static inline gfn_t domain_shared_info_gfn(struct domain *d) > +{ > + return INVALID_GFN; > +} > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 19486d5e32..cac8ffffe9 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo { > uint64_aligned_t outstanding_pages; > uint64_aligned_t shr_pages; > uint64_aligned_t paged_pages; > + /* > + * GFN of shared_info struct. Some architectures (e.g Arm) may not > + * provide a valid GFN. > + */ > uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */ > uint64_aligned_t cpu_time; > uint32_t nr_online_vcpus; /* Number of VCPUs currently online. */ > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > index d1bfc82f57..f1761fe183 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -118,4 +118,12 @@ struct vnuma_info { > > void vnuma_destroy(struct vnuma_info *vnuma); > > +#ifdef CONFIG_HAS_M2P > +#define domain_shared_info_gfn(d) ({ \ > + const struct domain *d_ = (d); \ > + \ > + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ Aren't you missing a _gfn here? _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)))); > +}) > +#endif > + > #endif /* __XEN_DOMAIN_H__ */
Hi, On 5/9/19 7:06 PM, Stefano Stabellini wrote: > On Tue, 7 May 2019, Julien Grall wrote: >> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h >> index d1bfc82f57..f1761fe183 100644 >> --- a/xen/include/xen/domain.h >> +++ b/xen/include/xen/domain.h >> @@ -118,4 +118,12 @@ struct vnuma_info { >> >> void vnuma_destroy(struct vnuma_info *vnuma); >> >> +#ifdef CONFIG_HAS_M2P >> +#define domain_shared_info_gfn(d) ({ \ >> + const struct domain *d_ = (d); \ >> + \ >> + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ > > Aren't you missing a _gfn here? > > _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)))); Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So the function now return a gfn_t. Cheers,
On Thu, 9 May 2019, Julien Grall wrote: > Hi, > > On 5/9/19 7:06 PM, Stefano Stabellini wrote: > > On Tue, 7 May 2019, Julien Grall wrote: > > > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > > > index d1bfc82f57..f1761fe183 100644 > > > --- a/xen/include/xen/domain.h > > > +++ b/xen/include/xen/domain.h > > > @@ -118,4 +118,12 @@ struct vnuma_info { > > > void vnuma_destroy(struct vnuma_info *vnuma); > > > +#ifdef CONFIG_HAS_M2P > > > +#define domain_shared_info_gfn(d) ({ \ > > > + const struct domain *d_ = (d); \ > > > + \ > > > + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ > > > > Aren't you missing a _gfn here? > > > > _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)))); > > Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So the > function now return a gfn_t. Ah! Somehow I am missing patches 2-3-4 in my inbox. I'll try to get them from the archive.
Hi Stefano, On 5/9/19 7:16 PM, Stefano Stabellini wrote: > On Thu, 9 May 2019, Julien Grall wrote: >> Hi, >> >> On 5/9/19 7:06 PM, Stefano Stabellini wrote: >>> On Tue, 7 May 2019, Julien Grall wrote: >>>> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h >>>> index d1bfc82f57..f1761fe183 100644 >>>> --- a/xen/include/xen/domain.h >>>> +++ b/xen/include/xen/domain.h >>>> @@ -118,4 +118,12 @@ struct vnuma_info { >>>> void vnuma_destroy(struct vnuma_info *vnuma); >>>> +#ifdef CONFIG_HAS_M2P >>>> +#define domain_shared_info_gfn(d) ({ \ >>>> + const struct domain *d_ = (d); \ >>>> + \ >>>> + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ >>> >>> Aren't you missing a _gfn here? >>> >>> _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)))); >> >> Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So the >> function now return a gfn_t. > > Ah! Somehow I am missing patches 2-3-4 in my inbox. I'll try to get them > from the archive. Because they are x86 specific :). The rationale of implementing domain_shared_info_gfn() in common code is any arch using M2P should provide a similar helper. Arm doesn't have an M2P, hence why mfn_to_gfn is not existent. Cheers,
On Thu, 9 May 2019, Julien Grall wrote: > Hi Stefano, > > On 5/9/19 7:16 PM, Stefano Stabellini wrote: > > On Thu, 9 May 2019, Julien Grall wrote: > > > Hi, > > > > > > On 5/9/19 7:06 PM, Stefano Stabellini wrote: > > > > On Tue, 7 May 2019, Julien Grall wrote: > > > > > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h > > > > > index d1bfc82f57..f1761fe183 100644 > > > > > --- a/xen/include/xen/domain.h > > > > > +++ b/xen/include/xen/domain.h > > > > > @@ -118,4 +118,12 @@ struct vnuma_info { > > > > > void vnuma_destroy(struct vnuma_info *vnuma); > > > > > +#ifdef CONFIG_HAS_M2P > > > > > +#define domain_shared_info_gfn(d) ({ \ > > > > > + const struct domain *d_ = (d); \ > > > > > + \ > > > > > + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ > > > > > > > > Aren't you missing a _gfn here? > > > > > > > > _gfn(mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info)))); > > > > > > Patch #3 of this series convert mfn_to_gfn to use typesafe MFN & GFN. So > > > the > > > function now return a gfn_t. > > > > Ah! Somehow I am missing patches 2-3-4 in my inbox. I'll try to get them > > from the archive. > > Because they are x86 specific :). The rationale of implementing > domain_shared_info_gfn() in common code is any arch using M2P should provide a > similar helper. > > Arm doesn't have an M2P, hence why mfn_to_gfn is not existent. All right. Add my acked-by to this patch.
>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: > This patch introduces a config option HAS_M2P to tell whether an > architecture implements the M2P. > - iommu_hwdom_init: For now, we require the M2P support when the IOMMU > is not sharing the P2M. > - memory_exchange: The hypercall is marked as not supported when the > M2P does not exist. Was it you or someone else to suggest it be restricted to non-translated guests in the first place? I'd prefer this over the #ifdef-ary you add. > - getdomaininfo: A new helper is introduced to wrap the call to > mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping > will fail. There's no use of mfn_to_gmfn() in either of the wrappers, so why mention this to-be-removed one? > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct > xen_domctl_getdomaininfo *info) > info->outstanding_pages = d->outstanding_pages; > info->shr_pages = atomic_read(&d->shr_pages); > info->paged_pages = atomic_read(&d->paged_pages); > - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); > + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); What is the intended behavior on 32-bit Arm here? Do you really mean to return a value with 32 bits of ones (instead of 64 bits of them) in this 64-bit field? > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); > if ( need_iommu_pt_sync(d) ) > { > + int rc = 0; > +#ifdef CONFIG_HAS_M2P > struct page_info *page; > unsigned int i = 0, flush_flags = 0; > - int rc = 0; > > page_list_for_each ( page, &d->page_list ) > { > @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > /* Use while-break to avoid compiler warning */ > while ( iommu_iotlb_flush_all(d, flush_flags) ) > break; > +#else > + rc = -EOPNOTSUPP; > +#endif > > if ( rc ) > printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", Would you mind extending the scope of the #ifdef beyond this printk()? It seems pretty pointless to me to unconditionally emit a log message here. > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo { > uint64_aligned_t outstanding_pages; > uint64_aligned_t shr_pages; > uint64_aligned_t paged_pages; > + /* > + * GFN of shared_info struct. Some architectures (e.g Arm) may not > + * provide a valid GFN. > + */ Along the lines of what I've said above, I think you want to spell out here what the value is going to be in this case. > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -118,4 +118,12 @@ struct vnuma_info { > > void vnuma_destroy(struct vnuma_info *vnuma); > > +#ifdef CONFIG_HAS_M2P > +#define domain_shared_info_gfn(d) ({ \ > + const struct domain *d_ = (d); \ > + \ > + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ > +}) > +#endif And an inline function doesn't work here? Jan
Hi, On 10/05/2019 13:31, Jan Beulich wrote: >>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >> This patch introduces a config option HAS_M2P to tell whether an >> architecture implements the M2P. >> - iommu_hwdom_init: For now, we require the M2P support when the IOMMU >> is not sharing the P2M. >> - memory_exchange: The hypercall is marked as not supported when the >> M2P does not exist. > > Was it you or someone else to suggest it be restricted to non-translated > guests in the first place? I'd prefer this over the #ifdef-ary you add. I never suggested that as I have no idea who is using it on x86. But then, we would still need to implement mfn_to_gfn on Arm to make it compile. I really want to avoid such macro on Arm. > >> - getdomaininfo: A new helper is introduced to wrap the call to >> mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping >> will fail. > > There's no use of mfn_to_gmfn() in either of the wrappers, so why > mention this to-be-removed one? Because code has been changed over the revision and I forgot to update the commit message. > >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct >> xen_domctl_getdomaininfo *info) >> info->outstanding_pages = d->outstanding_pages; >> info->shr_pages = atomic_read(&d->shr_pages); >> info->paged_pages = atomic_read(&d->paged_pages); >> - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); >> + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); > > What is the intended behavior on 32-bit Arm here? Do you really > mean to return a value with 32 bits of ones (instead of 64 bits of > them) in this 64-bit field? It does not matter as long as the GFN is invalid so it can't be mapped afterwards. The exact value is not documented in the header to avoid any assumption. > >> --- a/xen/drivers/passthrough/iommu.c >> +++ b/xen/drivers/passthrough/iommu.c >> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >> hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); >> if ( need_iommu_pt_sync(d) ) >> { >> + int rc = 0; >> +#ifdef CONFIG_HAS_M2P >> struct page_info *page; >> unsigned int i = 0, flush_flags = 0; >> - int rc = 0; >> >> page_list_for_each ( page, &d->page_list ) >> { >> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >> /* Use while-break to avoid compiler warning */ >> while ( iommu_iotlb_flush_all(d, flush_flags) ) >> break; >> +#else >> + rc = -EOPNOTSUPP; >> +#endif >> >> if ( rc ) >> printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", > > Would you mind extending the scope of the #ifdef beyond this printk()? > It seems pretty pointless to me to unconditionally emit a log message > here. Well, it at least tell you the function can't work. So I think it is still makes sense to have it. > >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo { >> uint64_aligned_t outstanding_pages; >> uint64_aligned_t shr_pages; >> uint64_aligned_t paged_pages; >> + /* >> + * GFN of shared_info struct. Some architectures (e.g Arm) may not >> + * provide a valid GFN. >> + */ > > Along the lines of what I've said above, I think you want to spell out > here what the value is going to be in this case. Spelling out the exact value gives us less freedom. What matters here is the GFN return can't be mapped. > >> --- a/xen/include/xen/domain.h >> +++ b/xen/include/xen/domain.h >> @@ -118,4 +118,12 @@ struct vnuma_info { >> >> void vnuma_destroy(struct vnuma_info *vnuma); >> >> +#ifdef CONFIG_HAS_M2P >> +#define domain_shared_info_gfn(d) ({ \ >> + const struct domain *d_ = (d); \ >> + \ >> + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ >> +}) >> +#endif > > And an inline function doesn't work here? With enough time to rework the headers maybe. At the moment, no because mfn_to_gfn is implemented in p2m.h which depends on domain.h for the definition of struct domain d: In file included from /home/julieng/works/xen/xen/include/xen/sched.h:11:0, from x86_64/asm-offsets.c:9: /home/julieng/works/xen/xen/include/xen/domain.h: In function ‘domain_shared_info_gfn’: /home/julieng/works/xen/xen/include/xen/domain.h:124:12: error: implicit declaration of function ‘mfn_to_gfn’ [-Werror=implicit-function-declaration] return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))); ^~~~~~~~~~ /home/julieng/works/xen/xen/include/xen/domain.h:124:5: error: nested extern declaration of ‘mfn_to_gfn’ [-Werror=nested-externs] return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))); ^~~~~~ In file included from /home/julieng/works/xen/xen/include/asm/current.h:12:0, from /home/julieng/works/xen/xen/include/asm/smp.h:10, from /home/julieng/works/xen/xen/include/xen/smp.h:4, from /home/julieng/works/xen/xen/include/asm/processor.h:10, from /home/julieng/works/xen/xen/include/asm/system.h:6, from /home/julieng/works/xen/xen/include/xen/spinlock.h:4, from /home/julieng/works/xen/xen/include/xen/sched.h:6, from x86_64/asm-offsets.c:9: /home/julieng/works/xen/xen/include/xen/domain.h:124:46: error: dereferencing pointer to incomplete type ‘struct domain’ return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))); ^ /home/julieng/works/xen/xen/include/asm/page.h:265:61: note: in definition of macro ‘virt_to_maddr’ #define virt_to_maddr(va) __virt_to_maddr((unsigned long)(va)) ^~ /home/julieng/works/xen/xen/include/xen/domain.h:124:31: note: in expansion of macro ‘__virt_to_mfn’ return mfn_to_gfn(d, _mfn(__virt_to_mfn(d->shared_info))); ^~~~~~~~~~~~~ cc1: all warnings being treated as errors Makefile:217: recipe for target 'asm-offsets.s' failed make[2]: *** [asm-offsets.s] Error 1 make[2]: Leaving directory '/home/julieng/works/xen/xen/arch/x86' Makefile:136: recipe for target '/home/julieng/works/xen/xen/xen' failed make[1]: *** [/home/julieng/works/xen/xen/xen] Error 2 make[1]: Leaving directory '/home/julieng/works/xen/xen' Makefile:45: recipe for target 'build' failed make: *** [build] Error 2 Cheers,
>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote: > On 10/05/2019 13:31, Jan Beulich wrote: >>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>> --- a/xen/common/domctl.c >>> +++ b/xen/common/domctl.c >>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct >>> xen_domctl_getdomaininfo *info) >>> info->outstanding_pages = d->outstanding_pages; >>> info->shr_pages = atomic_read(&d->shr_pages); >>> info->paged_pages = atomic_read(&d->paged_pages); >>> - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); >>> + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); >> >> What is the intended behavior on 32-bit Arm here? Do you really >> mean to return a value with 32 bits of ones (instead of 64 bits of >> them) in this 64-bit field? > It does not matter as long as the GFN is invalid so it can't be mapped > afterwards. The exact value is not documented in the header to avoid any > assumption. That's not helpful - how would a consumer know to avoid the mapping attempt in the first place? >>> --- a/xen/drivers/passthrough/iommu.c >>> +++ b/xen/drivers/passthrough/iommu.c >>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>> hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); >>> if ( need_iommu_pt_sync(d) ) >>> { >>> + int rc = 0; >>> +#ifdef CONFIG_HAS_M2P >>> struct page_info *page; >>> unsigned int i = 0, flush_flags = 0; >>> - int rc = 0; >>> >>> page_list_for_each ( page, &d->page_list ) >>> { >>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>> /* Use while-break to avoid compiler warning */ >>> while ( iommu_iotlb_flush_all(d, flush_flags) ) >>> break; >>> +#else >>> + rc = -EOPNOTSUPP; >>> +#endif >>> >>> if ( rc ) >>> printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", >> >> Would you mind extending the scope of the #ifdef beyond this printk()? >> It seems pretty pointless to me to unconditionally emit a log message >> here. > > Well, it at least tell you the function can't work. So I think it is still makes > sense to have it. I disagree. Jan
Hi Jan, On 10/05/2019 14:32, Jan Beulich wrote: >>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote: >> On 10/05/2019 13:31, Jan Beulich wrote: >>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>>> --- a/xen/common/domctl.c >>>> +++ b/xen/common/domctl.c >>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct >>>> xen_domctl_getdomaininfo *info) >>>> info->outstanding_pages = d->outstanding_pages; >>>> info->shr_pages = atomic_read(&d->shr_pages); >>>> info->paged_pages = atomic_read(&d->paged_pages); >>>> - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); >>>> + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); >>> >>> What is the intended behavior on 32-bit Arm here? Do you really >>> mean to return a value with 32 bits of ones (instead of 64 bits of >>> them) in this 64-bit field? >> It does not matter as long as the GFN is invalid so it can't be mapped >> afterwards. The exact value is not documented in the header to avoid any >> assumption. > > That's not helpful - how would a consumer know to avoid the mapping > attempt in the first place? I can't see any issue with the consumer to try to map it. Ok, you will waste a couple of cycles, but that should be pretty rare. The point here, we keep within the hypervisor the idea of what's valid or invalid. This allows us more flexibility on the value here (imagine we decide to change the value of GFN_INVALID in the future...). > >>>> --- a/xen/drivers/passthrough/iommu.c >>>> +++ b/xen/drivers/passthrough/iommu.c >>>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>>> hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); >>>> if ( need_iommu_pt_sync(d) ) >>>> { >>>> + int rc = 0; >>>> +#ifdef CONFIG_HAS_M2P >>>> struct page_info *page; >>>> unsigned int i = 0, flush_flags = 0; >>>> - int rc = 0; >>>> >>>> page_list_for_each ( page, &d->page_list ) >>>> { >>>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>>> /* Use while-break to avoid compiler warning */ >>>> while ( iommu_iotlb_flush_all(d, flush_flags) ) >>>> break; >>>> +#else >>>> + rc = -EOPNOTSUPP; >>>> +#endif >>>> >>>> if ( rc ) >>>> printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", >>> >>> Would you mind extending the scope of the #ifdef beyond this printk()? >>> It seems pretty pointless to me to unconditionally emit a log message >>> here. >> >> Well, it at least tell you the function can't work. So I think it is still makes >> sense to have it. > > I disagree. You disagree because...? I hope you are aware, this is unlikely going to be printed as the code should not be called. If it ever happens, it is easier for a user to grep the code for the message rather than having to add some to find out where the -EOPNOTSUPP is coming from. Cheers,
>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote: > On 10/05/2019 14:32, Jan Beulich wrote: >>>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote: >>> On 10/05/2019 13:31, Jan Beulich wrote: >>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>>>> --- a/xen/common/domctl.c >>>>> +++ b/xen/common/domctl.c >>>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct >>>>> xen_domctl_getdomaininfo *info) >>>>> info->outstanding_pages = d->outstanding_pages; >>>>> info->shr_pages = atomic_read(&d->shr_pages); >>>>> info->paged_pages = atomic_read(&d->paged_pages); >>>>> - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); >>>>> + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); >>>> >>>> What is the intended behavior on 32-bit Arm here? Do you really >>>> mean to return a value with 32 bits of ones (instead of 64 bits of >>>> them) in this 64-bit field? >>> It does not matter as long as the GFN is invalid so it can't be mapped >>> afterwards. The exact value is not documented in the header to avoid any >>> assumption. >> >> That's not helpful - how would a consumer know to avoid the mapping >> attempt in the first place? > > I can't see any issue with the consumer to try to map it. Ok, you will waste a > couple of cycles, but that should be pretty rare. The attempt may result in a log message spit out. > The point here, we keep within the hypervisor the idea of what's valid or > invalid. This allows us more flexibility on the value here (imagine we decide to > change the value of GFN_INVALID in the future...). Exactly, and hence INVALID_GFN should not become visible to the outside. Hence my request to use an all-ones value here. >>>>> --- a/xen/drivers/passthrough/iommu.c >>>>> +++ b/xen/drivers/passthrough/iommu.c >>>>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>>>> hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); >>>>> if ( need_iommu_pt_sync(d) ) >>>>> { >>>>> + int rc = 0; >>>>> +#ifdef CONFIG_HAS_M2P >>>>> struct page_info *page; >>>>> unsigned int i = 0, flush_flags = 0; >>>>> - int rc = 0; >>>>> >>>>> page_list_for_each ( page, &d->page_list ) >>>>> { >>>>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>>>> /* Use while-break to avoid compiler warning */ >>>>> while ( iommu_iotlb_flush_all(d, flush_flags) ) >>>>> break; >>>>> +#else >>>>> + rc = -EOPNOTSUPP; >>>>> +#endif >>>>> >>>>> if ( rc ) >>>>> printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", >>>> >>>> Would you mind extending the scope of the #ifdef beyond this printk()? >>>> It seems pretty pointless to me to unconditionally emit a log message >>>> here. >>> >>> Well, it at least tell you the function can't work. So I think it is still makes >>> sense to have it. >> >> I disagree. > You disagree because...? Because of what I've said in my initial reply (still quoted above). > I hope you are aware, this is unlikely going to be printed as the code should > not be called. ASSERT_UNREACHABLE() then? Jan
Hi Jan, On 10/05/2019 14:45, Jan Beulich wrote: >>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote: >> On 10/05/2019 14:32, Jan Beulich wrote: >>>>>> On 10.05.19 at 15:22, <julien.grall@arm.com> wrote: >>>> On 10/05/2019 13:31, Jan Beulich wrote: >>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>>>>> --- a/xen/common/domctl.c >>>>>> +++ b/xen/common/domctl.c >>>>>> @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct >>>>>> xen_domctl_getdomaininfo *info) >>>>>> info->outstanding_pages = d->outstanding_pages; >>>>>> info->shr_pages = atomic_read(&d->shr_pages); >>>>>> info->paged_pages = atomic_read(&d->paged_pages); >>>>>> - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); >>>>>> + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); >>>>> >>>>> What is the intended behavior on 32-bit Arm here? Do you really >>>>> mean to return a value with 32 bits of ones (instead of 64 bits of >>>>> them) in this 64-bit field? >>>> It does not matter as long as the GFN is invalid so it can't be mapped >>>> afterwards. The exact value is not documented in the header to avoid any >>>> assumption. >>> >>> That's not helpful - how would a consumer know to avoid the mapping >>> attempt in the first place? >> >> I can't see any issue with the consumer to try to map it. Ok, you will waste a >> couple of cycles, but that should be pretty rare. > > The attempt may result in a log message spit out. I still can't see the issue here. This is nothing different than the frame were not setup beforehand. > >> The point here, we keep within the hypervisor the idea of what's valid or >> invalid. This allows us more flexibility on the value here (imagine we decide to >> change the value of GFN_INVALID in the future...). > > Exactly, and hence INVALID_GFN should not become visible to the > outside. Hence my request to use an all-ones value here. It is only visible if you put an exact value in the documentation. Your suggestion is to put a exactly value and I would rather not see that. >>>>>> --- a/xen/drivers/passthrough/iommu.c >>>>>> +++ b/xen/drivers/passthrough/iommu.c >>>>>> @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>>>>> hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); >>>>>> if ( need_iommu_pt_sync(d) ) >>>>>> { >>>>>> + int rc = 0; >>>>>> +#ifdef CONFIG_HAS_M2P >>>>>> struct page_info *page; >>>>>> unsigned int i = 0, flush_flags = 0; >>>>>> - int rc = 0; >>>>>> >>>>>> page_list_for_each ( page, &d->page_list ) >>>>>> { >>>>>> @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >>>>>> /* Use while-break to avoid compiler warning */ >>>>>> while ( iommu_iotlb_flush_all(d, flush_flags) ) >>>>>> break; >>>>>> +#else >>>>>> + rc = -EOPNOTSUPP; >>>>>> +#endif >>>>>> >>>>>> if ( rc ) >>>>>> printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", >>>>> >>>>> Would you mind extending the scope of the #ifdef beyond this printk()? >>>>> It seems pretty pointless to me to unconditionally emit a log message >>>>> here. >>>> >>>> Well, it at least tell you the function can't work. So I think it is still makes >>>> sense to have it. >>> >>> I disagree. >> You disagree because...? > > Because of what I've said in my initial reply (still quoted above). I still don't see the problem of unconditional log message. It is not really the first place we have that. > >> I hope you are aware, this is unlikely going to be printed as the code should >> not be called. > > ASSERT_UNREACHABLE() then? And still avoiding the printk? Cheers,
>>> On 10.05.19 at 16:04, <julien.grall@arm.com> wrote: > On 10/05/2019 14:45, Jan Beulich wrote: >>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote: >>> The point here, we keep within the hypervisor the idea of what's valid or >>> invalid. This allows us more flexibility on the value here (imagine we decide to >>> change the value of GFN_INVALID in the future...). >> >> Exactly, and hence INVALID_GFN should not become visible to the >> outside. Hence my request to use an all-ones value here. > It is only visible if you put an exact value in the documentation. Your > suggestion is to put a exactly value and I would rather not see that. I did specifically suggest to _not_ store INVALID_GFN here, but to store 64-bit bits of ones. Note the difference between the two on 32-bit Arm. >>>>> Well, it at least tell you the function can't work. So I think it is still makes >>>>> sense to have it. >>>> >>>> I disagree. >>> You disagree because...? >> >> Because of what I've said in my initial reply (still quoted above). > > I still don't see the problem of unconditional log message. It is not really the > first place we have that. > >> >>> I hope you are aware, this is unlikely going to be printed as the code should >>> not be called. >> >> ASSERT_UNREACHABLE() then? > > And still avoiding the printk? Preferably yes; depends on how exactly you code the assertion. If you follow the if()-ASSERT_UNREACHABLE()-return style we've been using elsewhere, then no matter how you place the #else or #endif the printk() will be compiled out. Jan
Hi Jan, On 10/05/2019 15:19, Jan Beulich wrote: >>>> On 10.05.19 at 16:04, <julien.grall@arm.com> wrote: >> On 10/05/2019 14:45, Jan Beulich wrote: >>>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote: >>>> The point here, we keep within the hypervisor the idea of what's valid or >>>> invalid. This allows us more flexibility on the value here (imagine we decide to >>>> change the value of GFN_INVALID in the future...). >>> >>> Exactly, and hence INVALID_GFN should not become visible to the >>> outside. Hence my request to use an all-ones value here. >> It is only visible if you put an exact value in the documentation. Your >> suggestion is to put a exactly value and I would rather not see that. > > I did specifically suggest to _not_ store INVALID_GFN here, but to > store 64-bit bits of ones. Note the difference between the two on > 32-bit Arm. Your point of having an exact value is only useful if you want to toolstack to silently ignore the missing frame and avoid a call. The former is pretty much wrong as if you were trying to read the frame then most likely you wanted to access it. So a message makes sense here. For the latter, avoiding the call is only going to save you a couple of cycles in a likely cold path. You really don't need to give an exact (including say all ones). You only need to say that the address return may not be mappable. The toolstack will try to map it and fail. That's not a big deal. Anyway, I will wait and see what's the view from the tools maintainer. > > Preferably yes; depends on how exactly you code the assertion. > If you follow the if()-ASSERT_UNREACHABLE()-return style we've > been using elsewhere, then no matter how you place the #else > or #endif the printk() will be compiled out. I will have a look. Cheers,
Hi, Answering to myself. On 10/05/2019 15:34, Julien Grall wrote: > On 10/05/2019 15:19, Jan Beulich wrote: >>>>> On 10.05.19 at 16:04, <julien.grall@arm.com> wrote: >>> On 10/05/2019 14:45, Jan Beulich wrote: >>>>>>> On 10.05.19 at 15:41, <julien.grall@arm.com> wrote: >>>>> The point here, we keep within the hypervisor the idea of what's valid or >>>>> invalid. This allows us more flexibility on the value here (imagine we >>>>> decide to >>>>> change the value of GFN_INVALID in the future...). >>>> >>>> Exactly, and hence INVALID_GFN should not become visible to the >>>> outside. Hence my request to use an all-ones value here. >>> It is only visible if you put an exact value in the documentation. Your >>> suggestion is to put a exactly value and I would rather not see that. >> >> I did specifically suggest to _not_ store INVALID_GFN here, but to >> store 64-bit bits of ones. Note the difference between the two on >> 32-bit Arm. > Your point of having an exact value is only useful if you want to toolstack to > silently ignore the missing frame and avoid a call. > > The former is pretty much wrong as if you were trying to read the frame then > most likely you wanted to access it. So a message makes sense here. > > For the latter, avoiding the call is only going to save you a couple of cycles > in a likely cold path. > > You really don't need to give an exact (including say all ones). You only need > to say that the address return may not be mappable. The toolstack will try to > map it and fail. That's not a big deal. > > Anyway, I will wait and see what's the view from the tools maintainer. I had a discussion with Ian on IRC regarding the value here. After some debate we agreed that specifying a single value would be best. At the moment, Xen only supports 48-bits address that could be covered by 36-bit MFN. Newer Arm revision supports up to 52-bit address, but that's only with 64KB page granularity. Even if we were supporting 64-bit address, it would only cover 48-bit, so all ones should still be invalid. So I am happy with the all ones version. I will introduce a define so the tools can use it rather than an hardcoded value. Cheers,
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 4b8b07b549..52922a87e7 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -16,6 +16,7 @@ config X86 select HAS_IOPORTS select HAS_KEXEC select MEM_ACCESS_ALWAYS_ON + select HAS_M2P select HAS_MEM_PAGING select HAS_MEM_SHARING select HAS_NS16550 diff --git a/xen/common/Kconfig b/xen/common/Kconfig index c838506241..df871adc8f 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -63,6 +63,9 @@ config HAS_GDBSX config HAS_IOPORTS bool +config HAS_M2P + bool + config NEEDS_LIBELF bool diff --git a/xen/common/domctl.c b/xen/common/domctl.c index bade9a63b1..29940fdea5 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -205,7 +205,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info) info->outstanding_pages = d->outstanding_pages; info->shr_pages = atomic_read(&d->shr_pages); info->paged_pages = atomic_read(&d->paged_pages); - info->shared_info_frame = mfn_to_gmfn(d, virt_to_mfn(d->shared_info)); + info->shared_info_frame = gfn_x(domain_shared_info_gfn(d)); BUG_ON(SHARED_M2P(info->shared_info_frame)); info->cpupool = d->cpupool ? d->cpupool->cpupool_id : CPUPOOLID_NONE; diff --git a/xen/common/memory.c b/xen/common/memory.c index 86567e6117..d6a580da31 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -512,6 +512,7 @@ static bool propagate_node(unsigned int xmf, unsigned int *memflags) static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) { +#ifdef CONFIG_M2P struct xen_memory_exchange exch; PAGE_LIST_HEAD(in_chunk_list); PAGE_LIST_HEAD(out_chunk_list); @@ -806,6 +807,9 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg) if ( __copy_field_to_guest(arg, &exch, nr_exchanged) ) rc = -EFAULT; return rc; +#else /* !CONFIG_M2P */ + return -EOPNOTSUPP; +#endif } int xenmem_add_to_physmap(struct domain *d, struct xen_add_to_physmap *xatp, diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index a6697d58fb..dbb64b13bc 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -188,9 +188,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d); if ( need_iommu_pt_sync(d) ) { + int rc = 0; +#ifdef CONFIG_HAS_M2P struct page_info *page; unsigned int i = 0, flush_flags = 0; - int rc = 0; page_list_for_each ( page, &d->page_list ) { @@ -217,6 +218,9 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) /* Use while-break to avoid compiler warning */ while ( iommu_iotlb_flush_all(d, flush_flags) ) break; +#else + rc = -EOPNOTSUPP; +#endif if ( rc ) printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 312fec8932..d61b0188da 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -267,6 +267,11 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc) static inline void arch_vcpu_block(struct vcpu *v) {} +static inline gfn_t domain_shared_info_gfn(struct domain *d) +{ + return INVALID_GFN; +} + #endif /* __ASM_DOMAIN_H__ */ /* diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h index 19486d5e32..cac8ffffe9 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -118,6 +118,10 @@ struct xen_domctl_getdomaininfo { uint64_aligned_t outstanding_pages; uint64_aligned_t shr_pages; uint64_aligned_t paged_pages; + /* + * GFN of shared_info struct. Some architectures (e.g Arm) may not + * provide a valid GFN. + */ uint64_aligned_t shared_info_frame; /* GMFN of shared_info struct */ uint64_aligned_t cpu_time; uint32_t nr_online_vcpus; /* Number of VCPUs currently online. */ diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index d1bfc82f57..f1761fe183 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -118,4 +118,12 @@ struct vnuma_info { void vnuma_destroy(struct vnuma_info *vnuma); +#ifdef CONFIG_HAS_M2P +#define domain_shared_info_gfn(d) ({ \ + const struct domain *d_ = (d); \ + \ + mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info))); \ +}) +#endif + #endif /* __XEN_DOMAIN_H__ */
While Arm never had a M2P, the implementation of mfn_to_gmfn is pretty bogus as we directly return the MFN passed in parameter. Thankfully, the use of mfn_to_gmfn is pretty limited on Arm today. There are only 3 callers: - iommu_hwdom_init: mfn_to_gmfn is used for creating IOMMU page-tables when the P2M is not shared with the IOMMU. No issues so far as Arm does not yet support non-shared P2M case. - memory_exchange: Arm cannot not use it because steal_page is not implemented. - getdomaininfo: Toolstack may map the shared page. It looks like this is mostly used for mapping the P2M of PV guest. Therefore the issue might be minor. Implementing the M2P on Arm is not planned. The M2P would require significant amount of VA address (very tough on 32-bit) that can hardly be justified with the current use of mfn_to_gmfn. - iommu_hwdom_init: mfn_to_gmfn is used because the creating of the IOMMU page-tables is delayed until the first device is assigned. In the embedded case, we will known in most of the times what devices are assigned during the domain creation. So it is possible to take to enable the IOMMU from start. See [1] for the patch. - memory_exchange: This does not work and I haven't seen any request for it so far. - getdomaininfo: The structure on Arm does not seem to contain a lot of useful information on Arm. It is unclear whether we want to allow the toolstack mapping it on Arm. This patch introduces a config option HAS_M2P to tell whether an architecture implements the M2P. - iommu_hwdom_init: For now, we require the M2P support when the IOMMU is not sharing the P2M. - memory_exchange: The hypercall is marked as not supported when the M2P does not exist. - getdomaininfo: A new helper is introduced to wrap the call to mfn_to_gfn/mfn_to_gmfn. For Arm, it returns an invalid GFN so the mapping will fail. [1] https://patchwork.kernel.org/patch/9719913/ Signed-off-by Julien Grall <julien.grall@arm.com> --- Cc: oleksandr_tyshchenko@epam.com Cc: andrii_anisov@epam.com Changes in v2: - Add a warning in public headers - Constify local variable in domain_shared_info_gfn - Invert the naming (_d / d) in domain_shared_info_gfn - Use -EOPNOTSUPP rather than -ENOSYS - Rework how the memory_exchange hypercall is removed from Arm --- xen/arch/x86/Kconfig | 1 + xen/common/Kconfig | 3 +++ xen/common/domctl.c | 2 +- xen/common/memory.c | 4 ++++ xen/drivers/passthrough/iommu.c | 6 +++++- xen/include/asm-arm/domain.h | 5 +++++ xen/include/public/domctl.h | 4 ++++ xen/include/xen/domain.h | 8 ++++++++ 8 files changed, 31 insertions(+), 2 deletions(-)