Messages in this thread | | | Date | Mon, 9 Jan 2023 10:51:19 -0800 | Subject | Re: [PATCH v8 11/28] gunyah: rsc_mgr: Add VM lifecycle RPC | From | Elliot Berman <> |
| |
On 1/8/2023 11:13 PM, Srivatsa Vaddagiri wrote: > * Elliot Berman <quic_eberman@quicinc.com> [2022-12-19 14:58:32]: > >> +/* Call: CONSOLE_OPEN, CONSOLE_CLOSE, CONSOLE_FLUSH */ > > I think this struct is used by other calls as well? > Also CONSOLE_** functions are not yet introduced in this patch > >> +struct gh_vm_common_vmid_req { >> + __le16 vmid; >> + __le16 reserved0; >> +} __packed; > > [snip] > >> +int gh_rm_alloc_vmid(struct gh_rm_rpc *rm, u16 vmid) >> +{ >> + void *resp; >> + struct gh_vm_common_vmid_req req_payload = { >> + .vmid = cpu_to_le16(vmid), >> + }; >> + struct gh_vm_common_vmid_req *resp_payload; >> + size_t resp_size; >> + int ret; >> + >> + if (vmid == GH_VMID_INVAL) >> + vmid = 0; >> + >> + ret = gh_rm_call(rm, GH_RM_RPC_VM_ALLOC_VMID, &req_payload, sizeof(req_payload), &resp, >> + &resp_size); >> + if (ret) >> + return ret; >> + >> + if (!vmid) { >> + if (resp_size != sizeof(*resp_payload)) { >> + ret = -EINVAL; >> + } else { >> + resp_payload = resp; >> + ret = resp_payload->vmid; > > Do we need a le_to_cpu() wrapper on the response here? > >> +int gh_rm_vm_stop(struct gh_rm_rpc *rm, u16 vmid) >> +{ >> + struct gh_vm_stop_req req_payload = { >> + .vmid = cpu_to_le16(vmid), >> + }; >> + void *resp; >> + size_t resp_size; >> + int ret; >> + >> + ret = gh_rm_call(rm, GH_RM_RPC_VM_STOP, &req_payload, sizeof(req_payload), >> + &resp, &resp_size); >> + if (ret) >> + return ret; >> + kfree(resp); > > Why not use gh_rm_common_vmid_call() here as well? > > return gh_rm_common_vmid_call(rm, GH_RM_RPC_VM_STOP, vmid); > >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(gh_rm_vm_stop); >> + > > [snip] > >> +ssize_t gh_rm_get_hyp_resources(struct gh_rm_rpc *rm, u16 vmid, >> + struct gh_rm_hyp_resource **resources) >> +{ >> + struct gh_vm_get_hyp_resources_resp *resp; >> + size_t resp_size; >> + int ret; >> + struct gh_vm_common_vmid_req req_payload = { >> + .vmid = cpu_to_le16(vmid), >> + }; >> + >> + ret = gh_rm_call(rm, GH_RM_RPC_VM_GET_HYP_RESOURCES, >> + &req_payload, sizeof(req_payload), >> + (void **)&resp, &resp_size); >> + if (ret) >> + return ret; >> + >> + if (resp_size < sizeof(*resp) || >> + (sizeof(*resp->entries) && (resp->n_entries > U32_MAX / sizeof(*resp->entries))) || >> + (resp_size != sizeof(*resp) + (resp->n_entries * sizeof(*resp->entries)))) { >> + ret = -EIO; >> + goto out; >> + } >> + >> + *resources = kmemdup(resp->entries, (resp->n_entries * sizeof(*resp->entries)), GFP_KERNEL); > > Consider NULL return value from kmemdup > >> + ret = resp->n_entries; >> + >> +out: >> + kfree(resp); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(gh_rm_get_hyp_resources); >> + >> +/** >> + * gh_rm_get_vmid() - Retrieve VMID of this virtual machine >> + * @vmid: Filled with the VMID of this VM >> + */ >> +int gh_rm_get_vmid(struct gh_rm_rpc *rm, u16 *vmid) >> +{ >> + static u16 cached_vmid = GH_VMID_INVAL; >> + void *resp; >> + size_t resp_size; >> + int ret; >> + int payload = 0; >> + >> + if (cached_vmid != GH_VMID_INVAL) { >> + *vmid = cached_vmid; >> + return 0; >> + } >> + >> + ret = gh_rm_call(rm, GH_RM_RPC_VM_GET_VMID, &payload, sizeof(payload), &resp, &resp_size); >> + if (ret) >> + return ret; >> + >> + if (resp_size != sizeof(*vmid)) > > kfree(resp) in this case? > >> + return -EIO; >> + *vmid = *(u16 *)resp; > > Do we need a le_to_cpu() wrapper on the response? > Also update cached_vmid in success case. > >> + kfree(resp); >> + >> + return ret; >> +}
Applied all these.
Thanks, Elliot
| |