Hi Gavin,
More comments, didn't see exactly how all of these structures are
getting used.
On Tue, Mar 22, 2022 at 04:06:50PM +0800, Gavin Shan wrote:
[...]
> diff --git a/arch/arm64/include/uapi/asm/kvm_sdei_state.h b/arch/arm64/include/uapi/asm/kvm_sdei_state.h
> new file mode 100644
> index 000000000000..b14844230117
> --- /dev/null
> +++ b/arch/arm64/include/uapi/asm/kvm_sdei_state.h
> @@ -0,0 +1,72 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Definitions of various KVM SDEI event states.
> + *
> + * Copyright (C) 2022 Red Hat, Inc.
> + *
> + * Author(s): Gavin Shan <gshan@xxxxxxxxxx>
> + */
> +
> +#ifndef _UAPI__ASM_KVM_SDEI_STATE_H
> +#define _UAPI__ASM_KVM_SDEI_STATE_H
> +
> +#ifndef __ASSEMBLY__
> +#include <linux/types.h>
> +
> +/*
> + * The software signaled event is the default one, which is
> + * defined in v1.1 specification.
> + */
> +#define KVM_SDEI_INVALID_EVENT 0xFFFFFFFF
Isn't the constraint that bit 31 must be zero? (DEN 0054C 4.4 "Event
number allocation")
> +#define KVM_SDEI_DEFAULT_EVENT 0
> +
> +#define KVM_SDEI_MAX_VCPUS 512 /* Aligned to 64 */
> +#define KVM_SDEI_MAX_EVENTS 128
I would *strongly* recommend against having these limits. I find the
vCPU limit especially concerning, because we're making KVM_MAX_VCPUS
ABI, which it definitely is not. Anything that deals with a vCPU should
be accessed through a vCPU FD (and thus agnostic to the maximum number
of vCPUs) to avoid such a complication.
> +struct kvm_sdei_exposed_event_state {
> + __u64 num;
> +
> + __u8 type;
> + __u8 signaled;
> + __u8 priority;
> + __u8 padding[5];
> + __u64 notifier;
Wait, isn't this a kernel function pointer!?
> +};
> +
> +struct kvm_sdei_registered_event_state {
You should fold these fields together with kvm_sdei_exposed_event_state
into a single 'kvm_sdei_event' structure:
> + __u64 num;
> +
> + __u8 route_mode;
> + __u8 padding[3];
> + __u64 route_affinity;
And these shouldn't be UAPI at the VM scope. Each of these properties
could be accessed via a synthetic/'pseudo-firmware' register on a vCPU FD:
> + __u64 ep_address[KVM_SDEI_MAX_VCPUS];
> + __u64 ep_arg[KVM_SDEI_MAX_VCPUS];
> + __u64 registered[KVM_SDEI_MAX_VCPUS/64];
> + __u64 enabled[KVM_SDEI_MAX_VCPUS/64];
> + __u64 unregister_pending[KVM_SDEI_MAX_VCPUS/64];
> +};
> +
> +struct kvm_sdei_vcpu_event_state {
> + __u64 num;
> +
> + __u32 event_count;
> + __u32 padding;
> +};
> +
> +struct kvm_sdei_vcpu_regs_state {
> + __u64 regs[18];
> + __u64 pc;
> + __u64 pstate;
> +};
> +
> +struct kvm_sdei_vcpu_state {
Same goes here, I strongly recommend you try to expose this through the
KVM_{GET,SET}_ONE_REG interface if at all possible since it
significantly reduces the UAPI burden, both on KVM to maintain it and
VMMs to actually use it.
--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
