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
