[PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

Add a xc_domain_hvm_get_mtrr_type() call to the libxc API,
to support functionality similar to get_mtrr_type() (which
is only available at the hypervisor level).

Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>

diff -r f50aab21f9f2 -r a23515aabc91 tools/libxc/xc_domain.c
--- a/tools/libxc/xc_domain.c	Thu Dec 13 14:39:31 2012 +0000
+++ b/tools/libxc/xc_domain.c	Mon Dec 17 12:53:11 2012 +0200
@@ -379,6 +379,45 @@ int xc_domain_hvm_setcontext(xc_interfac
     return ret;
 }
 
+#define MTRR_PHYSMASK_VALID_BIT  11
+#define MTRR_PHYSMASK_SHIFT      12
+#define MTRR_PHYSBASE_TYPE_MASK  0xff   /* lowest 8 bits */
+
+int xc_domain_hvm_get_mtrr_type(xc_interface *xch, 
+                                uint32_t domid,
+                                unsigned long paddress,
+                                uint8_t *type)
+{
+    struct      hvm_hw_mtrr hw_mtrr;
+    int32_t     seg;
+    uint8_t     num_var_ranges;
+    uint64_t    phys_base;
+    uint64_t    phys_mask;
+
+    if ( xc_domain_hvm_getcontext_partial(xch, domid, HVM_SAVE_CODE(MTRR), 0, &hw_mtrr, sizeof hw_mtrr) )
+        return -1;
+
+    num_var_ranges = hw_mtrr.msr_mtrr_cap & 0xff;
+
+    for ( seg = 0; seg < num_var_ranges; seg++ )
+    {
+        phys_base = hw_mtrr.msr_mtrr_var[seg*2];
+        phys_mask = hw_mtrr.msr_mtrr_var[seg*2 + 1];
+
+        if ( phys_mask & (1 << MTRR_PHYSMASK_VALID_BIT) )
+        {
+            if ( ((uint64_t) paddress & phys_mask) >> MTRR_PHYSMASK_SHIFT ==
+                 (phys_base & phys_mask) >> MTRR_PHYSMASK_SHIFT )
+            {
+                *type = phys_base & MTRR_PHYSBASE_TYPE_MASK;
+                return 0;
+            }
+        }
+    }
+
+    return -1;
+}
+
 int xc_vcpu_getcontext(xc_interface *xch,
                        uint32_t domid,
                        uint32_t vcpu,
diff -r f50aab21f9f2 -r a23515aabc91 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h	Thu Dec 13 14:39:31 2012 +0000
+++ b/tools/libxc/xenctrl.h	Mon Dec 17 12:53:11 2012 +0200
@@ -633,6 +633,15 @@ int xc_domain_hvm_setcontext(xc_interfac
                              uint32_t size);
 
 /**
+ * This function returns information about the MTRR type of
+ * a given guest physical address/
+ */
+int xc_domain_hvm_get_mtrr_type(xc_interface *xch, 
+                                uint32_t domid,
+                                unsigned long paddress,
+                                uint8_t *type);
+
+/**
  * This function returns information about the execution context of a
  * particular vcpu of a domain.
  *

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Ian Campbell]

On Mon, 2012-12-17 at 11:38 +0000, Razvan Cojocaru wrote:
> Add a xc_domain_hvm_get_mtrr_type() call to the libxc API,
> to support functionality similar to get_mtrr_type() (which
> is only available at the hypervisor level).

Do you have a user in mind for this new functionality?

> Signed-off-by: Razvan Cojocaru <rzvncj@gmail.com>
> 
> diff -r f50aab21f9f2 -r a23515aabc91 tools/libxc/xc_domain.c
> --- a/tools/libxc/xc_domain.c	Thu Dec 13 14:39:31 2012 +0000
> +++ b/tools/libxc/xc_domain.c	Mon Dec 17 12:53:11 2012 +0200
> @@ -379,6 +379,45 @@ int xc_domain_hvm_setcontext(xc_interfac
>      return ret;
>  }
>  
> +#define MTRR_PHYSMASK_VALID_BIT  11
> +#define MTRR_PHYSMASK_SHIFT      12
> +#define MTRR_PHYSBASE_TYPE_MASK  0xff   /* lowest 8 bits */
> +
> +int xc_domain_hvm_get_mtrr_type(xc_interface *xch, 
> +                                uint32_t domid,
> +                                unsigned long paddress,
> +                                uint8_t *type)
> +{

This version seems to do a lot less than get_mtrr_type() in the
hypervisor. Is that deliberate? Why isn't the fixed mtrr slot and
overlap handling required here?

> [...]

> diff -r f50aab21f9f2 -r a23515aabc91 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h	Thu Dec 13 14:39:31 2012 +0000
> +++ b/tools/libxc/xenctrl.h	Mon Dec 17 12:53:11 2012 +0200
> @@ -633,6 +633,15 @@ int xc_domain_hvm_setcontext(xc_interfac
>                               uint32_t size);
>  
>  /**
> + * This function returns information about the MTRR type of
> + * a given guest physical address/

                                   ^ you mean . not / I think.

Ian.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

Thanks for the reply!

> Do you have a user in mind for this new functionality?

Yes. An (userspace) application that needs to look at this information 
to decide if a set of pages are interesting for monitoring.

> This version seems to do a lot less than get_mtrr_type() in the
> hypervisor. Is that deliberate? Why isn't the fixed mtrr slot and
> overlap handling required here?

It does do less. It's somewhat deliberate :), ideally it should have 
done everything that get_mtrr_type() does. It did work with my initial 
test addresses, but clearly more is required if it is to provide the 
full functionality of get_mtrr_type(). The code currently only iterates 
through the var_ranges, not the fixed array, etc.

The overlap handling isn't there because there doesn't seem to be a 
clear correspondence between struct mtrr_state and  struct hvm_hw_mtrr. 
I implemented as much of the get_mtrr_type() logic as was obvious using 
what mapping was clear bewtween them.

Is having the full functionality in libxc feasible?

>>   /**
>> + * This function returns information about the MTRR type of
>> + * a given guest physical address/
>
>                                     ^ you mean . not / I think.

You're right, sorry for the typo.

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Ian Campbell]

On Tue, 2012-12-18 at 17:24 +0000, Razvan Cojocaru wrote:
> Thanks for the reply!
> 
> > Do you have a user in mind for this new functionality?
> 
> Yes. An (userspace) application that needs to look at this information 
> to decide if a set of pages are interesting for monitoring.
> 
> > This version seems to do a lot less than get_mtrr_type() in the
> > hypervisor. Is that deliberate? Why isn't the fixed mtrr slot and
> > overlap handling required here?
> 
> It does do less. It's somewhat deliberate :), ideally it should have 
> done everything that get_mtrr_type() does. It did work with my initial 
> test addresses, but clearly more is required if it is to provide the 
> full functionality of get_mtrr_type(). The code currently only iterates 
> through the var_ranges, not the fixed array, etc.
> 
> The overlap handling isn't there because there doesn't seem to be a 
> clear correspondence between struct mtrr_state and  struct hvm_hw_mtrr. 
> I implemented as much of the get_mtrr_type() logic as was obvious using 
> what mapping was clear bewtween them.
> 
> Is having the full functionality in libxc feasible?

It would certainly be preferable to have libxc do it now rather than
every application it or having to introduce cleverer versions of the API
in the future.

I don't know enough about MTRRs or how Xen represents them either
internally or at the hypercall interface to have a sensible opinion
about the feasibility but my gut feeling is that there's no reason it
shouldn't be.

>From a brief look it seems like hvm_hw_mtrr is a subset of mtrr_state,
which makes sense since you would expect the internal state (struct
mtrr_state) to cache or precompute some stuff for efficiency while
emulating but you don't want to expose that in the architectural state
(struct hvm_hw_mtrr). libxc can (and should) probably recompute anything
it needs above what is in hvm_hw_mtrr on the fly (e.g. check for
overlaps).


Ian.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

> It would certainly be preferable to have libxc do it now rather than
> every application it or having to introduce cleverer versions of the API
> in the future.

I agree.

> I don't know enough about MTRRs or how Xen represents them either
> internally or at the hypercall interface to have a sensible opinion
> about the feasibility but my gut feeling is that there's no reason it
> shouldn't be.

Unfortunately I'm far from being an expert on MTRRs too, so maybe 
someone can tell us what the equivalent of this:

uint8_t get_mtrr_type(struct mtrr_state *m, paddr_t pa)
{
     [...]
     if ( unlikely(!(m->enabled & 0x2)) )
         return MTRR_TYPE_UNCACHABLE;
     [...]
}

whould be with libxc code using struct hvm_hw_mtrr? Or, more to the 
point, what the equivalent of m->enabled is in this:

struct hvm_hw_mtrr {
#define MTRR_VCNT 8
#define NUM_FIXED_MSR 11
     uint64_t msr_pat_cr;
     /* mtrr physbase & physmask msr pair*/
     uint64_t msr_mtrr_var[MTRR_VCNT*2];
     uint64_t msr_mtrr_fixed[NUM_FIXED_MSR];
     uint64_t msr_mtrr_cap;
     uint64_t msr_mtrr_def_type;
};

I'm also having a hard time figuring out how to map m->overlapped on the 
hvm_hw_mtrr members. It's also quite possible (to me, at least, at this 
stage) that they're not needed because at libxc level they're assumed to 
be set to some value (i.e. 'overlapped' is assumed to be 0, 'enabled' to 
be 1).

Any help is appreciated, if we clear this up I'll modify and re-submit 
the patch.

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Ian Campbell]

