[PATCH v1 ppc-for-5.0 0/2] ppc/spapr: Support reboot of secure pseries guest

[PATCH v1 ppc-for-5.0 0/2] ppc/spapr: Support reboot of secure pseries guest

[Bharata B Rao]

This patchset adds KVM_PPC_SVM_OFF ioctl which is required to support
reset of secure guest. This includes linux-headers update so that we get
the newly introduced ioctl.

v0: https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg02408.html

Bharata B Rao (2):
  linux-headers: Update
  ppc/spapr: Support reboot of secure pseries guest

 hw/ppc/spapr.c                                |  1 +
 include/standard-headers/asm-x86/bootparam.h  |  7 +-
 .../infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 15 +++-
 include/standard-headers/drm/drm_fourcc.h     | 28 ++++++-
 .../linux/input-event-codes.h                 | 77 +++++++++++++++++++
 include/standard-headers/linux/pci_regs.h     |  3 +
 .../standard-headers/rdma/vmw_pvrdma-abi.h    |  5 ++
 linux-headers/linux/kvm.h                     |  1 +
 target/ppc/kvm.c                              |  7 ++
 target/ppc/kvm_ppc.h                          |  6 ++
 10 files changed, 146 insertions(+), 4 deletions(-)

-- 
2.21.0

[PATCH v1 ppc-for-5.0 1/2] linux-headers: Update

[Bharata B Rao]

Update to mainline commit: e42617b825f8 ("Linux 5.5-rc1")

Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
---
 include/standard-headers/asm-x86/bootparam.h  |  7 +-
 .../infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 15 +++-
 include/standard-headers/drm/drm_fourcc.h     | 28 ++++++-
 .../linux/input-event-codes.h                 | 77 +++++++++++++++++++
 include/standard-headers/linux/pci_regs.h     |  3 +
 .../standard-headers/rdma/vmw_pvrdma-abi.h    |  5 ++
 linux-headers/linux/kvm.h                     |  1 +
 7 files changed, 132 insertions(+), 4 deletions(-)

diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h
index a6f7cf535e..072e2ed546 100644
--- a/include/standard-headers/asm-x86/bootparam.h
+++ b/include/standard-headers/asm-x86/bootparam.h
@@ -2,7 +2,7 @@
 #ifndef _ASM_X86_BOOTPARAM_H
 #define _ASM_X86_BOOTPARAM_H
 
-/* setup_data types */
+/* setup_data/setup_indirect types */
 #define SETUP_NONE			0
 #define SETUP_E820_EXT			1
 #define SETUP_DTB			2
@@ -11,6 +11,11 @@
 #define SETUP_APPLE_PROPERTIES		5
 #define SETUP_JAILHOUSE			6
 
+#define SETUP_INDIRECT			(1<<31)
+
+/* SETUP_INDIRECT | max(SETUP_*) */
+#define SETUP_TYPE_MAX			(SETUP_INDIRECT | SETUP_JAILHOUSE)
+
 /* ram_size flags */
 #define RAMDISK_IMAGE_START_MASK	0x07FF
 #define RAMDISK_PROMPT_FLAG		0x8000
diff --git a/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h
index d019872608..a5a1c8234e 100644
--- a/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h
+++ b/include/standard-headers/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h
@@ -58,7 +58,8 @@
 #define PVRDMA_ROCEV1_VERSION		17
 #define PVRDMA_ROCEV2_VERSION		18
 #define PVRDMA_PPN64_VERSION		19
-#define PVRDMA_VERSION			PVRDMA_PPN64_VERSION
+#define PVRDMA_QPHANDLE_VERSION		20
+#define PVRDMA_VERSION			PVRDMA_QPHANDLE_VERSION
 
 #define PVRDMA_BOARD_ID			1
 #define PVRDMA_REV_ID			1
@@ -581,6 +582,17 @@ struct pvrdma_cmd_create_qp_resp {
 	uint32_t max_inline_data;
 };
 
