diff mbox series

[RFC,v2,04/12] i386/sev: initialize SNP context

Message ID 20210826222627.3556-5-michael.roth@amd.com
State New
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) support | expand

Commit Message

Michael Roth Aug. 26, 2021, 10:26 p.m. UTC 
From: Brijesh Singh <brijesh.singh@amd.com>

When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
the platform. The command checks whether SNP is enabled in the KVM, if
enabled then it allocates a new ASID from the SNP pool and calls the
firmware to initialize the all the resources.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 target/i386/sev-stub.c |  6 ++++++
 target/i386/sev.c      | 27 ++++++++++++++++++++++++---
 target/i386/sev_i386.h |  1 +
 3 files changed, 31 insertions(+), 3 deletions(-)

Comments

Dov Murik Sept. 5, 2021, 7:07 a.m. UTC | #1 
Hi Michael,

On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
> the platform. The command checks whether SNP is enabled in the KVM, if
> enabled then it allocates a new ASID from the SNP pool and calls the
> firmware to initialize the all the resources.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  target/i386/sev-stub.c |  6 ++++++
>  target/i386/sev.c      | 27 ++++++++++++++++++++++++---
>  target/i386/sev_i386.h |  1 +
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 0227cb5177..e4fb8e882e 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -81,3 +81,9 @@ sev_get_attestation_report(const char *mnonce, Error **errp)
>      error_setg(errp, "SEV is not available in this QEMU");
>      return NULL;
>  }
> +
> +bool
> +sev_snp_enabled(void)
> +{
> +    return false;
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index ba08b7d3ab..b8bd6ed9ea 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -614,12 +614,21 @@ sev_enabled(void)
>      return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON);
>  }
>  
> +bool
> +sev_snp_enabled(void)
> +{
> +    ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
> +
> +    return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_SNP_GUEST);
> +}
> +
>  bool
>  sev_es_enabled(void)
>  {
>      ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
>  
> -    return sev_enabled() && (SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
> +    return sev_snp_enabled() ||
> +            (sev_enabled() && SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
>  }
>  
>  uint64_t
> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      uint32_t ebx;
>      uint32_t host_cbitpos;
>      struct sev_user_data_status status = {};
> +    void *init_args = NULL;
>  
>      if (!sev_common) {
>          return 0;
> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      sev_common->api_major = status.api_major;
>      sev_common->api_minor = status.api_minor;

Not visible here in the context: the code here is using the
SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor.

I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a
struct sev_data_snp_platform_status (hmmm, I can't find the struct's
definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI
document).

My questions are:

1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init
an SNP guest?
2. Do we want to save some info like installed TCB version and reported
TCB version, and maybe other fields from SNP platform status?
3. Should we check the state field in the platform status?



>  
> -    if (sev_es_enabled()) {
> +    if (sev_snp_enabled()) {
> +        SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common);
> +        if (!kvm_kernel_irqchip_allowed()) {
> +            error_report("%s: SEV-SNP guests require in-kernel irqchip support",
> +                         __func__);

Most errors in this function use error_setg(errp, ...).  This should follow.


> +            goto err;
> +        }
> +
> +        cmd = KVM_SEV_SNP_INIT;
> +        init_args = (void *)&sev_snp_guest->kvm_init_conf;
> +
> +    } else if (sev_es_enabled()) {
>          if (!kvm_kernel_irqchip_allowed()) {
>              error_report("%s: SEV-ES guests require in-kernel irqchip support",
>                           __func__);

Not part of this patch, but this error_report (and another one in the
SEV-ES case) should be converted to error_setg similarly.  Maybe add a
separate patch for fixing this for SEV-ES.



> @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      }
>  
>      trace_kvm_sev_init();

Suggestions:

1. log the guest type (SEV / SEV-ES / SEV-SNP)
2. log the SNP init flags value when initializing an SNP guest


-Dov

> -    ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
> +    ret = sev_ioctl(sev_common->sev_fd, cmd, init_args, &fw_error);
>      if (ret) {
>          error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
>                     __func__, ret, fw_error, fw_error_to_str(fw_error));
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index ae6d840478..e0e1a599be 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -29,6 +29,7 @@
>  #define SEV_POLICY_SEV          0x20
>  
>  extern bool sev_es_enabled(void);
> +extern bool sev_snp_enabled(void);
>  extern uint64_t sev_get_me_mask(void);
>  extern SevInfo *sev_get_info(void);
>  extern uint32_t sev_get_cbit_position(void);
>
Dov Murik Sept. 5, 2021, 9:19 a.m. UTC | #2 
On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
> the platform. The command checks whether SNP is enabled in the KVM, if
> enabled then it allocates a new ASID from the SNP pool and calls the
> firmware to initialize the all the resources.
> 


From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add
KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not*
allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START.

If that is indeed the case, I suggest removing this sentence here and
adding it in the appropriate QEMU step (patch 5?).

-Dov



> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  target/i386/sev-stub.c |  6 ++++++
>  target/i386/sev.c      | 27 ++++++++++++++++++++++++---
>  target/i386/sev_i386.h |  1 +
>  3 files changed, 31 insertions(+), 3 deletions(-)
> 

[...]
Brijesh Singh Sept. 5, 2021, 1:58 p.m. UTC | #3 
Hi Dov,

On 9/5/21 2:07 AM, Dov Murik wrote:
...
>
>>  
>>  uint64_t
>> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>      uint32_t ebx;
>>      uint32_t host_cbitpos;
>>      struct sev_user_data_status status = {};
>> +    void *init_args = NULL;
>>  
>>      if (!sev_common) {
>>          return 0;
>> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>      sev_common->api_major = status.api_major;
>>      sev_common->api_minor = status.api_minor;
> Not visible here in the context: the code here is using the
> SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor.
>
> I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a
> struct sev_data_snp_platform_status (hmmm, I can't find the struct's
> definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI
> document).

The API version can be queries either through the SNP_PLATFORM_STATUS or
SEV_PLATFORM_STATUS and they both report the same info. As the
definition of the sev_data_platform_status is concerned it should be
defined in the kernel include/linux/psp-sev.h.


> My questions are:
>
> 1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init
> an SNP guest?

Yes, the legacy platform status command can be called on the SNP
initialized host.

I choose not to new command because we only care about the verison
string and that is available through either of these commands (SNP or
SEV platform status).

> 2. Do we want to save some info like installed TCB version and reported
> TCB version, and maybe other fields from SNP platform status?

If we decide to add a new QMP (query-sev-snp) then it makes sense to
export those fields so that a hypervisor console can give additional
information; But note that for the guest, all these are available in the
attestation report.


> 3. Should we check the state field in the platform status?
>
>
Good point, we could use the SNP platform status. I don't expect the
state to be different between the SNP platform_status and SEV
platform_status.


>>  
>> -    if (sev_es_enabled()) {
>> +    if (sev_snp_enabled()) {
>> +        SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common);
>> +        if (!kvm_kernel_irqchip_allowed()) {
>> +            error_report("%s: SEV-SNP guests require in-kernel irqchip support",
>> +                         __func__);
> Most errors in this function use error_setg(errp, ...).  This should follow.
>
>
>> +            goto err;
>> +        }
>> +
>> +        cmd = KVM_SEV_SNP_INIT;
>> +        init_args = (void *)&sev_snp_guest->kvm_init_conf;
>> +
>> +    } else if (sev_es_enabled()) {
>>          if (!kvm_kernel_irqchip_allowed()) {
>>              error_report("%s: SEV-ES guests require in-kernel irqchip support",
>>                           __func__);
> Not part of this patch, but this error_report (and another one in the
> SEV-ES case) should be converted to error_setg similarly.  Maybe add a
> separate patch for fixing this for SEV-ES.
>
>
>
>> @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>      }
>>  
>>      trace_kvm_sev_init();
> Suggestions:
>
> 1. log the guest type (SEV / SEV-ES / SEV-SNP)
> 2. log the SNP init flags value when initializing an SNP guest

Noted.

thanks
Brijesh Singh Sept. 5, 2021, 2:05 p.m. UTC | #4 
On 9/5/21 4:19 AM, Dov Murik wrote:
>
> On 27/08/2021 1:26, Michael Roth wrote:
>> From: Brijesh Singh <brijesh.singh@amd.com>
>>
>> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
>> the platform. The command checks whether SNP is enabled in the KVM, if
>> enabled then it allocates a new ASID from the SNP pool and calls the
>> firmware to initialize the all the resources.
>>
>
> From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add
> KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not*
> allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START.

Actually, the KVM_SNP_INIT does allocate the ASID. If you look at the
driver code then in switch state, the SNP_INIT fallthrough to SEV_INIT
which will call sev_guest_init(). The sev_guest_init() allocates a new
ASID.
https://github.com/AMDESE/linux/blob/bb9ba49cd9b749d5551aae295c091d8757153dd7/arch/x86/kvm/svm/sev.c#L255

The LAUNCH_START simply binds the ASID to a guest.

thanks
Dov Murik Sept. 5, 2021, 5:03 p.m. UTC | #5 
On 05/09/2021 17:05, Brijesh Singh wrote:
> 
> On 9/5/21 4:19 AM, Dov Murik wrote:
>>
>> On 27/08/2021 1:26, Michael Roth wrote:
>>> From: Brijesh Singh <brijesh.singh@amd.com>
>>>
>>> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize
>>> the platform. The command checks whether SNP is enabled in the KVM, if
>>> enabled then it allocates a new ASID from the SNP pool and calls the
>>> firmware to initialize the all the resources.
>>>
>>
>> From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add
>> KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not*
>> allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START.
> 
> Actually, the KVM_SNP_INIT does allocate the ASID. If you look at the
> driver code then in switch state, the SNP_INIT fallthrough to SEV_INIT
> which will call sev_guest_init(). The sev_guest_init() allocates a new
> ASID.
> https://github.com/AMDESE/linux/blob/bb9ba49cd9b749d5551aae295c091d8757153dd7/arch/x86/kvm/svm/sev.c#L255
> 
> The LAUNCH_START simply binds the ASID to a guest.

OK thank you for clearing this up.  So the kernel is choosing the new
ASID during the KVM_SNP_INIT ioctl, but doesn't "tell" the firmware
about it.  Then later in SNP_LAUNCH_START that integer (saved in the
kernel sev structure) is given to the firmware as an argument of the
SNP_LAUNCH_START (binding?).  Is this description correct?


-Dov
Dov Murik Sept. 5, 2021, 5:09 p.m. UTC | #6 
On 05/09/2021 16:58, Brijesh Singh wrote:
> Hi Dov,
> 
> On 9/5/21 2:07 AM, Dov Murik wrote:
> ...
>>
>>>  
>>>  uint64_t
>>> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>>      uint32_t ebx;
>>>      uint32_t host_cbitpos;
>>>      struct sev_user_data_status status = {};
>>> +    void *init_args = NULL;
>>>  
>>>      if (!sev_common) {
>>>          return 0;
>>> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>>      sev_common->api_major = status.api_major;
>>>      sev_common->api_minor = status.api_minor;
>> Not visible here in the context: the code here is using the
>> SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor.
>>
>> I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a
>> struct sev_data_snp_platform_status (hmmm, I can't find the struct's
>> definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI
>> document).
> 
> The API version can be queries either through the SNP_PLATFORM_STATUS or
> SEV_PLATFORM_STATUS and they both report the same info. As the
> definition of the sev_data_platform_status is concerned it should be
> defined in the kernel include/linux/psp-sev.h.
> 
> 
>> My questions are:
>>
>> 1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init
>> an SNP guest?
> 
> Yes, the legacy platform status command can be called on the SNP
> initialized host.
> 
> I choose not to new command because we only care about the verison
> string and that is available through either of these commands (SNP or
> SEV platform status).
> 
>> 2. Do we want to save some info like installed TCB version and reported
>> TCB version, and maybe other fields from SNP platform status?
> 
> If we decide to add a new QMP (query-sev-snp) then it makes sense to
> export those fields so that a hypervisor console can give additional
> information; But note that for the guest, all these are available in the
> attestation report.
> 

We have new QMP response for SNP guests (SevSnpGuestProperties, patch 3
in this series).  I think it would make sense to add the
installed+reported TCB versions there (read-only properties), for
debugging/observability purposes.


-Dov
diff mbox series

Patch

diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 0227cb5177..e4fb8e882e 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -81,3 +81,9 @@  sev_get_attestation_report(const char *mnonce, Error **errp)
     error_setg(errp, "SEV is not available in this QEMU");
     return NULL;
 }
+
+bool
+sev_snp_enabled(void)
+{
+    return false;
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index ba08b7d3ab..b8bd6ed9ea 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -614,12 +614,21 @@  sev_enabled(void)
     return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON);
 }
 
+bool
+sev_snp_enabled(void)
+{
+    ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
+
+    return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_SNP_GUEST);
+}
+
 bool
 sev_es_enabled(void)
 {
     ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
 
-    return sev_enabled() && (SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
+    return sev_snp_enabled() ||
+            (sev_enabled() && SEV_GUEST(cgs)->policy & SEV_POLICY_ES);
 }
 
 uint64_t
@@ -1074,6 +1083,7 @@  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     uint32_t ebx;
     uint32_t host_cbitpos;
     struct sev_user_data_status status = {};
+    void *init_args = NULL;
 
     if (!sev_common) {
         return 0;
@@ -1126,7 +1136,18 @@  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     sev_common->api_major = status.api_major;
     sev_common->api_minor = status.api_minor;
 
-    if (sev_es_enabled()) {
+    if (sev_snp_enabled()) {
+        SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common);
+        if (!kvm_kernel_irqchip_allowed()) {
+            error_report("%s: SEV-SNP guests require in-kernel irqchip support",
+                         __func__);
+            goto err;
+        }
+
+        cmd = KVM_SEV_SNP_INIT;
+        init_args = (void *)&sev_snp_guest->kvm_init_conf;
+
+    } else if (sev_es_enabled()) {
         if (!kvm_kernel_irqchip_allowed()) {
             error_report("%s: SEV-ES guests require in-kernel irqchip support",
                          __func__);
@@ -1145,7 +1166,7 @@  int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     }
 
     trace_kvm_sev_init();
-    ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error);
+    ret = sev_ioctl(sev_common->sev_fd, cmd, init_args, &fw_error);
     if (ret) {
         error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'",
                    __func__, ret, fw_error, fw_error_to_str(fw_error));
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index ae6d840478..e0e1a599be 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -29,6 +29,7 @@ 
 #define SEV_POLICY_SEV          0x20
 
 extern bool sev_es_enabled(void);
+extern bool sev_snp_enabled(void);
 extern uint64_t sev_get_me_mask(void);
 extern SevInfo *sev_get_info(void);
 extern uint32_t sev_get_cbit_position(void);