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

Re: [Xen-devel] [PATCH v2] x86/shadow: Correct guest behaviour when creating PTEs above maxphysaddr



On 13/02/17 12:36, Jan Beulich wrote:
>>>> On 13.02.17 at 12:00, <andrew.cooper3@xxxxxxxxxx> wrote:
>> @@ -502,11 +503,9 @@ void recalculate_cpuid_policy(struct domain *d)
>>  
>>      cpuid_featureset_to_policy(fs, p);
>>  
>> -    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);
>> +                                paging_max_paddr_bits(d));
>> +    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
> The re-write of the commit message doesn't appear to have
> resulted in the PAE/PSE36 part being explained any better. I'm
> also unconvinced that exposing namely PSE36 to a guest
> with less than 36 physical address bits would be compatible with
> old OSes not knowing of CPUID leaf 0x80000008 yet - they
> would legitimately assume 36 bits. The same presumably also
> goes for PAE.

Hmm ok.  With the other bugfix of not dropping the first line, this hunk
is now simply:

@@ -504,7 +505,7 @@ void recalculate_cpuid_policy(struct domain *d)
 
     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);
+                                paging_max_paddr_bits(d));
     p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
                                 (p->basic.pae || p->basic.pse36) ? 36 :
32);
 

>
>> @@ -360,6 +361,21 @@ void paging_dump_vcpu_info(struct vcpu *v);
>>  int paging_set_allocation(struct domain *d, unsigned int pages,
>>                            bool *preempted);
>>  
>> +/* Maxphysaddr supportable by the paging infrastructure. */
>> +static inline unsigned int paging_max_paddr_bits(const struct domain *d)
>> +{
>> +    unsigned int bits = 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. */
>> +        bits = min(bits, 32U + PAGE_SHIFT);
>> +    }
>> +
>> +    return bits;
>> +}
> Does this really need to be an inline function? With the overall goal
> of not including every header everywhere, I particularly dislike the
> need to include xen/kconfig.h here for things to build.

It is not on a critically path, but it seems wasteful to force something
this small to be out of line.

As for kconfig.h, it is tiny.  What is the problem with adding it here? 
We already pull generated/autoconf.h into everything via xen/config.h

~Andrew

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

 


Rackspace

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