[01/17] xen/x86: Introduce helpers to generate/convert the CR3 from/to a MFN/GFN
diff mbox series

Message ID 20200322161418.31606-2-julien@xen.org
State New
Headers show
Series
  • Bunch of typesafe conversion
Related show

Commit Message

Julien Grall March 22, 2020, 4:14 p.m. UTC
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.

Take the opportunity to use the new helpers when possible.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/domain.c    |  4 ++--
 xen/arch/x86/mm.c        |  2 +-
 xen/include/asm-x86/mm.h | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

Comments

Jan Beulich March 25, 2020, 2:46 p.m. UTC | #1
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
Julien Grall March 28, 2020, 10:14 a.m. UTC | #2
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,
Jan Beulich March 30, 2020, 7:38 a.m. UTC | #3
On 28.03.2020 11:14, Julien Grall wrote:
> 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.

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
Julien Grall April 16, 2020, 11:50 a.m. UTC | #4
Hi Jan,

On 30/03/2020 08:38, Jan Beulich wrote:
> 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.

I am happy with your suggested naming. I will have a look to see how 
they fit in the tree and respin the series.

Cheers,

Patch
diff mbox series

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index caf2ecad7e..15750ce210 100644
--- 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]);
     else
         cr3_mfn = _mfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3]));
     cr3_page = get_page_from_mfn(cr3_mfn, d);
@@ -1142,7 +1142,7 @@  int arch_set_info_guest(
         v->arch.guest_table = pagetable_from_page(cr3_page);
         if ( c.nat->ctrlreg[1] )
         {
-            cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[1]));
+            cr3_mfn = cr3_to_mfn(c.nat->ctrlreg[1]);
             cr3_page = get_page_from_mfn(cr3_mfn, d);
 
             if ( !cr3_page )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 62507ca651..069a61deb8 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -509,7 +509,7 @@  void make_cr3(struct vcpu *v, mfn_t mfn)
 {
     struct domain *d = v->domain;
 
-    v->arch.cr3 = mfn_x(mfn) << PAGE_SHIFT;
+    v->arch.cr3 = mfn_to_cr3(mfn);
     if ( is_pv_domain(d) && d->arch.pv.pcid )
         v->arch.cr3 |= get_pcid_bits(v, false);
 }
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index a06b2fb81f..9764362a38 100644
--- 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));
+}
+
 #ifdef MEMORY_GUARD
 void memguard_guard_range(void *p, unsigned long l);
 void memguard_unguard_range(void *p, unsigned long l);