[PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Peter Gonda]

If an SEV-ES guest requests termination, exit to userspace with
KVM_EXIT_SYSTEM_EVENT and a dedicated SEV_TERM type instead of -EINVAL
so that userspace can take appropriate action.

See AMD's GHCB spec section '4.1.13 Termination Request' for more details.

Suggested-by: Sean Christopherson <seanjc@google.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Peter Gonda <pgonda@google.com>

---
V4
 * Updated to Sean and Paolo's suggestion of reworking the
   kvm_run.system_event struct to ndata and data fields to fix the
   padding.
 * 4.1 Updated commit description

V3
 * Add Documentation/ update.
 * Updated other KVM_EXIT_SHUTDOWN exits to clear ndata and set reason
   to KVM_SHUTDOWN_REQ.

V2
 * Add KVM_CAP_EXIT_SHUTDOWN_REASON check for KVM_CHECK_EXTENSION.

Tested by making an SEV-ES guest call sev_es_terminate() with hardcoded
reason code set and reason code and then observing the codes from the
userspace VMM in the kvm_run.shutdown.data fields.

---
 arch/x86/kvm/svm/sev.c   | 9 +++++++--
 include/uapi/linux/kvm.h | 5 ++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 75fa6dd268f0..1a080f3f09d8 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2735,8 +2735,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
 			reason_set, reason_code);
 
-		ret = -EINVAL;
-		break;
+		vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
+		vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SEV_TERM |
+					       KVM_SYSTEM_EVENT_NDATA_VALID;
+		vcpu->run->system_event.ndata = 1;
+		vcpu->run->system_event.data[1] = control->ghcb_gpa;
+
+		return 0;
 	}
 	default:
 		/* Error, keep GHCB MSR value as-is */
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 8616af85dc5d..dd1d8167e71f 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -444,8 +444,11 @@ struct kvm_run {
 #define KVM_SYSTEM_EVENT_SHUTDOWN       1
 #define KVM_SYSTEM_EVENT_RESET          2
 #define KVM_SYSTEM_EVENT_CRASH          3
+#define KVM_SYSTEM_EVENT_SEV_TERM       4
+#define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
 			__u32 type;
-			__u64 flags;
+			__u32 ndata;
+			__u64 data[16];
 		} system_event;
 		/* KVM_EXIT_S390_STSI */
 		struct {
-- 
2.35.1.1178.g4f1659d476-goog

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson]

On Thu, Apr 07, 2022, Peter Gonda wrote:
> If an SEV-ES guest requests termination, exit to userspace with
> KVM_EXIT_SYSTEM_EVENT and a dedicated SEV_TERM type instead of -EINVAL
> so that userspace can take appropriate action.
> 
> See AMD's GHCB spec section '4.1.13 Termination Request' for more details.

Maybe it'll be obvious by the lack of compilation errors, but the changelog should
call out the flags => ndata+data shenanigans, otherwise this looks like ABI breakage.

> Suggested-by: Sean Christopherson <seanjc@google.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Peter Gonda <pgonda@google.com>
> 
> ---
> V4
>  * Updated to Sean and Paolo's suggestion of reworking the
>    kvm_run.system_event struct to ndata and data fields to fix the
>    padding.
>  * 4.1 Updated commit description
> 
> V3
>  * Add Documentation/ update.
>  * Updated other KVM_EXIT_SHUTDOWN exits to clear ndata and set reason
>    to KVM_SHUTDOWN_REQ.
> 
> V2
>  * Add KVM_CAP_EXIT_SHUTDOWN_REASON check for KVM_CHECK_EXTENSION.
> 
> Tested by making an SEV-ES guest call sev_es_terminate() with hardcoded
> reason code set and reason code and then observing the codes from the
> userspace VMM in the kvm_run.shutdown.data fields.
> 
> ---
>  arch/x86/kvm/svm/sev.c   | 9 +++++++--
>  include/uapi/linux/kvm.h | 5 ++++-
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 75fa6dd268f0..1a080f3f09d8 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2735,8 +2735,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>  		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
>  			reason_set, reason_code);
>  
> -		ret = -EINVAL;
> -		break;
> +		vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
> +		vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SEV_TERM |
> +					       KVM_SYSTEM_EVENT_NDATA_VALID;
> +		vcpu->run->system_event.ndata = 1;
> +		vcpu->run->system_event.data[1] = control->ghcb_gpa;
> +
> +		return 0;

Kinda silly, but

		ret = 0;
		break;

would be better so that this flows through the tracepoint.  I wouldn't care much
if it didn't result in an unpaired "entry" tracepoint (and I still don't care that
much...).