+struct pvrdma_cmd_create_qp_resp_v2 {
+	struct pvrdma_cmd_resp_hdr hdr;
+	uint32_t qpn;
+	uint32_t qp_handle;
+	uint32_t max_send_wr;
+	uint32_t max_recv_wr;
+	uint32_t max_send_sge;
+	uint32_t max_recv_sge;
+	uint32_t max_inline_data;
+};
+
 struct pvrdma_cmd_modify_qp {
 	struct pvrdma_cmd_hdr hdr;
 	uint32_t qp_handle;
@@ -663,6 +675,7 @@ union pvrdma_cmd_resp {
 	struct pvrdma_cmd_create_cq_resp create_cq_resp;
 	struct pvrdma_cmd_resize_cq_resp resize_cq_resp;
 	struct pvrdma_cmd_create_qp_resp create_qp_resp;
+	struct pvrdma_cmd_create_qp_resp_v2 create_qp_resp_v2;
 	struct pvrdma_cmd_query_qp_resp query_qp_resp;
 	struct pvrdma_cmd_destroy_qp_resp destroy_qp_resp;
 	struct pvrdma_cmd_create_srq_resp create_srq_resp;
diff --git a/include/standard-headers/drm/drm_fourcc.h b/include/standard-headers/drm/drm_fourcc.h
index a308c91b4f..46d279f515 100644
--- a/include/standard-headers/drm/drm_fourcc.h
+++ b/include/standard-headers/drm/drm_fourcc.h
@@ -68,7 +68,7 @@ extern "C" {
 #define fourcc_code(a, b, c, d) ((uint32_t)(a) | ((uint32_t)(b) << 8) | \
 				 ((uint32_t)(c) << 16) | ((uint32_t)(d) << 24))
 
-#define DRM_FORMAT_BIG_ENDIAN (1<<31) /* format is big endian instead of little endian */
+#define DRM_FORMAT_BIG_ENDIAN (1U<<31) /* format is big endian instead of little endian */
 
 /* Reserve 0 for the invalid format specifier */
 #define DRM_FORMAT_INVALID	0
@@ -647,7 +647,21 @@ extern "C" {
  * Further information on the use of AFBC modifiers can be found in
  * Documentation/gpu/afbc.rst
  */
-#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode)	fourcc_mod_code(ARM, __afbc_mode)
+
+/*
+ * The top 4 bits (out of the 56 bits alloted for specifying vendor specific
+ * modifiers) denote the category for modifiers. Currently we have only two
+ * categories of modifiers ie AFBC and MISC. We can have a maximum of sixteen
+ * different categories.
+ */
+#define DRM_FORMAT_MOD_ARM_CODE(__type, __val) \
+	fourcc_mod_code(ARM, ((uint64_t)(__type) << 52) | ((__val) & 0x000fffffffffffffULL))
+
+#define DRM_FORMAT_MOD_ARM_TYPE_AFBC 0x00
+#define DRM_FORMAT_MOD_ARM_TYPE_MISC 0x01
+
+#define DRM_FORMAT_MOD_ARM_AFBC(__afbc_mode) \
+	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_AFBC, __afbc_mode)
 
 /*
  * AFBC superblock size
@@ -741,6 +755,16 @@ extern "C" {
  */
 #define AFBC_FORMAT_MOD_BCH     (1ULL << 11)
 
+/*
+ * Arm 16x16 Block U-Interleaved modifier
+ *
+ * This is used by Arm Mali Utgard and Midgard GPUs. It divides the image
+ * into 16x16 pixel blocks. Blocks are stored linearly in order, but pixels
+ * in the block are reordered.
+ */
+#define DRM_FORMAT_MOD_ARM_16X16_BLOCK_U_INTERLEAVED \
+	DRM_FORMAT_MOD_ARM_CODE(DRM_FORMAT_MOD_ARM_TYPE_MISC, 1ULL)
+
 /*
  * Allwinner tiled modifier
  *
diff --git a/include/standard-headers/linux/input-event-codes.h b/include/standard-headers/linux/input-event-codes.h
index eb08cb8598..b484c25289 100644
--- a/include/standard-headers/linux/input-event-codes.h
+++ b/include/standard-headers/linux/input-event-codes.h
@@ -649,6 +649,83 @@
  */
 #define KEY_DATA			0x277
 #define KEY_ONSCREEN_KEYBOARD		0x278
+/* Electronic privacy screen control */
+#define KEY_PRIVACY_SCREEN_TOGGLE	0x279
+
+/*
+ * Some keyboards have keys which do not have a defined meaning, these keys
+ * are intended to be programmed / bound to macros by the user. For most
+ * keyboards with these macro-keys the key-sequence to inject, or action to
+ * take, is all handled by software on the host side. So from the kernel's
+ * point of view these are just normal keys.
+ *
+ * The KEY_MACRO# codes below are intended for such keys, which may be labeled
+ * e.g. G1-G18, or S1 - S30. The KEY_MACRO# codes MUST NOT be used for keys
+ * where the marking on the key does indicate a defined meaning / purpose.
+ *
+ * The KEY_MACRO# codes MUST also NOT be used as fallback for when no existing
+ * KEY_FOO define matches the marking / purpose. In this case a new KEY_FOO
+ * define MUST be added.
+ */
+#define KEY_MACRO1			0x290
+#define KEY_MACRO2			0x291
+#define KEY_MACRO3			0x292
+#define KEY_MACRO4			0x293
+#define KEY_MACRO5			0x294
+#define KEY_MACRO6			0x295
+#define KEY_MACRO7			0x296
+#define KEY_MACRO8			0x297
+#define KEY_MACRO9			0x298
+#define KEY_MACRO10			0x299
+#define KEY_MACRO11			0x29a
+#define KEY_MACRO12			0x29b
+#define KEY_MACRO13			0x29c
+#define KEY_MACRO14			0x29d
+#define KEY_MACRO15			0x29e
+#define KEY_MACRO16			0x29f
+#define KEY_MACRO17			0x2a0
+#define KEY_MACRO18			0x2a1
+#define KEY_MACRO19			0x2a2
+#define KEY_MACRO20			0x2a3
+#define KEY_MACRO21			0x2a4
+#define KEY_MACRO22			0x2a5
+#define KEY_MACRO23			0x2a6
+#define KEY_MACRO24			0x2a7
+#define KEY_MACRO25			0x2a8
+#define KEY_MACRO26			0x2a9
+#define KEY_MACRO27			0x2aa
+#define KEY_MACRO28			0x2ab
+#define KEY_MACRO29			0x2ac
+#define KEY_MACRO30			0x2ad
+
+/*
+ * Some keyboards with the macro-keys described above have some extra keys
+ * for controlling the host-side software responsible for the macro handling:
+ * -A macro recording start/stop key. Note that not all keyboards which emit
+ *  KEY_MACRO_RECORD_START will also emit KEY_MACRO_RECORD_STOP if
+ *  KEY_MACRO_RECORD_STOP is not advertised, then KEY_MACRO_RECORD_START
+ *  should be interpreted as a recording start/stop toggle;
+ * -Keys for switching between different macro (pre)sets, either a key for
+ *  cycling through the configured presets or keys to directly select a preset.
+ */
+#define KEY_MACRO_RECORD_START		0x2b0
+#define KEY_MACRO_RECORD_STOP		0x2b1
+#define KEY_MACRO_PRESET_CYCLE		0x2b2
+#define KEY_MACRO_PRESET1		0x2b3
+#define KEY_MACRO_PRESET2		0x2b4
+#define KEY_MACRO_PRESET3		0x2b5
+
+/*
+ * Some keyboards have a buildin LCD panel where the contents are controlled
+ * by the host. Often these have a number of keys directly below the LCD
+ * intended for controlling a menu shown on the LCD. These keys often don't
+ * have any labeling so we just name them KEY_KBD_LCD_MENU#
+ */
+#define KEY_KBD_LCD_MENU1		0x2b8
+#define KEY_KBD_LCD_MENU2		0x2b9
+#define KEY_KBD_LCD_MENU3		0x2ba
+#define KEY_KBD_LCD_MENU4		0x2bb
+#define KEY_KBD_LCD_MENU5		0x2bc
 
 #define BTN_TRIGGER_HAPPY		0x2c0
 #define BTN_TRIGGER_HAPPY1		0x2c0
diff --git a/include/standard-headers/linux/pci_regs.h b/include/standard-headers/linux/pci_regs.h
index 29d6e93fd1..acb7d2bdb4 100644
--- a/include/standard-headers/linux/pci_regs.h
+++ b/include/standard-headers/linux/pci_regs.h
@@ -34,6 +34,7 @@
  * of which the first 64 bytes are standardized as follows:
  */
 #define PCI_STD_HEADER_SIZEOF	64
+#define PCI_STD_NUM_BARS	6	/* Number of standard BARs */
 #define PCI_VENDOR_ID		0x00	/* 16 bits */
 #define PCI_DEVICE_ID		0x02	/* 16 bits */
 #define PCI_COMMAND		0x04	/* 16 bits */
@@ -673,6 +674,8 @@
 #define  PCI_EXP_LNKCTL2_TLS_8_0GT	0x0003 /* Supported Speed 8GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_16_0GT	0x0004 /* Supported Speed 16GT/s */
 #define  PCI_EXP_LNKCTL2_TLS_32_0GT	0x0005 /* Supported Speed 32GT/s */
+#define  PCI_EXP_LNKCTL2_ENTER_COMP	0x0010 /* Enter Compliance */
+#define  PCI_EXP_LNKCTL2_TX_MARGIN	0x0380 /* Transmit Margin */
 #define PCI_EXP_LNKSTA2		50	/* Link Status 2 */
 #define PCI_CAP_EXP_ENDPOINT_SIZEOF_V2	52	/* v2 endpoints with link end here */
 #define PCI_EXP_SLTCAP2		52	/* Slot Capabilities 2 */
diff --git a/include/standard-headers/rdma/vmw_pvrdma-abi.h b/include/standard-headers/rdma/vmw_pvrdma-abi.h
index 336a8d596f..0989426a3f 100644
--- a/include/standard-headers/rdma/vmw_pvrdma-abi.h
+++ b/include/standard-headers/rdma/vmw_pvrdma-abi.h
@@ -179,6 +179,11 @@ struct pvrdma_create_qp {
 	uint64_t __attribute__((aligned(8))) qp_addr;
 };
 
+struct pvrdma_create_qp_resp {
+	uint32_t qpn;
+	uint32_t qp_handle;
+};
+
 /* PVRDMA masked atomic compare and swap */
 struct pvrdma_ex_cmp_swap {
 	uint64_t __attribute__((aligned(8))) swap_val;
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 3b27a1ae85..9d647fad76 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1348,6 +1348,7 @@ struct kvm_s390_ucas_mapping {
 #define KVM_PPC_GET_CPU_CHAR	  _IOR(KVMIO,  0xb1, struct kvm_ppc_cpu_char)
 /* Available with KVM_CAP_PMU_EVENT_FILTER */
 #define KVM_SET_PMU_EVENT_FILTER  _IOW(KVMIO,  0xb2, struct kvm_pmu_event_filter)
+#define KVM_PPC_SVM_OFF		  _IO(KVMIO,  0xb3)
 
 /* ioctl for vm fd */
 #define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct kvm_create_device)
-- 
2.21.0

[PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[Bharata B Rao]

A pseries guest can be run as a secure guest on Ultravisor-enabled
POWER platforms. When such a secure guest is reset, we need to
release/reset a few resources both on ultravisor and hypervisor side.
This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
machine reset path.

As part of this ioctl, the secure guest is essentially transitioned
back to normal mode so that it can reboot like a regular guest and
become secure again.

This ioctl has no effect when invoked for a normal guest.

Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
---
 hw/ppc/spapr.c       | 1 +
 target/ppc/kvm.c     | 7 +++++++
 target/ppc/kvm_ppc.h | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f11422fc41..4c7ad3400d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
     void *fdt;
     int rc;
 
+    kvmppc_svm_off();
     spapr_caps_apply(spapr);
 
     first_ppc_cpu = POWERPC_CPU(first_cpu);
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 7406d18945..1a86fa4f0c 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2900,3 +2900,10 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t tb_offset)
         kvm_set_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &tb_offset);
     }
 }
+
+int kvmppc_svm_off(void)
+{
+    KVMState *s = KVM_STATE(current_machine->accelerator);
+
+    return kvm_vm_ioctl(s, KVM_PPC_SVM_OFF);
+}
diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 47b08a4030..5cc812e486 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -37,6 +37,7 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
 target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
                                      bool radix, bool gtse,
                                      uint64_t proc_tbl);
+int kvmppc_svm_off(void);
 #ifndef CONFIG_USER_ONLY
 bool kvmppc_spapr_use_multitce(void);
 int kvmppc_spapr_enable_inkernel_multitce(void);
@@ -201,6 +202,11 @@ static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
     return 0;
 }
 
+static inline int kvmppc_svm_off(void)
+{
+    return 0;
+}
+
 static inline void kvmppc_set_reg_ppc_online(PowerPCCPU *cpu,
                                              unsigned int online)
 {
-- 
2.21.0

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2860 bytes --]

On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
> A pseries guest can be run as a secure guest on Ultravisor-enabled
> POWER platforms. When such a secure guest is reset, we need to
> release/reset a few resources both on ultravisor and hypervisor side.
> This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> machine reset path.
> 
> As part of this ioctl, the secure guest is essentially transitioned
> back to normal mode so that it can reboot like a regular guest and
> become secure again.
> 
> This ioctl has no effect when invoked for a normal guest.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> ---
>  hw/ppc/spapr.c       | 1 +
>  target/ppc/kvm.c     | 7 +++++++
>  target/ppc/kvm_ppc.h | 6 ++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f11422fc41..4c7ad3400d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
>      void *fdt;
>      int rc;
>  
> +    kvmppc_svm_off();

