The command is used for encrypting the guest memory region using the encryption
context created with KVM_SEV_SEND_START.
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: "Radim Krčmář" <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Brijesh Singh <[email protected]>
---
.../virtual/kvm/amd-memory-encryption.rst | 24 ++++
arch/x86/kvm/svm.c | 120 +++++++++++++++++-
include/uapi/linux/kvm.h | 9 ++
3 files changed, 149 insertions(+), 4 deletions(-)
diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
index 0e9e1e9f9687..060ac2316d69 100644
--- a/Documentation/virtual/kvm/amd-memory-encryption.rst
+++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
@@ -265,6 +265,30 @@ Returns: 0 on success, -negative on error
__u32 session_len;
};
+11. KVM_SEV_SEND_UPDATE_DATA
+----------------------------
+
+The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
+outgoing guest memory region with the encryption context creating using
+KVM_SEV_SEND_START.
+
+Parameters (in): struct kvm_sev_send_update_data
+
+Returns: 0 on success, -negative on error
+
+::
+
+ struct kvm_sev_launch_send_update_data {
+ __u64 hdr_uaddr; /* userspace address containing the packet header */
+ __u32 hdr_len;
+
+ __u64 guest_uaddr; /* the source memory region to be encrypted */
+ __u32 guest_len;
+
+ __u64 trans_uaddr; /* the destition memory region */
+ __u32 trans_len;
+ };
+
References
==========
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0b0937f53520..8e815a53c420 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -418,6 +418,7 @@ enum {
static unsigned int max_sev_asid;
static unsigned int min_sev_asid;
+static unsigned long sev_me_mask;
static unsigned long *sev_asid_bitmap;
#define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
@@ -1216,16 +1217,21 @@ static int avic_ga_log_notifier(u32 ga_tag)
static __init int sev_hardware_setup(void)
{
struct sev_user_data_status *status;
+ int eax, ebx;
int rc;
- /* Maximum number of encrypted guests supported simultaneously */
- max_sev_asid = cpuid_ecx(0x8000001F);
+ /*
+ * Query the memory encryption information.
+ * EBX: Bit 0:5 Pagetable bit position used to indicate encryption (aka Cbit).
+ * ECX: Maximum number of encrypted guests supported simultaneously.
+ * EDX: Minimum ASID value that should be used for SEV guest.
+ */
+ cpuid(0x8000001f, &eax, &ebx, &max_sev_asid, &min_sev_asid);
if (!max_sev_asid)
return 1;
- /* Minimum ASID value that should be used for SEV guest */
- min_sev_asid = cpuid_edx(0x8000001F);
+ sev_me_mask = 1UL << (ebx & 0x3f);
/* Initialize SEV ASID bitmap */
sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
@@ -7059,6 +7065,109 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
return ret;
}
+static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+ struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+ struct sev_data_send_update_data *data;
+ struct kvm_sev_send_update_data params;
+ void *hdr = NULL, *trans_data = NULL;
+ struct page **guest_page = NULL;
+ unsigned long n;
+ int ret, offset;
+
+ if (!sev_guest(kvm))
+ return -ENOTTY;
+
+ if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
+ sizeof(struct kvm_sev_send_update_data)))
+ return -EFAULT;
+
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ /* userspace wants to query either header or trans length */
+ if (!params.trans_len || !params.hdr_len)
+ goto cmd;
+
+ ret = -EINVAL;
+ if (!params.trans_uaddr || !params.guest_uaddr ||
+ !params.guest_len || !params.hdr_uaddr)
+ goto e_free;
+
+ /* Check if we are crossing the page boundry */
+ ret = -EINVAL;
+ offset = params.guest_uaddr & (PAGE_SIZE - 1);
+ if ((params.guest_len + offset > PAGE_SIZE))
+ goto e_free;
+
+ ret = -ENOMEM;
+ hdr = kmalloc(params.hdr_len, GFP_KERNEL);
+ if (!hdr)
+ goto e_free;
+
+ data->hdr_address = __psp_pa(hdr);
+ data->hdr_len = params.hdr_len;
+
+ ret = -ENOMEM;
+ trans_data = kmalloc(params.trans_len, GFP_KERNEL);
+ if (!trans_data)
+ goto e_free;
+
+ data->trans_address = __psp_pa(trans_data);
+ data->trans_len = params.trans_len;
+
+ /* Pin guest memory */
+ ret = -EFAULT;
+ guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK,
+ PAGE_SIZE, &n, 0);
+ if (!guest_page)
+ goto e_free;
+
+ /* The SEND_UPDATE_DATA command requires C-bit to be always set. */
+ data->guest_address = (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
+ data->guest_address |= sev_me_mask;
+ data->guest_len = params.guest_len;
+
+cmd:
+ data->handle = sev->handle;
+ ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
+
+ /* userspace asked for header or trans length and FW responded with data */
+ if (!params.trans_len || !params.hdr_len) {
+ params.hdr_len = data->hdr_len;
+ params.trans_len = data->trans_len;
+ goto done;
+ }
+
+ if (ret)
+ goto e_unpin;
+
+ /* copy transport buffer to user space */
+ if (copy_to_user((void __user *)(uintptr_t)params.trans_uaddr,
+ trans_data, params.trans_len)) {
+ ret = -EFAULT;
+ goto e_unpin;
+ }
+
+ /* copy packet header to userspace */
+ if (copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr, params.hdr_len))
+ ret = -EFAULT;
+
+e_unpin:
+ sev_unpin_memory(kvm, guest_page, n);
+done:
+ if (copy_to_user((void __user *)(uintptr_t)argp->data, ¶ms,
+ sizeof(struct kvm_sev_send_update_data)))
+ ret = -EFAULT;
+e_free:
+ kfree(data);
+ kfree(trans_data);
+ kfree(hdr);
+
+ return ret;
+}
+
static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
{
struct kvm_sev_cmd sev_cmd;
@@ -7103,6 +7212,9 @@ static int svm_mem_enc_op(struct kvm *kvm, void __user *argp)
case KVM_SEV_SEND_START:
r = sev_send_start(kvm, &sev_cmd);
break;
+ case KVM_SEV_SEND_UPDATE_DATA:
+ r = sev_send_update_data(kvm, &sev_cmd);
+ break;
default:
r = -EINVAL;
goto out;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4e9e7a5b2066..4cb6c3774ec2 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1543,6 +1543,15 @@ struct kvm_sev_send_start {
__u32 session_len;
};
+struct kvm_sev_send_update_data {
+ __u64 hdr_uaddr;
+ __u32 hdr_len;
+ __u64 guest_uaddr;
+ __u32 guest_len;
+ __u64 trans_uaddr;
+ __u32 trans_len;
+};
+
#define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
#define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
#define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)
--
2.17.1
On Wed, Jul 10, 2019 at 08:13:01PM +0000, Singh, Brijesh wrote:
> The command is used for encrypting the guest memory region using the encryption
> context created with KVM_SEV_SEND_START.
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: "Radim Krčmář" <[email protected]>
> Cc: Joerg Roedel <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Brijesh Singh <[email protected]>
> ---
> .../virtual/kvm/amd-memory-encryption.rst | 24 ++++
> arch/x86/kvm/svm.c | 120 +++++++++++++++++-
> include/uapi/linux/kvm.h | 9 ++
> 3 files changed, 149 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
> index 0e9e1e9f9687..060ac2316d69 100644
> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
> @@ -265,6 +265,30 @@ Returns: 0 on success, -negative on error
> __u32 session_len;
> };
>
> +11. KVM_SEV_SEND_UPDATE_DATA
> +----------------------------
> +
> +The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
> +outgoing guest memory region with the encryption context creating using
s/creating/created/
> +KVM_SEV_SEND_START.
> +
> +Parameters (in): struct kvm_sev_send_update_data
> +
> +Returns: 0 on success, -negative on error
> +
> +::
> +
> + struct kvm_sev_launch_send_update_data {
> + __u64 hdr_uaddr; /* userspace address containing the packet header */
> + __u32 hdr_len;
> +
> + __u64 guest_uaddr; /* the source memory region to be encrypted */
> + __u32 guest_len;
> +
> + __u64 trans_uaddr; /* the destition memory region */
s/destition/destination/
> + __u32 trans_len;
Those addresses are all system physical addresses, according to the doc.
Why do you call them "uaddr"?
> + };
> +
> References
> ==========
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0b0937f53520..8e815a53c420 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -418,6 +418,7 @@ enum {
>
> static unsigned int max_sev_asid;
> static unsigned int min_sev_asid;
> +static unsigned long sev_me_mask;
> static unsigned long *sev_asid_bitmap;
> #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
>
> @@ -1216,16 +1217,21 @@ static int avic_ga_log_notifier(u32 ga_tag)
> static __init int sev_hardware_setup(void)
> {
> struct sev_user_data_status *status;
> + int eax, ebx;
> int rc;
>
> - /* Maximum number of encrypted guests supported simultaneously */
> - max_sev_asid = cpuid_ecx(0x8000001F);
> + /*
> + * Query the memory encryption information.
> + * EBX: Bit 0:5 Pagetable bit position used to indicate encryption (aka Cbit).
> + * ECX: Maximum number of encrypted guests supported simultaneously.
> + * EDX: Minimum ASID value that should be used for SEV guest.
> + */
> + cpuid(0x8000001f, &eax, &ebx, &max_sev_asid, &min_sev_asid);
>
> if (!max_sev_asid)
> return 1;
>
> - /* Minimum ASID value that should be used for SEV guest */
> - min_sev_asid = cpuid_edx(0x8000001F);
> + sev_me_mask = 1UL << (ebx & 0x3f);
>
> /* Initialize SEV ASID bitmap */
> sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
> @@ -7059,6 +7065,109 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> return ret;
> }
>
> +static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> + struct sev_data_send_update_data *data;
> + struct kvm_sev_send_update_data params;
> + void *hdr = NULL, *trans_data = NULL;
> + struct page **guest_page = NULL;
Ah, I see why you do init them to NULL - -Wmaybe-uninitialized. See below.
> + unsigned long n;
> + int ret, offset;
> +
> + if (!sev_guest(kvm))
> + return -ENOTTY;
> +
> + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
> + sizeof(struct kvm_sev_send_update_data)))
> + return -EFAULT;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + /* userspace wants to query either header or trans length */
> + if (!params.trans_len || !params.hdr_len)
> + goto cmd;
> +
> + ret = -EINVAL;
> + if (!params.trans_uaddr || !params.guest_uaddr ||
> + !params.guest_len || !params.hdr_uaddr)
> + goto e_free;
> +
> + /* Check if we are crossing the page boundry */
WARNING: 'boundry' may be misspelled - perhaps 'boundary'?
So the fact that you have to init local variables to NULL means that gcc
doesn't see the that kfree() can take a NULL.
But also, you can restructure your labels in a way so that gcc sees them
properly and doesn't issue the warning even without having to init those
local variables.
And also, you can cleanup that function and split out the header and
trans length query functionality into a separate helper and this way
make it a lot more readable. I gave it a try here and it looks more
readable to me but this could be just me.
I could've missed some case too... pasting the whole thing for easier
review than as a diff:
---
/* Userspace wants to query either header or trans length. */
static int
__sev_send_update_data_query_lengths(struct kvm *kvm, struct kvm_sev_cmd *argp,
struct kvm_sev_send_update_data *params)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
struct sev_data_send_update_data data;
memset(&data, 0, sizeof(data));
data.handle = sev->handle;
sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, &data, &argp->error);
params->hdr_len = data.hdr_len;
params->trans_len = data.trans_len;
if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
sizeof(struct kvm_sev_send_update_data)))
return -EFAULT;
return 0;
}
static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
{
struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
struct sev_data_send_update_data *data;
struct kvm_sev_send_update_data params;
struct page **guest_page;
void *hdr, *trans_data;
unsigned long n;
int ret, offset;
if (!sev_guest(kvm))
return -ENOTTY;
if (copy_from_user(¶ms,
(void __user *)(uintptr_t)argp->data,
sizeof(struct kvm_sev_send_update_data)))
return -EFAULT;
/* Userspace wants to query either header or trans length */
if (!params.trans_len || !params.hdr_len)
return __sev_send_update_data_query_lengths(kvm, argp, ¶ms);
if (!params.trans_uaddr || !params.guest_uaddr ||
!params.guest_len || !params.hdr_uaddr)
return -EINVAL;
/* Check if we are crossing the page boundary: */
offset = params.guest_uaddr & (PAGE_SIZE - 1);
if ((params.guest_len + offset > PAGE_SIZE))
return -EINVAL;
hdr = kmalloc(params.hdr_len, GFP_KERNEL);
if (!hdr)
return -ENOMEM;
ret = -ENOMEM;
trans_data = kmalloc(params.trans_len, GFP_KERNEL);
if (!trans_data)
goto free_hdr;
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
goto free_trans;
/* Pin guest memory */
ret = -EFAULT;
guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK, PAGE_SIZE, &n, 0);
if (!guest_page)
goto free_data;
/* The SEND_UPDATE_DATA command requires C-bit to be always set. */
data->guest_address = (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
data->guest_address |= sev_me_mask;
data->guest_len = params.guest_len;
data->hdr_address = __psp_pa(hdr);
data->hdr_len = params.hdr_len;
data->trans_address = __psp_pa(trans_data);
data->trans_len = params.trans_len;
data->handle = sev->handle;
ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
if (ret)
goto unpin_memory;
/* Copy transport buffer to user space. */
ret = copy_to_user((void __user *)(uintptr_t)params.trans_uaddr, trans_data, params.trans_len);
if (ret)
goto unpin_memory;
/* Copy packet header to userspace. */
ret = copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr, params.hdr_len);
unpin_memory:
sev_unpin_memory(kvm, guest_page, n);
free_data:
kfree(data);
free_trans:
kfree(trans_data);
free_hdr:
kfree(hdr);
return ret;
}
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Mary Higgins, Sri Rasiah, HRB 21284 (AG Nürnberg)
On 8/22/19 7:02 AM, Borislav Petkov wrote:
> On Wed, Jul 10, 2019 at 08:13:01PM +0000, Singh, Brijesh wrote:
>> The command is used for encrypting the guest memory region using the encryption
>> context created with KVM_SEV_SEND_START.
>>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: "H. Peter Anvin" <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: "Radim Krčmář" <[email protected]>
>> Cc: Joerg Roedel <[email protected]>
>> Cc: Borislav Petkov <[email protected]>
>> Cc: Tom Lendacky <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Signed-off-by: Brijesh Singh <[email protected]>
>> ---
>> .../virtual/kvm/amd-memory-encryption.rst | 24 ++++
>> arch/x86/kvm/svm.c | 120 +++++++++++++++++-
>> include/uapi/linux/kvm.h | 9 ++
>> 3 files changed, 149 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/amd-memory-encryption.rst b/Documentation/virtual/kvm/amd-memory-encryption.rst
>> index 0e9e1e9f9687..060ac2316d69 100644
>> --- a/Documentation/virtual/kvm/amd-memory-encryption.rst
>> +++ b/Documentation/virtual/kvm/amd-memory-encryption.rst
>> @@ -265,6 +265,30 @@ Returns: 0 on success, -negative on error
>> __u32 session_len;
>> };
>>
>> +11. KVM_SEV_SEND_UPDATE_DATA
>> +----------------------------
>> +
>> +The KVM_SEV_SEND_UPDATE_DATA command can be used by the hypervisor to encrypt the
>> +outgoing guest memory region with the encryption context creating using
>
> s/creating/created/
>
>> +KVM_SEV_SEND_START.
>> +
>> +Parameters (in): struct kvm_sev_send_update_data
>> +
>> +Returns: 0 on success, -negative on error
>> +
>> +::
>> +
>> + struct kvm_sev_launch_send_update_data {
>> + __u64 hdr_uaddr; /* userspace address containing the packet header */
>> + __u32 hdr_len;
>> +
>> + __u64 guest_uaddr; /* the source memory region to be encrypted */
>> + __u32 guest_len;
>> +
>> + __u64 trans_uaddr; /* the destition memory region */
>
> s/destition/destination/
>
>> + __u32 trans_len;
>
> Those addresses are all system physical addresses, according to the doc.
> Why do you call them "uaddr"?
>
FW accepts the system physical address but the userspace does not know
the system physical instead it will give host virtual address and we
will find its corresponding system physical address and make a FW
call. This is a userspace interface and not the FW.
>> + };
>> +
>> References
>> ==========
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 0b0937f53520..8e815a53c420 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -418,6 +418,7 @@ enum {
>>
>> static unsigned int max_sev_asid;
>> static unsigned int min_sev_asid;
>> +static unsigned long sev_me_mask;
>> static unsigned long *sev_asid_bitmap;
>> #define __sme_page_pa(x) __sme_set(page_to_pfn(x) << PAGE_SHIFT)
>>
>> @@ -1216,16 +1217,21 @@ static int avic_ga_log_notifier(u32 ga_tag)
>> static __init int sev_hardware_setup(void)
>> {
>> struct sev_user_data_status *status;
>> + int eax, ebx;
>> int rc;
>>
>> - /* Maximum number of encrypted guests supported simultaneously */
>> - max_sev_asid = cpuid_ecx(0x8000001F);
>> + /*
>> + * Query the memory encryption information.
>> + * EBX: Bit 0:5 Pagetable bit position used to indicate encryption (aka Cbit).
>> + * ECX: Maximum number of encrypted guests supported simultaneously.
>> + * EDX: Minimum ASID value that should be used for SEV guest.
>> + */
>> + cpuid(0x8000001f, &eax, &ebx, &max_sev_asid, &min_sev_asid);
>>
>> if (!max_sev_asid)
>> return 1;
>>
>> - /* Minimum ASID value that should be used for SEV guest */
>> - min_sev_asid = cpuid_edx(0x8000001F);
>> + sev_me_mask = 1UL << (ebx & 0x3f);
>>
>> /* Initialize SEV ASID bitmap */
>> sev_asid_bitmap = bitmap_zalloc(max_sev_asid, GFP_KERNEL);
>> @@ -7059,6 +7065,109 @@ static int sev_send_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>> return ret;
>> }
>>
>> +static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
>> +{
>> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> + struct sev_data_send_update_data *data;
>> + struct kvm_sev_send_update_data params;
>> + void *hdr = NULL, *trans_data = NULL;
>> + struct page **guest_page = NULL;
>
> Ah, I see why you do init them to NULL - -Wmaybe-uninitialized. See below.
>
>> + unsigned long n;
>> + int ret, offset;
>> +
>> + if (!sev_guest(kvm))
>> + return -ENOTTY;
>> +
>> + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
>> + sizeof(struct kvm_sev_send_update_data)))
>> + return -EFAULT;
>> +
>> + data = kzalloc(sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + /* userspace wants to query either header or trans length */
>> + if (!params.trans_len || !params.hdr_len)
>> + goto cmd;
>> +
>> + ret = -EINVAL;
>> + if (!params.trans_uaddr || !params.guest_uaddr ||
>> + !params.guest_len || !params.hdr_uaddr)
>> + goto e_free;
>> +
>> + /* Check if we are crossing the page boundry */
>
> WARNING: 'boundry' may be misspelled - perhaps 'boundary'?
>
> So the fact that you have to init local variables to NULL means that gcc
> doesn't see the that kfree() can take a NULL.
>
> But also, you can restructure your labels in a way so that gcc sees them
> properly and doesn't issue the warning even without having to init those
> local variables.
>
> And also, you can cleanup that function and split out the header and
> trans length query functionality into a separate helper and this way
> make it a lot more readable. I gave it a try here and it looks more
> readable to me but this could be just me.
>
> I could've missed some case too... pasting the whole thing for easier
> review than as a diff:
>
Okay, I will take a look and will probably reuse your functions. thank you.
-Brijesh
>
> ---
> /* Userspace wants to query either header or trans length. */
> static int
> __sev_send_update_data_query_lengths(struct kvm *kvm, struct kvm_sev_cmd *argp,
> struct kvm_sev_send_update_data *params)
> {
> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> struct sev_data_send_update_data data;
>
> memset(&data, 0, sizeof(data));
>
> data.handle = sev->handle;
> sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, &data, &argp->error);
>
> params->hdr_len = data.hdr_len;
> params->trans_len = data.trans_len;
>
> if (copy_to_user((void __user *)(uintptr_t)argp->data, params,
> sizeof(struct kvm_sev_send_update_data)))
> return -EFAULT;
>
> return 0;
> }
>
> static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
> {
> struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> struct sev_data_send_update_data *data;
> struct kvm_sev_send_update_data params;
> struct page **guest_page;
> void *hdr, *trans_data;
> unsigned long n;
> int ret, offset;
>
> if (!sev_guest(kvm))
> return -ENOTTY;
>
> if (copy_from_user(¶ms,
> (void __user *)(uintptr_t)argp->data,
> sizeof(struct kvm_sev_send_update_data)))
> return -EFAULT;
>
> /* Userspace wants to query either header or trans length */
> if (!params.trans_len || !params.hdr_len)
> return __sev_send_update_data_query_lengths(kvm, argp, ¶ms);
>
> if (!params.trans_uaddr || !params.guest_uaddr ||
> !params.guest_len || !params.hdr_uaddr)
> return -EINVAL;
>
> /* Check if we are crossing the page boundary: */
> offset = params.guest_uaddr & (PAGE_SIZE - 1);
> if ((params.guest_len + offset > PAGE_SIZE))
> return -EINVAL;
>
> hdr = kmalloc(params.hdr_len, GFP_KERNEL);
> if (!hdr)
> return -ENOMEM;
>
> ret = -ENOMEM;
> trans_data = kmalloc(params.trans_len, GFP_KERNEL);
> if (!trans_data)
> goto free_hdr;
>
> data = kzalloc(sizeof(*data), GFP_KERNEL);
> if (!data)
> goto free_trans;
>
> /* Pin guest memory */
> ret = -EFAULT;
> guest_page = sev_pin_memory(kvm, params.guest_uaddr & PAGE_MASK, PAGE_SIZE, &n, 0);
> if (!guest_page)
> goto free_data;
>
> /* The SEND_UPDATE_DATA command requires C-bit to be always set. */
> data->guest_address = (page_to_pfn(guest_page[0]) << PAGE_SHIFT) + offset;
> data->guest_address |= sev_me_mask;
> data->guest_len = params.guest_len;
> data->hdr_address = __psp_pa(hdr);
> data->hdr_len = params.hdr_len;
> data->trans_address = __psp_pa(trans_data);
> data->trans_len = params.trans_len;
> data->handle = sev->handle;
>
> ret = sev_issue_cmd(kvm, SEV_CMD_SEND_UPDATE_DATA, data, &argp->error);
> if (ret)
> goto unpin_memory;
>
> /* Copy transport buffer to user space. */
> ret = copy_to_user((void __user *)(uintptr_t)params.trans_uaddr, trans_data, params.trans_len);
> if (ret)
> goto unpin_memory;
>
> /* Copy packet header to userspace. */
> ret = copy_to_user((void __user *)(uintptr_t)params.hdr_uaddr, hdr, params.hdr_len);
>
> unpin_memory:
> sev_unpin_memory(kvm, guest_page, n);
>
> free_data:
> kfree(data);
>
> free_trans:
> kfree(trans_data);
>
> free_hdr:
> kfree(hdr);
>
> return ret;
> }
>
On Thu, Aug 22, 2019 at 01:27:25PM +0000, Singh, Brijesh wrote:
> FW accepts the system physical address but the userspace does not know
> the system physical instead it will give host virtual address and we
> will find its corresponding system physical address and make a FW
> call. This is a userspace interface and not the FW.
That fact could be in a sentence or two as a comment above the struct
definition.
> Okay, I will take a look and will probably reuse your functions. thank you.
Sure, and pls see if you can simplify the other command-sending
functions in a similar manner.
Thx.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Mary Higgins, Sri Rasiah, HRB 21284 (AG Nürnberg)
On Wed, Jul 10, 2019 at 1:14 PM Singh, Brijesh <[email protected]> wrote:
> +static int sev_send_update_data(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> + struct sev_data_send_update_data *data;
> + struct kvm_sev_send_update_data params;
> + void *hdr = NULL, *trans_data = NULL;
> + struct page **guest_page = NULL;
> + unsigned long n;
> + int ret, offset;
> +
> + if (!sev_guest(kvm))
> + return -ENOTTY;
> +
> + if (copy_from_user(¶ms, (void __user *)(uintptr_t)argp->data,
> + sizeof(struct kvm_sev_send_update_data)))
> + return -EFAULT;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + /* userspace wants to query either header or trans length */
> + if (!params.trans_len || !params.hdr_len)
> + goto cmd;
> +
> + ret = -EINVAL;
> + if (!params.trans_uaddr || !params.guest_uaddr ||
> + !params.guest_len || !params.hdr_uaddr)
> + goto e_free;
> +
> + /* Check if we are crossing the page boundry */
> + ret = -EINVAL;
> + offset = params.guest_uaddr & (PAGE_SIZE - 1);
> + if ((params.guest_len + offset > PAGE_SIZE))
> + goto e_free;
> +
> + ret = -ENOMEM;
> + hdr = kmalloc(params.hdr_len, GFP_KERNEL);
> + if (!hdr)
> + goto e_free;
Should we be checking params.hdr_len against SEV_FW_BLOB_MAX_SIZE?
> +
> + data->hdr_address = __psp_pa(hdr);
> + data->hdr_len = params.hdr_len;
> +
> + ret = -ENOMEM;
> + trans_data = kmalloc(params.trans_len, GFP_KERNEL);
> + if (!trans_data)
> + goto e_free;
Ditto, should we be checking params.hdr_len against SEV_FW_BLOB_MAX_SIZE?