On Wed, 2012-12-19 at 10:42 +0000, Razvan Cojocaru wrote:
> > It would certainly be preferable to have libxc do it now rather than
> > every application it or having to introduce cleverer versions of the API
> > in the future.
> 
> I agree.
> 
> > I don't know enough about MTRRs or how Xen represents them either
> > internally or at the hypercall interface to have a sensible opinion
> > about the feasibility but my gut feeling is that there's no reason it
> > shouldn't be.
> 
> Unfortunately I'm far from being an expert on MTRRs too,

There is a section in the Intel SDM (chapter 10.4 in my copy) which
explains the meanings etc of the registers, which are what is exposed in
hvm_hw_mtrr I think.

>  so maybe 
> someone can tell us what the equivalent of this:
> 
> uint8_t get_mtrr_type(struct mtrr_state *m, paddr_t pa)
> {
>      [...]
>      if ( unlikely(!(m->enabled & 0x2)) )
>          return MTRR_TYPE_UNCACHABLE;
>      [...]
> }
> 
> whould be with libxc code using struct hvm_hw_mtrr? Or, more to the 
> point, what the equivalent of m->enabled is in this:

xen/arch/x86/hvm/mtrr.c:mtrr_def_type_msr_set() seems to be where it is
written, and it is called with hw_mtrr.msr_mtrr_def_type so it looks
like it can be derived from that value in hvm_hw_mtrr.

