Re: [PATCH 3/4] x86: Adjust arch LBR CPU policy

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.11.2024 09:49, ngoc-tu.dinh@xxxxxxxxxx wrote:
> From: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
> 
> Allow virtual arch LBR with a single depth that's equal to that of the
> host. If this is not possible, disable arch LBR altogether.

How will this work across migration?

What does "single depth that's equal to that of the host" mean, when
multiple depths can be advertised as supported? Perhaps I'm irritated
by ...

> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -638,6 +638,36 @@ static void __init calculate_pv_max_policy(void)
>      p->extd.raw[0xa] = EMPTY_LEAF; /* No SVM for PV guests. */
>  }
>  
> +/*
> + * Allow virtual arch LBR with a single depth that's equal to that of the
> + * host. If this is not possible, disable arch LBR altogether.
> + */
> +static void adjust_arch_lbr_depth(uint32_t fs[FEATURESET_NR_ENTRIES])
> +{
> +    uint64_t host_lbr_depth;
> +    bool lbr_supported = true;
> +
> +    rdmsrl(MSR_IA32_LASTBRANCH_DEPTH, host_lbr_depth);

... you reading an MSR here which was never set by Xen. Whatever the firmware
left there should not be relevant to us. 

> +    if ((host_lbr_depth == 0) ||
> +        (host_lbr_depth % 8) ||
> +        (host_lbr_depth > 64))
> +        lbr_supported = false;

Here and below: Please familiarize yourself with Xen coding style. if() and
alike want blanks immediately inside the parentheses.

> +    host_lbr_depth = 1ul << ((host_lbr_depth / 8) - 1);
> +    if ((host_lbr_depth & fs[FEATURESET_1Ca] & 0xff) == 0)
> +        lbr_supported = false;
> +
> +    if (lbr_supported)
> +    {
> +        fs[FEATURESET_1Ca] = (fs[FEATURESET_1Ca] & ~0xffu) | host_lbr_depth;
> +    }
> +    else
> +    {
> +        __clear_bit(X86_FEATURE_ARCH_LBR, fs);
> +        fs[FEATURESET_1Ca] = fs[FEATURESET_1Cb] = fs[FEATURESET_1Cc] = 0;
> +    }
> +}

Hmm, is it really a good idea to fiddle with the featureset, rather than (after
conversion back) with the policy? Cc-ing Andrew. This would then also avoid use
of several plain integer literals.

Jan