[RFC PATCH v1 0/2] Improvements to SIGP handling [QEMU]

[RFC PATCH v1 0/2] Improvements to SIGP handling [QEMU]

[Eric Farman]

I'm cleaning up some of the SIGP code in KVM and QEMU,
and would like to propose the following changes.

Patch 1 should be less concerning than its KVM counterpart,
since the CZAM bit in question is already present in QEMU.

Patch 2 provides some handshaking with KVM. Since QEMU
injects a Stop IRQ for a couple of the SIGP orders, we
can provide the flags associated with it, to provide some
direction for how KVM should process it.

While this has no dependency on the KVM code, the KVM series
that I'm working on in parallel is here:

https://lore.kernel.org/r/20211008203112.1979843-1-farman@linux.ibm.com/

Eric Farman (2):
  s390x: sigp: Force Set Architecture to return Invalid Parameter
  s390x/kvm: Pass SIGP Stop flags

 target/s390x/kvm/kvm.c |  4 ++++
 target/s390x/sigp.c    | 18 +-----------------
 2 files changed, 5 insertions(+), 17 deletions(-)

-- 
2.25.1

[RFC PATCH v1 1/2] s390x: sigp: Force Set Architecture to return Invalid Parameter

[Eric Farman]

According to the Principles of Operation, the SIGP Set Architecture
order will return Incorrect State if some CPUs are not stopped, but
only if the CZAM facility is not present. If it is, the order will
return Invalid Parameter because the architecture mode cannot be
changed.

Since CZAM always exists when S390_FEAT_ZARCH exists, which in turn
exists for every defined CPU model, we can simplify this code.

Fixes: 075e52b81664 ("s390x/cpumodel: we are always in zarchitecture mode")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/sigp.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index d57427ced8..51c727834c 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -428,26 +428,10 @@ static int handle_sigp_single_dst(S390CPU *cpu, S390CPU *dst_cpu, uint8_t order,
 static int sigp_set_architecture(S390CPU *cpu, uint32_t param,
                                  uint64_t *status_reg)
 {
-    CPUState *cur_cs;
-    S390CPU *cur_cpu;
-    bool all_stopped = true;
-
-    CPU_FOREACH(cur_cs) {
-        cur_cpu = S390_CPU(cur_cs);
-
-        if (cur_cpu == cpu) {
-            continue;
-        }
-        if (s390_cpu_get_state(cur_cpu) != S390_CPU_STATE_STOPPED) {
-            all_stopped = false;
-        }
-    }
-
     *status_reg &= 0xffffffff00000000ULL;
 
     /* Reject set arch order, with czam we're always in z/Arch mode. */
-    *status_reg |= (all_stopped ? SIGP_STAT_INVALID_PARAMETER :
-                    SIGP_STAT_INCORRECT_STATE);
+    *status_reg |= SIGP_STAT_INVALID_PARAMETER;
     return SIGP_CC_STATUS_STORED;
 }
 
-- 
2.25.1

[RFC PATCH v1 2/2] s390x/kvm: Pass SIGP Stop flags

[Eric Farman]

When building a Stop IRQ to pass to KVM, we should incorporate
the flags if handling the SIGP Stop and Store Status order.
With that, KVM can reject other orders that are submitted for
the same CPU while the operation is fully processed.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Acked-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/kvm/kvm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 5b1fdb55c4..701b9ddc88 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2555,6 +2555,10 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
         .type = KVM_S390_SIGP_STOP,
     };
 
+    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
+        irq.u.stop.flags = KVM_S390_STOP_FLAG_STORE_STATUS;
+    }
+
     kvm_s390_vcpu_interrupt(cpu, &irq);
 }
 
-- 
2.25.1

Re: [RFC PATCH v1 1/2] s390x: sigp: Force Set Architecture to return Invalid Parameter

[Thomas Huth]

