Messages in this thread | | | Date | Thu, 23 Feb 2023 15:10:04 -0800 | Subject | Re: [PATCH v10 09/26] gunyah: rsc_mgr: Add VM lifecycle RPC | From | Elliot Berman <> |
| |
On 2/23/2023 1:36 PM, Alex Elder wrote: > On 2/14/23 3:23 PM, Elliot Berman wrote: >> >> Add Gunyah Resource Manager RPC to launch an unauthenticated VM. >> >> Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> >> --- >> drivers/virt/gunyah/Makefile | 2 +- >> drivers/virt/gunyah/rsc_mgr.h | 45 ++++++ >> drivers/virt/gunyah/rsc_mgr_rpc.c | 226 ++++++++++++++++++++++++++++++ >> include/linux/gunyah_rsc_mgr.h | 73 ++++++++++ >> 4 files changed, 345 insertions(+), 1 deletion(-) >> create mode 100644 drivers/virt/gunyah/rsc_mgr_rpc.c >> >> diff --git a/drivers/virt/gunyah/Makefile b/drivers/virt/gunyah/Makefile >> index cc864ff5abbb..de29769f2f3f 100644 >> --- a/drivers/virt/gunyah/Makefile >> +++ b/drivers/virt/gunyah/Makefile >> @@ -2,5 +2,5 @@ >> obj-$(CONFIG_GUNYAH) += gunyah.o >> -gunyah_rsc_mgr-y += rsc_mgr.o >> +gunyah_rsc_mgr-y += rsc_mgr.o rsc_mgr_rpc.o >> obj-$(CONFIG_GUNYAH) += gunyah_rsc_mgr.o >> diff --git a/drivers/virt/gunyah/rsc_mgr.h >> b/drivers/virt/gunyah/rsc_mgr.h >> index d4e799a7526f..7406237bc66d 100644 >> --- a/drivers/virt/gunyah/rsc_mgr.h >> +++ b/drivers/virt/gunyah/rsc_mgr.h >> @@ -74,4 +74,49 @@ struct gh_rm; >> int gh_rm_call(struct gh_rm *rsc_mgr, u32 message_id, void >> *req_buff, size_t req_buff_size, >> void **resp_buf, size_t *resp_buff_size); >> +/* Message IDs: VM Management */ >> +#define GH_RM_RPC_VM_ALLOC_VMID 0x56000001 >> +#define GH_RM_RPC_VM_DEALLOC_VMID 0x56000002 >> +#define GH_RM_RPC_VM_START 0x56000004 >> +#define GH_RM_RPC_VM_STOP 0x56000005 >> +#define GH_RM_RPC_VM_RESET 0x56000006 >> +#define GH_RM_RPC_VM_CONFIG_IMAGE 0x56000009 >> +#define GH_RM_RPC_VM_INIT 0x5600000B >> +#define GH_RM_RPC_VM_GET_HYP_RESOURCES 0x56000020 >> +#define GH_RM_RPC_VM_GET_VMID 0x56000024 >> + >> +struct gh_rm_vm_common_vmid_req { >> + __le16 vmid; >> + __le16 reserved0; >> +} __packed; >> + >> +/* Call: VM_ALLOC */ >> +struct gh_rm_vm_alloc_vmid_resp { >> + __le16 vmid; >> + __le16 reserved0; >> +} __packed; >> + >> +/* Call: VM_STOP */ >> +struct gh_rm_vm_stop_req { >> + __le16 vmid; >> +#define GH_RM_VM_STOP_FLAG_FORCE_STOP BIT(0) >> + u8 flags; >> + u8 reserved; >> +#define GH_RM_VM_STOP_REASON_FORCE_STOP 3 > > I suggested this before and you honored it. Now I'll suggest > it again, and ask you to do it throughout the driver. > > Please separate the definitions of constant values that > certain fields can take on from the structure definition. > I think doing it the way you have here makes it harder to > understand the structure definition. > > You could define an anonymous enumerated type to hold > the values meant to be held by each field. >
Done.
>> + __le32 stop_reason; >> +} __packed; >> + >> +/* Call: VM_CONFIG_IMAGE */ >> +struct gh_rm_vm_config_image_req { >> + __le16 vmid; >> + __le16 auth_mech; >> + __le32 mem_handle; >> + __le64 image_offset; >> + __le64 image_size; >> + __le64 dtb_offset; >> + __le64 dtb_size; >> +} __packed; >> + >> +/* Call: GET_HYP_RESOURCES */ >> + >> #endif >> diff --git a/drivers/virt/gunyah/rsc_mgr_rpc.c >> b/drivers/virt/gunyah/rsc_mgr_rpc.c >> new file mode 100644 >> index 000000000000..4515cdd80106 >> --- /dev/null >> +++ b/drivers/virt/gunyah/rsc_mgr_rpc.c >> @@ -0,0 +1,226 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All >> rights reserved. >> + */ >> + >> +#include <linux/gunyah_rsc_mgr.h> >> + >> +#include "rsc_mgr.h" >> + >> +/* >> + * Several RM calls take only a VMID as a parameter and give only >> standard >> + * response back. Deduplicate boilerplate code by using this common >> call. >> + */ >> +static int gh_rm_common_vmid_call(struct gh_rm *rm, u32 message_id, >> u16 vmid) >> +{ >> + struct gh_rm_vm_common_vmid_req req_payload = { >> + .vmid = cpu_to_le16(vmid), >> + }; >> + size_t resp_size; >> + void *resp; >> + >> + return gh_rm_call(rm, message_id, &req_payload, >> sizeof(req_payload), &resp, &resp_size); >> +} >> + >> +/** >> + * gh_rm_alloc_vmid() - Allocate a new VM in Gunyah. Returns the VM >> identifier. >> + * @rm: Handle to a Gunyah resource manager >> + * @vmid: Use GH_VMID_INVAL or 0 to dynamically allocate a VM. A >> reserved VMID can >> + * be supplied to request allocation of a platform-defined VM. > > Honestly, I'd rather just see 0 (and *not* GH_VMID_INVAL) be the > special value to mean "dynamically allocate the VMID." It seems > 0 is a reserved VMID anyway, and GH_VMID_INVAL might as well be > treated here as an invalid parameter.
Done.
> > Is there any definitition of which VMIDs are reserved? Like, > anything under 1024?
It's platform dependent. On Qualcomm platforms, VMIDs <= 63 (QCOM_SCM_MAX_MANAGED_VMID) are reserved. Of those reserved VMIDs, Gunyah only allows us to allocate the "special VMs" (today: TUIVM, CPUSYSVM, OEMVM). Passing any value except 0, tuivm_vmid, cpusysvm_vmid, or oemvm_vmid returns an error.
On current non-Qualcomm platforms, there aren't any reserved VMIDs so passing anything but 0 returns an error.
Thanks, Elliot
> > That's it on this patch for now. > > -Alex > >> + * >> + * Returns - the allocated VMID or negative value on error >> + */ >> +int gh_rm_alloc_vmid(struct gh_rm *rm, u16 vmid) >> +{ >> + struct gh_rm_vm_common_vmid_req req_payload = { 0 }; >> + struct gh_rm_vm_alloc_vmid_resp *resp_payload; >> + size_t resp_size; >> + void *resp; >> + int ret; > > . . . >
| |