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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi

On 30.10.25 14:47, Jan Beulich wrote:
> On 30.10.2025 13:28, Grygorii Strashko wrote:
> > On 30.10.25 13:08, Jan Beulich wrote:
> > > 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.
> > yes. it is patch 3 - It is not small.
> > And I really wanted this patch to contain as less modifications as possible on
> > top of code moving.
> I wonder what other x86 maintainers think here.
>
> > > 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.
> > Agree. but..
> > In this patch I wanted to minimize changes and do modifications step by step.
> >
> > I can add additional patch such as "rework struct domain access in cache disable 
> > mode code".
> > Will it work?
I'm planning to resend with:
- incorporating struct domain * as parameter in hvm_set_uc_mode()

> > > > +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.
- incorporating struct domain * as parameter local var

> > > >    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.
> > Right. Hence vmx_x_guest_pat() are also used by vmx_handle_cd() I decided to 
> > put before them.
> The main purpose of vmx_set_guest_pat() is, however, its use as a hook function.
> It's merely an optimization that the function is called directly by VMX code.
- moving code before vmx_handle_cd().

> > > Bottom line: The change could go in as is, but imo it would be nice if it
> > > was tidied some while moving.
> > I'd be very much appreciated if this could happen.
> "this" being what out of the two or more possible options? (I take it you mean
> "could go in as is", but that's guesswork.)
I'm not goint to squah rest of the series.

--
Best regards,
-grygorii