On 08/10/2021 22.38, Eric Farman wrote:
> According to the Principles of Operation, the SIGP Set Architecture
> order will return Incorrect State if some CPUs are not stopped, but
> only if the CZAM facility is not present. If it is, the order will
> return Invalid Parameter because the architecture mode cannot be
> changed.
> 
> Since CZAM always exists when S390_FEAT_ZARCH exists, which in turn
> exists for every defined CPU model, we can simplify this code.
> 
> Fixes: 075e52b81664 ("s390x/cpumodel: we are always in zarchitecture mode")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/sigp.c | 18 +-----------------
>   1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index d57427ced8..51c727834c 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -428,26 +428,10 @@ static int handle_sigp_single_dst(S390CPU *cpu, S390CPU *dst_cpu, uint8_t order,
>   static int sigp_set_architecture(S390CPU *cpu, uint32_t param,
>                                    uint64_t *status_reg)
>   {
> -    CPUState *cur_cs;
> -    S390CPU *cur_cpu;
> -    bool all_stopped = true;
> -
> -    CPU_FOREACH(cur_cs) {
> -        cur_cpu = S390_CPU(cur_cs);
> -
> -        if (cur_cpu == cpu) {
> -            continue;
> -        }
> -        if (s390_cpu_get_state(cur_cpu) != S390_CPU_STATE_STOPPED) {
> -            all_stopped = false;
> -        }
> -    }
> -
>       *status_reg &= 0xffffffff00000000ULL;
>   
>       /* Reject set arch order, with czam we're always in z/Arch mode. */
> -    *status_reg |= (all_stopped ? SIGP_STAT_INVALID_PARAMETER :
> -                    SIGP_STAT_INCORRECT_STATE);
> +    *status_reg |= SIGP_STAT_INVALID_PARAMETER;
>       return SIGP_CC_STATUS_STORED;
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

By the way, I think we could now also get rid of SIGP_MODE_ESA_S390, 
SIGP_MODE_Z_ARCH_TRANS_ALL_PSW and SIGP_MODE_Z_ARCH_TRANS_CUR_PSW now (in a 
separate patch)...

Re: [RFC PATCH v1 1/2] s390x: sigp: Force Set Architecture to return Invalid Parameter

[David Hildenbrand]

On 08.10.21 22:38, Eric Farman wrote:
> According to the Principles of Operation, the SIGP Set Architecture
> order will return Incorrect State if some CPUs are not stopped, but
> only if the CZAM facility is not present. If it is, the order will
> return Invalid Parameter because the architecture mode cannot be
> changed.
> 
> Since CZAM always exists when S390_FEAT_ZARCH exists, which in turn
> exists for every defined CPU model, we can simplify this code.
> 
> Fixes: 075e52b81664 ("s390x/cpumodel: we are always in zarchitecture mode")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v1 2/2] s390x/kvm: Pass SIGP Stop flags

[David Hildenbrand]

On 08.10.21 22:38, Eric Farman wrote:
> When building a Stop IRQ to pass to KVM, we should incorporate
> the flags if handling the SIGP Stop and Store Status order.
> With that, KVM can reject other orders that are submitted for
> the same CPU while the operation is fully processed.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> Acked-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   target/s390x/kvm/kvm.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 5b1fdb55c4..701b9ddc88 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2555,6 +2555,10 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
>           .type = KVM_S390_SIGP_STOP,
>       };
>   
> +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
> +        irq.u.stop.flags = KVM_S390_STOP_FLAG_STORE_STATUS;
> +    }
> +

KVM_S390_STOP_FLAG_STORE_STATUS tells KVM to perform the store status as 
well ... is that really what we want?

Maybe we want a different (more generic) way to tell KVM that a CPU is 
temporarily busy for SIGP orders?

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v1 2/2] s390x/kvm: Pass SIGP Stop flags

[Christian Borntraeger]

Am 11.10.21 um 09:09 schrieb David Hildenbrand:
> On 08.10.21 22:38, Eric Farman wrote:
>> When building a Stop IRQ to pass to KVM, we should incorporate
>> the flags if handling the SIGP Stop and Store Status order.
>> With that, KVM can reject other orders that are submitted for
>> the same CPU while the operation is fully processed.
>>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>   target/s390x/kvm/kvm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 5b1fdb55c4..701b9ddc88 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -2555,6 +2555,10 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
>>           .type = KVM_S390_SIGP_STOP,
>>       };
>> +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>> +        irq.u.stop.flags = KVM_S390_STOP_FLAG_STORE_STATUS;
>> +    }
>> +
> 
> KVM_S390_STOP_FLAG_STORE_STATUS tells KVM to perform the store status as well ... is that really what we want?
At least it should not hurt I guess. QEMU then does it again?
> 
> Maybe we want a different (more generic) way to tell KVM that a CPU is temporarily busy for SIGP orders?
> 

Re: [RFC PATCH v1 2/2] s390x/kvm: Pass SIGP Stop flags

[David Hildenbrand]

