Message-ID: <20251120182849.1109773-1-hi@josie.lol>
Date: Thu, 20 Nov 2025 19:28:49 +0100
From: Josephine Pfeiffer <hi@...ie.lol>
To: borntraeger@...ux.ibm.com
Cc: frankja@...ux.ibm.com,
	imbrenda@...ux.ibm.com,
	david@...nel.org,
	hca@...ux.ibm.com,
	gor@...ux.ibm.com,
	agordeev@...ux.ibm.com,
	svens@...ux.ibm.com,
	kvm@...r.kernel.org,
	linux-s390@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: s390: Implement CHECK_STOP support and fix GET_MP_STATE

On Mon, 17 Nov 2025 19:14:57 +0100, Christian Borntraeger wrote:
> Am 17.11.25 um 16:18 schrieb Josephine Pfeiffer:
> > Add support for KVM_MP_STATE_CHECK_STOP to enable proper VM migration
> > and error handling for s390 guests. The CHECK_STOP state represents a
> > CPU that encountered a severe machine check and is halted in an error
> > state.
>
> I think the patch description is misleading. We do have proper VM
> migration and we also have error handling in the kvm module. The host
> machine check handler will forward guest machine checks to the guest.
> This logic  is certainly not perfect but kind of good enough for most
> cases.

First of all, thank you for taking the time to look at my patch, and sorry
for taking so long to write up the reply.

You're right, QEMU migrates cpu_state via vmstate [1] and only uses
KVM_SET_MP_STATE to restore the state after migration [2], never calling
KVM_GET_MP_STATE. So I misunderstood something there.

What prompted me to look into this was that the KVM API has advertised
CHECK_STOP support without implementing it. 
Looking at commit 6352e4d2dd9a [3] from 2014: "KVM: s390: implement 
KVM_(S|G)ET_MP_STATE for user space state control"

This commit added KVM_MP_STATE_CHECK_STOP to include/uapi/linux/kvm.h [4] and
documented it in Documentation/virtual/kvm/api.txt with:

  "KVM_MP_STATE_CHECK_STOP: the vcpu is in a special error state [s390]"

But the implementation was explicitly deferred with a fallthrough comment [3]:

  case KVM_MP_STATE_LOAD:
  case KVM_MP_STATE_CHECK_STOP:
      /* fall through - CHECK_STOP and LOAD are not supported yet */
  default:
      rc = -ENXIO;

This created a bit of an API asymmetry where:
- Documentation/virt/kvm/api.rst:1546 [5] advertises CHECK_STOP as valid
- KVM_SET_MP_STATE rejects it with -ENXIO
- KVM_GET_MP_STATE never returns it (always returns STOPPED or OPERATING) [6]

> Now: The architecture defines that state and the interface is certainly
> there. So implementing it will allow userspace to put a CPU into checkstop
> state if you ever need that. We also have a checkstop state that you
> can put a secure CPU in.
>
> The usecase is dubious though. The only case of the options from POP
> chapter11 that makes sense to me in a virtualized environment is an exigent
> machine check but a problem to actually deliver that (multiple reasons,
> like the OS has machine checks disabled in PSW, or the prefix register
> is broken).
>
> So I am curious, do you have any specific usecase in mind?
> I assume you have a related QEMU patch somewhere?

The use cases I see are:

1. API completeness: The state was added to the UAPI 11 years ago but never
   implemented. Userspace cannot use a documented API feature.

2. Fault injection testing: Administrators testing failover/monitoring for
   hardware failures could programmatically put a CPU into CHECK_STOP to
   verify their procedures work.

3. Protected Virtualization: The patch adds PV_CPU_STATE_CHKSTP support [7]
   for the Ultravisor, which can put secure CPUs into CHECK_STOP. Without this,
   userspace cannot detect/handle that condition.

I don't have a QEMU patch, since QEMU already handles the state internally
via cpu_state [1] and only needs KVM_SET_MP_STATE to work (which this enables).

> From a quick glance the patch in general does not look wrong but at
> least this is wrong:
> > diff --git a/arch/s390/include/asm/kvm_host_types.h b/arch/s390/include/asm/kvm_host_types.h
> > index 1394d3fb648f..a86e326a2eee 100644
> > --- a/arch/s390/include/asm/kvm_host_types.h
> > +++ b/arch/s390/include/asm/kvm_host_types.h
> > @@ -111,6 +111,7 @@ struct mcck_volatile_info {
> >       ((((sie_block)->sidad & SIDAD_SIZE_MASK) + 1) * PAGE_SIZE)
> >
> >   #define CPUSTAT_STOPPED    0x80000000
> > +#define CPUSTAT_CHECK_STOP 0x40000000
> Bit 1 of the sie control block is a hardware defined bit and
> its meaning is not checkstop, so this is not the right way to do it.
> Lets first clarify your usecase so that we can see what the right way
> forward is.

Understood - using bit 1 of the hardware SIE control block is wrong.

I guess the correct approach would be to add a software-only flag to track CHECK_STOP
state. Unlike STOPPED (which uses CPUSTAT_STOPPED [8]), CHECK_STOP would need software 
tracking since there isn't a corresponding hardware bit with the correct meaning.

Would adding a field to struct kvm_vcpu_arch [9] be the right approach? Or is
there a better way to track this state that I'm missing?

I think completing the API makes sense, even if the use case is uncommon.

Thanks,
Josephine

References:
[1] https://github.com/qemu/qemu/blob/master/target/s390x/machine.c#L270
    VMSTATE_UINT8(env.cpu_state, S390CPU)
[2] https://github.com/qemu/qemu/blob/master/target/s390x/kvm/kvm.c#L410
    kvm_arch_init_vcpu() -> kvm_s390_set_cpu_state()
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6352e4d2dd9a
    KVM: s390: implement KVM_(S|G)ET_MP_STATE for user space state control
[4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/kvm.h#n596
    #define KVM_MP_STATE_CHECK_STOP 6
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/virt/kvm/api.rst#n1546
    KVM_MP_STATE_CHECK_STOP the vcpu is in a special error state [s390]
[6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/s390/kvm/kvm-s390.c#n4465
    kvm_arch_vcpu_ioctl_get_mpstate() implementation
[7] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/s390/include/asm/uv.h#n252
    #define PV_CPU_STATE_CHKSTP 3
[8] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/s390/include/asm/kvm_host_types.h#n113
    #define CPUSTAT_STOPPED 0x80000000 (bit 0 of SIE control block cpuflags)
[9] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/s390/include/asm/kvm_host.h#n414
    struct kvm_vcpu_arch

Powered by blists - more mailing lists