Re: [XEN][PATCH 1/3] x86/hvm: move hvm_shadow_handle_cd() in vmx code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.10.2025 00:54, Grygorii Strashko wrote:
> From: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> 
> Functions:
>  hvm_shadow_handle_cd()
>  hvm_set_uc_mode()
>  domain_exit_uc_mode()
> are used only by Intel VMX code, so move them in VMX code.

Nit: I think both in the title and here you mean "to" or "into".

> While here:
> - minor format change in domain_exit_uc_mode()
> - s/(0/1)/(false/true) for bool types
> 
> No functional changes.
> 
> Signed-off-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>

You did read Andrew's request to also move the involved structure field(s),
didn't you? Oh, wait - maybe that's going to be the subject of patch 3. While
often splitting steps helps, I'm not sure that's very useful here. You're
touching again immediately what you just have moved, all to reach a single
goal.

> @@ -1421,6 +1422,64 @@ static void cf_check vmx_set_segment_register(
>      vmx_vmcs_exit(v);
>  }
>  
> +/* Exit UC mode only if all VCPUs agree on MTRR/PAT and are not in no_fill. 
> */
> +static bool domain_exit_uc_mode(struct vcpu *v)
> +{
> +    struct domain *d = v->domain;
> +    struct vcpu *vs;
> +
> +    for_each_vcpu(d, vs)
> +    {
> +        if ( (vs == v) || !vs->is_initialised )
> +            continue;
> +        if ( (vs->arch.hvm.cache_mode == NO_FILL_CACHE_MODE) ||
> +             mtrr_pat_not_equal(vs, v) )
> +            return false;
> +    }
> +
> +    return true;
> +}
> +
> +static void hvm_set_uc_mode(struct vcpu *v, bool is_in_uc_mode)
> +{
> +    v->domain->arch.hvm.is_in_uc_mode = is_in_uc_mode;
> +    shadow_blow_tables_per_domain(v->domain);
> +}

Similarly I wonder whether this function wouldn't better change to taking
struct domain * right away. "v" itself is only ever used to get hold of
its domain. At the call sites this will then make obvious that this is a
domain-wide operation.

> +static void hvm_shadow_handle_cd(struct vcpu *v, unsigned long value)
> +{
> +    if ( value & X86_CR0_CD )
> +    {
> +        /* Entering no fill cache mode. */
> +        spin_lock(&v->domain->arch.hvm.uc_lock);
> +        v->arch.hvm.cache_mode = NO_FILL_CACHE_MODE;
> +
> +        if ( !v->domain->arch.hvm.is_in_uc_mode )
> +        {
> +            domain_pause_nosync(v->domain);
> +
> +            /* Flush physical caches. */
> +            flush_all(FLUSH_CACHE_EVICT);
> +            hvm_set_uc_mode(v, true);
> +
> +            domain_unpause(v->domain);
> +        }
> +        spin_unlock(&v->domain->arch.hvm.uc_lock);
> +    }
> +    else if ( !(value & X86_CR0_CD) &&
> +              (v->arch.hvm.cache_mode == NO_FILL_CACHE_MODE) )
> +    {
> +        /* Exit from no fill cache mode. */
> +        spin_lock(&v->domain->arch.hvm.uc_lock);
> +        v->arch.hvm.cache_mode = NORMAL_CACHE_MODE;
> +
> +        if ( domain_exit_uc_mode(v) )
> +            hvm_set_uc_mode(v, false);
> +
> +        spin_unlock(&v->domain->arch.hvm.uc_lock);
> +    }
> +}

This function, in turn, could do with a local struct domain *d.

>  static int cf_check vmx_set_guest_pat(struct vcpu *v, u64 gpat)
>  {
>      if ( !paging_mode_hap(v->domain) ||

Why did you put the code above this function? It's solely a helper of
vmx_handle_cd(), so would imo best be placed immediately ahead of that one.

Bottom line: The change could go in as is, but imo it would be nice if it
was tidied some while moving.

Jan