| Message ID | 20190603160350.29806-8-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm | expand |
>>> On 03.06.19 at 18:03, <julien.grall@arm.com> 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. But where's the connection between there being M2P and the availability of this operation? I think I've suggested so before: Why don't we simply disable this operation for translated guests (in an independent patch)? > - getdomaininfo: A new helper is introduced to wrap the call to > mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will > fail on mapping if used. This reads slightly stale now. > --- 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 ) > { > @@ -221,6 +222,11 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > if ( rc ) > printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", > d->domain_id, rc); > +#else > + ASSERT_UNREACHABLE(); > + rc = -EOPNOTSUPP; > +#endif > + > } Please don't add a blank line ahead of a closing brace. Jan
Hi Jan, On 6/4/19 5:14 PM, Jan Beulich wrote: >>>> On 03.06.19 at 18:03, <julien.grall@arm.com> 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. > > But where's the connection between there being M2P and the > availability of this operation? I think I've suggested so before: > Why don't we simply disable this operation for translated > guests (in an independent patch)? And I answered that mfn_to_gmfn() is used in the function. I really don't want to implement the macro on Arm (even if it is dummy). You haven't answered back to that comment and I assumed this was fine with you... > >> - getdomaininfo: A new helper is introduced to wrap the call to >> mfn_to_gfn/mfn_to_gmfn. For Arm, a fixed value will be provided that will >> fail on mapping if used. > > This reads slightly stale now. ok. > >> --- 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 ) >> { >> @@ -221,6 +222,11 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) >> if ( rc ) >> printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", >> d->domain_id, rc); >> +#else >> + ASSERT_UNREACHABLE(); >> + rc = -EOPNOTSUPP; >> +#endif >> + >> } > > Please don't add a blank line ahead of a closing brace. ok. Cheers,
>>> On 04.06.19 at 18:22, <julien.grall@arm.com> wrote: > On 6/4/19 5:14 PM, Jan Beulich wrote: >>>>> On 03.06.19 at 18:03, <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. >> >> But where's the connection between there being M2P and the >> availability of this operation? I think I've suggested so before: >> Why don't we simply disable this operation for translated >> guests (in an independent patch)? > > And I answered that mfn_to_gmfn() is used in the function. I really > don't want to implement the macro on Arm (even if it is dummy). > > You haven't answered back to that comment and I assumed this was fine > with you... Well, I guess it was, but supplying the "why" in the description (or attached as a brief comment to the #ifdef) would have helped avoid re-raising the same question. However, thinking about it again I'm not sure I agree with #ifdef-ing out the entire (large) function body - I'd really prefer the alternative approach suggested above. Or otherwise I'd see yet another separate Kconfig option identifying whether an arch supports non-translated mode in the first place. That option would select the M2P one then, as I can't see how one would go about supporting non-translated guests without M2P. In this case you'd add an #ifdef here (placement subject to further discussion; personally I'd favor if it was put around just the problematic invocation of mfn_to_gmfn(), with a suitable #else; alternatively have common code provide a stub mfn_to_gmfn()) _and_ a paging_mode_translate() check near the top of the function, thus yielding consistent behavior. I find it odd that no such check is there right now, as the public header clearly says this is a PV-only interface. Note that with a paging_mode_translate() check at the top of the function (or even at its only call site) the one towards the end of the function should then be ditched. Jan
Hi, On 05/06/2019 08:14, Jan Beulich wrote: >>>> On 04.06.19 at 18:22, <julien.grall@arm.com> wrote: >> On 6/4/19 5:14 PM, Jan Beulich wrote: >>>>>> On 03.06.19 at 18:03, <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. >>> >>> But where's the connection between there being M2P and the >>> availability of this operation? I think I've suggested so before: >>> Why don't we simply disable this operation for translated >>> guests (in an independent patch)? >> >> And I answered that mfn_to_gmfn() is used in the function. I really >> don't want to implement the macro on Arm (even if it is dummy). >> >> You haven't answered back to that comment and I assumed this was fine >> with you... > > Well, I guess it was, but supplying the "why" in the description (or > attached as a brief comment to the #ifdef) would have helped > avoid re-raising the same question. However, thinking about it > again I'm not sure I agree with #ifdef-ing out the entire (large) > function body - I'd really prefer the alternative approach > suggested above. I would appreciate if you clarified it in the previous version rather than expecting me to understand the hidden meaning. Because ... > > Or otherwise I'd see yet another separate Kconfig option > identifying whether an arch supports non-translated mode in the > first place. That option would select the M2P one then, as I can't > see how one would go about supporting non-translated guests > without M2P. In this case you'd add an #ifdef here (placement > subject to further discussion; personally I'd favor if it was put > around just the problematic invocation of mfn_to_gmfn(), with > a suitable #else; alternatively have common code provide a > stub mfn_to_gmfn()) _and_ a paging_mode_translate() check > near the top of the function, thus yielding consistent behavior. ... I would have avoided to waste time resending the series without addressing your concern. In this case, I will strongly nack any definition of mfn_to_gmfn() in the non-M2P case. We saw the implication on Arm and I just want to see the macro completely disappear from Arm. Furthermore, #ifdef chunk of code means they completely disappear from the code base without the aid of the compiler itself (via optimization). This is less error-prone than a potentially wrong check and would also help safety certification as we don't need. So I still favor using #ifdef here. I could replace by a specific Kconfig (see a previous attempt [1]). > > I find it odd that no such check is there right now, as the public > header clearly says this is a PV-only interface. > > Note that with a paging_mode_translate() check at the top of > the function (or even at its only call site) the one towards the > end of the function should then be ditched. See above my opinion on paging_mode_translate(). Cheers, [1] https://lists.gt.net/xen/devel/411530 > > Jan > >
>>> On 05.06.19 at 11:35, <julien.grall@arm.com> wrote: > On 05/06/2019 08:14, Jan Beulich wrote: >>>>> On 04.06.19 at 18:22, <julien.grall@arm.com> wrote: >>> On 6/4/19 5:14 PM, Jan Beulich wrote: >>>>>>> On 03.06.19 at 18:03, <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. >>>> >>>> But where's the connection between there being M2P and the >>>> availability of this operation? I think I've suggested so before: >>>> Why don't we simply disable this operation for translated >>>> guests (in an independent patch)? >>> >>> And I answered that mfn_to_gmfn() is used in the function. I really >>> don't want to implement the macro on Arm (even if it is dummy). >>> >>> You haven't answered back to that comment and I assumed this was fine >>> with you... >> >> Well, I guess it was, but supplying the "why" in the description (or >> attached as a brief comment to the #ifdef) would have helped >> avoid re-raising the same question. However, thinking about it >> again I'm not sure I agree with #ifdef-ing out the entire (large) >> function body - I'd really prefer the alternative approach >> suggested above. > > I would appreciate if you clarified it in the previous version rather than > expecting me to understand the hidden meaning. Because ... I'm sorry for this, but "thinking about this again" means I was coming to that conclusion now, not back then. >> Or otherwise I'd see yet another separate Kconfig option >> identifying whether an arch supports non-translated mode in the >> first place. That option would select the M2P one then, as I can't >> see how one would go about supporting non-translated guests >> without M2P. In this case you'd add an #ifdef here (placement >> subject to further discussion; personally I'd favor if it was put >> around just the problematic invocation of mfn_to_gmfn(), with >> a suitable #else; alternatively have common code provide a >> stub mfn_to_gmfn()) _and_ a paging_mode_translate() check >> near the top of the function, thus yielding consistent behavior. > > ... I would have avoided to waste time resending the series without > addressing > your concern. > > In this case, I will strongly nack any definition of mfn_to_gmfn() in the > non-M2P case. We saw the implication on Arm and I just want to see the macro > completely disappear from Arm. > > Furthermore, #ifdef chunk of code means they completely disappear from the code > base without the aid of the compiler itself (via optimization). This is less > error-prone than a potentially wrong check and would also help safety > certification as we don't need. That's one perspective to take. The other is that people changing and testing only Arm code will not easily notice if they break that function. See for example Andrew's request and follow-up patch to my hweight() series, converting from #ifdef to if(). > So I still favor using #ifdef here. I could replace by a specific Kconfig (see a > previous attempt [1]). Did you perhaps supply the wrong link? There's no Kconfig option being introduced/suggested there. >> I find it odd that no such check is there right now, as the public >> header clearly says this is a PV-only interface. >> >> Note that with a paging_mode_translate() check at the top of >> the function (or even at its only call site) the one towards the >> end of the function should then be ditched. > > See above my opinion on paging_mode_translate(). As said - as per the public header such a check should be added _anyway_. The #ifdef, if we decide to go that route, imo should then live _after_ that check, with ASSERT_UNREACHABLE() in its #else. Jan
Hi, On 6/5/19 11:10 AM, Jan Beulich wrote: >>>> On 05.06.19 at 11:35, <julien.grall@arm.com> wrote: >> On 05/06/2019 08:14, Jan Beulich wrote: >>>>>> On 04.06.19 at 18:22, <julien.grall@arm.com> wrote: >>>> On 6/4/19 5:14 PM, Jan Beulich wrote: >>>>>>>> On 03.06.19 at 18:03, <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. >>>>> >>>>> But where's the connection between there being M2P and the >>>>> availability of this operation? I think I've suggested so before: >>>>> Why don't we simply disable this operation for translated >>>>> guests (in an independent patch)? >>>> >>>> And I answered that mfn_to_gmfn() is used in the function. I really >>>> don't want to implement the macro on Arm (even if it is dummy). >>>> >>>> You haven't answered back to that comment and I assumed this was fine >>>> with you... >>> >>> Well, I guess it was, but supplying the "why" in the description (or >>> attached as a brief comment to the #ifdef) would have helped >>> avoid re-raising the same question. However, thinking about it >>> again I'm not sure I agree with #ifdef-ing out the entire (large) >>> function body - I'd really prefer the alternative approach >>> suggested above. >> >> I would appreciate if you clarified it in the previous version rather than >> expecting me to understand the hidden meaning. Because ... > > I'm sorry for this, but "thinking about this again" means I was coming > to that conclusion now, not back then. > >>> Or otherwise I'd see yet another separate Kconfig option >>> identifying whether an arch supports non-translated mode in the >>> first place. That option would select the M2P one then, as I can't >>> see how one would go about supporting non-translated guests >>> without M2P. In this case you'd add an #ifdef here (placement >>> subject to further discussion; personally I'd favor if it was put >>> around just the problematic invocation of mfn_to_gmfn(), with >>> a suitable #else; alternatively have common code provide a >>> stub mfn_to_gmfn()) _and_ a paging_mode_translate() check >>> near the top of the function, thus yielding consistent behavior. >> >> ... I would have avoided to waste time resending the series without >> addressing >> your concern. >> >> In this case, I will strongly nack any definition of mfn_to_gmfn() in the >> non-M2P case. We saw the implication on Arm and I just want to see the macro >> completely disappear from Arm. >> >> Furthermore, #ifdef chunk of code means they completely disappear from the code >> base without the aid of the compiler itself (via optimization). This is less >> error-prone than a potentially wrong check and would also help safety >> certification as we don't need. > > That's one perspective to take. The other is that people changing and > testing only Arm code will not easily notice if they break that function. That's not different from any files that are not compiled out on Arm. See compat, kexec & co. So why #ifdef is a concern here? > See for example Andrew's request and follow-up patch to my hweight() > series, converting from #ifdef to if(). >> So I still favor using #ifdef here. I could replace by a specific Kconfig (see a >> previous attempt [1]). > > Did you perhaps supply the wrong link? There's no Kconfig option > being introduced/suggested there. This was a link to an attempt to add support for XENMEM_exchange on Arm. From the discussion back then it is quite clear that the hypercall is unlikely to be implemented on Arm. So it feels to me that trying to keep the code around is not an option. > >>> I find it odd that no such check is there right now, as the public >>> header clearly says this is a PV-only interface. >>> >>> Note that with a paging_mode_translate() check at the top of >>> the function (or even at its only call site) the one towards the >>> end of the function should then be ditched. >> >> See above my opinion on paging_mode_translate(). > > As said - as per the public header such a check should be added > _anyway_. The #ifdef, if we decide to go that route, imo should > then live _after_ that check, with ASSERT_UNREACHABLE() in its #else. That's an option. Cheers,
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index f502d765ba..c78eaf820a 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -17,6 +17,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 10a759b31f..aae4c93002 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -66,6 +66,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..5746daeb80 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -154,6 +154,7 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info) u64 cpu_time = 0; int flags = XEN_DOMINF_blocked; struct vcpu_runstate_info runstate; + gfn_t shared_info_frame; info->domain = d->domain_id; info->max_vcpu_id = XEN_INVALID_MAX_VCPU_ID; @@ -205,8 +206,12 @@ 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)); - BUG_ON(SHARED_M2P(info->shared_info_frame)); + + shared_info_frame = domain_shared_info_gfn(d); + if ( gfn_eq(shared_info_frame, INVALID_GFN) ) + info->shared_info_frame = XEN_INVALID_SHARED_INFO_FRAME; + else + info->shared_info_frame = gfn_x(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 520d6f4803..7a94250e50 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -504,6 +504,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); @@ -794,6 +795,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 79ec6719f5..43174176c2 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 ) { @@ -221,6 +222,11 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) if ( rc ) printk(XENLOG_WARNING "d%d: IOMMU mapping failed: %d\n", d->domain_id, rc); +#else + ASSERT_UNREACHABLE(); + rc = -EOPNOTSUPP; +#endif + } hd->platform_ops->hwdom_init(d); 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..9e9d28a8da 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -118,6 +118,12 @@ struct xen_domctl_getdomaininfo { uint64_aligned_t outstanding_pages; uint64_aligned_t shr_pages; uint64_aligned_t paged_pages; +#define XEN_INVALID_SHARED_INFO_FRAME (~(uint64_t)0) + /* + * GFN of shared_info struct. Some architectures (e.g Arm) may not + * provide a mappable address in the field. In that case, the field + * will be set to XEN_INVALID_SHARED_INFO_FRAME. + */ 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..f5b9f3ef2a 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -118,4 +118,16 @@ 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); \ + gfn_t gfn_; \ + \ + gfn_ = mfn_to_gfn(d_, _mfn(__virt_to_mfn(d_->shared_info)));\ + BUG_ON(SHARED_M2P(gfn_x(gfn_))); \ + \ + gfn_; \ +}) +#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, a fixed value will be provided that will fail on mapping if used. [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 v3: - Move the BUG_ON() in domain_shared_info_gfn() - Use a fixed value when the field shared_info_frame is not supported. - Add an ASSERT_UNREACHABLE in iommu_hwdom_init + move printk within the #ifdef. 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 | 9 +++++++-- xen/common/memory.c | 4 ++++ xen/drivers/passthrough/iommu.c | 8 +++++++- xen/include/asm-arm/domain.h | 5 +++++ xen/include/public/domctl.h | 6 ++++++ xen/include/xen/domain.h | 12 ++++++++++++ 8 files changed, 45 insertions(+), 3 deletions(-)