| Message ID | 20200322161418.31606-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bunch of typesafe conversion | expand |
On 22.03.2020 17:14, julien@xen.org wrote: > From: Julien Grall <jgrall@amazon.com> > > Introduce handy helpers to generate/convert the CR3 from/to a MFN/GFN. > > Note that we are using cr3_pa() rather than xen_cr3_to_pfn() because the > latter does not ignore the top 12-bits. I'm afraid this remark of yours points at some issue here: cr3_pa() is meant to act on (real or virtual) CR3 values, but not (necessarily) on para-virtual ones. E.g. ... > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -1096,7 +1096,7 @@ int arch_set_info_guest( > set_bit(_VPF_in_reset, &v->pause_flags); > > if ( !compat ) > - cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[3])); > + cr3_mfn = cr3_to_mfn(c.nat->ctrlreg[3]); ... you're now losing the top 12 bits here, potentially making ... > else > cr3_mfn = _mfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3])); > cr3_page = get_page_from_mfn(cr3_mfn, d); ... this succeed when it shouldn't. > --- a/xen/include/asm-x86/mm.h > +++ b/xen/include/asm-x86/mm.h > @@ -524,6 +524,26 @@ extern struct rangeset *mmio_ro_ranges; > #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20)) > #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20)) > > +static inline unsigned long mfn_to_cr3(mfn_t mfn) > +{ > + return xen_pfn_to_cr3(mfn_x(mfn)); > +} > + > +static inline mfn_t cr3_to_mfn(unsigned long cr3) > +{ > + return maddr_to_mfn(cr3_pa(cr3)); > +} > + > +static inline unsigned long gfn_to_cr3(gfn_t gfn) > +{ > + return xen_pfn_to_cr3(gfn_x(gfn)); > +} > + > +static inline gfn_t cr3_to_gfn(unsigned long cr3) > +{ > + return gaddr_to_gfn(cr3_pa(cr3)); > +} Overall I think that when introducing such helpers we need to be very clear about their intended uses: Bare underlying hardware, PV guests, or HVM guests. From this perspective I also think that having MFN and GFN conversions next to each other may be more confusing than helpful, the more that there are no uses introduced here for the latter. When applied to HVM guests, xen_pfn_to_cr3() also shouldn't be used, as that's a PV construct in the public headers. Yet I thing conversions to/from GFNs should first and foremost be applicable to HVM guests. A possible route to go may be to e.g. accompany {xen,compat}_pfn_to_cr3() with {xen,compat}_mfn_to_cr3(), and leave the GFN aspect out until such patch that would actually use them (which may then make clear that these actually want to live in a header specifically applicable to translated guests). Jan
Hi Jan, On 25/03/2020 14:46, Jan Beulich wrote: > On 22.03.2020 17:14, julien@xen.org wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Introduce handy helpers to generate/convert the CR3 from/to a MFN/GFN. >> >> Note that we are using cr3_pa() rather than xen_cr3_to_pfn() because the >> latter does not ignore the top 12-bits. > > I'm afraid this remark of yours points at some issue here: > cr3_pa() is meant to act on (real or virtual) CR3 values, but > not (necessarily) on para-virtual ones. E.g. ... > >> --- a/xen/arch/x86/domain.c >> +++ b/xen/arch/x86/domain.c >> @@ -1096,7 +1096,7 @@ int arch_set_info_guest( >> set_bit(_VPF_in_reset, &v->pause_flags); >> >> if ( !compat ) >> - cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[3])); >> + cr3_mfn = cr3_to_mfn(c.nat->ctrlreg[3]); > > ... you're now losing the top 12 bits here, potentially > making ... > >> else >> cr3_mfn = _mfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3])); >> cr3_page = get_page_from_mfn(cr3_mfn, d); > > ... this succeed when it shouldn't. > >> --- a/xen/include/asm-x86/mm.h >> +++ b/xen/include/asm-x86/mm.h >> @@ -524,6 +524,26 @@ extern struct rangeset *mmio_ro_ranges; >> #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20)) >> #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20)) >> >> +static inline unsigned long mfn_to_cr3(mfn_t mfn) >> +{ >> + return xen_pfn_to_cr3(mfn_x(mfn)); >> +} >> + >> +static inline mfn_t cr3_to_mfn(unsigned long cr3) >> +{ >> + return maddr_to_mfn(cr3_pa(cr3)); >> +} >> + >> +static inline unsigned long gfn_to_cr3(gfn_t gfn) >> +{ >> + return xen_pfn_to_cr3(gfn_x(gfn)); >> +} >> + >> +static inline gfn_t cr3_to_gfn(unsigned long cr3) >> +{ >> + return gaddr_to_gfn(cr3_pa(cr3)); >> +} > > Overall I think that when introducing such helpers we need to be > very clear about their intended uses: Bare underlying hardware, > PV guests, or HVM guests. From this perspective I also think that > having MFN and GFN conversions next to each other may be more > confusing than helpful, the more that there are no uses > introduced here for the latter. When applied to HVM guests, > xen_pfn_to_cr3() also shouldn't be used, as that's a PV construct > in the public headers. Yet I thing conversions to/from GFNs > should first and foremost be applicable to HVM guests. There are use of GFN helpers in the series, but I wanted to avoid introducing them in the middle of something else. I can try to find a couple of occurences I can switch to use them now. Regarding the term GFN, it is not meant to be HVM only. So we may want to prefix the helpers with hvm_ to make it clear. > > A possible route to go may be to e.g. accompany > {xen,compat}_pfn_to_cr3() with {xen,compat}_mfn_to_cr3(), and > leave the GFN aspect out until such patch that would actually > use them (which may then make clear that these actually want > to live in a header specifically applicable to translated > guests). I am thinking to introduce 3 sets of helpers: - hvm_cr3_to_gfn()/hvm_gfn_to_cr3(): Handle the CR3 for HVM guest - {xen, compat}_mfn_to_cr3()/{xen, compat}_cr3_to_mfn(): Handle the CR3 for PV guest. - host_cr3_to_mfn()/host_mfn_to_cr3(): To handle the host cr3. What do you think? Cheers,
On 28.03.2020 11:14, Julien Grall wrote: > On 25/03/2020 14:46, Jan Beulich wrote: >> On 22.03.2020 17:14, julien@xen.org wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> Introduce handy helpers to generate/convert the CR3 from/to a MFN/GFN. >>> >>> Note that we are using cr3_pa() rather than xen_cr3_to_pfn() because the >>> latter does not ignore the top 12-bits. >> >> I'm afraid this remark of yours points at some issue here: >> cr3_pa() is meant to act on (real or virtual) CR3 values, but >> not (necessarily) on para-virtual ones. E.g. ... >> >>> --- a/xen/arch/x86/domain.c >>> +++ b/xen/arch/x86/domain.c >>> @@ -1096,7 +1096,7 @@ int arch_set_info_guest( >>> set_bit(_VPF_in_reset, &v->pause_flags); >>> if ( !compat ) >>> - cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[3])); >>> + cr3_mfn = cr3_to_mfn(c.nat->ctrlreg[3]); >> >> ... you're now losing the top 12 bits here, potentially >> making ... >> >>> else >>> cr3_mfn = _mfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3])); >>> cr3_page = get_page_from_mfn(cr3_mfn, d); >> >> ... this succeed when it shouldn't. >> >>> --- a/xen/include/asm-x86/mm.h >>> +++ b/xen/include/asm-x86/mm.h >>> @@ -524,6 +524,26 @@ extern struct rangeset *mmio_ro_ranges; >>> #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20)) >>> #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20)) >>> +static inline unsigned long mfn_to_cr3(mfn_t mfn) >>> +{ >>> + return xen_pfn_to_cr3(mfn_x(mfn)); >>> +} >>> + >>> +static inline mfn_t cr3_to_mfn(unsigned long cr3) >>> +{ >>> + return maddr_to_mfn(cr3_pa(cr3)); >>> +} >>> + >>> +static inline unsigned long gfn_to_cr3(gfn_t gfn) >>> +{ >>> + return xen_pfn_to_cr3(gfn_x(gfn)); >>> +} >>> + >>> +static inline gfn_t cr3_to_gfn(unsigned long cr3) >>> +{ >>> + return gaddr_to_gfn(cr3_pa(cr3)); >>> +} >> >> Overall I think that when introducing such helpers we need to be >> very clear about their intended uses: Bare underlying hardware, >> PV guests, or HVM guests. From this perspective I also think that >> having MFN and GFN conversions next to each other may be more >> confusing than helpful, the more that there are no uses >> introduced here for the latter. When applied to HVM guests, >> xen_pfn_to_cr3() also shouldn't be used, as that's a PV construct >> in the public headers. Yet I thing conversions to/from GFNs >> should first and foremost be applicable to HVM guests. > > There are use of GFN helpers in the series, but I wanted to avoid > introducing them in the middle of something else. I can try to > find a couple of occurences I can switch to use them now. With your proposal below splitting patches at the HVM/PV/host boundaries may make sense nevertheless. > Regarding the term GFN, it is not meant to be HVM only. Of course, hence my "first and foremost". > So we may want to prefix the helpers with hvm_ to make it clear. > >> >> A possible route to go may be to e.g. accompany >> {xen,compat}_pfn_to_cr3() with {xen,compat}_mfn_to_cr3(), and >> leave the GFN aspect out until such patch that would actually >> use them (which may then make clear that these actually want >> to live in a header specifically applicable to translated >> guests). > > I am thinking to introduce 3 sets of helpers: > - hvm_cr3_to_gfn()/hvm_gfn_to_cr3(): Handle the CR3 for HVM guest > - {xen, compat}_mfn_to_cr3()/{xen, compat}_cr3_to_mfn(): Handle the CR3 for PV guest. > - host_cr3_to_mfn()/host_mfn_to_cr3(): To handle the host cr3. > > What do you think? Maybe some variation thereof: - hvm_cr3_to_gfn()/hvm_gfn_to_cr3(): Handle the CR3 for HVM guest - {pv,compat}_mfn_to_cr3()/{pv,compat}_cr3_to_mfn(): Handle the CR3 for PV guest - cr3_to_mfn()/mfn_to_cr3(): To handle the host cr3 ? This is because I'd prefer to avoid host_ prefixes (albeit I'm not entirely opposed to such), and I'd also prefer to use xen_ prefixes as they're generally ambiguous as to what aspect of "Xen" they actually mean. Jan
Hi Jan, On 30/03/2020 08:38, Jan Beulich wrote: > Maybe some variation thereof: > > - hvm_cr3_to_gfn()/hvm_gfn_to_cr3(): Handle the CR3 for HVM guest > - {pv,compat}_mfn_to_cr3()/{pv,compat}_cr3_to_mfn(): Handle the CR3 for PV guest > - cr3_to_mfn()/mfn_to_cr3(): To handle the host cr3 > > ? This is because I'd prefer to avoid host_ prefixes (albeit I'm > not entirely opposed to such), and I'd also prefer to use xen_ > prefixes as they're generally ambiguous as to what aspect of "Xen" > they actually mean. I am happy with your suggested naming. I will have a look to see how they fit in the tree and respin the series. Cheers,
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index caf2ecad7e..15750ce210 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1096,7 +1096,7 @@ int arch_set_info_guest( set_bit(_VPF_in_reset, &v->pause_flags); if ( !compat ) - cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[3])); + cr3_mfn = cr3_to_mfn(c.nat->ctrlreg[3]); else cr3_mfn = _mfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3])); cr3_page = get_page_from_mfn(cr3_mfn, d); @@ -1142,7 +1142,7 @@ int arch_set_info_guest( v->arch.guest_table = pagetable_from_page(cr3_page); if ( c.nat->ctrlreg[1] ) { - cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[1])); + cr3_mfn = cr3_to_mfn(c.nat->ctrlreg[1]); cr3_page = get_page_from_mfn(cr3_mfn, d); if ( !cr3_page ) diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 62507ca651..069a61deb8 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -509,7 +509,7 @@ void make_cr3(struct vcpu *v, mfn_t mfn) { struct domain *d = v->domain; - v->arch.cr3 = mfn_x(mfn) << PAGE_SHIFT; + v->arch.cr3 = mfn_to_cr3(mfn); if ( is_pv_domain(d) && d->arch.pv.pcid ) v->arch.cr3 |= get_pcid_bits(v, false); } diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h index a06b2fb81f..9764362a38 100644 --- a/xen/include/asm-x86/mm.h +++ b/xen/include/asm-x86/mm.h @@ -524,6 +524,26 @@ extern struct rangeset *mmio_ro_ranges; #define compat_pfn_to_cr3(pfn) (((unsigned)(pfn) << 12) | ((unsigned)(pfn) >> 20)) #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 20)) +static inline unsigned long mfn_to_cr3(mfn_t mfn) +{ + return xen_pfn_to_cr3(mfn_x(mfn)); +} + +static inline mfn_t cr3_to_mfn(unsigned long cr3) +{ + return maddr_to_mfn(cr3_pa(cr3)); +} + +static inline unsigned long gfn_to_cr3(gfn_t gfn) +{ + return xen_pfn_to_cr3(gfn_x(gfn)); +} + +static inline gfn_t cr3_to_gfn(unsigned long cr3) +{ + return gaddr_to_gfn(cr3_pa(cr3)); +} + #ifdef MEMORY_GUARD void memguard_guard_range(void *p, unsigned long l); void memguard_unguard_range(void *p, unsigned long l);