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

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

On Fri, Feb 09, 2024 at 09:45:38PM +0100, Eric Farman wrote:
> The routine ar_translation() is called by get_vcpu_asce(), which is
> called by both the instruction intercept path (where the access
> registers had been loaded with the guest's values), and the MEM_OP
> ioctl (which hadn't). This means that any ALET the guest expects to
> be used would be ignored.
> 
> Furthermore, the logic in ar_translation() will store the contents
> of the access registers back to the KVM_RUN struct. This unexpected
> change of AR values can lead to problems after invoking the MEM_OP,
> for example an ALET Specification Exception.
> 
> 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.
> 
> Suggested-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ea63ac769889..c2dfeea55dcf 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5391,6 +5391,10 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
>  			return -ENOMEM;
>  	}
>  
> +	/* Swap host/guest access registers in the event of a MEM_OP with AR specified */
> +	save_access_regs(vcpu->arch.host_acrs);
> +	restore_access_regs(vcpu->run->s.regs.acrs);
> +
>  	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 +5424,8 @@ 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);

I guess we will end up with more and more of such constructs until nobody
knows when which register contents are loaded. I would higly prefer a TIF flag
which indicates if the access registers contain the host or guest register
contents, and actual users grab the required content from the correct location
- or better: always take it from guest save area, and write to the guest save
area if the to be invented TIF flag indicates that access registers contain
guest registers...

Or maybe a TIF flag with different semantics: "guest save area does not
reflect current state - which is within registers".