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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: xen-devel@lists.xenproject.org,
	"Julien Grall" <jgrall@amazon.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>
Subject: Re: [Xen-devel] [PATCH 01/17] xen/x86: Introduce helpers to generate/convert the CR3 from/to a MFN/GFN
Date: Sat, 28 Mar 2020 10:14:14 +0000	[thread overview]
Message-ID: <6d544a04-72a2-0407-64da-789f9a82b0e0@xen.org> (raw)
In-Reply-To: <4896eacc-10ce-5db9-3990-d74fb05e2ef0@suse.com>

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,

-- 
Julien Grall