On Thu, 24 Oct 2019 07:40:24 -0400
Janosch Frank <frankja@xxxxxxxxxxxxx> wrote:
> From: Vasily Gorbik <gor@xxxxxxxxxxxxx>
>
> Introduce KVM_S390_PROTECTED_VIRTUALIZATION_HOST kbuild option for
> protected virtual machines hosting support code.
>
> Add "prot_virt" command line option which controls if the kernel
> protected VMs support is enabled at runtime.
>
> Extend ultravisor info definitions and expose it via uv_info struct
> filled in during startup.
>
> Signed-off-by: Vasily Gorbik <gor@xxxxxxxxxxxxx>
> ---
> .../admin-guide/kernel-parameters.txt | 5 ++
> arch/s390/boot/Makefile | 2 +-
> arch/s390/boot/uv.c | 20 +++++++-
> arch/s390/include/asm/uv.h | 46 ++++++++++++++++--
> arch/s390/kernel/Makefile | 1 +
> arch/s390/kernel/setup.c | 4 --
> arch/s390/kernel/uv.c | 48 +++++++++++++++++++
> arch/s390/kvm/Kconfig | 9 ++++
> 8 files changed, 126 insertions(+), 9 deletions(-)
> create mode 100644 arch/s390/kernel/uv.c
(...)
> diff --git a/arch/s390/boot/uv.c b/arch/s390/boot/uv.c
> index ed007f4a6444..88cf8825d169 100644
> --- a/arch/s390/boot/uv.c
> +++ b/arch/s390/boot/uv.c
> @@ -3,7 +3,12 @@
> #include <asm/facility.h>
> #include <asm/sections.h>
>
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> int __bootdata_preserved(prot_virt_guest);
> +#endif
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> +struct uv_info __bootdata_preserved(uv_info);
> +#endif
Two functions with the same name, but different signatures look really
ugly.
Also, what happens if I want to build just a single kernel image for
both guest and host?
>
> void uv_query_info(void)
> {
> @@ -18,7 +23,20 @@ void uv_query_info(void)
> if (uv_call(0, (uint64_t)&uvcb))
> return;
>
> - if (test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
> + if (IS_ENABLED(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)) {
Do we always have everything needed for a host if uv_call() is
successful?
> + memcpy(uv_info.inst_calls_list, uvcb.inst_calls_list, sizeof(uv_info.inst_calls_list));
> + uv_info.uv_base_stor_len = uvcb.uv_base_stor_len;
> + uv_info.guest_base_stor_len = uvcb.conf_base_phys_stor_len;
> + uv_info.guest_virt_base_stor_len = uvcb.conf_base_virt_stor_len;
> + uv_info.guest_virt_var_stor_len = uvcb.conf_virt_var_stor_len;
> + uv_info.guest_cpu_stor_len = uvcb.cpu_stor_len;
> + uv_info.max_sec_stor_addr = ALIGN(uvcb.max_guest_stor_addr, PAGE_SIZE);
> + uv_info.max_num_sec_conf = uvcb.max_num_sec_conf;
> + uv_info.max_guest_cpus = uvcb.max_guest_cpus;
> + }
> +
> + if (IS_ENABLED(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) &&
> + test_bit_inv(BIT_UVC_CMD_SET_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list) &&
> test_bit_inv(BIT_UVC_CMD_REMOVE_SHARED_ACCESS, (unsigned long *)uvcb.inst_calls_list))
Especially as it looks like we need to test for those two commands to
determine whether we have support for a guest.
> prot_virt_guest = 1;
> }
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index ef3c00b049ab..6db1bc495e67 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -44,7 +44,19 @@ struct uv_cb_qui {
> struct uv_cb_header header;
> u64 reserved08;
> u64 inst_calls_list[4];
> - u64 reserved30[15];
> + u64 reserved30[2];
> + u64 uv_base_stor_len;
> + u64 reserved48;
> + u64 conf_base_phys_stor_len;
> + u64 conf_base_virt_stor_len;
> + u64 conf_virt_var_stor_len;
> + u64 cpu_stor_len;
> + u32 reserved68[3];
> + u32 max_num_sec_conf;
> + u64 max_guest_stor_addr;
> + u8 reserved80[150-128];
> + u16 max_guest_cpus;
> + u64 reserved98;
> } __packed __aligned(8);
>
> struct uv_cb_share {
> @@ -69,9 +81,21 @@ static inline int uv_call(unsigned long r1, unsigned long r2)
> return cc;
> }
>
> -#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> +struct uv_info {
> + unsigned long inst_calls_list[4];
> + unsigned long uv_base_stor_len;
> + unsigned long guest_base_stor_len;
> + unsigned long guest_virt_base_stor_len;
> + unsigned long guest_virt_var_stor_len;
> + unsigned long guest_cpu_stor_len;
> + unsigned long max_sec_stor_addr;
> + unsigned int max_num_sec_conf;
> + unsigned short max_guest_cpus;
> +};
What is the main difference between uv_info and uv_cb_qui? The
alignment of max_sec_stor_addr?
> +extern struct uv_info uv_info;
> extern int prot_virt_guest;
>
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> static inline int is_prot_virt_guest(void)
> {
> return prot_virt_guest;
> @@ -121,11 +145,27 @@ static inline int uv_remove_shared(unsigned long addr)
> return share(addr, UVC_CMD_REMOVE_SHARED_ACCESS);
> }
>
> -void uv_query_info(void);
> #else
> #define is_prot_virt_guest() 0
> static inline int uv_set_shared(unsigned long addr) { return 0; }
> static inline int uv_remove_shared(unsigned long addr) { return 0; }
> +#endif
> +
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> +extern int prot_virt_host;
> +
> +static inline int is_prot_virt_host(void)
> +{
> + return prot_virt_host;
> +}
> +#else
> +#define is_prot_virt_host() 0
> +#endif
> +
> +#if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) || \
> + defined(CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST)
> +void uv_query_info(void);
> +#else
> static inline void uv_query_info(void) {}
> #endif
(...)
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> new file mode 100644
> index 000000000000..35ce89695509
> --- /dev/null
> +++ b/arch/s390/kernel/uv.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Common Ultravisor functions and initialization
> + *
> + * Copyright IBM Corp. 2019
> + */
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/sizes.h>
> +#include <linux/bitmap.h>
> +#include <linux/memblock.h>
> +#include <asm/facility.h>
> +#include <asm/sections.h>
> +#include <asm/uv.h>
> +
> +#ifdef CONFIG_PROTECTED_VIRTUALIZATION_GUEST
> +int __bootdata_preserved(prot_virt_guest);
> +#endif
> +
> +#ifdef CONFIG_KVM_S390_PROTECTED_VIRTUALIZATION_HOST
> +int prot_virt_host;
> +EXPORT_SYMBOL(prot_virt_host);
> +struct uv_info __bootdata_preserved(uv_info);
> +EXPORT_SYMBOL(uv_info);
> +
> +static int __init prot_virt_setup(char *val)
> +{
> + bool enabled;
> + int rc;
> +
> + rc = kstrtobool(val, &enabled);
> + if (!rc && enabled)
> + prot_virt_host = 1;
> +
> + if (is_prot_virt_guest() && prot_virt_host) {
> + prot_virt_host = 0;
> + pr_info("Running as protected virtualization guest.");
> + }
> +
> + if (prot_virt_host && !test_facility(158)) {
Why not check for that facility earlier? If it is not present, we
cannot run as a prot virt guest, either.
> + prot_virt_host = 0;
> + pr_info("The ultravisor call facility is not available.");
> + }
> +
> + return rc;
> +}
> +early_param("prot_virt", prot_virt_setup);
Maybe rename this to prot_virt_host?
> +#endif
(...)
