Re: [Xen-devel] [PATCH] libxc: Add xc_domain_hvm_get_mtrr_type() call

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel