| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0ba4a554-3a40-1a67-c666-77502f4e6ed6@quicinc.com>
Date: Thu, 23 Feb 2023 15:10:04 -0800
From: Elliot Berman <quic_eberman@...cinc.com>
To: Alex Elder <alex.elder@...aro.org>, Alex Elder <elder@...aro.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>
CC: Murali Nalajala <quic_mnalajal@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>,
Carl van Schaik <quic_cvanscha@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Bjorn Andersson <andersson@...nel.org>,
"Konrad Dybcio" <konrad.dybcio@...aro.org>,
Arnd Bergmann <arnd@...db.de>,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Jonathan Corbet <corbet@....net>,
Bagas Sanjaya <bagasdotme@...il.com>,
Catalin Marinas <catalin.marinas@....com>,
Jassi Brar <jassisinghbrar@...il.com>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v10 09/26] gunyah: rsc_mgr: Add VM lifecycle RPC
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@...cinc.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;
>
> . . .
>
Powered by blists - more mailing lists