>  	}
>  	default:
>  		/* Error, keep GHCB MSR value as-is */
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 8616af85dc5d..dd1d8167e71f 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -444,8 +444,11 @@ struct kvm_run {
>  #define KVM_SYSTEM_EVENT_SHUTDOWN       1
>  #define KVM_SYSTEM_EVENT_RESET          2
>  #define KVM_SYSTEM_EVENT_CRASH          3
> +#define KVM_SYSTEM_EVENT_SEV_TERM       4
> +#define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
>  			__u32 type;
> -			__u64 flags;
> +			__u32 ndata;
> +			__u64 data[16];
>  		} system_event;
>  		/* KVM_EXIT_S390_STSI */
>  		struct {
> -- 
> 2.35.1.1178.g4f1659d476-goog
> 

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[kernel test robot]

Hi Peter,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/master]
[also build test ERROR on v5.18-rc1 next-20220407]
[cannot apply to v4.1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Peter-Gonda/KVM-SEV-Add-KVM_EXIT_SHUTDOWN-metadata-for-SEV-ES/20220408-050628
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git master
config: arm64-randconfig-r035-20220408 (https://download.01.org/0day-ci/archive/20220408/202204081255.SUNyj4S3-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c29a51b3a257908aebc01cd7c4655665db317d66)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/3b310e5891d172b59042783c128f6efcf5bf6198
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Peter-Gonda/KVM-SEV-Add-KVM_EXIT_SHUTDOWN-metadata-for-SEV-ES/20220408-050628
        git checkout 3b310e5891d172b59042783c128f6efcf5bf6198
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/kvm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> arch/arm64/kvm/psci.c:184:26: error: no member named 'flags' in 'struct kvm_run::(unnamed at include/uapi/linux/kvm.h:443:3)'
           vcpu->run->system_event.flags = flags;
           ~~~~~~~~~~~~~~~~~~~~~~~ ^
   1 error generated.


vim +184 arch/arm64/kvm/psci.c

e6bc13c8a70eab arch/arm/kvm/psci.c   Anup Patel       2014-04-29  163  
34739fd95fab3a arch/arm64/kvm/psci.c Will Deacon      2022-02-21  164  static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type, u64 flags)
4b1238269ed340 arch/arm/kvm/psci.c   Anup Patel       2014-04-29  165  {
46808a4cb89708 arch/arm64/kvm/psci.c Marc Zyngier     2021-11-16  166  	unsigned long i;
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  167  	struct kvm_vcpu *tmp;
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  168  
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  169  	/*
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  170  	 * The KVM ABI specifies that a system event exit may call KVM_RUN
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  171  	 * again and may perform shutdown/reboot at a later time that when the
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  172  	 * actual request is made.  Since we are implementing PSCI and a
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  173  	 * caller of PSCI reboot and shutdown expects that the system shuts
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  174  	 * down or reboots immediately, let's make sure that VCPUs are not run
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  175  	 * after this call is handled and before the VCPUs have been
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  176  	 * re-initialized.
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  177  	 */
cc9b43f99d5ff4 virt/kvm/arm/psci.c   Andrew Jones     2017-06-04  178  	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
3781528e3045e7 arch/arm/kvm/psci.c   Eric Auger       2015-09-25  179  		tmp->arch.power_off = true;
7b244e2be654d9 virt/kvm/arm/psci.c   Andrew Jones     2017-06-04  180  	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);
cf5d318865e25f arch/arm/kvm/psci.c   Christoffer Dall 2014-10-16  181  
4b1238269ed340 arch/arm/kvm/psci.c   Anup Patel       2014-04-29  182  	memset(&vcpu->run->system_event, 0, sizeof(vcpu->run->system_event));
4b1238269ed340 arch/arm/kvm/psci.c   Anup Patel       2014-04-29  183  	vcpu->run->system_event.type = type;
34739fd95fab3a arch/arm64/kvm/psci.c Will Deacon      2022-02-21 @184  	vcpu->run->system_event.flags = flags;
4b1238269ed340 arch/arm/kvm/psci.c   Anup Patel       2014-04-29  185  	vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
4b1238269ed340 arch/arm/kvm/psci.c   Anup Patel       2014-04-29  186  }
4b1238269ed340 arch/arm/kvm/psci.c   Anup Patel       2014-04-29  187  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[kernel test robot]

