Re: [PATCH v3 12/16] KVM: arm64: Compute MDCR_EL2 at vcpu_load()

[Oliver Upton <oliver.upton@linux.dev>]

On Wed, Dec 18, 2024 at 06:40:46PM +0000, Marc Zyngier wrote:
> On Mon, 09 Dec 2024 18:09:22 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > @@ -288,6 +270,12 @@ void kvm_debug_set_guest_ownership(struct kvm_vcpu *vcpu)
> >  
> >  	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
> >  	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
> > +
> > +	preempt_disable();
> > +	kvm_arm_setup_mdcr_el2(vcpu);
> > +	if (has_vhe())
> > +		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
> > +	preempt_enable();
> 
> I'm not confident that the preemption_disable() does anything.
> Specially given that it doesn't include setting the ownership as part
> of the critical section. So if that works at it is now, then the
> critical section is pointless.

We use percpu data to construct mdcr_el2, so disabling preemption
ensures that we use the right copy + stick mdcr in the right CPU.

But point taken about guest ownership, nothing stops preemption from
blowing away the GUEST_OWNED state.

> I also find it rather fragile that we have multiple places where we
> write MDCR_EL2, and I would love this to go for good. Any idea?

Totally agree, that was at least one of the motivators for this series
originally. For v4 I'm planning to hoist the preempt_disable() + programming
of MDCR_EL2 into kvm_arm_setup_mdcr_el2(), which would be shared w/
vcpu_load() and hide the details of how MDCR gets set up.

I still want us to have a way of reconfiguring MDCR_EL2 on the fly to
lazily restore the debug registers without going through an entire
put/load. I'm thinking that eventually we can do the same thing for the
PMU when Colton + co have partitioned PMU support worked out.

-- 
Thanks,
Oliver