linux-kernel - Re: [PATCH v5 02/22] KVM: arm64: Add SDEI virtualization infrastructure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YjpRArSezR3gVv2K@google.com>
Date:   Tue, 22 Mar 2022 22:43:14 +0000
From:   Oliver Upton <oupton@...gle.com>
To:     Gavin Shan <gshan@...hat.com>
Cc:     kvmarm@...ts.cs.columbia.edu, maz@...nel.org,
        linux-kernel@...r.kernel.org, eauger@...hat.com,
        shan.gavin@...il.com, Jonathan.Cameron@...wei.com,
        pbonzini@...hat.com, vkuznets@...hat.com, will@...nel.org
Subject: Re: [PATCH v5 02/22] KVM: arm64: Add SDEI virtualization
 infrastructure

Hi Gavin,

On Tue, Mar 22, 2022 at 04:06:50PM +0800, Gavin Shan wrote:
> Software Delegated Exception Interface (SDEI) provides a mechanism for
> registering and servicing system events. Those system events are high
> priority events, which must be serviced immediately. It's going to be
> used by Asynchronous Page Fault (APF) to deliver notification from KVM
> to guest. It's noted that SDEI is defined by ARM DEN0054C specification.

I'm guessing that you're using linked lists for stitching all of this
together because the specification provides for 24 bits of event
encoding. However, it seems that there will be a finite number of events
in KVM. So the APF stuff and a software signaled event.

Given that the number of events in KVM is rather small, would it make
more sense to do away with the overhead of linked lists and having the
state just represented in a static or allocated array? I think you can
cram all of the VM scoped event state into a single structure and hang
the implementation off of that.

> This introduces SDEI virtualization infrastructure where the SDEI events
> are registered and manipulated by the guest through hypercall. The SDEI
> event is delivered to one specific vCPU by KVM once it's raised. This
> introduces data structures to represent the needed objects to support
> the feature, which is highlighted as below. As those objects could be
> migrated between VMs, these data structures are partially exposed to
> user space.
> 
>    * kvm_sdei_exposed_event
>      The exposed events are determined and added by VMM through ioctl
>      interface. Only the exposed events can be registered from the
>      guest.
> 
>    * kvm_sdei_registered_event
>      The events that have been registered from the guest through the
>      SDEI_1_0_FN_SDEI_EVENT_REGISTER hypercall.
> 
>    * kvm_sdei_vcpu_event
>      The events that have been delivered to the target vCPU.
> 
>    * kvm_sdei_vcpu
>      Used to save the preempted context when the SDEI event is serviced
>      and delivered. After the SDEI event handling is completed, the
>      execution is resumed from the preempted context.
> 
>    * kvm_sdei_kvm
>      Place holder for the exposed and registered events.

It might be a good idea to expand these a bit and move them into
comments on each of the structures.

> The error of SDEI_NOT_SUPPORTED is returned for all SDEI hypercalls for
> now. They will be supported in the subsequent patches.
> 
> Signed-off-by: Gavin Shan <gshan@...hat.com>
> ---
>  arch/arm64/include/asm/kvm_host.h            |   3 +
>  arch/arm64/include/asm/kvm_sdei.h            | 171 +++++++++++++
>  arch/arm64/include/uapi/asm/kvm.h            |   1 +
>  arch/arm64/include/uapi/asm/kvm_sdei_state.h |  72 ++++++
>  arch/arm64/kvm/Makefile                      |   2 +-
>  arch/arm64/kvm/arm.c                         |   8 +
>  arch/arm64/kvm/hypercalls.c                  |  21 ++
>  arch/arm64/kvm/sdei.c                        | 244 +++++++++++++++++++
>  include/uapi/linux/arm_sdei.h                |   2 +
>  9 files changed, 523 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/include/asm/kvm_sdei.h
>  create mode 100644 arch/arm64/include/uapi/asm/kvm_sdei_state.h
>  create mode 100644 arch/arm64/kvm/sdei.c
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 031e3a2537fc..5d37e046a458 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -113,6 +113,8 @@ struct kvm_arch {
>  	/* Interrupt controller */
>  	struct vgic_dist	vgic;
>  
> +	struct kvm_sdei_kvm *sdei;
> +

nit: avoid repeating 'kvm'. struct kvm_sdei should be descriptive enough
:)

>  	/* Mandated version of PSCI */
>  	u32 psci_version;
>  
> @@ -338,6 +340,7 @@ struct kvm_vcpu_arch {
>  	 * Anything that is not used directly from assembly code goes
>  	 * here.
>  	 */
> +	struct kvm_sdei_vcpu *sdei;
>

nit: put your scoping tokens at the beginning of a symbol name, so
'struct kvm_vcpu_sdei'.

[...]

> diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
> index 202b8c455724..3c20fee72bb4 100644
> --- a/arch/arm64/kvm/hypercalls.c
> +++ b/arch/arm64/kvm/hypercalls.c
> @@ -5,6 +5,7 @@
>  #include <linux/kvm_host.h>
>  
>  #include <asm/kvm_emulate.h>
> +#include <asm/kvm_sdei.h>
>  
>  #include <kvm/arm_hypercalls.h>
>  #include <kvm/arm_psci.h>
> @@ -151,6 +152,26 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
>  	case ARM_SMCCC_TRNG_RND32:
>  	case ARM_SMCCC_TRNG_RND64:
>  		return kvm_trng_call(vcpu);
> +	case SDEI_1_0_FN_SDEI_VERSION:
> +	case SDEI_1_0_FN_SDEI_EVENT_REGISTER:
> +	case SDEI_1_0_FN_SDEI_EVENT_ENABLE:
> +	case SDEI_1_0_FN_SDEI_EVENT_DISABLE:
> +	case SDEI_1_0_FN_SDEI_EVENT_CONTEXT:
> +	case SDEI_1_0_FN_SDEI_EVENT_COMPLETE:
> +	case SDEI_1_0_FN_SDEI_EVENT_COMPLETE_AND_RESUME:
> +	case SDEI_1_0_FN_SDEI_EVENT_UNREGISTER:
> +	case SDEI_1_0_FN_SDEI_EVENT_STATUS:
> +	case SDEI_1_0_FN_SDEI_EVENT_GET_INFO:
> +	case SDEI_1_0_FN_SDEI_EVENT_ROUTING_SET:
> +	case SDEI_1_0_FN_SDEI_PE_MASK:
> +	case SDEI_1_0_FN_SDEI_PE_UNMASK:
> +	case SDEI_1_0_FN_SDEI_INTERRUPT_BIND:
> +	case SDEI_1_0_FN_SDEI_INTERRUPT_RELEASE:
> +	case SDEI_1_1_FN_SDEI_EVENT_SIGNAL:
> +	case SDEI_1_0_FN_SDEI_PRIVATE_RESET:
> +	case SDEI_1_0_FN_SDEI_SHARED_RESET:
> +	case SDEI_1_1_FN_SDEI_FEATURES:

Consider only adding switch statements for hypercalls when they're
actually implemented.

Additionally, while this isn't directly related to your patch, I do have
a gripe about kvm_hvc_call_handler(). It is really ugly that we
enumerate the specific hypercalls we support, and otherwise fall through
to PSCI.

IMO, a cleaner approach would be to have kvm_hvc_call_handler() simply
route a particular service range/service owner to the appropriate
handler. We can then terminate individual hypercalls in those handlers,
avoiding a catch-all switch such as this one is currently.

--
Thanks,
Oliver