Hi Peter,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kvm/master]
[also build test ERROR on v5.18-rc1 next-20220407]
[cannot apply to v4.1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Peter-Gonda/KVM-SEV-Add-KVM_EXIT_SHUTDOWN-metadata-for-SEV-ES/20220408-050628
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git master
config: riscv-randconfig-c006-20220408 (https://download.01.org/0day-ci/archive/20220408/202204081314.dqs8Tnxd-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project c29a51b3a257908aebc01cd7c4655665db317d66)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/3b310e5891d172b59042783c128f6efcf5bf6198
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Peter-Gonda/KVM-SEV-Add-KVM_EXIT_SHUTDOWN-metadata-for-SEV-ES/20220408-050628
        git checkout 3b310e5891d172b59042783c128f6efcf5bf6198
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> arch/riscv/kvm/vcpu_sbi.c:97:20: error: no member named 'flags' in 'struct kvm_run::(unnamed at include/uapi/linux/kvm.h:443:3)'
           run->system_event.flags = flags;
           ~~~~~~~~~~~~~~~~~ ^
   1 error generated.


vim +97 arch/riscv/kvm/vcpu_sbi.c

dea8ee31a03927 Atish Patra 2021-09-27   83  
4b11d86571c447 Anup Patel  2022-01-31   84  void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
4b11d86571c447 Anup Patel  2022-01-31   85  				     struct kvm_run *run,
4b11d86571c447 Anup Patel  2022-01-31   86  				     u32 type, u64 flags)
4b11d86571c447 Anup Patel  2022-01-31   87  {
4b11d86571c447 Anup Patel  2022-01-31   88  	unsigned long i;
4b11d86571c447 Anup Patel  2022-01-31   89  	struct kvm_vcpu *tmp;
4b11d86571c447 Anup Patel  2022-01-31   90  
4b11d86571c447 Anup Patel  2022-01-31   91  	kvm_for_each_vcpu(i, tmp, vcpu->kvm)
4b11d86571c447 Anup Patel  2022-01-31   92  		tmp->arch.power_off = true;
4b11d86571c447 Anup Patel  2022-01-31   93  	kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);
4b11d86571c447 Anup Patel  2022-01-31   94  
4b11d86571c447 Anup Patel  2022-01-31   95  	memset(&run->system_event, 0, sizeof(run->system_event));
4b11d86571c447 Anup Patel  2022-01-31   96  	run->system_event.type = type;
4b11d86571c447 Anup Patel  2022-01-31  @97  	run->system_event.flags = flags;
4b11d86571c447 Anup Patel  2022-01-31   98  	run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
4b11d86571c447 Anup Patel  2022-01-31   99  }
4b11d86571c447 Anup Patel  2022-01-31  100  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Peter Gonda]

On Thu, Apr 7, 2022 at 8:55 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Apr 07, 2022, Peter Gonda wrote:
> > If an SEV-ES guest requests termination, exit to userspace with
> > KVM_EXIT_SYSTEM_EVENT and a dedicated SEV_TERM type instead of -EINVAL
> > so that userspace can take appropriate action.
> >
> > See AMD's GHCB spec section '4.1.13 Termination Request' for more details.
>
> Maybe it'll be obvious by the lack of compilation errors, but the changelog should
> call out the flags => ndata+data shenanigans, otherwise this looks like ABI breakage.

Hmm I am not sure we can do this change anymore given that we have two
call sites using 'flags'

arch/arm64/kvm/psci.c:184
arch/riscv/kvm/vcpu_sbi.c:97

I am not at all familiar with ARM and RISC-V but some quick reading
tells me these archs also require 64-bit alignment on their 64-bit
accesses. If thats correct, should I fix this call sites up by
proceeding with this ndata + data[] change and move whatever they are
assigning to flags into data[0] like I am doing here? It looks like
both of these changes are not in a kernel release so IIUC we can still
fix the ABI here?

>
> > Suggested-by: Sean Christopherson <seanjc@google.com>
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: kvm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> >
> > ---
> > V4
> >  * Updated to Sean and Paolo's suggestion of reworking the
> >    kvm_run.system_event struct to ndata and data fields to fix the
> >    padding.
> >  * 4.1 Updated commit description
> >
> > V3
> >  * Add Documentation/ update.
> >  * Updated other KVM_EXIT_SHUTDOWN exits to clear ndata and set reason
> >    to KVM_SHUTDOWN_REQ.
> >
> > V2
> >  * Add KVM_CAP_EXIT_SHUTDOWN_REASON check for KVM_CHECK_EXTENSION.
> >
> > Tested by making an SEV-ES guest call sev_es_terminate() with hardcoded
> > reason code set and reason code and then observing the codes from the
> > userspace VMM in the kvm_run.shutdown.data fields.
> >
> > ---
> >  arch/x86/kvm/svm/sev.c   | 9 +++++++--
> >  include/uapi/linux/kvm.h | 5 ++++-
> >  2 files changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 75fa6dd268f0..1a080f3f09d8 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2735,8 +2735,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
> >               pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
> >                       reason_set, reason_code);
> >
> > -             ret = -EINVAL;
> > -             break;
> > +             vcpu->run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
> > +             vcpu->run->system_event.type = KVM_SYSTEM_EVENT_SEV_TERM |
> > +                                            KVM_SYSTEM_EVENT_NDATA_VALID;
> > +             vcpu->run->system_event.ndata = 1;
> > +             vcpu->run->system_event.data[1] = control->ghcb_gpa;
> > +
> > +             return 0;
>
> Kinda silly, but
>
>                 ret = 0;
>                 break;
>
> would be better so that this flows through the tracepoint.  I wouldn't care much
> if it didn't result in an unpaired "entry" tracepoint (and I still don't care that
> much...).

Ah I'll fix that up.


>
> >       }
> >       default:
> >               /* Error, keep GHCB MSR value as-is */
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> > index 8616af85dc5d..dd1d8167e71f 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -444,8 +444,11 @@ struct kvm_run {
> >  #define KVM_SYSTEM_EVENT_SHUTDOWN       1
> >  #define KVM_SYSTEM_EVENT_RESET          2
> >  #define KVM_SYSTEM_EVENT_CRASH          3
> > +#define KVM_SYSTEM_EVENT_SEV_TERM       4
> > +#define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
> >                       __u32 type;
> > -                     __u64 flags;
> > +                     __u32 ndata;
> > +                     __u64 data[16];
> >               } system_event;
> >               /* KVM_EXIT_S390_STSI */
> >               struct {
> > --
> > 2.35.1.1178.g4f1659d476-goog
> >

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Paolo Bonzini]

