Re: [PATCH v2 3/5] KVM: s390: refactor some functions in priv.c

[Heiko Carstens <hca@linux.ibm.com>]

On Tue, May 20, 2025 at 08:26:37PM +0200, Claudio Imbrenda wrote:
> Refactor some functions in priv.c to make them more readable.
> 
> handle_{iske,rrbe,sske}: move duplicated checks into a single function.
> handle{pfmf,epsw}: improve readability.
> handle_lpswe{,y}: merge implementations since they are almost the same.
> 
> Use a helper function to replace open-coded bit twiddling operations.
> 
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

...since you asked me to look into this... :)

For the sake of reviewability: I guess this really should be split
into separate patches which address one function each.

> +static inline void replace_selected_bits(u64 *w, unsigned long mask, unsigned long val)
> +{
> +	*w = (*w & ~mask) | (val & mask);
> +}
> +
> +struct skeys_ops_state {
> +	int reg1;
> +	int reg2;
> +	u64 *r1;
> +	u64 *r2;
> +	unsigned long effective;
> +	unsigned long absolute;
> +};
> +
> +static void get_regs_rre_ptr(struct kvm_vcpu *vcpu, int *reg1, int *reg2, u64 **r1, u64 **r2)
> +{
> +	kvm_s390_get_regs_rre(vcpu, reg1, reg2);
> +	*r1 = vcpu->run->s.regs.gprs + *reg1;
> +	*r2 = vcpu->run->s.regs.gprs + *reg2;
> +}

Ewww...

> +static int skeys_common_checks(struct kvm_vcpu *vcpu, struct skeys_ops_state *state)
> +{
> +	int rc;
> +
> +	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE) {
> +		rc = kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
> +		return rc ? rc : -EAGAIN;
> +	}

Hm.. first you introduce helper functions which use psw_bits() and now
this is open-coded again?

> +	rc = try_handle_skey(vcpu);
> +	if (rc)
> +		return rc;
> +
> +	get_regs_rre_ptr(vcpu, &state->reg1, &state->reg2, &state->r1, &state->r2);
> +
> +	state->effective = vcpu->run->s.regs.gprs[state->reg2] & PAGE_MASK;
> +	state->effective = kvm_s390_logical_to_effective(vcpu, state->effective);
> +	state->absolute = kvm_s390_real_to_abs(vcpu, state->effective);
> +
> +	return 0;
> +}

So a function which is called "*common_checks" actually may or may not
set up a state which is later used. This is anything but obvious.

>  static int handle_iske(struct kvm_vcpu *vcpu)
>  {

...

> -	vcpu->run->s.regs.gprs[reg1] &= ~0xff;
> -	vcpu->run->s.regs.gprs[reg1] |= key;
> +	replace_selected_bits(state.r1, 0xff, key);

Who is supposed to understand that this replace_selected_bits() call
actually changes vcpu->run->s.regs.gprs[reg1]? To me this obfuscates
the code and makes it much less understandable.

From my point of view this state structure and passing it back and
forth is a mistake, since it hides way too much what is actually going
on.

Anyway, just my 0.02. :)