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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 25/03/2020 14:46, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@xxxxxxx wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > 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,

--
Julien Grall