Queued, thanks.  But documentation was missing:

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index e7a0dfdc0178..72183ae628f7 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -6088,8 +6088,12 @@ should put the acknowledged interrupt vector into the 'epr' field.
   #define KVM_SYSTEM_EVENT_SHUTDOWN       1
   #define KVM_SYSTEM_EVENT_RESET          2
   #define KVM_SYSTEM_EVENT_CRASH          3
+  #define KVM_SYSTEM_EVENT_SEV_TERM       4
+  #define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
 			__u32 type;
+                        __u32 ndata;
 			__u64 flags;
+                        __u64 data[16];
 		} system_event;

 If exit_reason is KVM_EXIT_SYSTEM_EVENT then the vcpu has triggered
@@ -6099,7 +6103,7 @@ HVC instruction based PSCI call from the vcpu. The 'type' field describes
 the system-level event type. The 'flags' field describes architecture
 specific flags for the system-level event.

-Valid values for 'type' are:
+Valid values for bits 30:0 of 'type' are:

  - KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
    VM. Userspace is not obliged to honour this, and if it does honour
@@ -6112,12 +6116,18 @@ Valid values for 'type' are:
    has requested a crash condition maintenance. Userspace can choose
    to ignore the request, or to gather VM memory core dump and/or
    reset/shutdown of the VM.
+ - KVM_SYSTEM_EVENT_SEV_TERM -- an AMD SEV guest requested termination.
+   The guest physical address of the guest's GHCB is stored in `data[0]`.

 Valid flags are:

  - KVM_SYSTEM_EVENT_RESET_FLAG_PSCI_RESET2 (arm64 only) -- the guest issued
    a SYSTEM_RESET2 call according to v1.1 of the PSCI specification.

+Extra data for this event is stored in the `data[]` array, up to index
+`ndata-1` included, if bit 31 is set in `type`.  The data depends on the
+`type` field.  There is no extra data if bit 31 is clear or `ndata` is zero.
+
 ::

 		/* KVM_EXIT_IOAPIC_EOI */


Paolo

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson]

+Anup and Will

On Fri, Apr 08, 2022, Peter Gonda wrote:
> On Thu, Apr 7, 2022 at 8:55 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Apr 07, 2022, Peter Gonda wrote:
> > > If an SEV-ES guest requests termination, exit to userspace with
> > > KVM_EXIT_SYSTEM_EVENT and a dedicated SEV_TERM type instead of -EINVAL
> > > so that userspace can take appropriate action.
> > >
> > > See AMD's GHCB spec section '4.1.13 Termination Request' for more details.
> >
> > Maybe it'll be obvious by the lack of compilation errors, but the changelog should
> > call out the flags => ndata+data shenanigans, otherwise this looks like ABI breakage.
> 
> Hmm I am not sure we can do this change anymore given that we have two
> call sites using 'flags'
> 
> arch/arm64/kvm/psci.c:184
> arch/riscv/kvm/vcpu_sbi.c:97
> 
> I am not at all familiar with ARM and RISC-V but some quick reading
> tells me these archs also require 64-bit alignment on their 64-bit
> accesses. If thats correct, should I fix this call sites up by
> proceeding with this ndata + data[] change and move whatever they are
> assigning to flags into data[0] like I am doing here? It looks like
> both of these changes are not in a kernel release so IIUC we can still
> fix the ABI here?

Yeah, both came in for v5.18.  Given that there will be multiple paths that need
to set data, it's worth adding a common helper to the dirty work.

Anup and Will,

system_event.flags is broken (at least on x86) due to the prior 'type' field not
being propery padded, e.g. userspace will read/write garbage if the userspace
and kernel compilers pad structs differently.

		struct {
			__u32 type;
			__u64 flags;
		} system_event;

Our plan to unhose this is to change the struct as follows and use bit 31 in the
'type' to indicate that ndata+data are valid.

		struct {
                        __u32 type;
			__u32 ndata;
			__u64 data[16];
                } system_event;

Any objection to updating your architectures to use a helper to set the bit and
populate ndata+data accordingly?  It'll require a userspace update, but v5.18
hasn't officially released yet so it's not kinda sort not ABI breakage.

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Will Deacon]

Hi Sean,

Cheers for the heads-up.

[+Marc and Alex as this looks similar to [1]]