If you're going to have this return an error value, you should really
check it here.

>      spapr_caps_apply(spapr);
>  
>      first_ppc_cpu = POWERPC_CPU(first_cpu);
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 7406d18945..1a86fa4f0c 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2900,3 +2900,10 @@ void kvmppc_set_reg_tb_offset(PowerPCCPU *cpu, int64_t tb_offset)
>          kvm_set_one_reg(cs, KVM_REG_PPC_TB_OFFSET, &tb_offset);
>      }
>  }
> +
> +int kvmppc_svm_off(void)
> +{
> +    KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +    return kvm_vm_ioctl(s, KVM_PPC_SVM_OFF);
> +}
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 47b08a4030..5cc812e486 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -37,6 +37,7 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
>  target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
>                                       bool radix, bool gtse,
>                                       uint64_t proc_tbl);
> +int kvmppc_svm_off(void);
>  #ifndef CONFIG_USER_ONLY
>  bool kvmppc_spapr_use_multitce(void);
>  int kvmppc_spapr_enable_inkernel_multitce(void);
> @@ -201,6 +202,11 @@ static inline target_ulong kvmppc_configure_v3_mmu(PowerPCCPU *cpu,
>      return 0;
>  }
>  
> +static inline int kvmppc_svm_off(void)
> +{
> +    return 0;
> +}
> +
>  static inline void kvmppc_set_reg_ppc_online(PowerPCCPU *cpu,
>                                               unsigned int online)
>  {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[Bharata B Rao]

On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
> On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
> > A pseries guest can be run as a secure guest on Ultravisor-enabled
> > POWER platforms. When such a secure guest is reset, we need to
> > release/reset a few resources both on ultravisor and hypervisor side.
> > This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> > machine reset path.
> > 
> > As part of this ioctl, the secure guest is essentially transitioned
> > back to normal mode so that it can reboot like a regular guest and
> > become secure again.
> > 
> > This ioctl has no effect when invoked for a normal guest.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> > ---
> >  hw/ppc/spapr.c       | 1 +
> >  target/ppc/kvm.c     | 7 +++++++
> >  target/ppc/kvm_ppc.h | 6 ++++++
> >  3 files changed, 14 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f11422fc41..4c7ad3400d 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
> >      void *fdt;
> >      int rc;
> >  
> > +    kvmppc_svm_off();
> 
> If you're going to have this return an error value, you should really
> check it here.

I could, by spapr_machine_reset() and the callers don't propagate the
errors up. So may be I could print a warning instead when ioctl fails?

Regards,
Bharata.

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[Alexey Kardashevskiy]

On 10/12/2019 14:50, Bharata B Rao wrote:
> On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
>> On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
>>> A pseries guest can be run as a secure guest on Ultravisor-enabled
>>> POWER platforms. When such a secure guest is reset, we need to
>>> release/reset a few resources both on ultravisor and hypervisor side.
>>> This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
>>> machine reset path.
>>>
>>> As part of this ioctl, the secure guest is essentially transitioned
>>> back to normal mode so that it can reboot like a regular guest and
>>> become secure again.
>>>
>>> This ioctl has no effect when invoked for a normal guest.
>>>
>>> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
>>> ---
>>>  hw/ppc/spapr.c       | 1 +
>>>  target/ppc/kvm.c     | 7 +++++++
>>>  target/ppc/kvm_ppc.h | 6 ++++++
>>>  3 files changed, 14 insertions(+)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index f11422fc41..4c7ad3400d 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
>>>      void *fdt;
>>>      int rc;
>>>  
>>> +    kvmppc_svm_off();
>>
>> If you're going to have this return an error value, you should really
>> check it here.
> 
> I could, by spapr_machine_reset() and the callers don't propagate the
> errors up. So may be I could print a warning instead when ioctl fails?

An error here means you cannot restart the machine and should probably
suspend, or try until it is not EBUSY (==all threads have stopped?).


-- 
Alexey

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Tue, Dec 10, 2019 at 03:03:01PM +1100, Alexey Kardashevskiy wrote:
> 
> 
> On 10/12/2019 14:50, Bharata B Rao wrote:
> > On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
> >> On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
> >>> A pseries guest can be run as a secure guest on Ultravisor-enabled
> >>> POWER platforms. When such a secure guest is reset, we need to
> >>> release/reset a few resources both on ultravisor and hypervisor side.
> >>> This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> >>> machine reset path.
> >>>
> >>> As part of this ioctl, the secure guest is essentially transitioned
> >>> back to normal mode so that it can reboot like a regular guest and
> >>> become secure again.
> >>>
> >>> This ioctl has no effect when invoked for a normal guest.
> >>>
> >>> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> >>> ---
> >>>  hw/ppc/spapr.c       | 1 +
> >>>  target/ppc/kvm.c     | 7 +++++++
> >>>  target/ppc/kvm_ppc.h | 6 ++++++
> >>>  3 files changed, 14 insertions(+)
> >>>
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>> index f11422fc41..4c7ad3400d 100644
> >>> --- a/hw/ppc/spapr.c
> >>> +++ b/hw/ppc/spapr.c
> >>> @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
> >>>      void *fdt;
> >>>      int rc;
> >>>  
> >>> +    kvmppc_svm_off();
> >>
> >> If you're going to have this return an error value, you should really
> >> check it here.
> > 
> > I could, by spapr_machine_reset() and the callers don't propagate the
> > errors up. So may be I could print a warning instead when ioctl fails?
> 
> An error here means you cannot restart the machine and should probably
> suspend, or try until it is not EBUSY (==all threads have stopped?).

Right, if this fails, something has gone badly wrong.  You should
absolutely print a message, and in fact it might be appropriate to
quit outright.  IIUC the way PEF resets work, a failure here means you
won't be able to boot after the reset, since the guest memory will
still be inaccessible to the host.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[Bharata B Rao]

On Tue, Dec 10, 2019 at 04:05:36PM +1100, David Gibson wrote:
> On Tue, Dec 10, 2019 at 03:03:01PM +1100, Alexey Kardashevskiy wrote:
> > 
> > 
> > On 10/12/2019 14:50, Bharata B Rao wrote:
> > > On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
> > >> On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
> > >>> A pseries guest can be run as a secure guest on Ultravisor-enabled
> > >>> POWER platforms. When such a secure guest is reset, we need to
> > >>> release/reset a few resources both on ultravisor and hypervisor side.
> > >>> This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> > >>> machine reset path.
> > >>>
> > >>> As part of this ioctl, the secure guest is essentially transitioned
> > >>> back to normal mode so that it can reboot like a regular guest and
> > >>> become secure again.
> > >>>
> > >>> This ioctl has no effect when invoked for a normal guest.
> > >>>
> > >>> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> > >>> ---
> > >>>  hw/ppc/spapr.c       | 1 +
> > >>>  target/ppc/kvm.c     | 7 +++++++
> > >>>  target/ppc/kvm_ppc.h | 6 ++++++
> > >>>  3 files changed, 14 insertions(+)
> > >>>
> > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > >>> index f11422fc41..4c7ad3400d 100644
> > >>> --- a/hw/ppc/spapr.c
> > >>> +++ b/hw/ppc/spapr.c
> > >>> @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
> > >>>      void *fdt;
> > >>>      int rc;
> > >>>  
> > >>> +    kvmppc_svm_off();
> > >>
> > >> If you're going to have this return an error value, you should really
> > >> check it here.
> > > 
> > > I could, by spapr_machine_reset() and the callers don't propagate the
> > > errors up. So may be I could print a warning instead when ioctl fails?
> > 
> > An error here means you cannot restart the machine and should probably
> > suspend, or try until it is not EBUSY (==all threads have stopped?).
> 
> Right, if this fails, something has gone badly wrong.  You should
> absolutely print a message, and in fact it might be appropriate to
> quit outright.  IIUC the way PEF resets work, a failure here means you
> won't be able to boot after the reset, since the guest memory will
> still be inaccessible to the host.

Correct. I will send next version with a message and abort() added in
the ioctl failure path.

Regards,
Bharata.

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]

