On Thu, 2021-11-11 at 10:15 +0100, David Hildenbrand wrote:
On 10.11.21 21:33, Eric Farman wrote:
With commit 2444b352c3ac ("KVM: s390: forward most SIGP
orders
to
user
space") we have a capability that allows the "fast" SIGP
orders
(as
defined by the Programming Notes for the SIGNAL PROCESSOR
instruction in
the Principles of Operation) to be handled in-kernel, while
all
others are
sent to userspace for processing.
This works fine but it creates a situation when, for
example, a
SIGP SENSE
might return CC1 (STATUS STORED, and status bits indicating
the
vcpu is
stopped), when in actuality userspace is still processing a
SIGP
STOP AND
STORE STATUS order, and the vcpu is not yet actually
stopped.
Thus,
the
SIGP SENSE should actually be returning CC2 (busy) instead
of
CC1.
To fix this, add another CPU capability, dependent on the
USER_SIGP
one,
and two associated IOCTLs. One IOCTL will be used by
userspace
to
mark a
vcpu "busy" processing a SIGP order, and cause concurrent
orders
handled
in-kernel to be returned with CC2 (busy). Another IOCTL
will be
used by
userspace to mark the SIGP "finished", and the vcpu free to
process
additional orders.
This looks much cleaner to me, thanks!
[...]
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-
s390.h
index c07a050d757d..54371cede485 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -82,6 +82,22 @@ static inline int is_vcpu_idle(struct
kvm_vcpu
*vcpu)
return test_bit(vcpu->vcpu_idx, vcpu->kvm-
arch.idle_mask);
}
+static inline bool kvm_s390_vcpu_is_sigp_busy(struct
kvm_vcpu
*vcpu)
+{
+ return (atomic_read(&vcpu->arch.sigp_busy) == 1);
You can drop ()
+}
+
+static inline bool kvm_s390_vcpu_set_sigp_busy(struct
kvm_vcpu
*vcpu)
+{
+ /* Return zero for success, or -EBUSY if another vcpu
won */
+ return (atomic_cmpxchg(&vcpu->arch.sigp_busy, 0, 1) ==
0) ? 0 :
-EBUSY;
You can drop () as well.
We might not need the -EBUSY semantics after all. User space
can
just
track if it was set, because it's in charge of setting it.
Hrm, I added this to distinguish a newer kernel with an older
QEMU,
but
of course an older QEMU won't know the difference either. I'll
doublecheck that this is works fine in the different
permutations.
+}
+
+static inline void kvm_s390_vcpu_clear_sigp_busy(struct
kvm_vcpu
*vcpu)
+{
+ atomic_set(&vcpu->arch.sigp_busy, 0);
+}
+
static inline int kvm_is_ucontrol(struct kvm *kvm)
{
#ifdef CONFIG_KVM_S390_UCONTROL
diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 5ad3fb4619f1..a37496ea6dfa 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -276,6 +276,10 @@ static int handle_sigp_dst(struct
kvm_vcpu
*vcpu, u8 order_code,
if (!dst_vcpu)
return SIGP_CC_NOT_OPERATIONAL;
+ if (kvm_s390_vcpu_is_sigp_busy(dst_vcpu)) {
+ return SIGP_CC_BUSY;
+ }
You can drop {}
Arg, I had some debug in there which needed the braces, and of
course
it's unnecessary now. Thanks.
+
switch (order_code) {
case SIGP_SENSE:
vcpu->stat.instruction_sigp_sense++;
@@ -411,6 +415,12 @@ int kvm_s390_handle_sigp(struct
kvm_vcpu
*vcpu)
if (handle_sigp_order_in_user_space(vcpu, order_code,
cpu_addr))
return -EOPNOTSUPP;
+ /* Check the current vcpu, if it was a target from
another vcpu
*/
+ if (kvm_s390_vcpu_is_sigp_busy(vcpu)) {
+ kvm_s390_set_psw_cc(vcpu, SIGP_CC_BUSY);
+ return 0;
+ }
I don't think we need this. I think the above (checking the
target of
a
SIGP order) is sufficient. Or which situation do you have in
mind?
Hrm... I think you're right. I was thinking of this:
VCPU 1 - SIGP STOP CPU 2
VCPU 2 - SIGP SENSE CPU 1
But of course either CPU2 is going to be marked "busy" first,
and
the
sense doesn't get processed until it's reset, or the sense
arrives
first, and the busy/notbusy doesn't matter. Let me doublecheck
my
tests
for the non-RFC version.
I do wonder if we want to make this a kvm_arch_vcpu_ioctl()
instead,
In one of my original attempts between v1 and v2, I had put
this
there.
This reliably deadlocks my guest, because the caller
(kvm_vcpu_ioctl())
tries to acquire vcpu->mutex, and racing SIGPs (via KVM_RUN)
might
already be holding it. Thus, it's an async ioctl. I could fold
it
into
the existing interrupt ioctl, but as those are architected
structs
it
seems more natural do it this way. Or I have mis-understood
something
along the way?
essentially just providing a KVM_S390_SET_SIGP_BUSY *and*
providing
the
order. "order == 0" sets it to !busy.
I'd tried this too, since it provided some nice debug-ability.
Unfortunately, I have a testcase (which I'll eventually get
folded
into
kvm-unit-tests :)) that picks a random order between 0-255,
knowing
that there's only a couple handfuls of valid orders, to check
the
response. Zero is valid architecturally (POPS figure 4-29),
even if
it's unassigned. The likelihood of it becoming assigned is
probably
quite low, but I'm not sure that I like special-casing an order
of
zero
in this way.
