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

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

[Eric Farman]

The routine ar_translation() is called by get_vcpu_asce(), which is
called from a handful of places, such as an interception that is
being handled during KVM_RUN processing. In that case, the access
registers of the vcpu had been saved to a host_acrs struct and then
the guest access registers loaded from the KVM_RUN struct prior to
entering SIE. Saving them back to KVM_RUN at this point doesn't do
any harm, since it will be done again at the end of the KVM_RUN
loop when the host access registers are restored.

But that's not the only path into this code. The MEM_OP ioctl can
be used while specifying an access register, and will arrive here.

Linux itself doesn't use the access registers for much, but it does
squirrel the thread local storage variable into ACRs 0 and 1 in
copy_thread() [1]. This means that the MEM_OP ioctl may copy
non-zero access registers (the upper- and lower-halves of the TLS
pointer) to the KVM_RUN struct, which will end up getting propogated
to the guest once KVM_RUN ioctls occur. Since these are almost
certainly invalid as far as an ALET goes, an ALET Specification
Exception would be triggered if it were attempted to be used.

[1] arch/s390/kernel/process.c:169

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    I've gone back and forth about whether the correct fix is
    to simply remove the save_access_regs() call and inspect
    the contents from the most recent KVM_RUN directly, versus
    storing the contents locally. Both work for me but I've
    opted for the latter, as it continues to behave the same
    as it does today but without the implicit use of the
    KVM_RUN space. As it is, this is (was) the only reference
    to vcpu->run in this file, which stands out since the
    routines are used by other callers.
    
    Curious about others' thoughts.

 arch/s390/kvm/gaccess.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index 5bfcc50c1a68..9205496195a4 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -380,6 +380,7 @@ void ipte_unlock(struct kvm *kvm)
 static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce, u8 ar,
 			  enum gacc_mode mode)
 {
+	int acrs[NUM_ACRS];
 	union alet alet;
 	struct ale ale;
 	struct aste aste;
@@ -391,8 +392,8 @@ 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);
-	alet.val = vcpu->run->s.regs.acrs[ar];
+	save_access_regs(acrs);
+	alet.val = acrs[ar];
 
 	if (ar == 0 || alet.val == 0) {
 		asce->val = vcpu->arch.sie_block->gcr[1];
-- 
2.40.1

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

[Heiko Carstens]

On Wed, Jan 31, 2024 at 09:58:32PM +0100, Eric Farman wrote:
> The routine ar_translation() is called by get_vcpu_asce(), which is
> called from a handful of places, such as an interception that is
> being handled during KVM_RUN processing. In that case, the access
> registers of the vcpu had been saved to a host_acrs struct and then
> the guest access registers loaded from the KVM_RUN struct prior to
> entering SIE. Saving them back to KVM_RUN at this point doesn't do
> any harm, since it will be done again at the end of the KVM_RUN
> loop when the host access registers are restored.
> 
> But that's not the only path into this code. The MEM_OP ioctl can
> be used while specifying an access register, and will arrive here.
> 
> Linux itself doesn't use the access registers for much, but it does
> squirrel the thread local storage variable into ACRs 0 and 1 in
> copy_thread() [1]. This means that the MEM_OP ioctl may copy
> non-zero access registers (the upper- and lower-halves of the TLS
> pointer) to the KVM_RUN struct, which will end up getting propogated
> to the guest once KVM_RUN ioctls occur. Since these are almost
> certainly invalid as far as an ALET goes, an ALET Specification
> Exception would be triggered if it were attempted to be used.

What's the code path that can lead to this scenario?

>  arch/s390/kvm/gaccess.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index 5bfcc50c1a68..9205496195a4 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -380,6 +380,7 @@ void ipte_unlock(struct kvm *kvm)
>  static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce, u8 ar,
>  			  enum gacc_mode mode)
>  {
> +	int acrs[NUM_ACRS];
>  	union alet alet;
>  	struct ale ale;
>  	struct aste aste;
> @@ -391,8 +392,8 @@ 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);
> -	alet.val = vcpu->run->s.regs.acrs[ar];
> +	save_access_regs(acrs);
> +	alet.val = acrs[ar];

If the above is like you said, then this code would use the host
access register contents for ar translation of the guest?

Or maybe I'm simply misunderstanding what you write.

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

