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