Re: [Xen-devel] [PATCH 01/17] xen/x86: Introduce helpers to generate/convert the CR3 from/to a MFN/GFN

[Jan Beulich <jbeulich@suse.com>]

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