| Message ID | 20190507151458.29350-4-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Properly disable M2P on Arm. | expand |
>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -474,7 +474,8 @@ static inline void trace_resync(int event, mfn_t gmfn) > if ( tb_init_done ) > { > /* Convert gmfn to gfn */ > - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); > + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, gmfn)); > + > __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); > } Can't you use gfn_t here, and hence avoid the gfn_x()? Same again further down. Jan
On 10/05/2019 12:35, Jan Beulich wrote: >>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >> --- a/xen/arch/x86/mm/shadow/common.c >> +++ b/xen/arch/x86/mm/shadow/common.c >> @@ -474,7 +474,8 @@ static inline void trace_resync(int event, mfn_t gmfn) >> if ( tb_init_done ) >> { >> /* Convert gmfn to gfn */ >> - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); >> + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, gmfn)); >> + >> __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); >> } > > Can't you use gfn_t here, and hence avoid the gfn_x()? Same again further > down. Because __trace_var will export the value to the guest. I wasn't sure whether we can safely consider that gfn_t is exactly the same as unsigned long in debug-build. Cheers,
>>> On 10.05.19 at 15:02, <julien.grall@arm.com> wrote: > > On 10/05/2019 12:35, Jan Beulich wrote: >>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>> --- a/xen/arch/x86/mm/shadow/common.c >>> +++ b/xen/arch/x86/mm/shadow/common.c >>> @@ -474,7 +474,8 @@ static inline void trace_resync(int event, mfn_t gmfn) >>> if ( tb_init_done ) >>> { >>> /* Convert gmfn to gfn */ >>> - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); >>> + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, gmfn)); >>> + >>> __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); >>> } >> >> Can't you use gfn_t here, and hence avoid the gfn_x()? Same again further >> down. > Because __trace_var will export the value to the guest. I wasn't sure > whether we > can safely consider that gfn_t is exactly the same as unsigned long in > debug-build. Hmm, well - see the definition of gfn_t. George, what do you think? Jan
On 10/05/2019 14:24, Jan Beulich wrote: >>>> On 10.05.19 at 15:02, <julien.grall@arm.com> wrote: > >> >> On 10/05/2019 12:35, Jan Beulich wrote: >>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>>> --- a/xen/arch/x86/mm/shadow/common.c >>>> +++ b/xen/arch/x86/mm/shadow/common.c >>>> @@ -474,7 +474,8 @@ static inline void trace_resync(int event, mfn_t gmfn) >>>> if ( tb_init_done ) >>>> { >>>> /* Convert gmfn to gfn */ >>>> - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); >>>> + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, gmfn)); >>>> + >>>> __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); >>>> } >>> >>> Can't you use gfn_t here, and hence avoid the gfn_x()? Same again further >>> down. >> Because __trace_var will export the value to the guest. I wasn't sure >> whether we >> can safely consider that gfn_t is exactly the same as unsigned long in >> debug-build. > > Hmm, well - see the definition of gfn_t. George, what do you think? I know what's the current definition. My point is we never made that assumption before. In all honesty, sure assumption would definitely help in a few places, but I think we ought to safeguard with BUILD_BUG(...). Cheers,
Hi, On 10/05/2019 14:25, Julien Grall wrote: > > > On 10/05/2019 14:24, Jan Beulich wrote: >>>>> On 10.05.19 at 15:02, <julien.grall@arm.com> wrote: >> >>> >>> On 10/05/2019 12:35, Jan Beulich wrote: >>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>>>> --- a/xen/arch/x86/mm/shadow/common.c >>>>> +++ b/xen/arch/x86/mm/shadow/common.c >>>>> @@ -474,7 +474,8 @@ static inline void trace_resync(int event, mfn_t gmfn) >>>>> if ( tb_init_done ) >>>>> { >>>>> /* Convert gmfn to gfn */ >>>>> - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); >>>>> + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, gmfn)); >>>>> + >>>>> __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); >>>>> } >>>> >>>> Can't you use gfn_t here, and hence avoid the gfn_x()? Same again further >>>> down. >>> Because __trace_var will export the value to the guest. I wasn't sure >>> whether we >>> can safely consider that gfn_t is exactly the same as unsigned long in >>> debug-build. >> >> Hmm, well - see the definition of gfn_t. George, what do you think? > > I know what's the current definition. My point is we never made that assumption > before. In all honesty, sure assumption would definitely help in a few places, > but I think we ought to safeguard with BUILD_BUG(...). George, do you have any opinions? Cheers,
On 5/20/19 4:13 PM, Julien Grall wrote: > Hi, > > On 10/05/2019 14:25, Julien Grall wrote: >> >> >> On 10/05/2019 14:24, Jan Beulich wrote: >>>>>> On 10.05.19 at 15:02, <julien.grall@arm.com> wrote: >>> >>>> >>>> On 10/05/2019 12:35, Jan Beulich wrote: >>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>>>>> --- a/xen/arch/x86/mm/shadow/common.c >>>>>> +++ b/xen/arch/x86/mm/shadow/common.c >>>>>> @@ -474,7 +474,8 @@ static inline void trace_resync(int event, >>>>>> mfn_t gmfn) >>>>>> if ( tb_init_done ) >>>>>> { >>>>>> /* Convert gmfn to gfn */ >>>>>> - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); >>>>>> + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, >>>>>> gmfn)); >>>>>> + >>>>>> __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); >>>>>> } >>>>> >>>>> Can't you use gfn_t here, and hence avoid the gfn_x()? Same again >>>>> further >>>>> down. >>>> Because __trace_var will export the value to the guest. I wasn't sure >>>> whether we >>>> can safely consider that gfn_t is exactly the same as unsigned long in >>>> debug-build. >>> >>> Hmm, well - see the definition of gfn_t. George, what do you think? >> >> I know what's the current definition. My point is we never made that >> assumption before. In all honesty, sure assumption would definitely >> help in a few places, but I think we ought to safeguard with >> BUILD_BUG(...). > > George, do you have any opinions? Sorry, not sure how I missed this question earlier. The __trace_var() call here is designed to be generic: whatever type or size gfn is, it will copy the whole thing. Tracing is explicitly not meant to be a stable interface -- the toolstack needs to track the hypervisor in terms of what it's going to kick out. So, having gfn be a gfn_t in this case should be fine; in fact it should be _better_ than unsigned long, since if gfn_t ever does change size, the trace record will change size appropriately. If that happens, xenalyze will need to be modified to understand how to deal with the new size, but that's expected. All that to say: it looks like Jan's suggestion of having gfn_t here would be better. -George
Hi George, On 28/05/2019 18:29, George Dunlap wrote: > On 5/20/19 4:13 PM, Julien Grall wrote: >> Hi, >> >> On 10/05/2019 14:25, Julien Grall wrote: >>> >>> >>> On 10/05/2019 14:24, Jan Beulich wrote: >>>>>>> On 10.05.19 at 15:02, <julien.grall@arm.com> wrote: >>>> >>>>> >>>>> On 10/05/2019 12:35, Jan Beulich wrote: >>>>>>>>> On 07.05.19 at 17:14, <julien.grall@arm.com> wrote: >>>>>>> --- a/xen/arch/x86/mm/shadow/common.c >>>>>>> +++ b/xen/arch/x86/mm/shadow/common.c >>>>>>> @@ -474,7 +474,8 @@ static inline void trace_resync(int event, >>>>>>> mfn_t gmfn) >>>>>>> if ( tb_init_done ) >>>>>>> { >>>>>>> /* Convert gmfn to gfn */ >>>>>>> - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); >>>>>>> + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, >>>>>>> gmfn)); >>>>>>> + >>>>>>> __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); >>>>>>> } >>>>>> >>>>>> Can't you use gfn_t here, and hence avoid the gfn_x()? Same again >>>>>> further >>>>>> down. >>>>> Because __trace_var will export the value to the guest. I wasn't sure >>>>> whether we >>>>> can safely consider that gfn_t is exactly the same as unsigned long in >>>>> debug-build. >>>> >>>> Hmm, well - see the definition of gfn_t. George, what do you think? >>> >>> I know what's the current definition. My point is we never made that >>> assumption before. In all honesty, sure assumption would definitely >>> help in a few places, but I think we ought to safeguard with >>> BUILD_BUG(...). >> >> George, do you have any opinions? > > Sorry, not sure how I missed this question earlier. > > The __trace_var() call here is designed to be generic: whatever type or > size gfn is, it will copy the whole thing. Tracing is explicitly not > meant to be a stable interface -- the toolstack needs to track the > hypervisor in terms of what it's going to kick out. > > So, having gfn be a gfn_t in this case should be fine; in fact it should > be _better_ than unsigned long, since if gfn_t ever does change size, > the trace record will change size appropriately. If that happens, > xenalyze will need to be modified to understand how to deal with the new > size, but that's expected. > > All that to say: it looks like Jan's suggestion of having gfn_t here > would be better. Make sense, thank you for the answer. I will respin the series. Cheers,
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 9e81a30cc4..b16117dc56 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -935,7 +935,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, } if ( page_get_owner(mfn_to_page(mfn_add(mfn, i))) != d ) continue; - ogfn = _gfn(mfn_to_gfn(d, mfn_add(mfn, i))); + ogfn = mfn_to_gfn(d, mfn_add(mfn, i)); if ( !gfn_eq(ogfn, _gfn(INVALID_M2P_ENTRY)) && !gfn_eq(ogfn, gfn_add(gfn, i)) ) { diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c index 2d44855388..480fcc740d 100644 --- a/xen/arch/x86/mm/shadow/common.c +++ b/xen/arch/x86/mm/shadow/common.c @@ -474,7 +474,8 @@ static inline void trace_resync(int event, mfn_t gmfn) if ( tb_init_done ) { /* Convert gmfn to gfn */ - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, gmfn)); + __trace_var(event, 0/*!tsc*/, sizeof(gfn), &gfn); } } @@ -986,8 +987,9 @@ static inline void trace_shadow_prealloc_unpin(struct domain *d, mfn_t smfn) { /* Convert smfn to gfn */ unsigned long gfn; + ASSERT(mfn_valid(smfn)); - gfn = mfn_to_gfn(d, backpointer(mfn_to_page(smfn))); + gfn = gfn_x(mfn_to_gfn(d, backpointer(mfn_to_page(smfn)))); __trace_var(TRC_SHADOW_PREALLOC_UNPIN, 0/*!tsc*/, sizeof(gfn), &gfn); } } @@ -1861,7 +1863,8 @@ static inline void trace_shadow_wrmap_bf(mfn_t gmfn) if ( tb_init_done ) { /* Convert gmfn to gfn */ - unsigned long gfn = mfn_to_gfn(current->domain, gmfn); + unsigned long gfn = gfn_x(mfn_to_gfn(current->domain, gmfn)); + __trace_var(TRC_SHADOW_WRMAP_BF, 0/*!tsc*/, sizeof(gfn), &gfn); } } @@ -1946,7 +1949,7 @@ int sh_remove_write_access(struct domain *d, mfn_t gmfn, #if SHADOW_OPTIMIZATIONS & SHOPT_WRITABLE_HEURISTIC if ( curr->domain == d ) { - unsigned long gfn; + gfn_t gfn; /* Heuristic: there is likely to be only one writeable mapping, * and that mapping is likely to be in the current pagetable, * in the guest's linear map (on non-HIGHPTE linux and windows)*/ @@ -1969,8 +1972,9 @@ int sh_remove_write_access(struct domain *d, mfn_t gmfn, GUESS(0xC0000000UL + (fault_addr >> 10), 1); /* Linux lowmem: first 896MB is mapped 1-to-1 above 0xC0000000 */ - if ((gfn = mfn_to_gfn(d, gmfn)) < 0x38000 ) - GUESS(0xC0000000UL + (gfn << PAGE_SHIFT), 4); + gfn = mfn_to_gfn(d, gmfn); + if ( gfn_x(gfn) < 0x38000 ) + GUESS(0xC0000000UL + gfn_to_gaddr(gfn), 4); /* FreeBSD: Linear map at 0xBFC00000 */ if ( level == 1 ) @@ -1987,8 +1991,9 @@ int sh_remove_write_access(struct domain *d, mfn_t gmfn, } /* Linux lowmem: first 896MB is mapped 1-to-1 above 0xC0000000 */ - if ((gfn = mfn_to_gfn(d, gmfn)) < 0x38000 ) - GUESS(0xC0000000UL + (gfn << PAGE_SHIFT), 4); + gfn = mfn_to_gfn(d, gmfn); + if ( gfn_x(gfn) < 0x38000 ) + GUESS(0xC0000000UL + gfn_to_gaddr(gfn), 4); /* FreeBSD PAE: Linear map at 0xBF800000 */ switch ( level ) @@ -2016,15 +2021,15 @@ int sh_remove_write_access(struct domain *d, mfn_t gmfn, * had it at 0xffff810000000000, and older kernels yet had it * at 0x0000010000000000UL */ gfn = mfn_to_gfn(d, gmfn); - GUESS(0xffff880000000000UL + (gfn << PAGE_SHIFT), 4); - GUESS(0xffff810000000000UL + (gfn << PAGE_SHIFT), 4); - GUESS(0x0000010000000000UL + (gfn << PAGE_SHIFT), 4); + GUESS(0xffff880000000000UL + gfn_to_gaddr(gfn), 4); + GUESS(0xffff810000000000UL + gfn_to_gaddr(gfn), 4); + GUESS(0x0000010000000000UL + gfn_to_gaddr(gfn), 4); /* * 64bit Solaris kernel page map at * kpm_vbase; 0xfffffe0000000000UL */ - GUESS(0xfffffe0000000000UL + (gfn << PAGE_SHIFT), 4); + GUESS(0xfffffe0000000000UL + gfn_to_gaddr(gfn), 4); /* FreeBSD 64bit: linear map 0xffff800000000000 */ switch ( level ) @@ -2037,7 +2042,7 @@ int sh_remove_write_access(struct domain *d, mfn_t gmfn, + ((fault_addr & VADDR_MASK) >> 27), 6); break; } /* FreeBSD 64bit: direct map at 0xffffff0000000000 */ - GUESS(0xffffff0000000000 + (gfn << PAGE_SHIFT), 6); + GUESS(0xffffff0000000000 + gfn_to_gaddr(gfn), 6); } #undef GUESS diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index 1d282c928f..8781bdcfe5 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -1076,7 +1076,7 @@ static inline void shadow_vram_get_l1e(shadow_l1e_t new_sl1e, || !mfn_valid(mfn) ) /* mfn can be invalid in mmio_direct */ return; - gfn = mfn_to_gfn(d, mfn); + gfn = gfn_x(mfn_to_gfn(d, mfn)); /* Page sharing not supported on shadow PTs */ BUG_ON(SHARED_M2P(gfn)); @@ -1107,7 +1107,7 @@ static inline void shadow_vram_put_l1e(shadow_l1e_t old_sl1e, || !mfn_valid(mfn) ) /* mfn can be invalid in mmio_direct */ return; - gfn = mfn_to_gfn(d, mfn); + gfn = gfn_x(mfn_to_gfn(d, mfn)); /* Page sharing not supported on shadow PTs */ BUG_ON(SHARED_M2P(gfn)); diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h index c3bd12020e..0157568be9 100644 --- a/xen/include/asm-x86/p2m.h +++ b/xen/include/asm-x86/p2m.h @@ -506,12 +506,12 @@ static inline struct page_info *get_page_from_gfn( } /* General conversion function from mfn to gfn */ -static inline unsigned long mfn_to_gfn(const struct domain *d, mfn_t mfn) +static inline gfn_t mfn_to_gfn(const struct domain *d, mfn_t mfn) { if ( paging_mode_translate(d) ) - return get_gpfn_from_mfn(mfn_x(mfn)); + return _gfn(get_gpfn_from_mfn(mfn_x(mfn))); else - return mfn_x(mfn); + return _gfn(mfn_x(mfn)); } /* Deadlock-avoidance scheme when calling get_gfn on different gfn's */
No functional changes intended. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Patch added --- xen/arch/x86/mm/p2m.c | 2 +- xen/arch/x86/mm/shadow/common.c | 31 ++++++++++++++++++------------- xen/arch/x86/mm/shadow/multi.c | 4 ++-- xen/include/asm-x86/p2m.h | 6 +++--- 4 files changed, 24 insertions(+), 19 deletions(-)