diff mbox series

[v2,04/11] virt: sev-guest: Add simplified helper to assign vmpck

Message ID 20230326144701.3039598-5-nikunj@amd.com
State New
Headers
Series Add Secure TSC support for SNP guests |

Commit Message

Nikunj A. Dadhania March 26, 2023, 2:46 p.m. UTC 
  Simplify get_vmpck and prepare it to be used as an API. Update the
snp_guest_dev structure in snp_assign_vmpck(). Added vmpck_id to the
snp_guest_dev structure which can be used in SNP guest request API and
will remove direct use of vmpck_id command line parameter.

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c | 41 ++++++++-----------------
 1 file changed, 12 insertions(+), 29 deletions(-)

Comments

Tom Lendacky April 3, 2023, 8:26 p.m. UTC | #1 
On 3/26/23 09:46, Nikunj A Dadhania wrote:
> Simplify get_vmpck and prepare it to be used as an API. Update the
> snp_guest_dev structure in snp_assign_vmpck(). Added vmpck_id to the
> snp_guest_dev structure which can be used in SNP guest request API and
> will remove direct use of vmpck_id command line parameter.

This needs to say more about why you're doing this. The first two 
sentences need more explanation as they don't really stand on their own.

The last sentence talks about removing direct use of vmpck_id, so you 
should probably do that in this patch since it seems to be the real reason 
for all this.

Thanks,
Tom

> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>   drivers/virt/coco/sev-guest/sev-guest.c | 41 ++++++++-----------------
>   1 file changed, 12 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index ec93dee330f2..4901ebc8fa1a 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -45,6 +45,7 @@ struct snp_guest_dev {
>   	struct snp_req_data input;
>   	u32 *os_area_msg_seqno;
>   	u8 *vmpck;
> +	u8 vmpck_id;
>   };
>   
>   static u32 vmpck_id;
> @@ -80,7 +81,7 @@ static inline unsigned int get_ctx_authsize(struct snp_guest_dev *snp_dev)
>   static void snp_disable_vmpck(struct snp_guest_dev *snp_dev)
>   {
>   	dev_alert(snp_dev->dev, "Disabling vmpck_id %d to prevent IV reuse.\n",
> -		  vmpck_id);
> +		  snp_dev->vmpck_id);
>   	memzero_explicit(snp_dev->vmpck, VMPCK_KEY_LEN);
>   	snp_dev->vmpck = NULL;
>   }
> @@ -339,7 +340,7 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
>   	memset(snp_dev->response, 0, sizeof(struct snp_guest_msg));
>   
>   	/* Encrypt the userspace provided payload */
> -	rc = enc_payload(snp_dev, seqno, req, vmpck_id);
> +	rc = enc_payload(snp_dev, seqno, req, snp_dev->vmpck_id);
>   	if (rc)
>   		return rc;
>   
> @@ -364,7 +365,6 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
>   	return 0;
>   }
>   
> -
>   static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, u8 msg_version,
>   				u8 msg_type, void *req_buf, size_t req_sz, void *resp_buf,
>   				u32 resp_sz, __u64 *fw_err)
> @@ -625,32 +625,16 @@ static const struct file_operations snp_guest_fops = {
>   	.unlocked_ioctl = snp_guest_ioctl,
>   };
>   
> -static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno)
> +bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
>   {
> -	u8 *key = NULL;
> +	if (WARN_ON(vmpck_id > 3))
> +		return false;
>   
> -	switch (id) {
> -	case 0:
> -		*seqno = &layout->os_area.msg_seqno_0;
> -		key = layout->vmpck0;
> -		break;
> -	case 1:
> -		*seqno = &layout->os_area.msg_seqno_1;
> -		key = layout->vmpck1;
> -		break;
> -	case 2:
> -		*seqno = &layout->os_area.msg_seqno_2;
> -		key = layout->vmpck2;
> -		break;
> -	case 3:
> -		*seqno = &layout->os_area.msg_seqno_3;
> -		key = layout->vmpck3;
> -		break;
> -	default:
> -		break;
> -	}
> +	dev->vmpck_id = vmpck_id;
> +	dev->vmpck = dev->layout->vmpck0 + vmpck_id * VMPCK_KEY_LEN;
> +	dev->os_area_msg_seqno = &dev->layout->os_area.msg_seqno_0 + vmpck_id;
>   
> -	return key;
> +	return true;
>   }
>   
>   static int __init sev_guest_probe(struct platform_device *pdev)
> @@ -682,8 +666,8 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>   		goto e_unmap;
>   
>   	ret = -EINVAL;
> -	snp_dev->vmpck = get_vmpck(vmpck_id, layout, &snp_dev->os_area_msg_seqno);
> -	if (!snp_dev->vmpck) {
> +	snp_dev->layout = layout;
> +	if (!snp_assign_vmpck(snp_dev, vmpck_id)) {
>   		dev_err(dev, "invalid vmpck id %d\n", vmpck_id);
>   		goto e_unmap;
>   	}
> @@ -697,7 +681,6 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>   	mutex_init(&snp_dev->cmd_mutex);
>   	platform_set_drvdata(pdev, snp_dev);
>   	snp_dev->dev = dev;
> -	snp_dev->layout = layout;
>   
>   	/* Allocate the shared page used for the request and response message. */
>   	snp_dev->request = alloc_shared_pages(dev, sizeof(struct snp_guest_msg));
Nikunj A. Dadhania April 5, 2023, 6:18 a.m. UTC | #2 
On 4/4/2023 1:56 AM, Tom Lendacky wrote:
> On 3/26/23 09:46, Nikunj A Dadhania wrote:
>> Simplify get_vmpck and prepare it to be used as an API. Update the
>> snp_guest_dev structure in snp_assign_vmpck(). Added vmpck_id to the
>> snp_guest_dev structure which can be used in SNP guest request API and
>> will remove direct use of vmpck_id command line parameter.
> 
> This needs to say more about why you're doing this. The first two 
> sentences need more explanation as they don't really stand on their own.
> How about the this:

Add vmpck_id to snp_guest_dev struct instead of directly using vmpck_id 
command line parameter. Each SNP guest has four VMPCK, which the firmware 
generates and provides to the guest in a special secrets page. Each VMPCK 
has a sequence number. Simplify assignment of key and sequence number 
instead of using a switch case.

> The last sentence talks about removing direct use of vmpck_id, so you 
> should probably do that in this patch since it seems to be the real 
> reason for all this.

I did change all places using vmpck_id to snp_dev->vmpck_id. This is the 
only place the command line parameter is being used.

+       if (!snp_assign_vmpck(snp_dev, vmpck_id)) {

Regards
Nikunj

> 
> Thanks,
> Tom
> 
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
>> ---
>>   drivers/virt/coco/sev-guest/sev-guest.c | 41 ++++++++-----------------
>>   1 file changed, 12 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
>> index ec93dee330f2..4901ebc8fa1a 100644
>> --- a/drivers/virt/coco/sev-guest/sev-guest.c
>> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
>> @@ -45,6 +45,7 @@ struct snp_guest_dev {
>>       struct snp_req_data input;
>>       u32 *os_area_msg_seqno;
>>       u8 *vmpck;
>> +    u8 vmpck_id;
>>   };
>>     static u32 vmpck_id;
>> @@ -80,7 +81,7 @@ static inline unsigned int get_ctx_authsize(struct snp_guest_dev *snp_dev)
>>   static void snp_disable_vmpck(struct snp_guest_dev *snp_dev)
>>   {
>>       dev_alert(snp_dev->dev, "Disabling vmpck_id %d to prevent IV reuse.\n",
>> -          vmpck_id);
>> +          snp_dev->vmpck_id);
>>       memzero_explicit(snp_dev->vmpck, VMPCK_KEY_LEN);
>>       snp_dev->vmpck = NULL;
>>   }
>> @@ -339,7 +340,7 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
>>       memset(snp_dev->response, 0, sizeof(struct snp_guest_msg));
>>         /* Encrypt the userspace provided payload */
>> -    rc = enc_payload(snp_dev, seqno, req, vmpck_id);
>> +    rc = enc_payload(snp_dev, seqno, req, snp_dev->vmpck_id);
>>       if (rc)
>>           return rc;
>>   @@ -364,7 +365,6 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
>>       return 0;
>>   }
>>   -
>>   static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, u8 msg_version,
>>                   u8 msg_type, void *req_buf, size_t req_sz, void *resp_buf,
>>                   u32 resp_sz, __u64 *fw_err)
>> @@ -625,32 +625,16 @@ static const struct file_operations snp_guest_fops = {
>>       .unlocked_ioctl = snp_guest_ioctl,
>>   };
>>   -static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno)
>> +bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
>>   {
>> -    u8 *key = NULL;
>> +    if (WARN_ON(vmpck_id > 3))
>> +        return false;
>>   -    switch (id) {
>> -    case 0:
>> -        *seqno = &layout->os_area.msg_seqno_0;
>> -        key = layout->vmpck0;
>> -        break;
>> -    case 1:
>> -        *seqno = &layout->os_area.msg_seqno_1;
>> -        key = layout->vmpck1;
>> -        break;
>> -    case 2:
>> -        *seqno = &layout->os_area.msg_seqno_2;
>> -        key = layout->vmpck2;
>> -        break;
>> -    case 3:
>> -        *seqno = &layout->os_area.msg_seqno_3;
>> -        key = layout->vmpck3;
>> -        break;
>> -    default:
>> -        break;
>> -    }
>> +    dev->vmpck_id = vmpck_id;
>> +    dev->vmpck = dev->layout->vmpck0 + vmpck_id * VMPCK_KEY_LEN;
>> +    dev->os_area_msg_seqno = &dev->layout->os_area.msg_seqno_0 + vmpck_id;
>>   -    return key;
>> +    return true;
>>   }
>>     static int __init sev_guest_probe(struct platform_device *pdev)
>> @@ -682,8 +666,8 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>>           goto e_unmap;
>>         ret = -EINVAL;
>> -    snp_dev->vmpck = get_vmpck(vmpck_id, layout, &snp_dev->os_area_msg_seqno);
>> -    if (!snp_dev->vmpck) {
>> +    snp_dev->layout = layout;
>> +    if (!snp_assign_vmpck(snp_dev, vmpck_id)) {
>>           dev_err(dev, "invalid vmpck id %d\n", vmpck_id);
>>           goto e_unmap;
>>       }
>> @@ -697,7 +681,6 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>>       mutex_init(&snp_dev->cmd_mutex);
>>       platform_set_drvdata(pdev, snp_dev);
>>       snp_dev->dev = dev;
>> -    snp_dev->layout = layout;
>>         /* Allocate the shared page used for the request and response message. */
>>       snp_dev->request = alloc_shared_pages(dev, sizeof(struct snp_guest_msg));
Peter Gonda April 10, 2023, 4:31 p.m. UTC | #3 
>
> -static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno)
> +bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
>  {
> -       u8 *key = NULL;
> +       if (WARN_ON(vmpck_id > 3))
> +               return false;

Should vmpck_id be a uint to match the module parameter or should we
check if vmpck_id < 0 ?
Nikunj A. Dadhania April 14, 2023, 4:51 a.m. UTC | #4 
On 4/10/2023 10:01 PM, Peter Gonda wrote:
>>
>> -static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno)
>> +bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
>>  {
>> -       u8 *key = NULL;
>> +       if (WARN_ON(vmpck_id > 3))
>> +               return false;
> 
> Should vmpck_id be a uint to match the module parameter or should we
> check if vmpck_id < 0 ?

Thanks for pointing that out, I will make vmpck_id as a uint.

Regards
Nikunj
diff mbox series

Patch

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index ec93dee330f2..4901ebc8fa1a 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -45,6 +45,7 @@  struct snp_guest_dev {
 	struct snp_req_data input;
 	u32 *os_area_msg_seqno;
 	u8 *vmpck;
+	u8 vmpck_id;
 };
 
 static u32 vmpck_id;
@@ -80,7 +81,7 @@  static inline unsigned int get_ctx_authsize(struct snp_guest_dev *snp_dev)
 static void snp_disable_vmpck(struct snp_guest_dev *snp_dev)
 {
 	dev_alert(snp_dev->dev, "Disabling vmpck_id %d to prevent IV reuse.\n",
-		  vmpck_id);
+		  snp_dev->vmpck_id);
 	memzero_explicit(snp_dev->vmpck, VMPCK_KEY_LEN);
 	snp_dev->vmpck = NULL;
 }