On Fri, Apr 08, 2022 at 05:01:21PM +0000, Sean Christopherson wrote:
> On Fri, Apr 08, 2022, Peter Gonda wrote:
> > On Thu, Apr 7, 2022 at 8:55 PM Sean Christopherson <seanjc@google.com> wrote:
> > > On Thu, Apr 07, 2022, Peter Gonda wrote:
> > > > If an SEV-ES guest requests termination, exit to userspace with
> > > > KVM_EXIT_SYSTEM_EVENT and a dedicated SEV_TERM type instead of -EINVAL
> > > > so that userspace can take appropriate action.
> > > >
> > > > See AMD's GHCB spec section '4.1.13 Termination Request' for more details.
> > >
> > > Maybe it'll be obvious by the lack of compilation errors, but the changelog should
> > > call out the flags => ndata+data shenanigans, otherwise this looks like ABI breakage.
> > 
> > Hmm I am not sure we can do this change anymore given that we have two
> > call sites using 'flags'
> > 
> > arch/arm64/kvm/psci.c:184
> > arch/riscv/kvm/vcpu_sbi.c:97
> > 
> > I am not at all familiar with ARM and RISC-V but some quick reading
> > tells me these archs also require 64-bit alignment on their 64-bit
> > accesses. If thats correct, should I fix this call sites up by
> > proceeding with this ndata + data[] change and move whatever they are
> > assigning to flags into data[0] like I am doing here? It looks like
> > both of these changes are not in a kernel release so IIUC we can still
> > fix the ABI here?
> 
> Yeah, both came in for v5.18.  Given that there will be multiple paths that need
> to set data, it's worth adding a common helper to the dirty work.
> 
> Anup and Will,
> 
> system_event.flags is broken (at least on x86) due to the prior 'type' field not
> being propery padded, e.g. userspace will read/write garbage if the userspace
> and kernel compilers pad structs differently.
> 
> 		struct {
> 			__u32 type;
> 			__u64 flags;
> 		} system_event;

On arm64, I think the compiler is required to put the padding between type
and flags so that both the struct and 'flags' are 64-bit aligned [2]. Does
x86 not offer any guarantees on the overall structure alignment?

> Our plan to unhose this is to change the struct as follows and use bit 31 in the
> 'type' to indicate that ndata+data are valid.
> 
> 		struct {
>                         __u32 type;
> 			__u32 ndata;
> 			__u64 data[16];
>                 } system_event;
> 
> Any objection to updating your architectures to use a helper to set the bit and
> populate ndata+data accordingly?  It'll require a userspace update, but v5.18
> hasn't officially released yet so it's not kinda sort not ABI breakage.

It's a bit annoying, as we're using the current structure in Android 13 :/
Obviously, if there's no choice then upstream shouldn't worry, but it means
we'll have to carry a delta in crosvm. Specifically, the new 'ndata' field
is going to be unusable for us because it coincides with the padding.

Will

[1] https://lore.kernel.org/r/20220407162327.396183-6-alexandru.elisei@arm.com
[2] https://github.com/ARM-software/abi-aa/blob/60a8eb8c55e999d74dac5e368fc9d7e36e38dda4/aapcs64/aapcs64.rst#composite-types

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Marc Zyngier]

On Fri, 08 Apr 2022 17:56:42 +0100,
Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> Queued, thanks.  But documentation was missing:
> 
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index e7a0dfdc0178..72183ae628f7 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -6088,8 +6088,12 @@ should put the acknowledged interrupt vector into the 'epr' field.
>    #define KVM_SYSTEM_EVENT_SHUTDOWN       1
>    #define KVM_SYSTEM_EVENT_RESET          2
>    #define KVM_SYSTEM_EVENT_CRASH          3
> +  #define KVM_SYSTEM_EVENT_SEV_TERM       4
> +  #define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
>  			__u32 type;
> +                        __u32 ndata;
>  			__u64 flags;
> +                        __u64 data[16];
>  		} system_event;
> 
>  If exit_reason is KVM_EXIT_SYSTEM_EVENT then the vcpu has triggered
> @@ -6099,7 +6103,7 @@ HVC instruction based PSCI call from the vcpu. The 'type' field describes
>  the system-level event type. The 'flags' field describes architecture
>  specific flags for the system-level event.
> 
> -Valid values for 'type' are:
> +Valid values for bits 30:0 of 'type' are:
> 
>   - KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
>     VM. Userspace is not obliged to honour this, and if it does honour
> @@ -6112,12 +6116,18 @@ Valid values for 'type' are:
>     has requested a crash condition maintenance. Userspace can choose
>     to ignore the request, or to gather VM memory core dump and/or
>     reset/shutdown of the VM.
> + - KVM_SYSTEM_EVENT_SEV_TERM -- an AMD SEV guest requested termination.
> +   The guest physical address of the guest's GHCB is stored in `data[0]`.
> 
>  Valid flags are:
> 
>   - KVM_SYSTEM_EVENT_RESET_FLAG_PSCI_RESET2 (arm64 only) -- the guest issued
>     a SYSTEM_RESET2 call according to v1.1 of the PSCI specification.
> 
> +Extra data for this event is stored in the `data[]` array, up to index
> +`ndata-1` included, if bit 31 is set in `type`.  The data depends on the
> +`type` field.  There is no extra data if bit 31 is clear or `ndata` is zero.
> +