> 
> struct hvm_hw_mtrr {
> #define MTRR_VCNT 8
> #define NUM_FIXED_MSR 11
>      uint64_t msr_pat_cr;
>      /* mtrr physbase & physmask msr pair*/
>      uint64_t msr_mtrr_var[MTRR_VCNT*2];
>      uint64_t msr_mtrr_fixed[NUM_FIXED_MSR];
>      uint64_t msr_mtrr_cap;
>      uint64_t msr_mtrr_def_type;
> };
> 
> I'm also having a hard time figuring out how to map m->overlapped on the 
> hvm_hw_mtrr members.

    m->overlapped = is_var_mtrr_overlapped(m);

Looks like that function contains the necessary logic.

>  It's also quite possible (to me, at least, at this 
> stage) that they're not needed because at libxc level they're assumed to 
> be set to some value (i.e. 'overlapped' is assumed to be 0, 'enabled' to 
> be 1).
> 
> Any help is appreciated, if we clear this up I'll modify and re-submit 
> the patch.
> 
> Thanks,
> Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

> There is a section in the Intel SDM (chapter 10.4 in my copy) which
> explains the meanings etc of the registers, which are what is exposed in
> hvm_hw_mtrr I think.

Thanks, I'll look that up.

> xen/arch/x86/hvm/mtrr.c:mtrr_def_type_msr_set() seems to be where it is
> written, and it is called with hw_mtrr.msr_mtrr_def_type so it looks
> like it can be derived from that value in hvm_hw_mtrr.

Indeed it is, but this happens there:

uint8_t def_type = msr_content & 0xff;
uint8_t enabled = (msr_content >> 10) & 0x3;

So what ends up being put in def_type is only one byte of msr_content, 
whereas enabled is some bits from another byte of msr_content. To make 
matters worse, hw_mtrr.msr_mtrr_def_type is an uint64_t, so would that 
mean that hw_mtrr.msr_mtrr_def_type is actually the whole of msr_content?

>> I'm also having a hard time figuring out how to map m->overlapped on the
>> hvm_hw_mtrr members.
>
>      m->overlapped = is_var_mtrr_overlapped(m);
>
> Looks like that function contains the necessary logic.

You're right, but what happens there is that that function depends on 
the get_mtrr_range() function, which in turn depends on the size_or_mask 
global variable, which is initialized in hvm_mtrr_pat_init(), which then 
depends on a global table, and so on. Putting that into libxc is pretty 
much putting the whole mtrr.c file there.

Is this amount of copy/paste code a good thing, and wouldn't it be less 
tedious and bug-prone to have that code in a single place, and just add 
overlapped and enabled to hw_mtrr before sending it out into userspace?

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

>> xen/arch/x86/hvm/mtrr.c:mtrr_def_type_msr_set() seems to be where it is
>> written, and it is called with hw_mtrr.msr_mtrr_def_type so it looks
>> like it can be derived from that value in hvm_hw_mtrr.
>
> Indeed it is, but this happens there:
>
> uint8_t def_type = msr_content & 0xff;
> uint8_t enabled = (msr_content >> 10) & 0x3;

Nevermind, found it :)

mtrr.c / static int hvm_save_mtrr_msr(struct domain *d, 
hvm_domain_context_t *h):

hw_mtrr.msr_mtrr_def_type = mtrr_state->def_type
     | (mtrr_state->enabled << 10);

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

>>      m->overlapped = is_var_mtrr_overlapped(m);
>>
>> Looks like that function contains the necessary logic.
>
> You're right, but what happens there is that that function depends on
> the get_mtrr_range() function, which in turn depends on the size_or_mask
> global variable, which is initialized in hvm_mtrr_pat_init(), which then
> depends on a global table, and so on. Putting that into libxc is pretty
> much putting the whole mtrr.c file there.

This is where it gets tricky:

static void get_mtrr_range(uint64_t base_msr, uint64_t mask_msr,
                            uint64_t *base, uint64_t *end)
{
     [...]
     phys_addr = 36;

     if ( cpuid_eax(0x80000000) >= 0x80000008 )
         phys_addr = (uint8_t)cpuid_eax(0x80000008);

     size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
     [...]
}

specifically, in the cpuid_eax() call, which doesn't make much sense in 
dom0 userspace.

I did manage to take 'enabled' into account with what appears to be 
success, but if I've read the situation correctly, there's not much to 
do about 'overlap', unless we save it in hvm_save_mtrr_msr() like it's 
done with 'enabled'. What do you think?

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Ian Campbell]

On Wed, 2012-12-19 at 14:57 +0000, Razvan Cojocaru wrote:
> >>      m->overlapped = is_var_mtrr_overlapped(m);
> >>
> >> Looks like that function contains the necessary logic.
> >
> > You're right, but what happens there is that that function depends on
> > the get_mtrr_range() function, which in turn depends on the size_or_mask
> > global variable, which is initialized in hvm_mtrr_pat_init(), which then
> > depends on a global table, and so on. Putting that into libxc is pretty
> > much putting the whole mtrr.c file there.
> 
> This is where it gets tricky:
> 
> static void get_mtrr_range(uint64_t base_msr, uint64_t mask_msr,
>                             uint64_t *base, uint64_t *end)
> {
>      [...]
>      phys_addr = 36;
> 
>      if ( cpuid_eax(0x80000000) >= 0x80000008 )
>          phys_addr = (uint8_t)cpuid_eax(0x80000008);
> 
>      size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
>      [...]
> }
> 
> specifically, in the cpuid_eax() call, which doesn't make much sense in 
> dom0 userspace.

The fact that get_mtrr_range is querying the underlying physical CPUID
suggests it has something to do with the translation from virtual to
physical MTRR and is therefore not something userspace needs to worry
about, but I'm only speculating.

> I did manage to take 'enabled' into account with what appears to be 
> success, but if I've read the situation correctly, there's not much to 
> do about 'overlap', unless we save it in hvm_save_mtrr_msr() like it's 
> done with 'enabled'. What do you think?

It's not an architectural thing so I don't think it belongs in there.

TBH until someone figures out or explains what overlap actually is I
don't know if it even needs exporting or taking into account in
userspace.

Ian.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

>> I did manage to take 'enabled' into account with what appears to be
>> success, but if I've read the situation correctly, there's not much to
>> do about 'overlap', unless we save it in hvm_save_mtrr_msr() like it's
>> done with 'enabled'. What do you think?
>
> It's not an architectural thing so I don't think it belongs in there.
>
> TBH until someone figures out or explains what overlap actually is I
> don't know if it even needs exporting or taking into account in
> userspace.

