Re: [PATCH v2 1/2] KVM: s390: load guest access registers in MEM_OP ioctl

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

On Thu, Feb 15, 2024 at 09:53:43PM +0100, Eric Farman wrote:
> The routine ar_translation() can be reached by both the instruction
> intercept path (where the access registers had been loaded with the
> guest register contents), and the MEM_OP ioctls (which hadn't).
> This latter case means that any ALET the guest expects to be used
> would be ignored.
> 
> Fix this by swapping the host/guest access registers around the
> MEM_OP ioctl, in the same way that the KVM_RUN ioctl does with
> sync_regs()/store_regs(). The full register swap isn't needed here,
> since only the access registers are used in this interface.
> 
> Introduce a boolean in the kvm_vcpu_arch struct to indicate the
> guest ARs have been loaded into the registers. This permits a
> warning to be emitted if entering this path without a proper
> register setup.
> 
> Suggested-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  1 +
>  arch/s390/kvm/gaccess.c          |  2 ++
>  arch/s390/kvm/kvm-s390.c         | 11 +++++++++++
>  3 files changed, 14 insertions(+)
...
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 5bfcc50c1a68..33587bb4c9e8 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -391,6 +391,8 @@ static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce, u8 ar,
>  	if (ar >= NUM_ACRS)
>  		return -EINVAL;
>  
> +	WARN_ON_ONCE(!vcpu->arch.acrs_loaded);
> +
>  	save_access_regs(vcpu->run->s.regs.acrs);

Why not simply:

	if (vcpu->arch.acrs_loaded)
		save_access_regs(vcpu->run->s.regs.acrs);

?

This will always work, and the WARN_ON_ONCE() would not be needed. Besides
that: _if_ the WARN_ON_ONCE() would trigger, damage would have happened
already: host registers would have been made visible to the guest.

Or did I miss anything?

> +	/* Swap host/guest access registers */
> +	save_access_regs(vcpu->arch.host_acrs);
> +	restore_access_regs(vcpu->run->s.regs.acrs);
> +	vcpu->arch.acrs_loaded = true;
> +
>  	acc_mode = mop->op == KVM_S390_MEMOP_LOGICAL_READ ? GACC_FETCH : GACC_STORE;
>  	if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
>  		r = check_gva_range(vcpu, mop->gaddr, mop->ar, mop->size,
> @@ -5420,6 +5428,9 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
>  		kvm_s390_inject_prog_irq(vcpu, &vcpu->arch.pgm);
>  
>  out_free:
> +	save_access_regs(vcpu->run->s.regs.acrs);
> +	restore_access_regs(vcpu->arch.host_acrs);
> +	vcpu->arch.acrs_loaded = false;

... these two hunks wouldn't be required if the code above would be changed
like I proposed.