This has the potential to break userspace as it expects a strict match
on the whole of 'type', and does not expect to treat it as a bitfield.

Case in point, QEMU:

accel/kvm/kvm-all.c::kvm_cpu_exec()

        case KVM_EXIT_SYSTEM_EVENT:
            switch (run->system_event.type) {

CrosVM and kvmtool have similar constructs, and will break as soon as
KVM_SYSTEM_EVENT_NDATA_VALID is or'ed into 'type'.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Alexandru Elisei]

Hi,

On Mon, Apr 11, 2022 at 10:12:13AM +0100, Will Deacon wrote:
> Hi Sean,
> 
> Cheers for the heads-up.
> 
> [+Marc and Alex as this looks similar to [1]]
> 
> On Fri, Apr 08, 2022 at 05:01:21PM +0000, Sean Christopherson wrote:
> > On Fri, Apr 08, 2022, Peter Gonda wrote:
> > > On Thu, Apr 7, 2022 at 8:55 PM Sean Christopherson <seanjc@google.com> wrote:
> > > > On Thu, Apr 07, 2022, Peter Gonda wrote:
> > > > > If an SEV-ES guest requests termination, exit to userspace with
> > > > > KVM_EXIT_SYSTEM_EVENT and a dedicated SEV_TERM type instead of -EINVAL
> > > > > so that userspace can take appropriate action.
> > > > >
> > > > > See AMD's GHCB spec section '4.1.13 Termination Request' for more details.
> > > >
> > > > Maybe it'll be obvious by the lack of compilation errors, but the changelog should
> > > > call out the flags => ndata+data shenanigans, otherwise this looks like ABI breakage.
> > > 
> > > Hmm I am not sure we can do this change anymore given that we have two
> > > call sites using 'flags'
> > > 
> > > arch/arm64/kvm/psci.c:184
> > > arch/riscv/kvm/vcpu_sbi.c:97
> > > 
> > > I am not at all familiar with ARM and RISC-V but some quick reading
> > > tells me these archs also require 64-bit alignment on their 64-bit
> > > accesses. If thats correct, should I fix this call sites up by
> > > proceeding with this ndata + data[] change and move whatever they are
> > > assigning to flags into data[0] like I am doing here? It looks like
> > > both of these changes are not in a kernel release so IIUC we can still
> > > fix the ABI here?
> > 
> > Yeah, both came in for v5.18.  Given that there will be multiple paths that need
> > to set data, it's worth adding a common helper to the dirty work.
> > 
> > Anup and Will,
> > 
> > system_event.flags is broken (at least on x86) due to the prior 'type' field not
> > being propery padded, e.g. userspace will read/write garbage if the userspace
> > and kernel compilers pad structs differently.
> > 
> > 		struct {
> > 			__u32 type;
> > 			__u64 flags;
> > 		} system_event;
> 
> On arm64, I think the compiler is required to put the padding between type
> and flags so that both the struct and 'flags' are 64-bit aligned [2]. Does
> x86 not offer any guarantees on the overall structure alignment?

This is also my understanding. The "Procedure Call Standard for the Arm
64-bit Architecture" [1] has these rules for structs (called "aggregates"):

"An aggregate, where the members are laid out sequentially in memory
(possibly with inter-member padding)" => the field "type" is at offset 0 in
the struct.

"The member alignment of an element of a composite type is the alignment of
that member after the application of any language alignment modifiers to
that member" => "flags" is 8-byte aligned and "type" is 4-byte aligned.

"The alignment of an aggregate shall be the alignment of its most-aligned
member." => struct system_event is 8-byte aligned.

and finally:

"The size of an aggregate shall be the smallest multiple of its alignment
that is sufficient to hold all of its members."

I think this is the only possible layout that satisfies all the above
conditions:

offset 0-3: type (at an 8-byte aligned address in memory)
offset 4-7: padding
offset 8-15: flags

so under all compilers that correctly implement the architecture the struct
will have the same layout.

[1] https://github.com/ARM-software/abi-aa/releases/download/2021Q3/aapcs64.pdf

> 
> > Our plan to unhose this is to change the struct as follows and use bit 31 in the
> > 'type' to indicate that ndata+data are valid.
> > 
> > 		struct {
> >                         __u32 type;
> > 			__u32 ndata;
> > 			__u64 data[16];
> >                 } system_event;
> > 
> > Any objection to updating your architectures to use a helper to set the bit and
> > populate ndata+data accordingly?  It'll require a userspace update, but v5.18
> > hasn't officially released yet so it's not kinda sort not ABI breakage.
> 
> It's a bit annoying, as we're using the current structure in Android 13 :/
> Obviously, if there's no choice then upstream shouldn't worry, but it means
> we'll have to carry a delta in crosvm. Specifically, the new 'ndata' field
> is going to be unusable for us because it coincides with the padding.

Just a thought, but wouldn't such a drastical change be better implemented
as a new exit_reason and a new associated struct?

Thanks,
Alex

> 
> Will
> 
> [1] https://lore.kernel.org/r/20220407162327.396183-6-alexandru.elisei@arm.com
> [2] https://github.com/ARM-software/abi-aa/blob/60a8eb8c55e999d74dac5e368fc9d7e36e38dda4/aapcs64/aapcs64.rst#composite-types

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson]