[Eric Farman]

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:
> > The routine ar_translation() is called by get_vcpu_asce(), which is
> > called from a handful of places, such as an interception that is
> > being handled during KVM_RUN processing. In that case, the access
> > registers of the vcpu had been saved to a host_acrs struct and then
> > the guest access registers loaded from the KVM_RUN struct prior to
> > entering SIE. Saving them back to KVM_RUN at this point doesn't do
> > any harm, since it will be done again at the end of the KVM_RUN
> > loop when the host access registers are restored.
> > 
> > But that's not the only path into this code. The MEM_OP ioctl can
> > be used while specifying an access register, and will arrive here.
> > 
> > Linux itself doesn't use the access registers for much, but it does
> > squirrel the thread local storage variable into ACRs 0 and 1 in
> > copy_thread() [1]. This means that the MEM_OP ioctl may copy
> > non-zero access registers (the upper- and lower-halves of the TLS
> > pointer) to the KVM_RUN struct, which will end up getting
> > propogated
> > to the guest once KVM_RUN ioctls occur. Since these are almost
> > certainly invalid as far as an ALET goes, an ALET Specification
> > Exception would be triggered if it were attempted to be used.
> 
> 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)

> 
> >  arch/s390/kvm/gaccess.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> > index 5bfcc50c1a68..9205496195a4 100644
> > --- a/arch/s390/kvm/gaccess.c
> > +++ b/arch/s390/kvm/gaccess.c
> > @@ -380,6 +380,7 @@ void ipte_unlock(struct kvm *kvm)
> >  static int ar_translation(struct kvm_vcpu *vcpu, union asce *asce,
> > u8 ar,
> >  			  enum gacc_mode mode)
> >  {
> > +	int acrs[NUM_ACRS];
> >  	union alet alet;
> >  	struct ale ale;
> >  	struct aste aste;
> > @@ -391,8 +392,8 @@ 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);
> > -	alet.val = vcpu->run->s.regs.acrs[ar];
> > +	save_access_regs(acrs);
> > +	alet.val = acrs[ar];
> 
> If the above is like you said, then this code would use the host
> access register contents for ar translation of the guest?
> 
> Or maybe I'm simply misunderstanding what you write.

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(). 

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);
        alet.val = vcpu->run->s.regs.acrs[ar];
 
        if (ar == 0 || alet.val == 0) {

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

[Heiko Carstens]

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 :)

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

[Eric Farman]

On Tue, 2024-02-06 at 16:47 +0100, Heiko Carstens wrote:
> 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).

Right, the trouble is that both scenarios end up here. Storing the
access registers here in the intercept path "doesn't hurt" because
it'll be done again when we clean up after the SIE exit (store_regs()),
and the original patch keeps the behavior the same for the memop case
without disrupting the kvm_run space. I agree this behavior doesn't
seem right.

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

Yay? :)

>  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 :)
> 

Thanks for the quick look. Will wait for Janosch and Claudio to get a
couple cycles.

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

[Christian Borntraeger]

Am 31.01.24 um 21:58 schrieb Eric Farman:
> The routine ar_translation() is called by get_vcpu_asce(), which is
> called from a handful of places, such as an interception that is
> being handled during KVM_RUN processing. In that case, the access
> registers of the vcpu had been saved to a host_acrs struct and then
> the guest access registers loaded from the KVM_RUN struct prior to
> entering SIE. Saving them back to KVM_RUN at this point doesn't do
> any harm, since it will be done again at the end of the KVM_RUN
> loop when the host access registers are restored.
> 
> But that's not the only path into this code. The MEM_OP ioctl can
> be used while specifying an access register, and will arrive here.
> 
> Linux itself doesn't use the access registers for much, but it does
> squirrel the thread local storage variable into ACRs 0 and 1 in
> copy_thread() [1]. This means that the MEM_OP ioctl may copy
> non-zero access registers (the upper- and lower-halves of the TLS
> pointer) to the KVM_RUN struct, which will end up getting propogated
> to the guest once KVM_RUN ioctls occur. Since these are almost
> certainly invalid as far as an ALET goes, an ALET Specification
> Exception would be triggered if it were attempted to be used.
> 
> [1] arch/s390/kernel/process.c:169
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>      I've gone back and forth about whether the correct fix is
>      to simply remove the save_access_regs() call and inspect
>      the contents from the most recent KVM_RUN directly, versus
>      storing the contents locally. Both work for me but I've
>      opted for the latter, as it continues to behave the same
>      as it does today but without the implicit use of the
>      KVM_RUN space. As it is, this is (was) the only reference
>      to vcpu->run in this file, which stands out since the
>      routines are used by other callers.
>      
>      Curious about others' thoughts.

