| Message ID | 20200921180214.4842-3-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm | expand |
On 21/09/2020 19:02, Julien Grall wrote: > From: Julien Grall <julien.grall@arm.com> > > 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 2 callers in the common code: > - memory_exchange: Arm cannot not use it because steal_page is not > implemented. Note this is already protected by !CONFIG_PV. > - 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. If anyone cares, access to the shared info page should be wired up through acquire_resource, not through this foreign mapping bodge, because ... > - 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. > - memory_exchange: This is PV only (not supported on Arm) but take > the opportunity to gate with HAS_M2P as well in case someone decide > to implement PV without M2P. > - 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. > > Signed-off-by Julien Grall <julien.grall@arm.com> ... possibly the single best reason for returning -1 on ARM is that this is already the behaviour for x86 HVM guests, until the guest has mapped the shared info frame for the first time. (XEN) *** d0v6 getdomaininfo(d1) d->shared_info ffff83007cffe000, shared_info_frame 0x7cffe (XEN) *** d0v6 getdomaininfo(d2) d->shared_info ffff83007a401000, shared_info_frame 0xffffffffffffffff (d1 is PV, d2 is HVM. This is the result of creating domains at the python shell, then querying them for their info, without any further construction or execution). Furthermore, both PV and HVM guests can invalidate the M2P mappings for their shared_info page, which in the HVM case would cause -1 to be returned again. > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index 5ac6e9c5cafa..2bf3e6659018 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -68,6 +68,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; > @@ -111,8 +112,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); This can be simplified to just info->shared_info_frame = gfn_x(arch_shared_info_gfn(d)) based on the suggestion at the bottom. > > info->cpupool = cpupool_get_id(d); > > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 9300104943b0..c698e6872596 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -504,7 +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_PV > +#if defined(CONFIG_PV) && defined(CONFIG_M2P) There is no such thing as PV && !M2P. The M2P table is part of the PV ABI with guests. These two hunks should be dropped. > struct xen_memory_exchange exch; > PAGE_LIST_HEAD(in_chunk_list); > PAGE_LIST_HEAD(out_chunk_list); > @@ -801,7 +801,7 @@ 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_PV */ > +#else /* !(CONFIG_PV && CONFIG_M2P) */ > return -EOPNOTSUPP; > #endif > } > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 6819a3bf382f..90161dd079a1 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -262,6 +262,11 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > > #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) > > +static inline gfn_t domain_shared_info_gfn(struct domain *d) > +{ > + return INVALID_GFN; > +} We don't want every every architecture to provide a stub. Instead, ... (bottom reply) > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h > index 5c5e55ebcb76..7564df5e8374 100644 > --- a/xen/include/public/domctl.h > +++ b/xen/include/public/domctl.h > @@ -136,6 +136,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) We've already got INVALID_GFN as a constant used in the interface. Lets not proliferate more. > + /* > + * 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 */ Delete this comment, especially as it is using obsolete naming terminology. > 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 cde0d9c7fe63..7281eb7b36c7 100644 > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma); > static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); } > #endif > > +#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_; \ > +}) ... this wants to be #ifndef arch_shared_info_gfn static inline gfn_t arch_shared_info_gfn(const struct domain *d) { return INVALID_GFN; } #endif with gfn_t arch_shared_info_gfn(const struct domain *d); #define arch_shared_info_gfn arch_shared_info_gfn in asm-x86/domain.h and the actual implementation in arch/x86/domain.c ~Andrew
On 21.09.2020 20:02, Julien Grall wrote: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -63,6 +63,9 @@ config HAS_IOPORTS > config HAS_SCHED_GRANULARITY > bool > > +config HAS_M2P > + bool Following Andrew's comments I think the need for this addition disappears, but in case I'm missing something I'd like to ask for this to be added higher up - see the patch I've just sent to re-sort the various HAS_* here. Jan
Hi Andrew, On 21/09/2020 21:29, Andrew Cooper wrote: > On 21/09/2020 19:02, Julien Grall wrote: >> From: Julien Grall <julien.grall@arm.com> >> >> 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 2 callers in the common code: >> - memory_exchange: Arm cannot not use it because steal_page is not >> implemented. Note this is already protected by !CONFIG_PV. >> - 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. > > If anyone cares, access to the shared info page should be wired up > through acquire_resource, not through this foreign mapping bodge, > because ... > >> - 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. >> - memory_exchange: This is PV only (not supported on Arm) but take >> the opportunity to gate with HAS_M2P as well in case someone decide >> to implement PV without M2P. >> - 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. >> >> Signed-off-by Julien Grall <julien.grall@arm.com> > > ... possibly the single best reason for returning -1 on ARM is that this > is already the behaviour for x86 HVM guests, until the guest has mapped > the shared info frame for the first time. > > (XEN) *** d0v6 getdomaininfo(d1) d->shared_info ffff83007cffe000, > shared_info_frame 0x7cffe > (XEN) *** d0v6 getdomaininfo(d2) d->shared_info ffff83007a401000, > shared_info_frame 0xffffffffffffffff > > (d1 is PV, d2 is HVM. This is the result of creating domains at the > python shell, then querying them for their info, without any further > construction or execution). > > Furthermore, both PV and HVM guests can invalidate the M2P mappings for > their shared_info page, which in the HVM case would cause -1 to be > returned again. > >> diff --git a/xen/common/domctl.c b/xen/common/domctl.c >> index 5ac6e9c5cafa..2bf3e6659018 100644 >> --- a/xen/common/domctl.c >> +++ b/xen/common/domctl.c >> @@ -68,6 +68,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; >> @@ -111,8 +112,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); > > This can be simplified to just info->shared_info_frame = > gfn_x(arch_shared_info_gfn(d)) based on the suggestion at the bottom. > >> >> info->cpupool = cpupool_get_id(d); >> >> diff --git a/xen/common/memory.c b/xen/common/memory.c >> index 9300104943b0..c698e6872596 100644 >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -504,7 +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_PV >> +#if defined(CONFIG_PV) && defined(CONFIG_M2P) > > There is no such thing as PV && !M2P. The M2P table is part of the PV > ABI with guests. > > These two hunks should be dropped. I will drop them. > >> struct xen_memory_exchange exch; >> PAGE_LIST_HEAD(in_chunk_list); >> PAGE_LIST_HEAD(out_chunk_list); >> @@ -801,7 +801,7 @@ 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_PV */ >> +#else /* !(CONFIG_PV && CONFIG_M2P) */ >> return -EOPNOTSUPP; >> #endif >> } >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 6819a3bf382f..90161dd079a1 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -262,6 +262,11 @@ static inline void arch_vcpu_block(struct vcpu *v) {} >> >> #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) >> >> +static inline gfn_t domain_shared_info_gfn(struct domain *d) >> +{ >> + return INVALID_GFN; >> +} > > We don't want every every architecture to provide a stub. Instead, ... > (bottom reply) > >> + >> #endif /* __ASM_DOMAIN_H__ */ >> >> /* >> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >> index 5c5e55ebcb76..7564df5e8374 100644 >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -136,6 +136,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) > > We've already got INVALID_GFN as a constant used in the interface. Lets > not proliferate more. This was my original approach (see [1]) but this was reworked because: 1) INVALID_GFN is not technically defined in the ABI. So the toolstack has to hardcode the value in the check. 2) The value is different between 32-bit and 64-bit Arm as INVALID_GFN is defined as an unsigned long. So providing a new define is the right way to go. > >> + /* >> + * 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 */ > > Delete this comment, especially as it is using obsolete naming terminology. Sure. > >> 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 cde0d9c7fe63..7281eb7b36c7 100644 >> --- a/xen/include/xen/domain.h >> +++ b/xen/include/xen/domain.h >> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma); >> static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); } >> #endif >> >> +#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_; \ >> +}) > > ... this wants to be > > #ifndef arch_shared_info_gfn > static inline gfn_t arch_shared_info_gfn(const struct domain *d) { > return INVALID_GFN; } > #endif > > with > > gfn_t arch_shared_info_gfn(const struct domain *d); > #define arch_shared_info_gfn arch_shared_info_gfn > > in asm-x86/domain.h > > and the actual implementation in arch/x86/domain.c What's wrong with implement it in xen/domain.h? After all there is nothing x86 specific in the implementation... Cheers, [1] <20190507151458.29350-10-julien.grall@arm.com>
Hi Jan, On 22/09/2020 08:54, Jan Beulich wrote: > On 21.09.2020 20:02, Julien Grall wrote: >> --- a/xen/common/Kconfig >> +++ b/xen/common/Kconfig >> @@ -63,6 +63,9 @@ config HAS_IOPORTS >> config HAS_SCHED_GRANULARITY >> bool >> >> +config HAS_M2P >> + bool > > Following Andrew's comments I think the need for this addition > disappears, but in case I'm missing something I'd like to ask for > this to be added higher up - see the patch I've just sent to > re-sort the various HAS_* here. It is required for patch #4. Cheers,
On 22/09/2020 19:20, Julien Grall wrote: >>> + >>> #endif /* __ASM_DOMAIN_H__ */ >>> /* >>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >>> index 5c5e55ebcb76..7564df5e8374 100644 >>> --- a/xen/include/public/domctl.h >>> +++ b/xen/include/public/domctl.h >>> @@ -136,6 +136,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) >> >> We've already got INVALID_GFN as a constant used in the interface. Lets >> not proliferate more. > > This was my original approach (see [1]) but this was reworked because: > 1) INVALID_GFN is not technically defined in the ABI. So the > toolstack has to hardcode the value in the check. > 2) The value is different between 32-bit and 64-bit Arm as > INVALID_GFN is defined as an unsigned long. > > So providing a new define is the right way to go. There is nothing special about this field. It should not have a dedicated constant, when a general one is the appropriate one to use. libxl already has LIBXL_INVALID_GFN, which is already used. If this isn't good enough, them the right thing to do is put a proper INVALID_GFN in the tools interface. >>> 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 cde0d9c7fe63..7281eb7b36c7 100644 >>> --- a/xen/include/xen/domain.h >>> +++ b/xen/include/xen/domain.h >>> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma); >>> static inline void vnuma_destroy(struct vnuma_info *vnuma) { >>> ASSERT(!vnuma); } >>> #endif >>> +#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_; \ >>> +}) >> >> ... this wants to be >> >> #ifndef arch_shared_info_gfn >> static inline gfn_t arch_shared_info_gfn(const struct domain *d) { >> return INVALID_GFN; } >> #endif >> >> with >> >> gfn_t arch_shared_info_gfn(const struct domain *d); >> #define arch_shared_info_gfn arch_shared_info_gfn >> >> in asm-x86/domain.h >> >> and the actual implementation in arch/x86/domain.c > > What's wrong with implement it in xen/domain.h? After all there is > nothing x86 specific in the implementation... d->shared_info is allocated in arch specific code, not common code. This macro assumes that __virt_to_mfn() is safe to call on the pointer. For an approaching obsolete part of the API/ABI (particularly given the new HVM plans), I'd just stuff it in x86 and call it done. Its easy enough to re-evaluate if a second appears. ~Andrew
Hi Andrew, On 22/09/2020 19:56, Andrew Cooper wrote: > On 22/09/2020 19:20, Julien Grall wrote: >>>> + >>>> #endif /* __ASM_DOMAIN_H__ */ >>>> /* >>>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >>>> index 5c5e55ebcb76..7564df5e8374 100644 >>>> --- a/xen/include/public/domctl.h >>>> +++ b/xen/include/public/domctl.h >>>> @@ -136,6 +136,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) >>> >>> We've already got INVALID_GFN as a constant used in the interface. Lets >>> not proliferate more. >> >> This was my original approach (see [1]) but this was reworked because: >> 1) INVALID_GFN is not technically defined in the ABI. So the >> toolstack has to hardcode the value in the check. >> 2) The value is different between 32-bit and 64-bit Arm as >> INVALID_GFN is defined as an unsigned long. >> >> So providing a new define is the right way to go. > > There is nothing special about this field. It should not have a > dedicated constant, when a general one is the appropriate one to use. > > libxl already has LIBXL_INVALID_GFN, which is already used. Right, but that's imply it cannot be used by libxc as this would be a layer violation. > > If this isn't good enough, them the right thing to do is put a proper > INVALID_GFN in the tools interface. That would be nice but I can see some issue on x86 given that we don't consistenly define a GFN in the interface as a 64-bit value. So would you still be happy to consider introducing XEN_INVALID_GFN in the interface with some caveats? > >>>> 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 cde0d9c7fe63..7281eb7b36c7 100644 >>>> --- a/xen/include/xen/domain.h >>>> +++ b/xen/include/xen/domain.h >>>> @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma); >>>> static inline void vnuma_destroy(struct vnuma_info *vnuma) { >>>> ASSERT(!vnuma); } >>>> #endif >>>> +#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_; \ >>>> +}) >>> >>> ... this wants to be >>> >>> #ifndef arch_shared_info_gfn >>> static inline gfn_t arch_shared_info_gfn(const struct domain *d) { >>> return INVALID_GFN; } >>> #endif >>> >>> with >>> >>> gfn_t arch_shared_info_gfn(const struct domain *d); >>> #define arch_shared_info_gfn arch_shared_info_gfn >>> >>> in asm-x86/domain.h >>> >>> and the actual implementation in arch/x86/domain.c >> >> What's wrong with implement it in xen/domain.h? After all there is >> nothing x86 specific in the implementation... > > d->shared_info is allocated in arch specific code, not common code. > This macro assumes that __virt_to_mfn() is safe to call on the pointer. That's a fair point. I will move the code in an x86 specific files. Cheers,
Hi, On 26/09/2020 14:00, Julien Grall wrote: > Hi Andrew, > > On 22/09/2020 19:56, Andrew Cooper wrote: >> On 22/09/2020 19:20, Julien Grall wrote: >>>>> + >>>>> #endif /* __ASM_DOMAIN_H__ */ >>>>> /* >>>>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h >>>>> index 5c5e55ebcb76..7564df5e8374 100644 >>>>> --- a/xen/include/public/domctl.h >>>>> +++ b/xen/include/public/domctl.h >>>>> @@ -136,6 +136,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) >>>> >>>> We've already got INVALID_GFN as a constant used in the interface. >>>> Lets >>>> not proliferate more. >>> >>> This was my original approach (see [1]) but this was reworked because: >>> 1) INVALID_GFN is not technically defined in the ABI. So the >>> toolstack has to hardcode the value in the check. >>> 2) The value is different between 32-bit and 64-bit Arm as >>> INVALID_GFN is defined as an unsigned long. >>> >>> So providing a new define is the right way to go. >> >> There is nothing special about this field. It should not have a >> dedicated constant, when a general one is the appropriate one to use. >> >> libxl already has LIBXL_INVALID_GFN, which is already used. > > Right, but that's imply it cannot be used by libxc as this would be a > layer violation. > >> >> If this isn't good enough, them the right thing to do is put a proper >> INVALID_GFN in the tools interface. > > That would be nice but I can see some issue on x86 given that we don't > consistenly define a GFN in the interface as a 64-bit value. > > So would you still be happy to consider introducing XEN_INVALID_GFN in > the interface with some caveats? Gentle ping. @Andrew, are you happy with this approach? Cheers,
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index a636a4bb1e69..ad92e756cccc 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_NS16550 select HAS_PASSTHROUGH diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 15e3b79ff57f..b6b4db92d700 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -63,6 +63,9 @@ config HAS_IOPORTS config HAS_SCHED_GRANULARITY bool +config HAS_M2P + bool + config NEEDS_LIBELF bool diff --git a/xen/common/domctl.c b/xen/common/domctl.c index 5ac6e9c5cafa..2bf3e6659018 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -68,6 +68,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; @@ -111,8 +112,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 = cpupool_get_id(d); diff --git a/xen/common/memory.c b/xen/common/memory.c index 9300104943b0..c698e6872596 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -504,7 +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_PV +#if defined(CONFIG_PV) && defined(CONFIG_M2P) struct xen_memory_exchange exch; PAGE_LIST_HEAD(in_chunk_list); PAGE_LIST_HEAD(out_chunk_list); @@ -801,7 +801,7 @@ 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_PV */ +#else /* !(CONFIG_PV && CONFIG_M2P) */ return -EOPNOTSUPP; #endif } diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 6819a3bf382f..90161dd079a1 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -262,6 +262,11 @@ static inline void arch_vcpu_block(struct vcpu *v) {} #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag) +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 5c5e55ebcb76..7564df5e8374 100644 --- a/xen/include/public/domctl.h +++ b/xen/include/public/domctl.h @@ -136,6 +136,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 cde0d9c7fe63..7281eb7b36c7 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -131,4 +131,16 @@ void vnuma_destroy(struct vnuma_info *vnuma); static inline void vnuma_destroy(struct vnuma_info *vnuma) { ASSERT(!vnuma); } #endif +#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__ */
From: Julien Grall <julien.grall@arm.com> 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 2 callers in the common code: - memory_exchange: Arm cannot not use it because steal_page is not implemented. Note this is already protected by !CONFIG_PV. - 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. - 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. - memory_exchange: This is PV only (not supported on Arm) but take the opportunity to gate with HAS_M2P as well in case someone decide to implement PV without M2P. - 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. Signed-off-by Julien Grall <julien.grall@arm.com> --- Changes in v4: - The IOMMU code doesn't use the M2P anymore, so this can be dropped. - Reword the commit message - Fix rebase conflict 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/include/asm-arm/domain.h | 5 +++++ xen/include/public/domctl.h | 6 ++++++ xen/include/xen/domain.h | 12 ++++++++++++ 7 files changed, 36 insertions(+), 4 deletions(-)