Well, get_mtrr_range() is being called by is_var_mtrr_overlapped(), 
which is the function that sets m->overlapped, which is then used quite 
a lot in the logic of the get_mtrr_type(), which this patch attempts to 
bring into userspace via libxc.

I would quite happily discount all checks against the overlap boolean 
argument (and my code seems to work like that), but I suspect whoever 
wrote get_mtrr_type() had good reason to check for that.

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Andrew Cooper]

On 19/12/2012 15:00, Ian Campbell wrote:
> On Wed, 2012-12-19 at 14:57 +0000, Razvan Cojocaru wrote:
>>>>      m->overlapped = is_var_mtrr_overlapped(m);
>>>>
>>>> Looks like that function contains the necessary logic.
>>> You're right, but what happens there is that that function depends on
>>> the get_mtrr_range() function, which in turn depends on the size_or_mask
>>> global variable, which is initialized in hvm_mtrr_pat_init(), which then
>>> depends on a global table, and so on. Putting that into libxc is pretty
>>> much putting the whole mtrr.c file there.
>> This is where it gets tricky:
>>
>> static void get_mtrr_range(uint64_t base_msr, uint64_t mask_msr,
>>                             uint64_t *base, uint64_t *end)
>> {
>>      [...]
>>      phys_addr = 36;
>>
>>      if ( cpuid_eax(0x80000000) >= 0x80000008 )
>>          phys_addr = (uint8_t)cpuid_eax(0x80000008);
>>
>>      size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
>>      [...]
>> }
>>
>> specifically, in the cpuid_eax() call, which doesn't make much sense in 
>> dom0 userspace.
> The fact that get_mtrr_range is querying the underlying physical CPUID
> suggests it has something to do with the translation from virtual to
> physical MTRR and is therefore not something userspace needs to worry
> about, but I'm only speculating.

CPUID 0x80000008.EAX is the physical address size supported by the
processor (in bits).  Typical values on modern hardware are 40 or 48.

You need to know this information to work out which bits in the MTRR are
valid.  It is a variable scale of which bits are reserved.

Having said the above, this information is never going to change on the
same CPU, so should be cached once to save repeated emulates of cpuid.

~Andrew

>
>> I did manage to take 'enabled' into account with what appears to be 
>> success, but if I've read the situation correctly, there's not much to 
>> do about 'overlap', unless we save it in hvm_save_mtrr_msr() like it's 
>> done with 'enabled'. What do you think?
> It's not an architectural thing so I don't think it belongs in there.
>
> TBH until someone figures out or explains what overlap actually is I
> don't know if it even needs exporting or taking into account in
> userspace.
>
> Ian.
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

> TBH until someone figures out or explains what overlap actually is I
> don't know if it even needs exporting or taking into account in
> userspace.

http://www.mjmwired.net/kernel/Documentation/mtrr.txt

"Creating overlapping MTRRs:

%echo "base=0xfb000000 size=0x1000000 type=write-combining" >/proc/mtrr
%echo "base=0xfb000000 size=0x1000 type=uncachable" >/proc/mtrr

And the results: cat /proc/mtrr
reg00: base=0x00000000 (   0MB), size=  64MB: write-back, count=1
reg01: base=0xfb000000 (4016MB), size=  16MB: write-combining, count=1
reg02: base=0xfb000000 (4016MB), size=   4kB: uncachable, count=1"

Overlapped MTRRs are MTRRs with the same base, but different types of 
addresses at different offsets.

At least to my understanding.

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Ian Campbell]

On Wed, 2012-12-19 at 15:27 +0000, Andrew Cooper wrote:
> On 19/12/2012 15:00, Ian Campbell wrote:
> > On Wed, 2012-12-19 at 14:57 +0000, Razvan Cojocaru wrote:
> >>>>      m->overlapped = is_var_mtrr_overlapped(m);
> >>>>
> >>>> Looks like that function contains the necessary logic.
> >>> You're right, but what happens there is that that function depends on
> >>> the get_mtrr_range() function, which in turn depends on the size_or_mask
> >>> global variable, which is initialized in hvm_mtrr_pat_init(), which then
> >>> depends on a global table, and so on. Putting that into libxc is pretty
> >>> much putting the whole mtrr.c file there.
> >> This is where it gets tricky:
> >>
> >> static void get_mtrr_range(uint64_t base_msr, uint64_t mask_msr,
> >>                             uint64_t *base, uint64_t *end)
> >> {
> >>      [...]
> >>      phys_addr = 36;
> >>
> >>      if ( cpuid_eax(0x80000000) >= 0x80000008 )
> >>          phys_addr = (uint8_t)cpuid_eax(0x80000008);
> >>
> >>      size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
> >>      [...]
> >> }
> >>
> >> specifically, in the cpuid_eax() call, which doesn't make much sense in 
> >> dom0 userspace.
> > The fact that get_mtrr_range is querying the underlying physical CPUID
> > suggests it has something to do with the translation from virtual to
> > physical MTRR and is therefore not something userspace needs to worry
> > about, but I'm only speculating.
> 
> CPUID 0x80000008.EAX is the physical address size supported by the
> processor (in bits).  Typical values on modern hardware are 40 or 48.

