Re: [RFC PATCH] KVM: s390: remove extra copy of access registers into KVM_RUN

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

On Thu, Feb 01, 2024 at 11:56:26AM -0500, Eric Farman wrote:
> On Thu, 2024-02-01 at 16:14 +0100, Heiko Carstens wrote:
> > On Wed, Jan 31, 2024 at 09:58:32PM +0100, Eric Farman wrote:
> > What's the code path that can lead to this scenario?
> 
> When processing a KVM_RUN ioctl, the kernel is going to swap the
> host/guest access registers in sync_regs, enter SIE, and then swap them
> back in store_regs when it has to exit to userspace. So then on the
> QEMU side it might look something like this:
> 
> kvm_arch_handle_exit
>   handle_intercept
>     handle_instruction
>       handle_b2
>         ioinst_handle_stsch
>           s390_cpu_virt_mem_rw(ar=0xe, is_write=true)
>             kvm_s390_mem_op
> 
> Where the interesting registers at that point are:
> 
> acr0           0x3ff               1023
> acr1           0x33bff8c0          868219072
> ...
> acr14          0x0                 0
> 
> Note ACR0/1 are already buggered up from an earlier pass.
> 
> The above carries us through the kernel this way:
> 
> kvm_arch_vcpu_ioctl(KVM_S390_MEM_OP)
>   kvm_s390_vcpu_memsida_op
>     kvm_s390_vcpu_mem_op
>       write_guest_with_key
>         access_guest_with_key
>           get_vcpu_asce
>             ar_translate
>               save_access_regs(kvm_run)

...

> Well regardless of this patch, I think it's using the contents of the
> host registers today, isn't it? save_access_regs() does a STAM to put
> the current registers into some bit of memory, so ar_translation() can
> do regular logic against it. The above just changes WHERE that bit of
> memory lives from something shared with another ioctl to something
> local to ar_translation(). 

This seems to be true; but there are also other code paths which can
reach ar_translation() where the access register contents actually do
belong to the guest (e.g. intercept handling).

> My original change just removed the save_access_regs() call entirely
> and read the contents of the kvm_run struct since they were last saved
> (see below). This "feels" better to me, and works for the scenario I
> bumped into too. Maybe this is more appropriate?
> 
> ---8<---
> 
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 5bfcc50c1a68..c5ed3b0b665a 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -391,7 +391,6 @@ static int ar_translation(struct kvm_vcpu *vcpu,
> union asce *asce, u8 ar,
>         if (ar >= NUM_ACRS)
>                 return -EINVAL;
>  
> -       save_access_regs(vcpu->run->s.regs.acrs);

I guess this and your previous patch are both not correct. There is
different handling required depending on if current access register
contents belong to the host or guest (both seems to be possible), when
the function is entered.

But anyway, I'll leave that up to Janosch and Claudio, just had a quick
look at your patch :)