[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 ...

          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) >> 
  #define compat_cr3_to_pfn(cr3) (((unsigned)(cr3) >> 12) | ((unsigned)(cr3) << 
+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

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?


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.