* Re: [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests
2020-03-19 4:33 [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests Paul Mackerras
@ 2020-03-19 16:30 ` Greg Kurz
2020-03-19 17:44 ` Fabiano Rosas
2020-03-19 17:52 ` Cédric Le Goater
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Greg Kurz @ 2020-03-19 16:30 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, David Gibson, Ram Pai
On Thu, 19 Mar 2020 15:33:01 +1100
Paul Mackerras <paulus@ozlabs.org> wrote:
> At present, on Power systems with Protected Execution Facility
> hardware and an ultravisor, a KVM guest can transition to being a
> secure guest at will. Userspace (QEMU) has no way of knowing
> whether a host system is capable of running secure guests. This
> will present a problem in future when the ultravisor is capable of
> migrating secure guests from one host to another, because
> virtualization management software will have no way to ensure that
> secure guests only run in domains where all of the hosts can
> support secure guests.
>
> This adds a VM capability which has two functions: (a) userspace
> can query it to find out whether the host can support secure guests,
> and (b) userspace can enable it for a guest, which allows that
> guest to become a secure guest. If userspace does not enable it,
> KVM will return an error when the ultravisor does the hypercall
> that indicates that the guest is starting to transition to a
> secure guest. The ultravisor will then abort the transition and
> the guest will terminate.
>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
Is someone working on wiring this up in QEMU ?
> Note, only compile-tested. Ram, please test.
>
> Documentation/virt/kvm/api.rst | 17 +++++++++++++++++
> arch/powerpc/include/asm/kvm_host.h | 1 +
> arch/powerpc/include/asm/kvm_ppc.h | 1 +
> arch/powerpc/kvm/book3s_hv.c | 13 +++++++++++++
> arch/powerpc/kvm/book3s_hv_uvmem.c | 4 ++++
> arch/powerpc/kvm/powerpc.c | 13 +++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 50 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 158d118..a925500 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5779,6 +5779,23 @@ it hard or impossible to use it correctly. The availability of
> KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 signals that those bugs are fixed.
> Userspace should not try to use KVM_CAP_MANUAL_DIRTY_LOG_PROTECT.
>
> +7.19 KVM_CAP_PPC_SECURE_GUEST
> +------------------------------
> +
> +:Architectures: ppc
> +
> +This capability indicates that KVM is running on a host that has
> +ultravisor firmware and thus can support a secure guest. On such a
> +system, a guest can ask the ultravisor to make it a secure guest,
> +one whose memory is inaccessible to the host except for pages which
> +are explicitly requested to be shared with the host. The ultravisor
> +notifies KVM when a guest requests to become a secure guest, and KVM
> +has the opportunity to veto the transition.
> +
> +If present, this capability can be enabled for a VM, meaning that KVM
> +will allow the transition to secure guest mode. Otherwise KVM will
> +veto the transition.
> +
> 8. Other capabilities.
> ======================
>
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 6e8b8ff..f99b433 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -303,6 +303,7 @@ struct kvm_arch {
> u8 radix;
> u8 fwnmi_enabled;
> u8 secure_guest;
> + u8 svm_enabled;
> bool threads_indep;
> bool nested_enable;
> pgd_t *pgtable;
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 406ec46..0733618 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -316,6 +316,7 @@ struct kvmppc_ops {
> int size);
> int (*store_to_eaddr)(struct kvm_vcpu *vcpu, ulong *eaddr, void *ptr,
> int size);
> + int (*enable_svm)(struct kvm *kvm);
> int (*svm_off)(struct kvm *kvm);
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index fbc55a1..36da720 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -5423,6 +5423,18 @@ static void unpin_vpa_reset(struct kvm *kvm, struct kvmppc_vpa *vpa)
> }
>
> /*
> + * Enable a guest to become a secure VM.
> + * Called when the KVM_CAP_PPC_SECURE_GUEST capability is enabled.
> + */
> +static int kvmhv_enable_svm(struct kvm *kvm)
> +{
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + return -EINVAL;
> + kvm->arch.svm_enabled = 1;
> + return 0;
> +}
> +
> +/*
> * IOCTL handler to turn off secure mode of guest
> *
> * - Release all device pages
> @@ -5543,6 +5555,7 @@ static struct kvmppc_ops kvm_ops_hv = {
> .enable_nested = kvmhv_enable_nested,
> .load_from_eaddr = kvmhv_load_from_eaddr,
> .store_to_eaddr = kvmhv_store_to_eaddr,
> + .enable_svm = kvmhv_enable_svm,
> .svm_off = kvmhv_svm_off,
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 79b1202..2ad999f 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -216,6 +216,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> if (!kvm_is_radix(kvm))
> return H_UNSUPPORTED;
>
> + /* NAK the transition to secure if not enabled */
> + if (!kvm->arch.svm_enabled)
> + return H_AUTHORITY;
> +
> srcu_idx = srcu_read_lock(&kvm->srcu);
> slots = kvm_memslots(kvm);
> kvm_for_each_memslot(memslot, slots) {
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 62ee66d..c32e6cc2 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -670,6 +670,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> (hv_enabled && cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST));
> break;
> #endif
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> + case KVM_CAP_PPC_SECURE_GUEST:
> + r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR);
> + break;
> +#endif
> default:
> r = 0;
> break;
> @@ -2170,6 +2175,14 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> r = kvm->arch.kvm_ops->enable_nested(kvm);
> break;
> #endif
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> + case KVM_CAP_PPC_SECURE_GUEST:
> + r = -EINVAL;
> + if (!is_kvmppc_hv_enabled(kvm) || !kvm->arch.kvm_ops->enable_svm)
> + break;
> + r = kvm->arch.kvm_ops->enable_svm(kvm);
> + break;
> +#endif
> default:
> r = -EINVAL;
> break;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 5e6234c..428c7dd 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1016,6 +1016,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_INJECT_EXT_DABT 178
> #define KVM_CAP_S390_VCPU_RESETS 179
> #define KVM_CAP_S390_PROTECTED 180
> +#define KVM_CAP_PPC_SECURE_GUEST 181
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests
2020-03-19 16:30 ` Greg Kurz
@ 2020-03-19 17:44 ` Fabiano Rosas
0 siblings, 0 replies; 8+ messages in thread
From: Fabiano Rosas @ 2020-03-19 17:44 UTC (permalink / raw)
To: Greg Kurz, Paul Mackerras; +Cc: kvm, kvm-ppc, David Gibson, Ram Pai
Greg Kurz <groug@kaod.org> writes:
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> Is someone working on wiring this up in QEMU ?
>
I just did so I could test it. =)
If no one else is doing it I could send my patch as an RFC and we
iterate from that.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests
2020-03-19 4:33 [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests Paul Mackerras
2020-03-19 16:30 ` Greg Kurz
@ 2020-03-19 17:52 ` Cédric Le Goater
2020-03-19 19:41 ` Ram Pai
2020-03-23 3:18 ` David Gibson
3 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2020-03-19 17:52 UTC (permalink / raw)
To: Paul Mackerras, kvm; +Cc: kvm-ppc, David Gibson, Ram Pai
On 3/19/20 5:33 AM, Paul Mackerras wrote:
> At present, on Power systems with Protected Execution Facility
> hardware and an ultravisor, a KVM guest can transition to being a
> secure guest at will. Userspace (QEMU) has no way of knowing
> whether a host system is capable of running secure guests. This
> will present a problem in future when the ultravisor is capable of
> migrating secure guests from one host to another, because
> virtualization management software will have no way to ensure that
> secure guests only run in domains where all of the hosts can
> support secure guests.
>
> This adds a VM capability which has two functions: (a) userspace
> can query it to find out whether the host can support secure guests,
> and (b) userspace can enable it for a guest, which allows that
> guest to become a secure guest. If userspace does not enable it,
> KVM will return an error when the ultravisor does the hypercall
> that indicates that the guest is starting to transition to a
> secure guest. The ultravisor will then abort the transition and
> the guest will terminate.
>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> ---
> Note, only compile-tested. Ram, please test.
>
> Documentation/virt/kvm/api.rst | 17 +++++++++++++++++
> arch/powerpc/include/asm/kvm_host.h | 1 +
> arch/powerpc/include/asm/kvm_ppc.h | 1 +
> arch/powerpc/kvm/book3s_hv.c | 13 +++++++++++++
> arch/powerpc/kvm/book3s_hv_uvmem.c | 4 ++++
> arch/powerpc/kvm/powerpc.c | 13 +++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 50 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 158d118..a925500 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5779,6 +5779,23 @@ it hard or impossible to use it correctly. The availability of
> KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 signals that those bugs are fixed.
> Userspace should not try to use KVM_CAP_MANUAL_DIRTY_LOG_PROTECT.
>
> +7.19 KVM_CAP_PPC_SECURE_GUEST
> +------------------------------
> +
> +:Architectures: ppc
> +
> +This capability indicates that KVM is running on a host that has
> +ultravisor firmware and thus can support a secure guest. On such a
> +system, a guest can ask the ultravisor to make it a secure guest,
> +one whose memory is inaccessible to the host except for pages which
> +are explicitly requested to be shared with the host. The ultravisor
> +notifies KVM when a guest requests to become a secure guest, and KVM
> +has the opportunity to veto the transition.
> +
> +If present, this capability can be enabled for a VM, meaning that KVM
> +will allow the transition to secure guest mode. Otherwise KVM will
> +veto the transition.
> +
> 8. Other capabilities.
> ======================
>
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 6e8b8ff..f99b433 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -303,6 +303,7 @@ struct kvm_arch {
> u8 radix;
> u8 fwnmi_enabled;
> u8 secure_guest;
> + u8 svm_enabled;
> bool threads_indep;
> bool nested_enable;
> pgd_t *pgtable;
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 406ec46..0733618 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -316,6 +316,7 @@ struct kvmppc_ops {
> int size);
> int (*store_to_eaddr)(struct kvm_vcpu *vcpu, ulong *eaddr, void *ptr,
> int size);
> + int (*enable_svm)(struct kvm *kvm);
> int (*svm_off)(struct kvm *kvm);
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index fbc55a1..36da720 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -5423,6 +5423,18 @@ static void unpin_vpa_reset(struct kvm *kvm, struct kvmppc_vpa *vpa)
> }
>
> /*
> + * Enable a guest to become a secure VM.
> + * Called when the KVM_CAP_PPC_SECURE_GUEST capability is enabled.
> + */
> +static int kvmhv_enable_svm(struct kvm *kvm)
> +{
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + return -EINVAL;
> + kvm->arch.svm_enabled = 1;
> + return 0;
> +}
> +
> +/*
> * IOCTL handler to turn off secure mode of guest
> *
> * - Release all device pages
> @@ -5543,6 +5555,7 @@ static struct kvmppc_ops kvm_ops_hv = {
> .enable_nested = kvmhv_enable_nested,
> .load_from_eaddr = kvmhv_load_from_eaddr,
> .store_to_eaddr = kvmhv_store_to_eaddr,
> + .enable_svm = kvmhv_enable_svm,
> .svm_off = kvmhv_svm_off,
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 79b1202..2ad999f 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -216,6 +216,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> if (!kvm_is_radix(kvm))
> return H_UNSUPPORTED;
>
> + /* NAK the transition to secure if not enabled */
> + if (!kvm->arch.svm_enabled)
> + return H_AUTHORITY;
> +
> srcu_idx = srcu_read_lock(&kvm->srcu);
> slots = kvm_memslots(kvm);
> kvm_for_each_memslot(memslot, slots) {
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 62ee66d..c32e6cc2 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -670,6 +670,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> (hv_enabled && cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST));
> break;
> #endif
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> + case KVM_CAP_PPC_SECURE_GUEST:
> + r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR);
> + break;
> +#endif
> default:
> r = 0;
> break;
> @@ -2170,6 +2175,14 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> r = kvm->arch.kvm_ops->enable_nested(kvm);
> break;
> #endif
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> + case KVM_CAP_PPC_SECURE_GUEST:
> + r = -EINVAL;
> + if (!is_kvmppc_hv_enabled(kvm) || !kvm->arch.kvm_ops->enable_svm)
> + break;
> + r = kvm->arch.kvm_ops->enable_svm(kvm);
> + break;
> +#endif
> default:
> r = -EINVAL;
> break;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 5e6234c..428c7dd 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1016,6 +1016,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_INJECT_EXT_DABT 178
> #define KVM_CAP_S390_VCPU_RESETS 179
> #define KVM_CAP_S390_PROTECTED 180
> +#define KVM_CAP_PPC_SECURE_GUEST 181
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests
2020-03-19 4:33 [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests Paul Mackerras
2020-03-19 16:30 ` Greg Kurz
2020-03-19 17:52 ` Cédric Le Goater
@ 2020-03-19 19:41 ` Ram Pai
2020-03-19 23:17 ` Paul Mackerras
2020-03-23 3:18 ` David Gibson
3 siblings, 1 reply; 8+ messages in thread
From: Ram Pai @ 2020-03-19 19:41 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, David Gibson
On Thu, Mar 19, 2020 at 03:33:01PM +1100, Paul Mackerras wrote:
> At present, on Power systems with Protected Execution Facility
> hardware and an ultravisor, a KVM guest can transition to being a
> secure guest at will. Userspace (QEMU) has no way of knowing
> whether a host system is capable of running secure guests. This
> will present a problem in future when the ultravisor is capable of
> migrating secure guests from one host to another, because
> virtualization management software will have no way to ensure that
> secure guests only run in domains where all of the hosts can
> support secure guests.
>
> This adds a VM capability which has two functions: (a) userspace
> can query it to find out whether the host can support secure guests,
> and (b) userspace can enable it for a guest, which allows that
> guest to become a secure guest. If userspace does not enable it,
> KVM will return an error when the ultravisor does the hypercall
> that indicates that the guest is starting to transition to a
> secure guest. The ultravisor will then abort the transition and
> the guest will terminate.
>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> Note, only compile-tested. Ram, please test.
>
> Documentation/virt/kvm/api.rst | 17 +++++++++++++++++
> arch/powerpc/include/asm/kvm_host.h | 1 +
> arch/powerpc/include/asm/kvm_ppc.h | 1 +
> arch/powerpc/kvm/book3s_hv.c | 13 +++++++++++++
> arch/powerpc/kvm/book3s_hv_uvmem.c | 4 ++++
> arch/powerpc/kvm/powerpc.c | 13 +++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 50 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 158d118..a925500 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5779,6 +5779,23 @@ it hard or impossible to use it correctly. The availability of
> KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 signals that those bugs are fixed.
> Userspace should not try to use KVM_CAP_MANUAL_DIRTY_LOG_PROTECT.
>
> +7.19 KVM_CAP_PPC_SECURE_GUEST
> +------------------------------
> +
> +:Architectures: ppc
> +
> +This capability indicates that KVM is running on a host that has
> +ultravisor firmware and thus can support a secure guest. On such a
> +system, a guest can ask the ultravisor to make it a secure guest,
> +one whose memory is inaccessible to the host except for pages which
> +are explicitly requested to be shared with the host. The ultravisor
> +notifies KVM when a guest requests to become a secure guest, and KVM
> +has the opportunity to veto the transition.
> +
> +If present, this capability can be enabled for a VM, meaning that KVM
> +will allow the transition to secure guest mode. Otherwise KVM will
> +veto the transition.
> +
> 8. Other capabilities.
> ======================
>
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 6e8b8ff..f99b433 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -303,6 +303,7 @@ struct kvm_arch {
> u8 radix;
> u8 fwnmi_enabled;
> u8 secure_guest;
> + u8 svm_enabled;
> bool threads_indep;
> bool nested_enable;
> pgd_t *pgtable;
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 406ec46..0733618 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -316,6 +316,7 @@ struct kvmppc_ops {
> int size);
> int (*store_to_eaddr)(struct kvm_vcpu *vcpu, ulong *eaddr, void *ptr,
> int size);
> + int (*enable_svm)(struct kvm *kvm);
> int (*svm_off)(struct kvm *kvm);
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index fbc55a1..36da720 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -5423,6 +5423,18 @@ static void unpin_vpa_reset(struct kvm *kvm, struct kvmppc_vpa *vpa)
> }
>
> /*
> + * Enable a guest to become a secure VM.
> + * Called when the KVM_CAP_PPC_SECURE_GUEST capability is enabled.
> + */
> +static int kvmhv_enable_svm(struct kvm *kvm)
> +{
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + return -EINVAL;
> + kvm->arch.svm_enabled = 1;
> + return 0;
> +}
> +
> +/*
> * IOCTL handler to turn off secure mode of guest
> *
> * - Release all device pages
> @@ -5543,6 +5555,7 @@ static struct kvmppc_ops kvm_ops_hv = {
> .enable_nested = kvmhv_enable_nested,
> .load_from_eaddr = kvmhv_load_from_eaddr,
> .store_to_eaddr = kvmhv_store_to_eaddr,
> + .enable_svm = kvmhv_enable_svm,
> .svm_off = kvmhv_svm_off,
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 79b1202..2ad999f 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -216,6 +216,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> if (!kvm_is_radix(kvm))
> return H_UNSUPPORTED;
>
> + /* NAK the transition to secure if not enabled */
> + if (!kvm->arch.svm_enabled)
> + return H_AUTHORITY;
> +
> srcu_idx = srcu_read_lock(&kvm->srcu);
> slots = kvm_memslots(kvm);
> kvm_for_each_memslot(memslot, slots) {
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 62ee66d..c32e6cc2 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -670,6 +670,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> (hv_enabled && cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST));
> break;
> #endif
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> + case KVM_CAP_PPC_SECURE_GUEST:
> + r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR);
We also need to check if the kvmppc_uvmem_init() has been successfully
called and initialized.
r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR)
&& kvmppc_uvmem_bitmap;
RP
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests
2020-03-19 19:41 ` Ram Pai
@ 2020-03-19 23:17 ` Paul Mackerras
2020-03-20 1:20 ` Ram Pai
0 siblings, 1 reply; 8+ messages in thread
From: Paul Mackerras @ 2020-03-19 23:17 UTC (permalink / raw)
To: Ram Pai; +Cc: kvm, kvm-ppc, David Gibson
On Thu, Mar 19, 2020 at 12:41:08PM -0700, Ram Pai wrote:
> On Thu, Mar 19, 2020 at 03:33:01PM +1100, Paul Mackerras wrote:
[snip]
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -670,6 +670,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > (hv_enabled && cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST));
> > break;
> > #endif
> > +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> > + case KVM_CAP_PPC_SECURE_GUEST:
> > + r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR);
>
> We also need to check if the kvmppc_uvmem_init() has been successfully
> called and initialized.
>
> r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR)
> && kvmppc_uvmem_bitmap;
Well I can't do that exactly because kvmppc_uvmem_bitmap is in a
different module (the kvm_hv module, whereas this code is in the kvm
module), and I wouldn't want to depend on kvmppc_uvmem_bitmap, since
that's an internal implementation detail.
The firmware_has_feature(FW_FEATURE_ULTRAVISOR) test ultimately
depends on there being a device tree node with "ibm,ultravisor" in its
compatible property (see early_init_dt_scan_ultravisor()). So that
means there is an ultravisor there. The cases where that test would
pass but kvmppc_uvmem_bitmap == NULL would be those where the device
tree nodes are present but not right, or where the host is so short of
memory that it couldn't allocate the kvmppc_uvmem_bitmap. If you
think those cases are worth worrying about then I will have to devise
a way to do the test without depending on any symbols from the kvm-hv
module.
Paul.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests
2020-03-19 23:17 ` Paul Mackerras
@ 2020-03-20 1:20 ` Ram Pai
0 siblings, 0 replies; 8+ messages in thread
From: Ram Pai @ 2020-03-20 1:20 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, David Gibson
On Fri, Mar 20, 2020 at 10:17:13AM +1100, Paul Mackerras wrote:
> On Thu, Mar 19, 2020 at 12:41:08PM -0700, Ram Pai wrote:
> > On Thu, Mar 19, 2020 at 03:33:01PM +1100, Paul Mackerras wrote:
> [snip]
> > > --- a/arch/powerpc/kvm/powerpc.c
> > > +++ b/arch/powerpc/kvm/powerpc.c
> > > @@ -670,6 +670,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > (hv_enabled && cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST));
> > > break;
> > > #endif
> > > +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> > > + case KVM_CAP_PPC_SECURE_GUEST:
> > > + r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR);
> >
> > We also need to check if the kvmppc_uvmem_init() has been successfully
> > called and initialized.
> >
> > r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR)
> > && kvmppc_uvmem_bitmap;
>
> Well I can't do that exactly because kvmppc_uvmem_bitmap is in a
> different module (the kvm_hv module, whereas this code is in the kvm
> module), and I wouldn't want to depend on kvmppc_uvmem_bitmap, since
> that's an internal implementation detail.
yes. checking for kvmppc_uvmem_bitmap depends on internal implementation
detail. Its also a loose approximation. There has to be something
better which can tell, if everything needed to support secure guests, is
available and initialized.
>
> The firmware_has_feature(FW_FEATURE_ULTRAVISOR) test ultimately
> depends on there being a device tree node with "ibm,ultravisor" in its
> compatible property (see early_init_dt_scan_ultravisor()). So that
> means there is an ultravisor there. The cases where that test would
> pass but kvmppc_uvmem_bitmap == NULL would be those where the device
> tree nodes are present but not right, or where the host is so short of
> memory that it couldn't allocate the kvmppc_uvmem_bitmap. If you
> think those cases are worth worrying about then I will have to devise
> a way to do the test without depending on any symbols from the kvm-hv
> module.
the cases, where incorrect behavior can happen; if we do not have this additional
check, are --
a) zero secure memory in the system.
b) "kvmppc_uvmem" memory region is not defined.
c) the memory region fails to map.
d) kvmppc_uvmem_bitmap allocation failed.
All these are possible to varying level of certainity.
I do not know we should be concerned about these possibilities.
But if we do, than will a patch like this help? compile tested.
------------------
diff --git a/arch/powerpc/include/asm/kvm_book3s_uvmem.h b/arch/powerpc/include/asm/kvm_book3s_uvmem.h
index 5a9834e..643c497 100644
--- a/arch/powerpc/include/asm/kvm_book3s_uvmem.h
+++ b/arch/powerpc/include/asm/kvm_book3s_uvmem.h
@@ -4,6 +4,7 @@
#ifdef CONFIG_PPC_UV
int kvmppc_uvmem_init(void);
+int kvmppc_uv_enabled(void);
void kvmppc_uvmem_free(void);
int kvmppc_uvmem_slot_init(struct kvm *kvm, const struct kvm_memory_slot *slot);
void kvmppc_uvmem_slot_free(struct kvm *kvm,
@@ -28,6 +29,11 @@ static inline int kvmppc_uvmem_init(void)
return 0;
}
+static inline int kvmppc_uv_enabled(void)
+{
+ return 0;
+}
+
static inline void kvmppc_uvmem_free(void) { }
static inline int
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 79b1202..3331ac5 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -804,6 +804,11 @@ int kvmppc_uvmem_init(void)
return ret;
}
+int kvmppc_uv_enabled(void)
+{
+ return !kvmppc_uvmem_bitmap;
+}
+
void kvmppc_uvmem_free(void)
{
memunmap_pages(&kvmppc_uvmem_pgmap);
------------------
>
> Paul.
--
Ram Pai
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests
2020-03-19 4:33 [PATCH] KVM: PPC: Book3S HV: Add a capability for enabling secure guests Paul Mackerras
` (2 preceding siblings ...)
2020-03-19 19:41 ` Ram Pai
@ 2020-03-23 3:18 ` David Gibson
3 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2020-03-23 3:18 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm, kvm-ppc, Ram Pai
[-- Attachment #1: Type: text/plain, Size: 7128 bytes --]
On Thu, Mar 19, 2020 at 03:33:01PM +1100, Paul Mackerras wrote:
> At present, on Power systems with Protected Execution Facility
> hardware and an ultravisor, a KVM guest can transition to being a
> secure guest at will. Userspace (QEMU) has no way of knowing
> whether a host system is capable of running secure guests. This
> will present a problem in future when the ultravisor is capable of
> migrating secure guests from one host to another, because
> virtualization management software will have no way to ensure that
> secure guests only run in domains where all of the hosts can
> support secure guests.
>
> This adds a VM capability which has two functions: (a) userspace
> can query it to find out whether the host can support secure guests,
> and (b) userspace can enable it for a guest, which allows that
> guest to become a secure guest. If userspace does not enable it,
> KVM will return an error when the ultravisor does the hypercall
> that indicates that the guest is starting to transition to a
> secure guest. The ultravisor will then abort the transition and
> the guest will terminate.
>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> Note, only compile-tested. Ram, please test.
>
> Documentation/virt/kvm/api.rst | 17 +++++++++++++++++
> arch/powerpc/include/asm/kvm_host.h | 1 +
> arch/powerpc/include/asm/kvm_ppc.h | 1 +
> arch/powerpc/kvm/book3s_hv.c | 13 +++++++++++++
> arch/powerpc/kvm/book3s_hv_uvmem.c | 4 ++++
> arch/powerpc/kvm/powerpc.c | 13 +++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 7 files changed, 50 insertions(+)
>
> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
> index 158d118..a925500 100644
> --- a/Documentation/virt/kvm/api.rst
> +++ b/Documentation/virt/kvm/api.rst
> @@ -5779,6 +5779,23 @@ it hard or impossible to use it correctly. The availability of
> KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 signals that those bugs are fixed.
> Userspace should not try to use KVM_CAP_MANUAL_DIRTY_LOG_PROTECT.
>
> +7.19 KVM_CAP_PPC_SECURE_GUEST
> +------------------------------
> +
> +:Architectures: ppc
> +
> +This capability indicates that KVM is running on a host that has
> +ultravisor firmware and thus can support a secure guest. On such a
> +system, a guest can ask the ultravisor to make it a secure guest,
> +one whose memory is inaccessible to the host except for pages which
> +are explicitly requested to be shared with the host. The ultravisor
> +notifies KVM when a guest requests to become a secure guest, and KVM
> +has the opportunity to veto the transition.
> +
> +If present, this capability can be enabled for a VM, meaning that KVM
> +will allow the transition to secure guest mode. Otherwise KVM will
> +veto the transition.
> +
> 8. Other capabilities.
> ======================
>
> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
> index 6e8b8ff..f99b433 100644
> --- a/arch/powerpc/include/asm/kvm_host.h
> +++ b/arch/powerpc/include/asm/kvm_host.h
> @@ -303,6 +303,7 @@ struct kvm_arch {
> u8 radix;
> u8 fwnmi_enabled;
> u8 secure_guest;
> + u8 svm_enabled;
> bool threads_indep;
> bool nested_enable;
> pgd_t *pgtable;
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 406ec46..0733618 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -316,6 +316,7 @@ struct kvmppc_ops {
> int size);
> int (*store_to_eaddr)(struct kvm_vcpu *vcpu, ulong *eaddr, void *ptr,
> int size);
> + int (*enable_svm)(struct kvm *kvm);
> int (*svm_off)(struct kvm *kvm);
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index fbc55a1..36da720 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -5423,6 +5423,18 @@ static void unpin_vpa_reset(struct kvm *kvm, struct kvmppc_vpa *vpa)
> }
>
> /*
> + * Enable a guest to become a secure VM.
> + * Called when the KVM_CAP_PPC_SECURE_GUEST capability is enabled.
> + */
> +static int kvmhv_enable_svm(struct kvm *kvm)
> +{
> + if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> + return -EINVAL;
> + kvm->arch.svm_enabled = 1;
> + return 0;
> +}
> +
> +/*
> * IOCTL handler to turn off secure mode of guest
> *
> * - Release all device pages
> @@ -5543,6 +5555,7 @@ static struct kvmppc_ops kvm_ops_hv = {
> .enable_nested = kvmhv_enable_nested,
> .load_from_eaddr = kvmhv_load_from_eaddr,
> .store_to_eaddr = kvmhv_store_to_eaddr,
> + .enable_svm = kvmhv_enable_svm,
> .svm_off = kvmhv_svm_off,
> };
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 79b1202..2ad999f 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -216,6 +216,10 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
> if (!kvm_is_radix(kvm))
> return H_UNSUPPORTED;
>
> + /* NAK the transition to secure if not enabled */
> + if (!kvm->arch.svm_enabled)
> + return H_AUTHORITY;
> +
> srcu_idx = srcu_read_lock(&kvm->srcu);
> slots = kvm_memslots(kvm);
> kvm_for_each_memslot(memslot, slots) {
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 62ee66d..c32e6cc2 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -670,6 +670,11 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> (hv_enabled && cpu_has_feature(CPU_FTR_P9_TM_HV_ASSIST));
> break;
> #endif
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> + case KVM_CAP_PPC_SECURE_GUEST:
> + r = hv_enabled && !!firmware_has_feature(FW_FEATURE_ULTRAVISOR);
> + break;
> +#endif
> default:
> r = 0;
> break;
> @@ -2170,6 +2175,14 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> r = kvm->arch.kvm_ops->enable_nested(kvm);
> break;
> #endif
> +#if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_UV)
> + case KVM_CAP_PPC_SECURE_GUEST:
> + r = -EINVAL;
> + if (!is_kvmppc_hv_enabled(kvm) || !kvm->arch.kvm_ops->enable_svm)
> + break;
> + r = kvm->arch.kvm_ops->enable_svm(kvm);
> + break;
> +#endif
> default:
> r = -EINVAL;
> break;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 5e6234c..428c7dd 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1016,6 +1016,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_INJECT_EXT_DABT 178
> #define KVM_CAP_S390_VCPU_RESETS 179
> #define KVM_CAP_S390_PROTECTED 180
> +#define KVM_CAP_PPC_SECURE_GUEST 181
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
--
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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread