| Message ID | 20190218113600.9540-10-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm. | expand |
>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, > #define SHARED_M2P_ENTRY (~0UL - 1UL) > #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) > > -/* Xen always owns P2M on ARM */ > +/* We don't have a M2P on Arm */ > #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) > -#define mfn_to_gmfn(_d, mfn) (mfn) So is the plan to remove the other macro from Arm then as well? In any event Acked-by: Jan Beulich <jbeulich@suse.com> Jan
Hi Jan, On 13/03/2019 15:22, Jan Beulich wrote: >>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >> --- a/xen/include/asm-arm/mm.h >> +++ b/xen/include/asm-arm/mm.h >> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, >> #define SHARED_M2P_ENTRY (~0UL - 1UL) >> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >> >> -/* Xen always owns P2M on ARM */ >> +/* We don't have a M2P on Arm */ >> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) >> -#define mfn_to_gmfn(_d, mfn) (mfn) > > So is the plan to remove the other macro from Arm then as well? Do you mean mfn_to_gfn? If so it does not exist on Arm. > In any event > Acked-by: Jan Beulich <jbeulich@suse.com> Cheers, > > Jan > >
>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote: > On 13/03/2019 15:22, Jan Beulich wrote: >>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >>> --- a/xen/include/asm-arm/mm.h >>> +++ b/xen/include/asm-arm/mm.h >>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, >>> #define SHARED_M2P_ENTRY (~0UL - 1UL) >>> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >>> >>> -/* Xen always owns P2M on ARM */ >>> +/* We don't have a M2P on Arm */ >>> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) >>> -#define mfn_to_gmfn(_d, mfn) (mfn) >> >> So is the plan to remove the other macro from Arm then as well? > > Do you mean mfn_to_gfn? If so it does not exist on Arm. No, I mean the one in context above - set_gpfn_from_mfn(). Jan
Hi, On 13/03/2019 15:40, Jan Beulich wrote: >>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote: >> On 13/03/2019 15:22, Jan Beulich wrote: >>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >>>> --- a/xen/include/asm-arm/mm.h >>>> +++ b/xen/include/asm-arm/mm.h >>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, >>>> #define SHARED_M2P_ENTRY (~0UL - 1UL) >>>> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >>>> >>>> -/* Xen always owns P2M on ARM */ >>>> +/* We don't have a M2P on Arm */ >>>> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) >>>> -#define mfn_to_gmfn(_d, mfn) (mfn) >>> >>> So is the plan to remove the other macro from Arm then as well? >> >> Do you mean mfn_to_gfn? If so it does not exist on Arm. > > No, I mean the one in context above - set_gpfn_from_mfn(). It is used in common code, so we would need to #idef the caller. I think it is better to provide a NOP implementation. Could be moved somewhere in the common header though. Any opinions? Cheers,
>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote: > Hi, > > On 13/03/2019 15:40, Jan Beulich wrote: >>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote: >>> On 13/03/2019 15:22, Jan Beulich wrote: >>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >>>>> --- a/xen/include/asm-arm/mm.h >>>>> +++ b/xen/include/asm-arm/mm.h >>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, > vaddr_t va, >>>>> #define SHARED_M2P_ENTRY (~0UL - 1UL) >>>>> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >>>>> >>>>> -/* Xen always owns P2M on ARM */ >>>>> +/* We don't have a M2P on Arm */ >>>>> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } > while (0) >>>>> -#define mfn_to_gmfn(_d, mfn) (mfn) >>>> >>>> So is the plan to remove the other macro from Arm then as well? >>> >>> Do you mean mfn_to_gfn? If so it does not exist on Arm. >> >> No, I mean the one in context above - set_gpfn_from_mfn(). > > It is used in common code, so we would need to #idef the caller. Hmm, right, such #ifdef-ary would be undesirable (and two out of the three common code callers would need it. > I think it is better to provide a NOP implementation. Could be moved somewhere > in the common header though. Any opinions? This would perhaps be better, now that you have HAVE_M2P. Jan
On 13/03/2019 15:59, Jan Beulich wrote: >>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote: >> Hi, >> >> On 13/03/2019 15:40, Jan Beulich wrote: >>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote: >>>> On 13/03/2019 15:22, Jan Beulich wrote: >>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >>>>>> --- a/xen/include/asm-arm/mm.h >>>>>> +++ b/xen/include/asm-arm/mm.h >>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, >> vaddr_t va, >>>>>> #define SHARED_M2P_ENTRY (~0UL - 1UL) >>>>>> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >>>>>> >>>>>> -/* Xen always owns P2M on ARM */ >>>>>> +/* We don't have a M2P on Arm */ >>>>>> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } >> while (0) >>>>>> -#define mfn_to_gmfn(_d, mfn) (mfn) >>>>> So is the plan to remove the other macro from Arm then as well? >>>> Do you mean mfn_to_gfn? If so it does not exist on Arm. >>> No, I mean the one in context above - set_gpfn_from_mfn(). >> It is used in common code, so we would need to #idef the caller. > Hmm, right, such #ifdef-ary would be undesirable (and two out of > the three common code callers would need it. > >> I think it is better to provide a NOP implementation. Could be moved somewhere >> in the common header though. Any opinions? > This would perhaps be better, now that you have HAVE_M2P. Given that "having an M2P" is now an x86-specific concept, I think phasing set_gpfn_from_mfn()'s use out of common code is the way to go. ~Andrew
Hi Andrew, On 13/03/2019 17:34, Andrew Cooper wrote: > On 13/03/2019 15:59, Jan Beulich wrote: >>>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote: >>> Hi, >>> >>> On 13/03/2019 15:40, Jan Beulich wrote: >>>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote: >>>>> On 13/03/2019 15:22, Jan Beulich wrote: >>>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >>>>>>> --- a/xen/include/asm-arm/mm.h >>>>>>> +++ b/xen/include/asm-arm/mm.h >>>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, >>> vaddr_t va, >>>>>>> #define SHARED_M2P_ENTRY (~0UL - 1UL) >>>>>>> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >>>>>>> >>>>>>> -/* Xen always owns P2M on ARM */ >>>>>>> +/* We don't have a M2P on Arm */ >>>>>>> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } >>> while (0) >>>>>>> -#define mfn_to_gmfn(_d, mfn) (mfn) >>>>>> So is the plan to remove the other macro from Arm then as well? >>>>> Do you mean mfn_to_gfn? If so it does not exist on Arm. >>>> No, I mean the one in context above - set_gpfn_from_mfn(). >>> It is used in common code, so we would need to #idef the caller. >> Hmm, right, such #ifdef-ary would be undesirable (and two out of >> the three common code callers would need it. >> >>> I think it is better to provide a NOP implementation. Could be moved somewhere >>> in the common header though. Any opinions? >> This would perhaps be better, now that you have HAVE_M2P. > > Given that "having an M2P" is now an x86-specific concept, I think > phasing set_gpfn_from_mfn()'s use out of common code is the way to go. So you never expect other architecture to use the M2P? Cheers,
On 13/03/2019 17:42, Julien Grall wrote: > Hi Andrew, > > On 13/03/2019 17:34, Andrew Cooper wrote: >> On 13/03/2019 15:59, Jan Beulich wrote: >>>>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote: >>>> Hi, >>>> >>>> On 13/03/2019 15:40, Jan Beulich wrote: >>>>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote: >>>>>> On 13/03/2019 15:22, Jan Beulich wrote: >>>>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >>>>>>>> --- a/xen/include/asm-arm/mm.h >>>>>>>> +++ b/xen/include/asm-arm/mm.h >>>>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct >>>>>>>> vcpu *v, >>>> vaddr_t va, >>>>>>>> #define SHARED_M2P_ENTRY (~0UL - 1UL) >>>>>>>> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >>>>>>>> -/* Xen always owns P2M on ARM */ >>>>>>>> +/* We don't have a M2P on Arm */ >>>>>>>> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), >>>>>>>> (void)(pfn); } >>>> while (0) >>>>>>>> -#define mfn_to_gmfn(_d, mfn) (mfn) >>>>>>> So is the plan to remove the other macro from Arm then as well? >>>>>> Do you mean mfn_to_gfn? If so it does not exist on Arm. >>>>> No, I mean the one in context above - set_gpfn_from_mfn(). >>>> It is used in common code, so we would need to #idef the caller. >>> Hmm, right, such #ifdef-ary would be undesirable (and two out of >>> the three common code callers would need it. >>> >>>> I think it is better to provide a NOP implementation. Could be >>>> moved somewhere >>>> in the common header though. Any opinions? >>> This would perhaps be better, now that you have HAVE_M2P. >> >> Given that "having an M2P" is now an x86-specific concept, I think >> phasing set_gpfn_from_mfn()'s use out of common code is the way to go. > > So you never expect other architecture to use the M2P? I guess that depends on how likely it is going to be that Xen gains a new non-HVM-like virtualisation mode on a new architecture. ~Andrew
>>> On 13.03.19 at 18:34, <andrew.cooper3@citrix.com> wrote: > On 13/03/2019 15:59, Jan Beulich wrote: >>>>> On 13.03.19 at 16:48, <julien.grall@arm.com> wrote: >>> On 13/03/2019 15:40, Jan Beulich wrote: >>>>>>> On 13.03.19 at 16:24, <julien.grall@arm.com> wrote: >>>>> On 13/03/2019 15:22, Jan Beulich wrote: >>>>>>>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >>>>>>> --- a/xen/include/asm-arm/mm.h >>>>>>> +++ b/xen/include/asm-arm/mm.h >>>>>>> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, >>> vaddr_t va, >>>>>>> #define SHARED_M2P_ENTRY (~0UL - 1UL) >>>>>>> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >>>>>>> >>>>>>> -/* Xen always owns P2M on ARM */ >>>>>>> +/* We don't have a M2P on Arm */ >>>>>>> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } >>> while (0) >>>>>>> -#define mfn_to_gmfn(_d, mfn) (mfn) >>>>>> So is the plan to remove the other macro from Arm then as well? >>>>> Do you mean mfn_to_gfn? If so it does not exist on Arm. >>>> No, I mean the one in context above - set_gpfn_from_mfn(). >>> It is used in common code, so we would need to #idef the caller. >> Hmm, right, such #ifdef-ary would be undesirable (and two out of >> the three common code callers would need it. >> >>> I think it is better to provide a NOP implementation. Could be moved somewhere >>> in the common header though. Any opinions? >> This would perhaps be better, now that you have HAVE_M2P. > > Given that "having an M2P" is now an x86-specific concept, I think > phasing set_gpfn_from_mfn()'s use out of common code is the way to go. But what's the implication of this? There would need to be some arch_*() hook used in the place that set_gpfn_from_mfn() is invoked currently. But then it can as well remain set_gpfn_from_mfn() (with the !HAVE_M2P stubbed out properly in a common header), can't it? Jan
>>> On 13.03.19 at 19:41, <andrew.cooper3@citrix.com> wrote: > On 13/03/2019 17:42, Julien Grall wrote: >> On 13/03/2019 17:34, Andrew Cooper wrote: >>> Given that "having an M2P" is now an x86-specific concept, I think >>> phasing set_gpfn_from_mfn()'s use out of common code is the way to go. >> >> So you never expect other architecture to use the M2P? > > I guess that depends on how likely it is going to be that Xen gains a > new non-HVM-like virtualisation mode on a new architecture. Well, not quite. I don't think it would be straightforward to make x86 select HAVE_M2P only when PV is also enabled. Hence a HVM- like implementation may still want to maintain M2P. In fact it is my understanding that 64-bit Arm could easily do (leaving aside the question of whether the memory needed to build the tables would be well spent this), but it's prohibitive on 32-bit, and hence it's easier for the Arm code overall to uniformly resort to alternative means where such a translation is indeed needed. Jan
On Mon, 18 Feb 2019, Julien Grall wrote: > On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are > no more call to mfn_to_gmfn, so the helper can be dropped. > > At the same time rework a comment in Arm code that does not make sense. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Lovely. Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/drivers/passthrough/iommu.c | 6 +++--- > xen/include/asm-arm/mm.h | 4 +--- > xen/include/asm-x86/mm.h | 5 ----- > 3 files changed, 4 insertions(+), 11 deletions(-) > > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c > index 5742cd05b8..04ac46239e 100644 > --- a/xen/drivers/passthrough/iommu.c > +++ b/xen/drivers/passthrough/iommu.c > @@ -193,8 +193,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > > page_list_for_each ( page, &d->page_list ) > { > - unsigned long mfn = mfn_x(page_to_mfn(page)); > - unsigned long dfn = mfn_to_gmfn(d, mfn); > + mfn_t mfn = page_to_mfn(page); > + unsigned long dfn = mfn_to_gfn(d, mfn); > unsigned int mapping = IOMMUF_readable; > int ret; > > @@ -203,7 +203,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) > == PGT_writable_page) ) > mapping |= IOMMUF_writable; > > - ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping, > + ret = iommu_map(d, _dfn(dfn), mfn, 0, mapping, > &flush_flags); > > if ( !rc ) > diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h > index a9c8352b94..a9cb98a6c7 100644 > --- a/xen/include/asm-arm/mm.h > +++ b/xen/include/asm-arm/mm.h > @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, > #define SHARED_M2P_ENTRY (~0UL - 1UL) > #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) > > -/* Xen always owns P2M on ARM */ > +/* We don't have a M2P on Arm */ > #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) > -#define mfn_to_gmfn(_d, mfn) (mfn) > - > > /* Arch-specific portion of memory_op hypercall. */ > long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
Hi, On 3/13/19 3:22 PM, Jan Beulich wrote: >>>> On 18.02.19 at 12:36, <julien.grall@arm.com> wrote: >> --- a/xen/include/asm-arm/mm.h >> +++ b/xen/include/asm-arm/mm.h >> @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, >> #define SHARED_M2P_ENTRY (~0UL - 1UL) >> #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) >> >> -/* Xen always owns P2M on ARM */ >> +/* We don't have a M2P on Arm */ >> #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) >> -#define mfn_to_gmfn(_d, mfn) (mfn) > > So is the plan to remove the other macro from Arm then as well? > In any event > Acked-by: Jan Beulich <jbeulich@suse.com> Just to keep everyone aware, I kept this patch as is with the 2 acked-by and provided a separate patch to move the helpers in common code under !CONFIG_HAVE_M2P. Cheers,
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 5742cd05b8..04ac46239e 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -193,8 +193,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) page_list_for_each ( page, &d->page_list ) { - unsigned long mfn = mfn_x(page_to_mfn(page)); - unsigned long dfn = mfn_to_gmfn(d, mfn); + mfn_t mfn = page_to_mfn(page); + unsigned long dfn = mfn_to_gfn(d, mfn); unsigned int mapping = IOMMUF_readable; int ret; @@ -203,7 +203,7 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) == PGT_writable_page) ) mapping |= IOMMUF_writable; - ret = iommu_map(d, _dfn(dfn), _mfn(mfn), 0, mapping, + ret = iommu_map(d, _dfn(dfn), mfn, 0, mapping, &flush_flags); if ( !rc ) diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index a9c8352b94..a9cb98a6c7 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -321,10 +321,8 @@ struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va, #define SHARED_M2P_ENTRY (~0UL - 1UL) #define SHARED_M2P(_e) ((_e) == SHARED_M2P_ENTRY) -/* Xen always owns P2M on ARM */ +/* We don't have a M2P on Arm */ #define set_gpfn_from_mfn(mfn, pfn) do { (void) (mfn), (void)(pfn); } while (0) -#define mfn_to_gmfn(_d, mfn) (mfn) - /* Arch-specific portion of memory_op hypercall. */ long arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg); diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index 1ca4154382..3324dd7abf 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -506,11 +506,6 @@ extern struct rangeset *mmio_ro_ranges; #define get_gpfn_from_mfn(mfn) (machine_to_phys_mapping[(mfn)]) -#define mfn_to_gmfn(_d, mfn) \ - ( (paging_mode_translate(_d)) \ - ? get_gpfn_from_mfn(mfn) \ - : (mfn) ) - #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20)) #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20))
On x86, mfn_to_gmfn can be replaced with mfn_to_gfn. On Arm, there are no more call to mfn_to_gmfn, so the helper can be dropped. At the same time rework a comment in Arm code that does not make sense. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/drivers/passthrough/iommu.c | 6 +++--- xen/include/asm-arm/mm.h | 4 +--- xen/include/asm-x86/mm.h | 5 ----- 3 files changed, 4 insertions(+), 11 deletions(-)