Given the main idea that we have the guest ARs loaded in the kvm module
when running a guest and that the kernel does not use those. This avoids
saving/restoring the ARs for all the fast path exits.
The MEM_OP is indeed a separate path.
So what about making this slightly slower by doing something like this
(untested, white space damaged)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 7aa0e668488f0..79e8b3aa7b1c0 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -5402,6 +5402,7 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
                         return -ENOMEM;
         }
  
+       sync_regs(vcpu);
         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,
@@ -5432,6 +5433,7 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
  
  out_free:
         vfree(tmpbuf);
+       store_regs(vcpu);
         return r;
  }
  

Maybe we could even have a bit in sync/store regs and a BUG_ON in places where
we access any lazy register.

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

[Janosch Frank]

On 2/8/24 12:50, Christian Borntraeger wrote:
> Am 31.01.24 um 21:58 schrieb Eric Farman:
>> The routine ar_translation() is called by get_vcpu_asce(), which is
>> called from a handful of places, such as an interception that is
>> being handled during KVM_RUN processing. In that case, the access
>> registers of the vcpu had been saved to a host_acrs struct and then
>> the guest access registers loaded from the KVM_RUN struct prior to
>> entering SIE. Saving them back to KVM_RUN at this point doesn't do
>> any harm, since it will be done again at the end of the KVM_RUN
>> loop when the host access registers are restored.
>>
>> But that's not the only path into this code. The MEM_OP ioctl can
>> be used while specifying an access register, and will arrive here.
>>
>> Linux itself doesn't use the access registers for much, but it does
>> squirrel the thread local storage variable into ACRs 0 and 1 in
>> copy_thread() [1]. This means that the MEM_OP ioctl may copy
>> non-zero access registers (the upper- and lower-halves of the TLS
>> pointer) to the KVM_RUN struct, which will end up getting propogated
>> to the guest once KVM_RUN ioctls occur. Since these are almost
>> certainly invalid as far as an ALET goes, an ALET Specification
>> Exception would be triggered if it were attempted to be used.
>>
>> [1] arch/s390/kernel/process.c:169
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>
>> Notes:
>>       I've gone back and forth about whether the correct fix is
>>       to simply remove the save_access_regs() call and inspect
>>       the contents from the most recent KVM_RUN directly, versus
>>       storing the contents locally. Both work for me but I've
>>       opted for the latter, as it continues to behave the same
>>       as it does today but without the implicit use of the
>>       KVM_RUN space. As it is, this is (was) the only reference
>>       to vcpu->run in this file, which stands out since the
>>       routines are used by other callers.
>>       
>>       Curious about others' thoughts.
> 
> Given the main idea that we have the guest ARs loaded in the kvm module
> when running a guest and that the kernel does not use those. This avoids
> saving/restoring the ARs for all the fast path exits.
> The MEM_OP is indeed a separate path.
> So what about making this slightly slower by doing something like this
> (untested, white space damaged)

We could fence AR loading/storing via the the PSW address space bits for 
more performance and not do a full sync/store regs here.

> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 7aa0e668488f0..79e8b3aa7b1c0 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -5402,6 +5402,7 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
>                           return -ENOMEM;
>           }
>    
> +       sync_regs(vcpu);
>           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,
> @@ -5432,6 +5433,7 @@ static long kvm_s390_vcpu_mem_op(struct kvm_vcpu *vcpu,
>    
>    out_free:
>           vfree(tmpbuf);
> +       store_regs(vcpu);
>           return r;
>    }
>    
> 
> Maybe we could even have a bit in sync/store regs and a BUG_ON in places where
> we access any lazy register.

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

[Christian Borntraeger]