On Tue, Dec 10, 2019 at 12:20:07PM +0530, Bharata B Rao wrote:
> On Tue, Dec 10, 2019 at 04:05:36PM +1100, David Gibson wrote:
> > On Tue, Dec 10, 2019 at 03:03:01PM +1100, Alexey Kardashevskiy wrote:
> > > 
> > > 
> > > On 10/12/2019 14:50, Bharata B Rao wrote:
> > > > On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
> > > >> On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
> > > >>> A pseries guest can be run as a secure guest on Ultravisor-enabled
> > > >>> POWER platforms. When such a secure guest is reset, we need to
> > > >>> release/reset a few resources both on ultravisor and hypervisor side.
> > > >>> This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> > > >>> machine reset path.
> > > >>>
> > > >>> As part of this ioctl, the secure guest is essentially transitioned
> > > >>> back to normal mode so that it can reboot like a regular guest and
> > > >>> become secure again.
> > > >>>
> > > >>> This ioctl has no effect when invoked for a normal guest.
> > > >>>
> > > >>> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> > > >>> ---
> > > >>>  hw/ppc/spapr.c       | 1 +
> > > >>>  target/ppc/kvm.c     | 7 +++++++
> > > >>>  target/ppc/kvm_ppc.h | 6 ++++++
> > > >>>  3 files changed, 14 insertions(+)
> > > >>>
> > > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > >>> index f11422fc41..4c7ad3400d 100644
> > > >>> --- a/hw/ppc/spapr.c
> > > >>> +++ b/hw/ppc/spapr.c
> > > >>> @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
> > > >>>      void *fdt;
> > > >>>      int rc;
> > > >>>  
> > > >>> +    kvmppc_svm_off();
> > > >>
> > > >> If you're going to have this return an error value, you should really
> > > >> check it here.
> > > > 
> > > > I could, by spapr_machine_reset() and the callers don't propagate the
> > > > errors up. So may be I could print a warning instead when ioctl fails?
> > > 
> > > An error here means you cannot restart the machine and should probably
> > > suspend, or try until it is not EBUSY (==all threads have stopped?).
> > 
> > Right, if this fails, something has gone badly wrong.  You should
> > absolutely print a message, and in fact it might be appropriate to
> > quit outright.  IIUC the way PEF resets work, a failure here means you
> > won't be able to boot after the reset, since the guest memory will
> > still be inaccessible to the host.
> 
> Correct. I will send next version with a message and abort() added in
> the ioctl failure path.