On 11.10.21 10:40, Christian Borntraeger wrote:
> 
> 
> Am 11.10.21 um 09:09 schrieb David Hildenbrand:
>> On 08.10.21 22:38, Eric Farman wrote:
>>> When building a Stop IRQ to pass to KVM, we should incorporate
>>> the flags if handling the SIGP Stop and Store Status order.
>>> With that, KVM can reject other orders that are submitted for
>>> the same CPU while the operation is fully processed.
>>>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>    target/s390x/kvm/kvm.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>>> index 5b1fdb55c4..701b9ddc88 100644
>>> --- a/target/s390x/kvm/kvm.c
>>> +++ b/target/s390x/kvm/kvm.c
>>> @@ -2555,6 +2555,10 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
>>>            .type = KVM_S390_SIGP_STOP,
>>>        };
>>> +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>>> +        irq.u.stop.flags = KVM_S390_STOP_FLAG_STORE_STATUS;
>>> +    }
>>> +
>>
>> KVM_S390_STOP_FLAG_STORE_STATUS tells KVM to perform the store status as well ... is that really what we want?
> At least it should not hurt I guess. QEMU then does it again?

The thing is, that before we officially completed the action in user 
space (and let other SIGP actions actually succeed in user space on the 
CPU), the target CPU will be reported as !busy in the kernel already. 
And before we even inject the stop interrupt, the CPU will be detected 
as !busy in the kernel. I guess it will fix some cases where we poll via 
SENSE if the stop and store happened, because the store *did* happen in 
the kernel and we'll simply store again in user space.

However, I wonder if we want to handle it more generically: Properly 
flag a CPU as busy for SIGP when we start processing the order until we 
completed processing the order. That would allow to handle other SIGP 
operations in user space cleanly, without any chance for races with 
SENSE code running in the kernel.

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v1 2/2] s390x/kvm: Pass SIGP Stop flags

[Eric Farman]

On Mon, 2021-10-11 at 11:21 +0200, David Hildenbrand wrote:
> On 11.10.21 10:40, Christian Borntraeger wrote:
> > 
> > Am 11.10.21 um 09:09 schrieb David Hildenbrand:
> > > On 08.10.21 22:38, Eric Farman wrote:
> > > > When building a Stop IRQ to pass to KVM, we should incorporate
> > > > the flags if handling the SIGP Stop and Store Status order.
> > > > With that, KVM can reject other orders that are submitted for
> > > > the same CPU while the operation is fully processed.
> > > > 
> > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > Acked-by: Janosch Frank <frankja@linux.ibm.com>
> > > > ---
> > > >    target/s390x/kvm/kvm.c | 4 ++++
> > > >    1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> > > > index 5b1fdb55c4..701b9ddc88 100644
> > > > --- a/target/s390x/kvm/kvm.c
> > > > +++ b/target/s390x/kvm/kvm.c
> > > > @@ -2555,6 +2555,10 @@ void kvm_s390_stop_interrupt(S390CPU
> > > > *cpu)
> > > >            .type = KVM_S390_SIGP_STOP,
> > > >        };
> > > > +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
> > > > +        irq.u.stop.flags = KVM_S390_STOP_FLAG_STORE_STATUS;
> > > > +    }
> > > > +
> > > 
> > > KVM_S390_STOP_FLAG_STORE_STATUS tells KVM to perform the store
> > > status as well ... is that really what we want?
> > At least it should not hurt I guess. QEMU then does it again?
> 
> The thing is, that before we officially completed the action in user 
> space (and let other SIGP actions actually succeed in user space on
> the 
> CPU), the target CPU will be reported as !busy in the kernel
> already. 
> And before we even inject the stop interrupt, the CPU will be
> detected 
> as !busy in the kernel. I guess it will fix some cases where we poll
> via 
> SENSE if the stop and store happened, because the store *did* happen
> in 
> the kernel and we'll simply store again in user space.
> 
> However, I wonder if we want to handle it more generically: Properly 
> flag a CPU as busy for SIGP when we start processing the order until
> we 
> completed processing the order. That would allow to handle other
> SIGP 
> operations in user space cleanly, without any chance for races with 
> SENSE code running in the kernel.
> 

I think a generic solution would be ideal, but I'm wrestling with the
race with the kernel's SENSE code. Today, handle_sigp_single_dst
already checks to see if a CPU is currently processing an order and
returns a CC2 when it does, but of course the kernel's SENSE code
doesn't know that. We could flag the CPU as busy in the kernel when
sending a SIGP to userspace, so that the SENSE code indicates BUSY, but
then how do we know when userspace is finished and the CPU is no longer
BUSY?

Eric

Re: [RFC PATCH v1 2/2] s390x/kvm: Pass SIGP Stop flags

[David Hildenbrand]