Am 08.02.24 um 13:37 schrieb Janosch Frank:
> On 2/8/24 12:50, Christian Borntraeger wrote:
>> Am 31.01.24 um 21:58 schrieb Eric Farman:
>>> The routine ar_translation() is called by get_vcpu_asce(), which is
>>> called from a handful of places, such as an interception that is
>>> being handled during KVM_RUN processing. In that case, the access
>>> registers of the vcpu had been saved to a host_acrs struct and then
>>> the guest access registers loaded from the KVM_RUN struct prior to
>>> entering SIE. Saving them back to KVM_RUN at this point doesn't do
>>> any harm, since it will be done again at the end of the KVM_RUN
>>> loop when the host access registers are restored.
>>>
>>> But that's not the only path into this code. The MEM_OP ioctl can
>>> be used while specifying an access register, and will arrive here.
>>>
>>> Linux itself doesn't use the access registers for much, but it does
>>> squirrel the thread local storage variable into ACRs 0 and 1 in
>>> copy_thread() [1]. This means that the MEM_OP ioctl may copy
>>> non-zero access registers (the upper- and lower-halves of the TLS
>>> pointer) to the KVM_RUN struct, which will end up getting propogated
>>> to the guest once KVM_RUN ioctls occur. Since these are almost
>>> certainly invalid as far as an ALET goes, an ALET Specification
>>> Exception would be triggered if it were attempted to be used.
>>>
>>> [1] arch/s390/kernel/process.c:169
>>>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>
>>> Notes:
>>>       I've gone back and forth about whether the correct fix is
>>>       to simply remove the save_access_regs() call and inspect
>>>       the contents from the most recent KVM_RUN directly, versus
>>>       storing the contents locally. Both work for me but I've
>>>       opted for the latter, as it continues to behave the same
>>>       as it does today but without the implicit use of the
>>>       KVM_RUN space. As it is, this is (was) the only reference
>>>       to vcpu->run in this file, which stands out since the
>>>       routines are used by other callers.
>>>       Curious about others' thoughts.
>>
>> Given the main idea that we have the guest ARs loaded in the kvm module
>> when running a guest and that the kernel does not use those. This avoids
>> saving/restoring the ARs for all the fast path exits.
>> The MEM_OP is indeed a separate path.
>> So what about making this slightly slower by doing something like this
>> (untested, white space damaged)
> 
> We could fence AR loading/storing via the the PSW address space bits for more performance and not do a full sync/store regs here.

Hmm, we would then add a conditional branch which also is not ideal.
Maybe just load/restore the ARs instead of the full sync/save_reg dance?

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

[Eric Farman]

On Thu, 2024-02-08 at 14:51 +0100, Christian Borntraeger wrote:
> 
> 
> Am 08.02.24 um 13:37 schrieb Janosch Frank:
> > On 2/8/24 12:50, Christian Borntraeger wrote:
> > > Am 31.01.24 um 21:58 schrieb Eric Farman:
> > > > The routine ar_translation() is called by get_vcpu_asce(),
> > > > which is
> > > > called from a handful of places, such as an interception that
> > > > is
> > > > being handled during KVM_RUN processing. In that case, the
> > > > access
> > > > registers of the vcpu had been saved to a host_acrs struct and
> > > > then
> > > > the guest access registers loaded from the KVM_RUN struct prior
> > > > to
> > > > entering SIE. Saving them back to KVM_RUN at this point doesn't
> > > > do
> > > > any harm, since it will be done again at the end of the KVM_RUN
> > > > loop when the host access registers are restored.
> > > > 
> > > > But that's not the only path into this code. The MEM_OP ioctl
> > > > can
> > > > be used while specifying an access register, and will arrive
> > > > here.
> > > > 
> > > > Linux itself doesn't use the access registers for much, but it
> > > > does
> > > > squirrel the thread local storage variable into ACRs 0 and 1 in
> > > > copy_thread() [1]. This means that the MEM_OP ioctl may copy
> > > > non-zero access registers (the upper- and lower-halves of the
> > > > TLS
> > > > pointer) to the KVM_RUN struct, which will end up getting
> > > > propogated
> > > > to the guest once KVM_RUN ioctls occur. Since these are almost
> > > > certainly invalid as far as an ALET goes, an ALET Specification
> > > > Exception would be triggered if it were attempted to be used.
> > > > 
> > > > [1] arch/s390/kernel/process.c:169
> > > > 
> > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > ---
> > > > 
> > > > Notes:
> > > >       I've gone back and forth about whether the correct fix is
> > > >       to simply remove the save_access_regs() call and inspect
> > > >       the contents from the most recent KVM_RUN directly,
> > > > versus
> > > >       storing the contents locally. Both work for me but I've
> > > >       opted for the latter, as it continues to behave the same
> > > >       as it does today but without the implicit use of the
> > > >       KVM_RUN space. As it is, this is (was) the only reference
> > > >       to vcpu->run in this file, which stands out since the
> > > >       routines are used by other callers.
> > > >       Curious about others' thoughts.
> > > 
> > > Given the main idea that we have the guest ARs loaded in the kvm
> > > module
> > > when running a guest and that the kernel does not use those. This
> > > avoids
> > > saving/restoring the ARs for all the fast path exits.
> > > The MEM_OP is indeed a separate path.
> > > So what about making this slightly slower by doing something like
> > > this
> > > (untested, white space damaged)

