Re: [Xen-devel] [PATCH v2 4/9] x86/pagewalk: Clean up guest_supports_* predicates

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/03/17 08:45, Jan Beulich wrote:
>>>> On 16.03.17 at 17:31, <andrew.cooper3@xxxxxxxxxx> wrote:
>> Switch them to returning bool, and taking const parameters.
>>
>> Rename guest_supports_superpages() to guest_supports_l2_superpages() to
>> indicate which level of pagetables it is actually referring to, and rename
>> guest_supports_1G_superpages() to guest_supports_l3_superpages() for
>> consistency.
>>
>> guest_supports_l3_superpages() is a static property of the domain, rather 
>> than
>> control register settings, so is switched to take a domain pointer.
>> hvm_pse1gb_supported() is inlined into its sole user because it isn't 
>> strictly
>> hvm-specific (it is hap-specific) and really should be beside a comment
>> explaining why the cpuid policy is ignored.
>>
>> While cleaning up part of the file, clean up all trailing whilespace, and fix
>> one comment which accidently refered to PG living in CR4 rather than CR0.
>>
>> Requested-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with two remarks:
>
>> -static inline int
>> -guest_supports_1G_superpages(struct vcpu *v)
>> +static inline bool guest_supports_l3_superpages(const struct domain *d)
>>  {
>> -    return (GUEST_PAGING_LEVELS >= 4 && hvm_pse1gb_supported(v->domain));
>> +    /*
>> +     * There are no control register settings for the hardware pagewalk on 
>> the
>> +     * subject of 1G superpages.
>> +     *
>> +     * If the guest constructs a 1GB superpage on capable hardware, it will
>> +     * function irrespective of whether the feature is advertised.  Xen's
>> +     * model of performing a pagewalk should match.
>> +     */
>> +    return GUEST_PAGING_LEVELS >= 4 && paging_mode_hap(d) && 
>> cpu_has_page1gb;
> Is it perhaps worth adding half a sentence stating that shadow
> doesn't support 1Gb pages at all?

Good point.

>
> Also I'm still not really happy with the guest_supports_ prefixes
> for this and its L2 counterpart: The question here isn't whether the
> guest supports it (we can't know whether it does), but whether it
> enabled PSE/PAE/LM. Arguably the L3 case is less clear because
> of the mentioned lack of an explicit enabled bit, so I can live with
> the patch going in unchanged (the L2 side then simply for things
> to remain consistent, albeit there's then already the difference of
> parameter types).

How would you prefer them to be named?

~Andrew

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