by Simon Horman

[permalink] [raw]

Subject: Re: [PATCH v8 08/20] virt: geniezone: Add vcpu support

On Thu, Dec 28, 2023 at 06:51:35PM +0800, Yi-De Wu wrote:
> From: "Yingshiuan Pan" <[email protected]>
>
> VMM use this interface to create vcpu instance which is a fd, and this
> fd will be for any vcpu operations, such as setting vcpu registers and
> accepts the most important ioctl GZVM_VCPU_RUN which requests GenieZone
> hypervisor to do context switch to execute VM's vcpu context.
>
> Signed-off-by: Yingshiuan Pan <[email protected]>
> Signed-off-by: Jerry Wang <[email protected]>
> Signed-off-by: kevenny hsieh <[email protected]>
> Signed-off-by: Liju Chen <[email protected]>
> Signed-off-by: Yi-De Wu <[email protected]>

Hi Yi-De Wu,

some minor feedback from my side.

...

> diff --git a/include/uapi/linux/gzvm.h b/include/uapi/linux/gzvm.h
> index 77a58ee085df..bdf277fa248a 100644
> --- a/include/uapi/linux/gzvm.h
> +++ b/include/uapi/linux/gzvm.h
> @@ -25,6 +25,34 @@
> /* GZVM_CAP_PVM_SET_PROTECTED_VM only sets protected but not load pvmfw */
> #define GZVM_CAP_PVM_SET_PROTECTED_VM 2
>
> +/*
> + * Architecture specific registers are to be defined and ORed with
> + * the arch identifier.
> + */
> +#define GZVM_REG_ARCH_ARM64 0x6000000000000000ULL
> +#define GZVM_REG_ARCH_MASK 0xff00000000000000ULL

nit: using GENMASK_ULL and FIELD_PREP seems appropriate here.

> +
> +/*
> + * Reg size = BIT((reg.id & GZVM_REG_SIZE_MASK) >> GZVM_REG_SIZE_SHIFT) bytes
> + */
> +#define GZVM_REG_SIZE_SHIFT 52
> +#define GZVM_REG_SIZE_MASK 0x00f0000000000000ULL
> +
> +#define GZVM_REG_SIZE_U8 0x0000000000000000ULL
> +#define GZVM_REG_SIZE_U16 0x0010000000000000ULL
> +#define GZVM_REG_SIZE_U32 0x0020000000000000ULL
> +#define GZVM_REG_SIZE_U64 0x0030000000000000ULL
> +#define GZVM_REG_SIZE_U128 0x0040000000000000ULL
> +#define GZVM_REG_SIZE_U256 0x0050000000000000ULL
> +#define GZVM_REG_SIZE_U512 0x0060000000000000ULL
> +#define GZVM_REG_SIZE_U1024 0x0070000000000000ULL
> +#define GZVM_REG_SIZE_U2048 0x0080000000000000ULL

And here.

> +
> +/* Register type definitions */
> +#define GZVM_REG_TYPE_SHIFT 16
> +/* Register type: general purpose */
> +#define GZVM_REG_TYPE_GENERAL (0x10 << GZVM_REG_TYPE_SHIFT)

And using FIELD_PREP seems appropriate here too.

...