abort() or assert() isn't right either - that's reserved for things
that are definitely caused by a qemu code bug.  This should be an
exit(EXIT_FAILURE).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[Bharata B Rao]

On Wed, Dec 11, 2019 at 10:41:32AM +1100, David Gibson wrote:
> On Tue, Dec 10, 2019 at 12:20:07PM +0530, Bharata B Rao wrote:
> > On Tue, Dec 10, 2019 at 04:05:36PM +1100, David Gibson wrote:
> > > On Tue, Dec 10, 2019 at 03:03:01PM +1100, Alexey Kardashevskiy wrote:
> > > > 
> > > > 
> > > > On 10/12/2019 14:50, Bharata B Rao wrote:
> > > > > On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
> > > > >> On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
> > > > >>> A pseries guest can be run as a secure guest on Ultravisor-enabled
> > > > >>> POWER platforms. When such a secure guest is reset, we need to
> > > > >>> release/reset a few resources both on ultravisor and hypervisor side.
> > > > >>> This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> > > > >>> machine reset path.
> > > > >>>
> > > > >>> As part of this ioctl, the secure guest is essentially transitioned
> > > > >>> back to normal mode so that it can reboot like a regular guest and
> > > > >>> become secure again.
> > > > >>>
> > > > >>> This ioctl has no effect when invoked for a normal guest.
> > > > >>>
> > > > >>> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> > > > >>> ---
> > > > >>>  hw/ppc/spapr.c       | 1 +
> > > > >>>  target/ppc/kvm.c     | 7 +++++++
> > > > >>>  target/ppc/kvm_ppc.h | 6 ++++++
> > > > >>>  3 files changed, 14 insertions(+)
> > > > >>>
> > > > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > >>> index f11422fc41..4c7ad3400d 100644
> > > > >>> --- a/hw/ppc/spapr.c
> > > > >>> +++ b/hw/ppc/spapr.c
> > > > >>> @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
> > > > >>>      void *fdt;
> > > > >>>      int rc;
> > > > >>>  
> > > > >>> +    kvmppc_svm_off();
> > > > >>
> > > > >> If you're going to have this return an error value, you should really
> > > > >> check it here.
> > > > > 
> > > > > I could, by spapr_machine_reset() and the callers don't propagate the
> > > > > errors up. So may be I could print a warning instead when ioctl fails?
> > > > 
> > > > An error here means you cannot restart the machine and should probably
> > > > suspend, or try until it is not EBUSY (==all threads have stopped?).
> > > 
> > > Right, if this fails, something has gone badly wrong.  You should
> > > absolutely print a message, and in fact it might be appropriate to
> > > quit outright.  IIUC the way PEF resets work, a failure here means you
> > > won't be able to boot after the reset, since the guest memory will
> > > still be inaccessible to the host.
> > 
> > Correct. I will send next version with a message and abort() added in
> > the ioctl failure path.
> 
> abort() or assert() isn't right either - that's reserved for things
> that are definitely caused by a qemu code bug.  This should be an
> exit(EXIT_FAILURE).