I know what the bit is. This code seems to be leaking physical CPU
parameters into the virtual CPU state and the question is if userspace
needs to care about that. I suspect the answer is no.

What should matter for the guest state is the virtualised CPUID
0x80000008.EAX which, at least in theory, could be different (e.g. a
migrated guest?). 

Ian.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Andrew Cooper]

On 19/12/2012 15:54, Ian Campbell wrote:
> On Wed, 2012-12-19 at 15:27 +0000, Andrew Cooper wrote:
>> On 19/12/2012 15:00, Ian Campbell wrote:
>>> On Wed, 2012-12-19 at 14:57 +0000, Razvan Cojocaru wrote:
>>>>>>      m->overlapped = is_var_mtrr_overlapped(m);
>>>>>>
>>>>>> Looks like that function contains the necessary logic.
>>>>> You're right, but what happens there is that that function depends on
>>>>> the get_mtrr_range() function, which in turn depends on the size_or_mask
>>>>> global variable, which is initialized in hvm_mtrr_pat_init(), which then
>>>>> depends on a global table, and so on. Putting that into libxc is pretty
>>>>> much putting the whole mtrr.c file there.
>>>> This is where it gets tricky:
>>>>
>>>> static void get_mtrr_range(uint64_t base_msr, uint64_t mask_msr,
>>>>                             uint64_t *base, uint64_t *end)
>>>> {
>>>>      [...]
>>>>      phys_addr = 36;
>>>>
>>>>      if ( cpuid_eax(0x80000000) >= 0x80000008 )
>>>>          phys_addr = (uint8_t)cpuid_eax(0x80000008);
>>>>
>>>>      size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
>>>>      [...]
>>>> }
>>>>
>>>> specifically, in the cpuid_eax() call, which doesn't make much sense in 
>>>> dom0 userspace.
>>> The fact that get_mtrr_range is querying the underlying physical CPUID
>>> suggests it has something to do with the translation from virtual to
>>> physical MTRR and is therefore not something userspace needs to worry
>>> about, but I'm only speculating.
>> CPUID 0x80000008.EAX is the physical address size supported by the
>> processor (in bits).  Typical values on modern hardware are 40 or 48.
> I know what the bit is. This code seems to be leaking physical CPU
> parameters into the virtual CPU state and the question is if userspace
> needs to care about that. I suspect the answer is no.
>
> What should matter for the guest state is the virtualised CPUID
> 0x80000008.EAX which, at least in theory, could be different (e.g. a
> migrated guest?). 
>
> Ian.
>

Ah - I see your concern.  Yes - it might well be different.  Is this
information passed in an HVM save record?  It does not appear to be
associated with the MTRR HVM save record.

An HVM domain, booted on processor with 48bit physical addressing, which
makes use of the upper 8 bits when setting up the variable MTRRs, which
is subsequently migrated to a different server with only 40bit physical
addressing is going to have problems.  Whether a protection fault occurs
or the top bits are simply ignored, stuff will break.

As a result, I think the virtualised CPUID value needs feature levelling
across hosts, or restrictions applied to where you can migrate a started
VM to.

~Andrew

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Ian Campbell]

