Re: [Xen-devel] [PATCH] x86/shadow: Correct the behaviour of a guests view of maxphysaddr

[Tim Deegan <tim@xxxxxxx>]

At 15:29 +0000 on 08 Feb (1486567798), Andrew Cooper wrote:
> On Intel, a guest can get an PTE shadowed which should have faulted and
> didn't (because the smfn is actually within 39 bits)

Yes.

, while on AMD, a
> guest can try to create a PTE which shouldn't fault (because CPUID says
> anything up to 48 bits is fine) yet does fault because the shadow code
> rejects anything above 44 bits.

But in the code you're replacing (just below) the CPUID value is
capped at the paging.gfn_bits+PAGE_SHIFT, so won't it report 44?

> >> -    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
> >> -    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
> >> -                                d->arch.paging.gfn_bits + PAGE_SHIFT);
> >> -    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
> >> -                                (p->basic.pae || p->basic.pse36) ? 36 : 
> >> 32);
> >> +    maxphysaddr_limit = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
> >> +
> >> +    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
> >> +         (!is_pv_domain(d) || opt_allow_superpage) )
> >> +    {
> >> +        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
> >> +        maxphysaddr_limit = min(maxphysaddr_limit, 32U + PAGE_SHIFT);
> > I don't like this implementation detail escaping from x86/mm/shadow;
> > can we have a cpuid_limit_paddr_bits(d) that the shadow code can call
> > to tell cpuid about this?  Or perhaps a paging_max_paddr_bits(d) so
> > cpuid can ask paging when it needs to know?
> 
> I will see about doing this.
> 
> > In either case I wonder whether there needs to be some trigger of
> > recalculate_cpuid_policy() once shadow mode is enabled -- I guess
> > currently it relies on it being called by something late enough in hvm
> > domain creation?
> 
> Yes, although now I am worried about migrating PV guests.  I will double
> check to see whether that path works sensibly or not, because we can't
> have maxphysaddr suddenly shrink under the feet of a running guest.

We're lucky here: the shadow width restriction doesn't apply to PV
guests.

> >> +    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr, 
> >> maxphysaddr_limit);
> >> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
> > No longer requiring 36 bits for pae/pse36?  This is mentioned in the
> > description but not explained.
> 
> 32/36 is only the default to be assumed if no maxphysaddr is provided. 
> We always provide maxphysaddr, and passing 32 on a massive machine is
> legitimate.

OK.

Tim.

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