Re: [PATCH v9 24/27] virt: gunyah: Add proxy-scheduled vCPUs

From: Elliot Berman <quic_eberman@quicinc.com>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Bjorn Andersson <quic_bjorande@quicinc.com>,
	Alex Elder <elder@linaro.org>,
	Murali Nalajala <quic_mnalajal@quicinc.com>,
	Jonathan Corbet <corbet@lwn.net>,
	"Catalin Marinas" <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>
Cc: Trilok Soni <quic_tsoni@quicinc.com>,
	Srivatsa Vaddagiri <quic_svaddagi@quicinc.com>,
	Carl van Schaik <quic_cvanscha@quicinc.com>,
	Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Arnd Bergmann <arnd@arndb.de>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Bagas Sanjaya <bagasdotme@gmail.com>,
	Marc Zyngier <maz@kernel.org>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Sudeep Holla <sudeep.holla@arm.com>,
	<linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v9 24/27] virt: gunyah: Add proxy-scheduled vCPUs
Date: Wed, 8 Feb 2023 10:36:36 -0800	[thread overview]
Message-ID: <4e03be86-8a9a-13b2-db06-add22595ec96@quicinc.com> (raw)
In-Reply-To: <8b03eafd-8d8e-6e80-2b88-0e186f3ab392@linaro.org>

On 2/7/2023 6:43 AM, Srinivas Kandagatla wrote:
> 
> 
> On 20/01/2023 22:46, Elliot Berman wrote:
>> Gunyah allows host virtual machines to schedule guest virtual machines
>> and handle their MMIO accesses. vCPUs are presented to the host as a
>> Gunyah resource and represented to userspace as a Gunyah VM function.
>>
>> Creating the vcpu VM function will create a file descriptor that:
>>   - can run an ioctl: GH_VCPU_RUN to schedule the guest vCPU until the
>>     next interrupt occurs on the host or when the guest vCPU can no
>>     longer be run.
>>   - can be mmap'd to share a gh_vcpu_run structure which can look up the
>>     reason why GH_VCPU_RUN returned and provide return values for MMIO
>>     access.
>>
>> Co-developed-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Prakruthi Deepak Heragu <quic_pheragu@quicinc.com>
>> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com>
>> ---
>>   Documentation/virt/gunyah/vm-manager.rst |  30 +-
>>   arch/arm64/gunyah/gunyah_hypercall.c     |  28 ++
>>   drivers/virt/gunyah/Kconfig              |  11 +
>>   drivers/virt/gunyah/Makefile             |   2 +
>>   drivers/virt/gunyah/gunyah_vcpu.c        | 358 +++++++++++++++++++++++
>>   drivers/virt/gunyah/vm_mgr.c             |  25 ++
>>   drivers/virt/gunyah/vm_mgr.h             |   1 +
>>   include/linux/gunyah.h                   |   7 +
>>   include/uapi/linux/gunyah.h              |  30 ++
>>   9 files changed, 490 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/virt/gunyah/gunyah_vcpu.c
>>
> ...
> 
> 
>> diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
>> index ec8da6fde045..b4afb11f538a 100644
>> --- a/include/uapi/linux/gunyah.h
>> +++ b/include/uapi/linux/gunyah.h
>> @@ -53,9 +53,14 @@ struct gh_vm_dtb_config {
>>   #define GUNYAH_FUNCTION_NAME_SIZE        32
>>   #define GUNYAH_FUNCTION_MAX_ARG_SIZE        1024
>> +struct gh_fn_vcpu_arg {
>> +    __u32 vcpu_id;
>> +};
>> +
>>   struct gh_vm_function {
>>       char name[GUNYAH_FUNCTION_NAME_SIZE];
>>       union {
>> +        struct gh_device_vcpu_arg vcpu;
>>           char data[GUNYAH_FUNCTION_MAX_ARG_SIZE];
>>       };
>>   };
>> @@ -63,4 +68,29 @@ struct gh_vm_function {
>>   #define GH_VM_ADD_FUNCTION    _IOW(GH_IOCTL_TYPE, 0x4, struct 
>> gh_vm_function)
>>   #define GH_VM_REMOVE_FUNCTION    _IOW(GH_IOCTL_TYPE, 0x7, struct 
>> gh_vm_function)
>> +/* for GH_VCPU_RUN, returned by mmap(vcpu_fd, offset=0) */
>> +struct gh_vcpu_run {
> this looks unused, I dont see any reference to this.
> 

This structure gets mapped into userspace from the vCPU file descriptor, 
similar principle to KVM.

Thanks,
Elliot

>> +    /* in */
>> +    __u8 immediate_exit;
>> +    __u8 padding1[7];
>> +
>> +    /* out */
>> +#define GH_VM_EXIT_UNKNOWN            0
>> +#define GH_VM_EXIT_MMIO               1
>> +    __u32 exit_reason;
> 
> If this is ment to go in any of the ioctl arguments then this suffers 
> same issue of padding.
> 
>> +
>> +    union {
>> +        /* GH_VM_EXIT_MMIO */
>> +        struct {
>> +            __u64 phys_addr;
>> +            __u8  data[8];
>> +            __u32 len;
>> +            __u8  is_write;
>> +        } mmio;
>> +    };
>> +};
>> +
>> +#define GH_VCPU_RUN        _IO(GH_IOCTL_TYPE, 0x5)
>> +#define GH_VCPU_MMAP_SIZE    _IO(GH_IOCTL_TYPE, 0x6)
>> +
>>   #endif