On Mon, Apr 11, 2022, Marc Zyngier wrote:
> On Fri, 08 Apr 2022 17:56:42 +0100,
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > Queued, thanks.  But documentation was missing:
> > 
> > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> > index e7a0dfdc0178..72183ae628f7 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -6088,8 +6088,12 @@ should put the acknowledged interrupt vector into the 'epr' field.
> >    #define KVM_SYSTEM_EVENT_SHUTDOWN       1
> >    #define KVM_SYSTEM_EVENT_RESET          2
> >    #define KVM_SYSTEM_EVENT_CRASH          3
> > +  #define KVM_SYSTEM_EVENT_SEV_TERM       4
> > +  #define KVM_SYSTEM_EVENT_NDATA_VALID    (1u << 31)
> >  			__u32 type;
> > +                        __u32 ndata;
> >  			__u64 flags;
> > +                        __u64 data[16];
> >  		} system_event;
> > 
> >  If exit_reason is KVM_EXIT_SYSTEM_EVENT then the vcpu has triggered
> > @@ -6099,7 +6103,7 @@ HVC instruction based PSCI call from the vcpu. The 'type' field describes
> >  the system-level event type. The 'flags' field describes architecture
> >  specific flags for the system-level event.
> > 
> > -Valid values for 'type' are:
> > +Valid values for bits 30:0 of 'type' are:
> > 
> >   - KVM_SYSTEM_EVENT_SHUTDOWN -- the guest has requested a shutdown of the
> >     VM. Userspace is not obliged to honour this, and if it does honour
> > @@ -6112,12 +6116,18 @@ Valid values for 'type' are:
> >     has requested a crash condition maintenance. Userspace can choose
> >     to ignore the request, or to gather VM memory core dump and/or
> >     reset/shutdown of the VM.
> > + - KVM_SYSTEM_EVENT_SEV_TERM -- an AMD SEV guest requested termination.
> > +   The guest physical address of the guest's GHCB is stored in `data[0]`.
> > 
> >  Valid flags are:
> > 
> >   - KVM_SYSTEM_EVENT_RESET_FLAG_PSCI_RESET2 (arm64 only) -- the guest issued
> >     a SYSTEM_RESET2 call according to v1.1 of the PSCI specification.
> > 
> > +Extra data for this event is stored in the `data[]` array, up to index
> > +`ndata-1` included, if bit 31 is set in `type`.  The data depends on the
> > +`type` field.  There is no extra data if bit 31 is clear or `ndata` is zero.
> > +
> 
> This has the potential to break userspace as it expects a strict match
> on the whole of 'type', and does not expect to treat it as a bitfield.
> 
> Case in point, QEMU:
> 
> accel/kvm/kvm-all.c::kvm_cpu_exec()
> 
>         case KVM_EXIT_SYSTEM_EVENT:
>             switch (run->system_event.type) {
> 
> CrosVM and kvmtool have similar constructs, and will break as soon as
> KVM_SYSTEM_EVENT_NDATA_VALID is or'ed into 'type'.

Yeah, if we go this route, we'd have to make sure to document that only new types
can use the flag.

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson]

On Mon, Apr 11, 2022, Alexandru Elisei wrote:
> Hi,
>
> On Mon, Apr 11, 2022 at 10:12:13AM +0100, Will Deacon wrote:
> > Hi Sean,
> >
> > Cheers for the heads-up.
> >
> > [+Marc and Alex as this looks similar to [1]]
> >
> > On Fri, Apr 08, 2022 at 05:01:21PM +0000, Sean Christopherson wrote:
> > > system_event.flags is broken (at least on x86) due to the prior 'type' field not
> > > being propery padded, e.g. userspace will read/write garbage if the userspace
> > > and kernel compilers pad structs differently.
> > >
> > >           struct {
> > >                   __u32 type;
> > >                   __u64 flags;
> > >           } system_event;
> >
> > On arm64, I think the compiler is required to put the padding between type
> > and flags so that both the struct and 'flags' are 64-bit aligned [2]. Does
> > x86 not offer any guarantees on the overall structure alignment?
>
> This is also my understanding. The "Procedure Call Standard for the Arm
> 64-bit Architecture" [1] has these rules for structs (called "aggregates"):

AFAIK, all x86 compilers will pad structures accordingly, but a 32-bit userspace
running against a 64-bit kernel will have different alignment requirements, i.e.
won't pad, and x86 supports CONFIG_KVM_COMPAT=y.  And I have no idea what x86's
bizarre x32 ABI does.