@@ -339,7 +340,7 @@  static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
 	memset(snp_dev->response, 0, sizeof(struct snp_guest_msg));
 
 	/* Encrypt the userspace provided payload */
-	rc = enc_payload(snp_dev, seqno, req, vmpck_id);
+	rc = enc_payload(snp_dev, seqno, req, snp_dev->vmpck_id);
 	if (rc)
 		return rc;
 
@@ -364,7 +365,6 @@  static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues
 	return 0;
 }
 
-
 static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, u8 msg_version,
 				u8 msg_type, void *req_buf, size_t req_sz, void *resp_buf,
 				u32 resp_sz, __u64 *fw_err)
@@ -625,32 +625,16 @@  static const struct file_operations snp_guest_fops = {
 	.unlocked_ioctl = snp_guest_ioctl,
 };
 
-static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno)
+bool snp_assign_vmpck(struct snp_guest_dev *dev, int vmpck_id)
 {
-	u8 *key = NULL;
+	if (WARN_ON(vmpck_id > 3))
+		return false;
 
-	switch (id) {
-	case 0:
-		*seqno = &layout->os_area.msg_seqno_0;
-		key = layout->vmpck0;
-		break;
-	case 1:
-		*seqno = &layout->os_area.msg_seqno_1;
-		key = layout->vmpck1;
-		break;
-	case 2:
-		*seqno = &layout->os_area.msg_seqno_2;
-		key = layout->vmpck2;
-		break;
-	case 3:
-		*seqno = &layout->os_area.msg_seqno_3;
-		key = layout->vmpck3;
-		break;
-	default:
-		break;
-	}
+	dev->vmpck_id = vmpck_id;
+	dev->vmpck = dev->layout->vmpck0 + vmpck_id * VMPCK_KEY_LEN;
+	dev->os_area_msg_seqno = &dev->layout->os_area.msg_seqno_0 + vmpck_id;
 
-	return key;
+	return true;
 }
 
 static int __init sev_guest_probe(struct platform_device *pdev)
@@ -682,8 +666,8 @@  static int __init sev_guest_probe(struct platform_device *pdev)
 		goto e_unmap;
 
 	ret = -EINVAL;
-	snp_dev->vmpck = get_vmpck(vmpck_id, layout, &snp_dev->os_area_msg_seqno);
-	if (!snp_dev->vmpck) {
+	snp_dev->layout = layout;
+	if (!snp_assign_vmpck(snp_dev, vmpck_id)) {
 		dev_err(dev, "invalid vmpck id %d\n", vmpck_id);
 		goto e_unmap;
 	}
@@ -697,7 +681,6 @@  static int __init sev_guest_probe(struct platform_device *pdev)
 	mutex_init(&snp_dev->cmd_mutex);
 	platform_set_drvdata(pdev, snp_dev);
 	snp_dev->dev = dev;
-	snp_dev->layout = layout;
 
 	/* Allocate the shared page used for the request and response message. */
 	snp_dev->request = alloc_shared_pages(dev, sizeof(struct snp_guest_msg));