On Wed, 2012-12-19 at 16:11 +0000, Andrew Cooper wrote:
> On 19/12/2012 15:54, Ian Campbell wrote:
> > On Wed, 2012-12-19 at 15:27 +0000, Andrew Cooper wrote:
> >> On 19/12/2012 15:00, Ian Campbell wrote:
> >>> On Wed, 2012-12-19 at 14:57 +0000, Razvan Cojocaru wrote:
> >>>>>>      m->overlapped = is_var_mtrr_overlapped(m);
> >>>>>>
> >>>>>> Looks like that function contains the necessary logic.
> >>>>> You're right, but what happens there is that that function depends on
> >>>>> the get_mtrr_range() function, which in turn depends on the size_or_mask
> >>>>> global variable, which is initialized in hvm_mtrr_pat_init(), which then
> >>>>> depends on a global table, and so on. Putting that into libxc is pretty
> >>>>> much putting the whole mtrr.c file there.
> >>>> This is where it gets tricky:
> >>>>
> >>>> static void get_mtrr_range(uint64_t base_msr, uint64_t mask_msr,
> >>>>                             uint64_t *base, uint64_t *end)
> >>>> {
> >>>>      [...]
> >>>>      phys_addr = 36;
> >>>>
> >>>>      if ( cpuid_eax(0x80000000) >= 0x80000008 )
> >>>>          phys_addr = (uint8_t)cpuid_eax(0x80000008);
> >>>>
> >>>>      size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
> >>>>      [...]
> >>>> }
> >>>>
> >>>> specifically, in the cpuid_eax() call, which doesn't make much sense in 
> >>>> dom0 userspace.
> >>> The fact that get_mtrr_range is querying the underlying physical CPUID
> >>> suggests it has something to do with the translation from virtual to
> >>> physical MTRR and is therefore not something userspace needs to worry
> >>> about, but I'm only speculating.
> >> CPUID 0x80000008.EAX is the physical address size supported by the
> >> processor (in bits).  Typical values on modern hardware are 40 or 48.
> > I know what the bit is. This code seems to be leaking physical CPU
> > parameters into the virtual CPU state and the question is if userspace
> > needs to care about that. I suspect the answer is no.
> >
> > What should matter for the guest state is the virtualised CPUID
> > 0x80000008.EAX which, at least in theory, could be different (e.g. a
> > migrated guest?). 
> >
> > Ian.
> >
> 
> Ah - I see your concern.  Yes - it might well be different.  Is this
> information passed in an HVM save record?  It does not appear to be
> associated with the MTRR HVM save record.

No , this is the internal state not the save record but Razvan is
implementing the same logic in libxc which must necessarily be based on
the hvm saved state only and not the internal emulation state.

> An HVM domain, booted on processor with 48bit physical addressing, which
> makes use of the upper 8 bits when setting up the variable MTRRs, which
> is subsequently migrated to a different server with only 40bit physical
> addressing is going to have problems.  Whether a protection fault occurs
> or the top bits are simply ignored, stuff will break.

This is HVM, so I would hope the hypervisor would catch it and do
something sensible before it hits the real (or VMCS) registers.

> As a result, I think the virtualised CPUID value needs feature levelling
> across hosts, or restrictions applied to where you can migrate a started
> VM to.

This seems likely. 

Ian.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

>> Ah - I see your concern.  Yes - it might well be different.  Is this
>> information passed in an HVM save record?  It does not appear to be
>> associated with the MTRR HVM save record.
>
> No , this is the internal state not the save record but Razvan is
> implementing the same logic in libxc which must necessarily be based on
> the hvm saved state only and not the internal emulation state.

Exactly, and all I need extra an extra variable in the save record (or 
simply a single bit in a safe place in one of the existing ones), 
telling me if the MTRRs are overlapped or not. The CPUID code is just a 
part of the logic that finds this out in the hypervisor; and 
specifically it is a part that's better _left_in_ the hypervisor. That 
is in fact what I was trying to say a few messages ago, when I called 
the cpuid_eax() function the tricky part. :)

Do we agree that a bool_t overlapped should be added to struct 
hvm_hw_mtrr for this case?

Thanks,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Tim Deegan]

At 18:29 +0200 on 19 Dec (1355941750), Razvan Cojocaru wrote:
> >>Ah - I see your concern.  Yes - it might well be different.  Is this
> >>information passed in an HVM save record?  It does not appear to be
> >>associated with the MTRR HVM save record.
> >
> >No , this is the internal state not the save record but Razvan is
> >implementing the same logic in libxc which must necessarily be based on
> >the hvm saved state only and not the internal emulation state.
> 
> Exactly, and all I need extra an extra variable in the save record (or 
> simply a single bit in a safe place in one of the existing ones), 
> telling me if the MTRRs are overlapped or not. The CPUID code is just a 
> part of the logic that finds this out in the hypervisor; and 
> specifically it is a part that's better _left_in_ the hypervisor. That 
> is in fact what I was trying to say a few messages ago, when I called 
> the cpuid_eax() function the tricky part. :)
> 
> Do we agree that a bool_t overlapped should be added to struct 
> hvm_hw_mtrr for this case?

