Re: [PATCH v2 1/2] KVM: arm64: VHE: Initialize PMSCR_EL1

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Oliver,

On Wed, Sep 03, 2025 at 11:55:00PM -0700, Oliver Upton wrote:
> On Tue, Sep 02, 2025 at 02:08:32PM +0100, Alexandru Elisei wrote:
> > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > index 381382c19fe4..e7ce0d5a622d 100644
> > --- a/arch/arm64/kvm/debug.c
> > +++ b/arch/arm64/kvm/debug.c
> > @@ -74,13 +74,19 @@ void kvm_init_host_debug_data(void)
> >  	*host_data_ptr(debug_brps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, BRPs, dfr0);
> >  	*host_data_ptr(debug_wrps) = SYS_FIELD_GET(ID_AA64DFR0_EL1, WRPs, dfr0);
> >  
> > +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_PMSVer_SHIFT) &&
> > +	    !(read_sysreg_s(SYS_PMBIDR_EL1) & PMBIDR_EL1_P)) {
> > +		if (has_vhe()) {
> > +			/* Clear E{0,1}SPE, which reset to UNKNOWN values. */
> > +			write_sysreg_el1(0, SYS_PMSCR);
> > +		} else {
> > +			host_data_set_flag(HAS_SPE);
> > +		}
> > +	}
> > +
> 
> nit: While this is correct, from a code organization perspective it
> doesn't belong here. The rest of this function is concerned with probing
> hardware state and initializing the corresponding host data.

Yes, you're totally right.

> 
> I prefer Marc's suggestion which could be wrapped up in a function called
> 'kvm_debug_init_vhe()' or similar.

I forgot about Marc's suggestion, my bad. Will do that for the next iteration.

Thanks,
Alex

> 
> Thanks,
> Oliver