On 11.10.21 19:58, Eric Farman wrote:
> On Mon, 2021-10-11 at 11:21 +0200, David Hildenbrand wrote:
>> On 11.10.21 10:40, Christian Borntraeger wrote:
>>>
>>> Am 11.10.21 um 09:09 schrieb David Hildenbrand:
>>>> On 08.10.21 22:38, Eric Farman wrote:
>>>>> When building a Stop IRQ to pass to KVM, we should incorporate
>>>>> the flags if handling the SIGP Stop and Store Status order.
>>>>> With that, KVM can reject other orders that are submitted for
>>>>> the same CPU while the operation is fully processed.
>>>>>
>>>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>     target/s390x/kvm/kvm.c | 4 ++++
>>>>>     1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>>>>> index 5b1fdb55c4..701b9ddc88 100644
>>>>> --- a/target/s390x/kvm/kvm.c
>>>>> +++ b/target/s390x/kvm/kvm.c
>>>>> @@ -2555,6 +2555,10 @@ void kvm_s390_stop_interrupt(S390CPU
>>>>> *cpu)
>>>>>             .type = KVM_S390_SIGP_STOP,
>>>>>         };
>>>>> +    if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
>>>>> +        irq.u.stop.flags = KVM_S390_STOP_FLAG_STORE_STATUS;
>>>>> +    }
>>>>> +
>>>>
>>>> KVM_S390_STOP_FLAG_STORE_STATUS tells KVM to perform the store
>>>> status as well ... is that really what we want?
>>> At least it should not hurt I guess. QEMU then does it again?
>>
>> The thing is, that before we officially completed the action in user
>> space (and let other SIGP actions actually succeed in user space on
>> the
>> CPU), the target CPU will be reported as !busy in the kernel
>> already.
>> And before we even inject the stop interrupt, the CPU will be
>> detected
>> as !busy in the kernel. I guess it will fix some cases where we poll
>> via
>> SENSE if the stop and store happened, because the store *did* happen
>> in
>> the kernel and we'll simply store again in user space.
>>
>> However, I wonder if we want to handle it more generically: Properly
>> flag a CPU as busy for SIGP when we start processing the order until
>> we
>> completed processing the order. That would allow to handle other
>> SIGP
>> operations in user space cleanly, without any chance for races with
>> SENSE code running in the kernel.
>>
> 
> I think a generic solution would be ideal, but I'm wrestling with the
> race with the kernel's SENSE code. Today, handle_sigp_single_dst
> already checks to see if a CPU is currently processing an order and
> returns a CC2 when it does, but of course the kernel's SENSE code
> doesn't know that. We could flag the CPU as busy in the kernel when
> sending a SIGP to userspace, so that the SENSE code indicates BUSY, but
> then how do we know when userspace is finished and the CPU is no longer
> BUSY?

I'd just add a new IOCTL for marking a CPU busy/!busy for SIGP from user 
space. You can then either let user space perform both actions 
(set+unset), or let the kernel automatically set "busy" and user space 
only clear "busy". You can define a new capability to enable the 
"automatically set busy when going to user space on sigp" -- might 
require some thoughts on some corner cases.

Maybe there might be other scenarios in the future where we might want 
to set a CPU busy for sigp without that CPU triggering a sigp action 
itself (e.g., externally triggered reset of a CPU? Simulation of 
check-stop? store status?), so at least having a way to set/reset a CPU 
busy for SIGP might be valuable.

Once we go to user space to process a SIGP, we usually don't care too 
much about some additional overhead due to 1 or 2 ioctls IMHO.

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v1 0/2] Improvements to SIGP handling [QEMU]

[Thomas Huth]

On 08/10/2021 22.38, Eric Farman wrote:
> I'm cleaning up some of the SIGP code in KVM and QEMU,
> and would like to propose the following changes.
> 
> Patch 1 should be less concerning than its KVM counterpart,
> since the CZAM bit in question is already present in QEMU.
> 
> Patch 2 provides some handshaking with KVM. Since QEMU
> injects a Stop IRQ for a couple of the SIGP orders, we
> can provide the flags associated with it, to provide some
> direction for how KVM should process it.
> 
> While this has no dependency on the KVM code, the KVM series
> that I'm working on in parallel is here:
> 
> https://lore.kernel.org/r/20211008203112.1979843-1-farman@linux.ibm.com/
> 
> Eric Farman (2):
>    s390x: sigp: Force Set Architecture to return Invalid Parameter
>    s390x/kvm: Pass SIGP Stop flags
> 
>   target/s390x/kvm/kvm.c |  4 ++++
>   target/s390x/sigp.c    | 18 +-----------------
>   2 files changed, 5 insertions(+), 17 deletions(-)
> 

FYI, I've queued the first patch to my s390x-next branch:

https://gitlab.com/thuth/qemu/-/commits/s390x-next

... for the second patch, I'll wait for the discussion to settle.

  Thomas