Sorry, no.  The 'overlapped' bit is a piece of xen implementation, and
not an architectural state of the VM, so it doesn't belong in the save
record.  It's trivial to recalculate in a user-space tool, and the
result can be cached (since you can also get a mem-event on MSR writes,
you don't have to pull all this MTRR state out of the giuest except when
the MTRRs have been changed).

I'll take a proper look at this thread tomorrow and see if I can suggest
anything more helpful. 

Tim.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

> Sorry, no.  The 'overlapped' bit is a piece of xen implementation, and
> not an architectural state of the VM, so it doesn't belong in the save
> record.  It's trivial to recalculate in a user-space tool, and the

Is it trivial to recalculate that in a userspace tool? I thought the
only way to properly calculate that in the first place was to use
cpuid_eax() in the calculation, which is only available at hypervisor
level (hence much of this thread). Am I missing something?

> result can be cached (since you can also get a mem-event on MSR writes,
> you don't have to pull all this MTRR state out of the giuest except when
> the MTRRs have been changed).

Unfortunately, due to the design of components I don't control, my
application does not have the luxury of waiting for an MSR write event.
(Has my MSR mem_event patch been acknowledged? Or was this already work
in progress? I had to patch my Xen source code to be able to get MSR
events...)

> I'll take a proper look at this thread tomorrow and see if I can suggest
> anything more helpful. 

Thank you, I appreciate the reply, and your time.

All the best,
Razvan Cojocaru

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Tim Deegan]

At 16:57 +0200 on 19 Dec (1355936221), Razvan Cojocaru wrote:
> >>     m->overlapped = is_var_mtrr_overlapped(m);
> >>
> >>Looks like that function contains the necessary logic.
> >
> >You're right, but what happens there is that that function depends on
> >the get_mtrr_range() function, which in turn depends on the size_or_mask
> >global variable, which is initialized in hvm_mtrr_pat_init(), which then
> >depends on a global table, and so on. Putting that into libxc is pretty
> >much putting the whole mtrr.c file there.
> 
> This is where it gets tricky:
> 
> static void get_mtrr_range(uint64_t base_msr, uint64_t mask_msr,
>                            uint64_t *base, uint64_t *end)
> {
>     [...]
>     phys_addr = 36;
> 
>     if ( cpuid_eax(0x80000000) >= 0x80000008 )
>         phys_addr = (uint8_t)cpuid_eax(0x80000008);
> 
>     size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
>     [...]
> }
> 
> specifically, in the cpuid_eax() call, which doesn't make much sense in 
> dom0 userspace.

You can execute the CPUID instruction from dom0 userspace, and get
exactly the same result as Xen does.

(It may, separately, be a bug that Xen uses the host CPUID here and not
a well-defined guest width.  If that gets fixed, the guest address width
will be made available in the save record and you can extract it from
there.)

But also, you don't necessarily need to calculate 'overlapped' for each
MTRR in advance.  It may make sense to do so for speed, as Xen does, but
you could also just handle overlapping MTRRS in your lookup function, by
comparing the address to all MTRRs and handling the case where it
matches more than one.

Tim.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Tim Deegan]

At 21:35 +0200 on 19 Dec (1355952919), Razvan Cojocaru wrote:
> > result can be cached (since you can also get a mem-event on MSR writes,
> > you don't have to pull all this MTRR state out of the giuest except when
> > the MTRRs have been changed).
> 
> Unfortunately, due to the design of components I don't control, my
> application does not have the luxury of waiting for an MSR write event.

Hmm.  In that case I guess you can't cache the MTRR state between
lookups, which may be a performance problem for you.

> (Has my MSR mem_event patch been acknowledged? Or was this already work
> in progress? I had to patch my Xen source code to be able to get MSR
> events...)

I've replied to that separately. 

Cheers,

Tim.

Re: [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Razvan Cojocaru]

 >> Unfortunately, due to the design of components I don't control, my
 >> application does not have the luxury of waiting for an MSR write event.
 >
 > Hmm.  In that case I guess you can't cache the MTRR state between
 > lookups, which may be a performance problem for you.

Indeed, however I do need the ability to pull the MTRR type for a given 
address, via a userspace application, from the hypervisor.

I'll play around with the libxc implementation and see what I can come 
up with, then if it'll all seem to work out I'll send the patch to 
xen-devel.

Thanks,
Razvan Cojocaru