This idea seems to work fine for the case I was puzzling over.

> > 
> > We could fence AR loading/storing via the the PSW address space
> > bits for more performance and not do a full sync/store regs here.
> 
> Hmm, we would then add a conditional branch which also is not ideal.
> Maybe just load/restore the ARs instead of the full sync/save_reg
> dance?

This might work too. I'll give that a try later today.

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

[Janosch Frank]

On 1/31/24 21:58, Eric Farman wrote:
> The routine ar_translation() is called by get_vcpu_asce(), which is
> called from a handful of places, such as an interception that is
> being handled during KVM_RUN processing. In that case, the access
> registers of the vcpu had been saved to a host_acrs struct and then
> the guest access registers loaded from the KVM_RUN struct prior to
> entering SIE. Saving them back to KVM_RUN at this point doesn't do
> any harm, since it will be done again at the end of the KVM_RUN
> loop when the host access registers are restored.
> 
> But that's not the only path into this code. The MEM_OP ioctl can
> be used while specifying an access register, and will arrive here.
> 
> Linux itself doesn't use the access registers for much, but it does
> squirrel the thread local storage variable into ACRs 0 and 1 in
> copy_thread() [1]. This means that the MEM_OP ioctl may copy
> non-zero access registers (the upper- and lower-halves of the TLS
> pointer) to the KVM_RUN struct, which will end up getting propogated
> to the guest once KVM_RUN ioctls occur. Since these are almost
> certainly invalid as far as an ALET goes, an ALET Specification
> Exception would be triggered if it were attempted to be used.
> 
>

Would you be able to come up with a kvm-unit-test to verify a fix and 
for regression? Hmmm, maybe a kselftest would be even easier.

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

[Eric Farman]

On Thu, 2024-02-08 at 13:39 +0100, Janosch Frank wrote:
> On 1/31/24 21:58, Eric Farman wrote:
> > The routine ar_translation() is called by get_vcpu_asce(), which is
> > called from a handful of places, such as an interception that is
> > being handled during KVM_RUN processing. In that case, the access
> > registers of the vcpu had been saved to a host_acrs struct and then
> > the guest access registers loaded from the KVM_RUN struct prior to
> > entering SIE. Saving them back to KVM_RUN at this point doesn't do
> > any harm, since it will be done again at the end of the KVM_RUN
> > loop when the host access registers are restored.
> > 
> > But that's not the only path into this code. The MEM_OP ioctl can
> > be used while specifying an access register, and will arrive here.
> > 
> > Linux itself doesn't use the access registers for much, but it does
> > squirrel the thread local storage variable into ACRs 0 and 1 in
> > copy_thread() [1]. This means that the MEM_OP ioctl may copy
> > non-zero access registers (the upper- and lower-halves of the TLS
> > pointer) to the KVM_RUN struct, which will end up getting
> > propogated
> > to the guest once KVM_RUN ioctls occur. Since these are almost
> > certainly invalid as far as an ALET goes, an ALET Specification
> > Exception would be triggered if it were attempted to be used.
> > 
> > 
> 
> Would you be able to come up with a kvm-unit-test to verify a fix and
> for regression? Hmmm, maybe a kselftest would be even easier.
> 

Sure thing. I had started down the kselftest path as there's already
some building blocks there, but got distracted by some other things in
that space that were puzzling me. Will dig that branch back out.