On 28.03.2020 11:14, Julien Grall wrote: > On 25/03/2020 14:46, Jan Beulich wrote: >> On 22.03.2020 17:14, jul...@xen.org wrote: >>> From: Julien Grall <jgr...@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