Ok, but I see a problem with checking the return value of this
ioctl from userspace. If this ioctl is run on older kernels that don't
support this ioctl, we get -ENOTTY as return value. We shouldn't be
exiting in that case.

It looks like we may need a new KVM capability to advertise the presence
of KVM_PPC_SVM_OFF ioctl (or more generally, to advertise host kernel's
capability to support secure guests). 

Paul - Do you think we should add such a KVM capability? Here is the
summary of the problem:

1. QEMU invokes KVM_PPC_SVM_OFF ioctl from machine reset path and currently
   we don't check for its return value.
2. On host kernels that support secure guests,
   2a. this ioctl returns 0 for regular guests and has no effect.
   2b. the ioctl can fail for secure guests and here we could check the
       return value and exit the guest right away.
3. On host kernels that don't support secure guests, ioctl returns -ENOTTY
   but we ignore the return value and continue the reset as this is
   for a non-secure guest.

If we have such a KVM capability, we could invoke the ioctl only if it
is supported and handle the return value appropriately.

Regards,
Bharata.

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]

On Wed, Dec 11, 2019 at 10:38:24AM +0530, Bharata B Rao wrote:
> On Wed, Dec 11, 2019 at 10:41:32AM +1100, David Gibson wrote:
> > On Tue, Dec 10, 2019 at 12:20:07PM +0530, Bharata B Rao wrote:
> > > On Tue, Dec 10, 2019 at 04:05:36PM +1100, David Gibson wrote:
> > > > On Tue, Dec 10, 2019 at 03:03:01PM +1100, Alexey Kardashevskiy wrote:
> > > > > 
> > > > > 
> > > > > On 10/12/2019 14:50, Bharata B Rao wrote:
> > > > > > On Tue, Dec 10, 2019 at 02:28:51PM +1100, David Gibson wrote:
> > > > > >> On Mon, Dec 09, 2019 at 12:30:12PM +0530, Bharata B Rao wrote:
> > > > > >>> A pseries guest can be run as a secure guest on Ultravisor-enabled
> > > > > >>> POWER platforms. When such a secure guest is reset, we need to
> > > > > >>> release/reset a few resources both on ultravisor and hypervisor side.
> > > > > >>> This is achieved by invoking this new ioctl KVM_PPC_SVM_OFF from the
> > > > > >>> machine reset path.
> > > > > >>>
> > > > > >>> As part of this ioctl, the secure guest is essentially transitioned
> > > > > >>> back to normal mode so that it can reboot like a regular guest and
> > > > > >>> become secure again.
> > > > > >>>
> > > > > >>> This ioctl has no effect when invoked for a normal guest.
> > > > > >>>
> > > > > >>> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> > > > > >>> ---
> > > > > >>>  hw/ppc/spapr.c       | 1 +
> > > > > >>>  target/ppc/kvm.c     | 7 +++++++
> > > > > >>>  target/ppc/kvm_ppc.h | 6 ++++++
> > > > > >>>  3 files changed, 14 insertions(+)
> > > > > >>>
> > > > > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > > >>> index f11422fc41..4c7ad3400d 100644
> > > > > >>> --- a/hw/ppc/spapr.c
> > > > > >>> +++ b/hw/ppc/spapr.c
> > > > > >>> @@ -1597,6 +1597,7 @@ static void spapr_machine_reset(MachineState *machine)
> > > > > >>>      void *fdt;
> > > > > >>>      int rc;
> > > > > >>>  
> > > > > >>> +    kvmppc_svm_off();
> > > > > >>
> > > > > >> If you're going to have this return an error value, you should really
> > > > > >> check it here.
> > > > > > 
> > > > > > I could, by spapr_machine_reset() and the callers don't propagate the
> > > > > > errors up. So may be I could print a warning instead when ioctl fails?
> > > > > 
> > > > > An error here means you cannot restart the machine and should probably
> > > > > suspend, or try until it is not EBUSY (==all threads have stopped?).
> > > > 
> > > > Right, if this fails, something has gone badly wrong.  You should
> > > > absolutely print a message, and in fact it might be appropriate to
> > > > quit outright.  IIUC the way PEF resets work, a failure here means you
> > > > won't be able to boot after the reset, since the guest memory will
> > > > still be inaccessible to the host.
> > > 
> > > Correct. I will send next version with a message and abort() added in
> > > the ioctl failure path.
> > 
> > abort() or assert() isn't right either - that's reserved for things
> > that are definitely caused by a qemu code bug.  This should be an
> > exit(EXIT_FAILURE).
> 
> Ok, but I see a problem with checking the return value of this
> ioctl from userspace. If this ioctl is run on older kernels that don't
> support this ioctl, we get -ENOTTY as return value. We shouldn't be
> exiting in that case.

