On Thu, Dec 28, 2023 at 06:51:35PM +0800, Yi-De Wu wrote:
> From: "Yingshiuan Pan" <yingshiuan....@mediatek.com>
> 
> 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 <yingshiuan....@mediatek.com>
> Signed-off-by: Jerry Wang <ze-yu.w...@mediatek.com>
> Signed-off-by: kevenny hsieh <kevenny.hs...@mediatek.com>
> Signed-off-by: Liju Chen <liju-clr.c...@mediatek.com>
> Signed-off-by: Yi-De Wu <yi-de...@mediatek.com>

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];
> +     };
> +};

...

Reply via email to