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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

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).

Jan


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