Ah, right.  We'll need to check for -ENOTTY specifically and ignore
it, then.  We don't want this spewing warnings on every non-secure
guest.

> It looks like we may need a new KVM capability to advertise the presence
> of KVM_PPC_SVM_OFF ioctl (or more generally, to advertise host kernel's
> capability to support secure guests).

Actually, that's probably a better idea still.

> Paul - Do you think we should add such a KVM capability? Here is the
> summary of the problem:
> 
> 1. QEMU invokes KVM_PPC_SVM_OFF ioctl from machine reset path and currently
>    we don't check for its return value.
> 2. On host kernels that support secure guests,
>    2a. this ioctl returns 0 for regular guests and has no effect.
>    2b. the ioctl can fail for secure guests and here we could check the
>        return value and exit the guest right away.
> 3. On host kernels that don't support secure guests, ioctl returns -ENOTTY
>    but we ignore the return value and continue the reset as this is
>    for a non-secure guest.
> 
> If we have such a KVM capability, we could invoke the ioctl only if it
> is supported and handle the return value appropriately.
> 
> Regards,
> Bharata.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 ppc-for-5.0 2/2] ppc/spapr: Support reboot of secure pseries guest

[Bharata B Rao]

On Wed, Dec 11, 2019 at 04:27:42PM +1100, David Gibson wrote:
> Ah, right.  We'll need to check for -ENOTTY specifically and ignore
> it, then.  We don't want this spewing warnings on every non-secure
> guest.

I am posting v2 with explicit check for -ENOTTY.

> 
> > It looks like we may need a new KVM capability to advertise the presence
> > of KVM_PPC_SVM_OFF ioctl (or more generally, to advertise host kernel's
> > capability to support secure guests).
> 
> Actually, that's probably a better idea still.

If and when we decide to have this KVM capability and that goes upstream,
we can update the QEMU accordingly?

Regards,
Bharata.