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

[Marc Zyngier <maz@kernel.org>]

On Thu, 19 Dec 2024 18:00:24 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> 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.

Ah crap, I missed the copy of PMCR_EL0.N. Fair enough then.

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

Yeah, something like that is probably OK, at least for now.

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

A full load/put is overkill indeed. But something more lightweight and
limited to the debug stuff may be useful. That may cause some
restructuring, but probably worth it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.