> > > Our plan to unhose this is to change the struct as follows and use bit 31 in the
> > > 'type' to indicate that ndata+data are valid.
> > >
> > >           struct {
> > >                         __u32 type;
> > >                   __u32 ndata;
> > >                   __u64 data[16];
> > >                 } system_event;
> > >
> > > Any objection to updating your architectures to use a helper to set the bit and
> > > populate ndata+data accordingly?  It'll require a userspace update, but v5.18
> > > hasn't officially released yet so it's not kinda sort not ABI breakage.
> >
> > It's a bit annoying, as we're using the current structure in Android 13 :/
> > Obviously, if there's no choice then upstream shouldn't worry, but it means
> > we'll have to carry a delta in crosvm. Specifically, the new 'ndata' field
> > is going to be unusable for us because it coincides with the padding.

Yeah, it'd be unusuable for existing types.  One idea is that we could define the
ABI to be that the RESET and SHUTDOWN types have an implicit ndata=1 on arm64 and
RISC-V.  That would allow keeping the flags interpretation and so long as crosvm
doesn't do something stupid like compile with "pragma pack" (does clang even support
that?), there's no delta necessary for Android.

> Just a thought, but wouldn't such a drastical change be better implemented
> as a new exit_reason and a new associated struct?

Maybe?  I wasn't aware that arm64/RISC-V picked up usage of "flags" when I
suggested this, but I'm not sure it would have changed anything.  We could add
SYSTEM_EVENT2 or whatever, but since there's no official usage of flags, it seems
a bit gratutious.

Re: [PATCH v4.1] KVM, SEV: Add KVM_EXIT_SHUTDOWN metadata for SEV-ES

[Sean Christopherson]

PAOLO!!!!!!

Or maybe I need to try the Beetlejuice trick...

Paolo, Paolo, Paolo.

This is now sitting in kvm/next, which makes RISC-V and arm64 unhappy.  Thoughts
on how to proceed?

arch/riscv/kvm/vcpu_sbi.c: In function ‘kvm_riscv_vcpu_sbi_system_reset’:
arch/riscv/kvm/vcpu_sbi.c:97:26: error: ‘struct <anonymous>’ has no member named ‘flags’
   97 |         run->system_event.flags = flags;
      |                          ^


On Mon, Apr 11, 2022, Sean Christopherson wrote:
> On Mon, Apr 11, 2022, Alexandru Elisei wrote:
> > Hi,
> >
> > On Mon, Apr 11, 2022 at 10:12:13AM +0100, Will Deacon wrote:
> > > Hi Sean,
> > >
> > > Cheers for the heads-up.
> > >
> > > [+Marc and Alex as this looks similar to [1]]
> > >
> > > On Fri, Apr 08, 2022 at 05:01:21PM +0000, Sean Christopherson wrote:
> > > > system_event.flags is broken (at least on x86) due to the prior 'type' field not
> > > > being propery padded, e.g. userspace will read/write garbage if the userspace
> > > > and kernel compilers pad structs differently.
> > > >
> > > >           struct {
> > > >                   __u32 type;
> > > >                   __u64 flags;
> > > >           } system_event;
> > >
> > > On arm64, I think the compiler is required to put the padding between type
> > > and flags so that both the struct and 'flags' are 64-bit aligned [2]. Does
> > > x86 not offer any guarantees on the overall structure alignment?
> >
> > This is also my understanding. The "Procedure Call Standard for the Arm
> > 64-bit Architecture" [1] has these rules for structs (called "aggregates"):
> 
> AFAIK, all x86 compilers will pad structures accordingly, but a 32-bit userspace
> running against a 64-bit kernel will have different alignment requirements, i.e.
> won't pad, and x86 supports CONFIG_KVM_COMPAT=y.  And I have no idea what x86's
> bizarre x32 ABI does.
> 
> > > > Our plan to unhose this is to change the struct as follows and use bit 31 in the
> > > > 'type' to indicate that ndata+data are valid.
> > > >
> > > >           struct {
> > > >                         __u32 type;
> > > >                   __u32 ndata;
> > > >                   __u64 data[16];
> > > >                 } system_event;
> > > >
> > > > Any objection to updating your architectures to use a helper to set the bit and
> > > > populate ndata+data accordingly?  It'll require a userspace update, but v5.18
> > > > hasn't officially released yet so it's not kinda sort not ABI breakage.
> > >
> > > It's a bit annoying, as we're using the current structure in Android 13 :/
> > > Obviously, if there's no choice then upstream shouldn't worry, but it means
> > > we'll have to carry a delta in crosvm. Specifically, the new 'ndata' field
> > > is going to be unusable for us because it coincides with the padding.
> 
> Yeah, it'd be unusuable for existing types.  One idea is that we could define the
> ABI to be that the RESET and SHUTDOWN types have an implicit ndata=1 on arm64 and
> RISC-V.  That would allow keeping the flags interpretation and so long as crosvm
> doesn't do something stupid like compile with "pragma pack" (does clang even support
> that?), there's no delta necessary for Android.
> 
> > Just a thought, but wouldn't such a drastical change be better implemented
> > as a new exit_reason and a new associated struct?
> 
> Maybe?  I wasn't aware that arm64/RISC-V picked up usage of "flags" when I
> suggested this, but I'm not sure it would have changed anything.  We could add
> SYSTEM_EVENT2 or whatever, but since there's no official usage of flags, it seems
> a bit gratutious.