> @@ -51,6 +79,11 @@ struct gzvm_memory_region {
>
> #define GZVM_SET_MEMORY_REGION _IOW(GZVM_IOC_MAGIC, 0x40, \
> struct gzvm_memory_region)
> +/*
> + * GZVM_CREATE_VCPU receives as a parameter the vcpu slot,
> + * and returns a vcpu fd.
> + */
> +#define GZVM_CREATE_VCPU _IO(GZVM_IOC_MAGIC, 0x41)
>
> /* for GZVM_SET_USER_MEMORY_REGION */
> struct gzvm_userspace_memory_region {
> @@ -66,6 +99,124 @@ struct gzvm_userspace_memory_region {
> #define GZVM_SET_USER_MEMORY_REGION _IOW(GZVM_IOC_MAGIC, 0x46, \
> struct gzvm_userspace_memory_region)
>
> +/*
> + * ioctls for vcpu fds
> + */
> +#define GZVM_RUN _IO(GZVM_IOC_MAGIC, 0x80)
> +
> +/* VM exit reason */
> +enum {
> + GZVM_EXIT_UNKNOWN = 0x92920000,
> + GZVM_EXIT_MMIO = 0x92920001,
> + GZVM_EXIT_HYPERCALL = 0x92920002,
> + GZVM_EXIT_IRQ = 0x92920003,
> + GZVM_EXIT_EXCEPTION = 0x92920004,
> + GZVM_EXIT_DEBUG = 0x92920005,
> + GZVM_EXIT_FAIL_ENTRY = 0x92920006,
> + GZVM_EXIT_INTERNAL_ERROR = 0x92920007,
> + GZVM_EXIT_SYSTEM_EVENT = 0x92920008,
> + GZVM_EXIT_SHUTDOWN = 0x92920009,
> + GZVM_EXIT_GZ = 0x9292000a,
> +};
> +
> +/**
> + * struct gzvm_vcpu_run: Same purpose as kvm_run, this struct is
> + * shared between userspace, kernel and
> + * GenieZone hypervisor
> + * @exit_reason: The reason why gzvm_vcpu_run has stopped running the vCPU
> + * @immediate_exit: Polled when the vcpu is scheduled.
> + * If set, immediately returns -EINTR
> + * @padding1: Reserved for future-proof and must be zero filled
> + * @mmio: The nested struct in anonymous union. Handle mmio in host side
> + * @phys_addr: The address guest tries to access
> + * @data: The value to be written (is_write is 1) or
> + * be filled by user for reads (is_write is 0)
> + * @size: The size of written data.
> + * Only the first `size` bytes of `data` are handled
> + * @reg_nr: The register number where the data is stored
> + * @is_write: 1 for VM to perform a write or 0 for VM to perform a read
> + * @fail_entry: The nested struct in anonymous union.
> + * Handle invalid entry address at the first run
> + * @hardware_entry_failure_reason: The reason codes about hardware entry failure
> + * @cpu: The current processor number via smp_processor_id()
> + * @exception: The nested struct in anonymous union.
> + * Handle exception occurred in VM
> + * @exception: Which exception vector
> + * @error_code: Exception error codes
> + * @hypercall: The nested struct in anonymous union.
> + * Some hypercalls issued from VM must be handled
> + * @args: The hypercall's arguments
> + * @internal: The nested struct in anonymous union. The errors from hypervisor
> + * @suberror: The errors codes about GZVM_EXIT_INTERNAL_ERROR
> + * @ndata: The number of elements used in data[]
> + * @data: Keep the detailed information about GZVM_EXIT_INTERNAL_ERROR
> + * @system_event: The nested struct in anonymous union.
> + * VM's PSCI must be handled by host
> + * @type: System event type.
> + * Ex. GZVM_SYSTEM_EVENT_SHUTDOWN or GZVM_SYSTEM_EVENT_RESET...etc.
> + * @ndata: The number of elements used in data[]
> + * @data: Keep the detailed information about GZVM_EXIT_SYSTEM_EVENT
> + * @padding: Fix it to a reasonable size future-proof for keeping the same
> + * struct size when adding new variables in the union is needed
> + *
> + * Keep identical layout between the 3 modules
> + */

I am unsure how to address this, but ./scripts/kernel-doc seems confused
about the correlation between the fields documented above and the nested
structure below.

"./scripts/kernel-doc -none" says:

.../gzvm.h:219: warning: Excess struct member 'phys_addr' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'data' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'size' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'reg_nr' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'is_write' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'hardware_entry_failure_reason' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'cpu' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'error_code' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'args' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'suberror' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'ndata' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'data' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'type' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'ndata' description in 'gzvm_vcpu_run'
.../gzvm.h:219: warning: Excess struct member 'data' description in 'gzvm_vcpu_run'

> +struct gzvm_vcpu_run {
> + /* to userspace */
> + __u32 exit_reason;
> + __u8 immediate_exit;
> + __u8 padding1[3];
> + /* union structure of collection of guest exit reason */
> + union {
> + /* GZVM_EXIT_MMIO */
> + struct {
> + /* from FAR_EL2 */
> + __u64 phys_addr;
> + __u8 data[8];
> + /* from ESR_EL2 as */
> + __u64 size;
> + /* from ESR_EL2 */
> + __u32 reg_nr;
> + /* from ESR_EL2 */
> + __u8 is_write;
> + } mmio;
> + /* GZVM_EXIT_FAIL_ENTRY */
> + struct {
> + __u64 hardware_entry_failure_reason;
> + __u32 cpu;
> + } fail_entry;
> + /* GZVM_EXIT_EXCEPTION */
> + struct {
> + __u32 exception;
> + __u32 error_code;
> + } exception;
> + /* GZVM_EXIT_HYPERCALL */
> + struct {
> + __u64 args[8]; /* in-out */
> + } hypercall;
> + /* GZVM_EXIT_INTERNAL_ERROR */
> + struct {
> + __u32 suberror;
> + __u32 ndata;
> + __u64 data[16];
> + } internal;
> + /* GZVM_EXIT_SYSTEM_EVENT */
> + struct {
> +#define GZVM_SYSTEM_EVENT_SHUTDOWN 1
> +#define GZVM_SYSTEM_EVENT_RESET 2
> +#define GZVM_SYSTEM_EVENT_CRASH 3
> +#define GZVM_SYSTEM_EVENT_WAKEUP 4
> +#define GZVM_SYSTEM_EVENT_SUSPEND 5
> +#define GZVM_SYSTEM_EVENT_SEV_TERM 6
> +#define GZVM_SYSTEM_EVENT_S2IDLE 7
> + __u32 type;
> + __u32 ndata;
> + __u64 data[16];
> + } system_event;
> + char padding[256];
> + };
> +};

...