[PATCH 0/2] Limit memory encryption cpuid pass through

[PATCH 0/2] Limit memory encryption cpuid pass through

[Peter Gonda]

KVM_GET_SUPPORTED_CPUID for 0x8000001F currently passes through all data if
X86_FEATURE_SEV is enabled. Guests only need the SEV bit and Cbit location
to work correctly. This series moves handing of this cpuid function out of
svm.c to the general x86 function and masks out host data.

Peter Gonda (2):
  KVM x86: Move kvm cpuid support out of svm
  KVM x86: Mask memory encryption guest cpuid

 arch/x86/kvm/cpuid.c | 11 +++++++++++
 arch/x86/kvm/svm.c   |  7 -------
 2 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 1/2] KVM x86: Move kvm cpuid support out of svm

[Peter Gonda]

Memory encryption support does not have module parameter dependencies
and can be moved into the general x86 cpuid __do_cpuid_ent function.
This changes maintains current behavior of passing through all of
CPUID.8000001F.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 5 +++++
 arch/x86/kvm/svm.c   | 7 -------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index f68c0c753c38..946fa9cb9dd6 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -778,6 +778,11 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 0x8000001a:
 	case 0x8000001e:
 		break;
+	/* Support memory encryption cpuid if host supports it */
+	case 0x8000001F:
+		if (!boot_cpu_has(X86_FEATURE_SEV))
+			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		break;
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
 		/*Just support up to 0xC0000004 now*/
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index c5673bda4b66..79842329ebcd 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5936,13 +5936,6 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
 		if (npt_enabled)
 			entry->edx |= F(NPT);
 
-		break;
-	case 0x8000001F:
-		/* Support memory encryption cpuid if host supports it */
-		if (boot_cpu_has(X86_FEATURE_SEV))
-			cpuid(0x8000001f, &entry->eax, &entry->ebx,
-				&entry->ecx, &entry->edx);
-
 	}
 }
 
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Peter Gonda]

Only pass through guest relevant CPUID information: Cbit location and
SEV bit. The kernel does not support nested SEV guests so the other data
in this CPUID leaf is unneeded by the guest.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Peter Gonda <pgonda@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 946fa9cb9dd6..6439fb1dbe76 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
 		break;
 	/* Support memory encryption cpuid if host supports it */
 	case 0x8000001F:
-		if (!boot_cpu_has(X86_FEATURE_SEV))
+		if (boot_cpu_has(X86_FEATURE_SEV)) {
+			/* Expose only SEV bit and CBit location */
+			entry->eax &= F(SEV);
+			entry->ebx &= GENMASK(5, 0);
+			entry->edx = entry->ecx = 0;
+		} else {
 			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
+		}
 		break;
 	/*Add support for Centaur's CPUID instruction*/
 	case 0xC0000000:
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 1/2] KVM x86: Move kvm cpuid support out of svm

[Krish Sadhukhan]

On 11/21/2019 12:33 PM, Peter Gonda wrote:
> Memory encryption support does not have module parameter dependencies
> and can be moved into the general x86 cpuid __do_cpuid_ent function.
> This changes maintains current behavior of passing through all of
> CPUID.8000001F.
>
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
>   arch/x86/kvm/cpuid.c | 5 +++++
>   arch/x86/kvm/svm.c   | 7 -------
>   2 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index f68c0c753c38..946fa9cb9dd6 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -778,6 +778,11 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>   	case 0x8000001a:
>   	case 0x8000001e:
>   		break;
> +	/* Support memory encryption cpuid if host supports it */
> +	case 0x8000001F:
> +		if (!boot_cpu_has(X86_FEATURE_SEV))
> +			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> +		break;
>   	/*Add support for Centaur's CPUID instruction*/
>   	case 0xC0000000:
>   		/*Just support up to 0xC0000004 now*/
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index c5673bda4b66..79842329ebcd 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5936,13 +5936,6 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
>   		if (npt_enabled)
>   			entry->edx |= F(NPT);
>   
> -		break;
> -	case 0x8000001F:
> -		/* Support memory encryption cpuid if host supports it */
> -		if (boot_cpu_has(X86_FEATURE_SEV))
> -			cpuid(0x8000001f, &entry->eax, &entry->ebx,
> -				&entry->ecx, &entry->edx);
> -
>   	}
>   }
>   
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Brijesh Singh]

On 11/21/19 2:33 PM, Peter Gonda wrote:
> Only pass through guest relevant CPUID information: Cbit location and
> SEV bit. The kernel does not support nested SEV guests so the other data
> in this CPUID leaf is unneeded by the guest.
>
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 946fa9cb9dd6..6439fb1dbe76 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>  		break;
>  	/* Support memory encryption cpuid if host supports it */
>  	case 0x8000001F:
> -		if (!boot_cpu_has(X86_FEATURE_SEV))
> +		if (boot_cpu_has(X86_FEATURE_SEV)) {
> +			/* Expose only SEV bit and CBit location */
> +			entry->eax &= F(SEV);


I know SEV-ES patches are not accepted yet, but can I ask to pass the
SEV-ES bit in eax?


> +			entry->ebx &= GENMASK(5, 0);
> +			entry->edx = entry->ecx = 0;
> +		} else {
>  			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> +		}
>  		break;
>  	/*Add support for Centaur's CPUID instruction*/
>  	case 0xC0000000:

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Paolo Bonzini]

On 22/11/19 14:01, Brijesh Singh wrote:
> 
> On 11/21/19 2:33 PM, Peter Gonda wrote:
>> Only pass through guest relevant CPUID information: Cbit location and
>> SEV bit. The kernel does not support nested SEV guests so the other data
>> in this CPUID leaf is unneeded by the guest.
>>
>> Suggested-by: Jim Mattson <jmattson@google.com>
>> Signed-off-by: Peter Gonda <pgonda@google.com>
>> Reviewed-by: Jim Mattson <jmattson@google.com>
>> ---
>>  arch/x86/kvm/cpuid.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 946fa9cb9dd6..6439fb1dbe76 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>>  		break;
>>  	/* Support memory encryption cpuid if host supports it */
>>  	case 0x8000001F:
>> -		if (!boot_cpu_has(X86_FEATURE_SEV))
>> +		if (boot_cpu_has(X86_FEATURE_SEV)) {
>> +			/* Expose only SEV bit and CBit location */
>> +			entry->eax &= F(SEV);
> 
> 
> I know SEV-ES patches are not accepted yet, but can I ask to pass the
> SEV-ES bit in eax?

I think it shouldn't be passed, since KVM does not support SEV-ES.

Paolo

> 
>> +			entry->ebx &= GENMASK(5, 0);
>> +			entry->edx = entry->ecx = 0;
>> +		} else {
>>  			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>> +		}
>>  		break;
>>  	/*Add support for Centaur's CPUID instruction*/
>>  	case 0xC0000000:
> 

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Brijesh Singh]

On 11/22/19 7:46 AM, Paolo Bonzini wrote:
> On 22/11/19 14:01, Brijesh Singh wrote:
>>
>> On 11/21/19 2:33 PM, Peter Gonda wrote:
>>> Only pass through guest relevant CPUID information: Cbit location and
>>> SEV bit. The kernel does not support nested SEV guests so the other data
>>> in this CPUID leaf is unneeded by the guest.
>>>
>>> Suggested-by: Jim Mattson <jmattson@google.com>
>>> Signed-off-by: Peter Gonda <pgonda@google.com>
>>> Reviewed-by: Jim Mattson <jmattson@google.com>
>>> ---
>>>   arch/x86/kvm/cpuid.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>> index 946fa9cb9dd6..6439fb1dbe76 100644
>>> --- a/arch/x86/kvm/cpuid.c
>>> +++ b/arch/x86/kvm/cpuid.c
>>> @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>>>   		break;
>>>   	/* Support memory encryption cpuid if host supports it */
>>>   	case 0x8000001F:
>>> -		if (!boot_cpu_has(X86_FEATURE_SEV))
>>> +		if (boot_cpu_has(X86_FEATURE_SEV)) {
>>> +			/* Expose only SEV bit and CBit location */
>>> +			entry->eax &= F(SEV);
>>
>>
>> I know SEV-ES patches are not accepted yet, but can I ask to pass the
>> SEV-ES bit in eax?
> 
> I think it shouldn't be passed, since KVM does not support SEV-ES.
> 

Fair enough.

-Brijesh

Re: [PATCH 0/2] Limit memory encryption cpuid pass through

[Brijesh Singh]

On 11/21/19 2:33 PM, Peter Gonda wrote:
> KVM_GET_SUPPORTED_CPUID for 0x8000001F currently passes through all data if
> X86_FEATURE_SEV is enabled. Guests only need the SEV bit and Cbit location
> to work correctly. This series moves handing of this cpuid function out of
> svm.c to the general x86 function and masks out host data.
> 
> Peter Gonda (2):
>    KVM x86: Move kvm cpuid support out of svm
>    KVM x86: Mask memory encryption guest cpuid
> 
>   arch/x86/kvm/cpuid.c | 11 +++++++++++
>   arch/x86/kvm/svm.c   |  7 -------
>   2 files changed, 11 insertions(+), 7 deletions(-)
> 

Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Jim Mattson]

Does SEV-ES indicate that SEV-ES guests are supported, or that the
current (v)CPU is running with SEV-ES enabled, or both?

On Fri, Nov 22, 2019 at 5:01 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
>
> On 11/21/19 2:33 PM, Peter Gonda wrote:
> > Only pass through guest relevant CPUID information: Cbit location and
> > SEV bit. The kernel does not support nested SEV guests so the other data
> > in this CPUID leaf is unneeded by the guest.
> >
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > Reviewed-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 946fa9cb9dd6..6439fb1dbe76 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
> >               break;
> >       /* Support memory encryption cpuid if host supports it */
> >       case 0x8000001F:
> > -             if (!boot_cpu_has(X86_FEATURE_SEV))
> > +             if (boot_cpu_has(X86_FEATURE_SEV)) {
> > +                     /* Expose only SEV bit and CBit location */
> > +                     entry->eax &= F(SEV);
>
>
> I know SEV-ES patches are not accepted yet, but can I ask to pass the
> SEV-ES bit in eax?
>
>
> > +                     entry->ebx &= GENMASK(5, 0);
> > +                     entry->edx = entry->ecx = 0;
> > +             } else {
> >                       entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> > +             }
> >               break;
> >       /*Add support for Centaur's CPUID instruction*/
> >       case 0xC0000000:

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Peter Gonda]

I am not sure that the SevEs CPUID bit has the same problem as the Sev
bit. It seems the reason the Sev bit was to be passed to the guest was
to prevent the guest from reading the SEV MSR if it did not exist. If
the guest is running with SevEs it must be also running with Sev. So
the guest  can safely read the SevStatus MSR to check the SevEsEnabled
bit because the Sev CPUID bit will be set.

If I look at the AMD patches for ES. I see just that,
https://github.com/AMDESE/linux/commit/c19d84b803caf8e3130b1498868d0fcafc755da7,
it doesn't look for the SevEs CPUID bit.

} else {
  /* For SEV, check the SEV MSR */
  msr = __rdmsr(MSR_AMD64_SEV);
  if (!(msr & MSR_AMD64_SEV_ENABLED))
    return;
  /* SEV state cannot be controlled by a command line option */
  sme_me_mask = me_mask;
  sme_me_status |= SEV_ACTIVE;
  physical_mask &= ~sme_me_mask;
+
+  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
+    return;
+
+  sme_me_status |= SEV_ES_ACTIVE;
  return;
}

}


On Fri, Nov 22, 2019 at 9:18 AM Jim Mattson <jmattson@google.com> wrote:
>
> Does SEV-ES indicate that SEV-ES guests are supported, or that the
> current (v)CPU is running with SEV-ES enabled, or both?
>
> On Fri, Nov 22, 2019 at 5:01 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
> >
> >
> > On 11/21/19 2:33 PM, Peter Gonda wrote:
> > > Only pass through guest relevant CPUID information: Cbit location and
> > > SEV bit. The kernel does not support nested SEV guests so the other data
> > > in this CPUID leaf is unneeded by the guest.
> > >
> > > Suggested-by: Jim Mattson <jmattson@google.com>
> > > Signed-off-by: Peter Gonda <pgonda@google.com>
> > > Reviewed-by: Jim Mattson <jmattson@google.com>
> > > ---
> > >  arch/x86/kvm/cpuid.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 946fa9cb9dd6..6439fb1dbe76 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
> > >               break;
> > >       /* Support memory encryption cpuid if host supports it */
> > >       case 0x8000001F:
> > > -             if (!boot_cpu_has(X86_FEATURE_SEV))
> > > +             if (boot_cpu_has(X86_FEATURE_SEV)) {
> > > +                     /* Expose only SEV bit and CBit location */
> > > +                     entry->eax &= F(SEV);
> >
> >
> > I know SEV-ES patches are not accepted yet, but can I ask to pass the
> > SEV-ES bit in eax?
> >
> >
> > > +                     entry->ebx &= GENMASK(5, 0);
> > > +                     entry->edx = entry->ecx = 0;
> > > +             } else {
> > >                       entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> > > +             }
> > >               break;
> > >       /*Add support for Centaur's CPUID instruction*/
> > >       case 0xC0000000:

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Brijesh Singh]

Ah, I missed the fact that we don't need to pass the SevES
bit to the guest because guest actually does not need it.
It just needs the SevBit to make decision whether its
safe to call the RDMSR for SEV_STATUS. The SEV_STATUS
MSR will give information which SEV feature is enabled.

thanks

On 11/22/19 1:52 PM, Peter Gonda wrote:
> I am not sure that the SevEs CPUID bit has the same problem as the Sev
> bit. It seems the reason the Sev bit was to be passed to the guest was
> to prevent the guest from reading the SEV MSR if it did not exist. If
> the guest is running with SevEs it must be also running with Sev. So
> the guest  can safely read the SevStatus MSR to check the SevEsEnabled
> bit because the Sev CPUID bit will be set.
> 
> If I look at the AMD patches for ES. I see just that,
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&data=02%7C01%7Cbrijesh.singh%40amd.com%7Cfe5a46e348a5464ea52b08d76f85909b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100491764446005&sdata=R6RrRO0TpcfM7uzpBbsGhVp47bA%2BVoz624IBQif%2BxjA%3D&reserved=0,
> it doesn't look for the SevEs CPUID bit.
> 
> } else {
>    /* For SEV, check the SEV MSR */
>    msr = __rdmsr(MSR_AMD64_SEV);
>    if (!(msr & MSR_AMD64_SEV_ENABLED))
>      return;
>    /* SEV state cannot be controlled by a command line option */
>    sme_me_mask = me_mask;
>    sme_me_status |= SEV_ACTIVE;
>    physical_mask &= ~sme_me_mask;
> +
> +  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
> +    return;
> +
> +  sme_me_status |= SEV_ES_ACTIVE;
>    return;
> }
> 
> }
> 
> 
> On Fri, Nov 22, 2019 at 9:18 AM Jim Mattson <jmattson@google.com> wrote:
>>
>> Does SEV-ES indicate that SEV-ES guests are supported, or that the
>> current (v)CPU is running with SEV-ES enabled, or both?
>>
>> On Fri, Nov 22, 2019 at 5:01 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>>>
>>>
>>> On 11/21/19 2:33 PM, Peter Gonda wrote:
>>>> Only pass through guest relevant CPUID information: Cbit location and
>>>> SEV bit. The kernel does not support nested SEV guests so the other data
>>>> in this CPUID leaf is unneeded by the guest.
>>>>
>>>> Suggested-by: Jim Mattson <jmattson@google.com>
>>>> Signed-off-by: Peter Gonda <pgonda@google.com>
>>>> Reviewed-by: Jim Mattson <jmattson@google.com>
>>>> ---
>>>>   arch/x86/kvm/cpuid.c | 8 +++++++-
>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>>> index 946fa9cb9dd6..6439fb1dbe76 100644
>>>> --- a/arch/x86/kvm/cpuid.c
>>>> +++ b/arch/x86/kvm/cpuid.c
>>>> @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>>>>                break;
>>>>        /* Support memory encryption cpuid if host supports it */
>>>>        case 0x8000001F:
>>>> -             if (!boot_cpu_has(X86_FEATURE_SEV))
>>>> +             if (boot_cpu_has(X86_FEATURE_SEV)) {
>>>> +                     /* Expose only SEV bit and CBit location */
>>>> +                     entry->eax &= F(SEV);
>>>
>>>
>>> I know SEV-ES patches are not accepted yet, but can I ask to pass the
>>> SEV-ES bit in eax?
>>>
>>>
>>>> +                     entry->ebx &= GENMASK(5, 0);
>>>> +                     entry->edx = entry->ecx = 0;
>>>> +             } else {
>>>>                        entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>>>> +             }
>>>>                break;
>>>>        /*Add support for Centaur's CPUID instruction*/
>>>>        case 0xC0000000:

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Jim Mattson]

On Fri, Nov 22, 2019 at 1:22 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
> Ah, I missed the fact that we don't need to pass the SevES
> bit to the guest because guest actually does not need it.
> It just needs the SevBit to make decision whether its
> safe to call the RDMSR for SEV_STATUS. The SEV_STATUS
> MSR will give information which SEV feature is enabled.

Why does it have to be safe to read the SEV_STATUS MSR? We read
nonexistent MSRs all the time.

> thanks
>
> On 11/22/19 1:52 PM, Peter Gonda wrote:
> > I am not sure that the SevEs CPUID bit has the same problem as the Sev
> > bit. It seems the reason the Sev bit was to be passed to the guest was
> > to prevent the guest from reading the SEV MSR if it did not exist. If
> > the guest is running with SevEs it must be also running with Sev. So
> > the guest  can safely read the SevStatus MSR to check the SevEsEnabled
> > bit because the Sev CPUID bit will be set.
> >
> > If I look at the AMD patches for ES. I see just that,
> > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&amp;data=02%7C01%7Cbrijesh.singh%40amd.com%7Cfe5a46e348a5464ea52b08d76f85909b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100491764446005&amp;sdata=R6RrRO0TpcfM7uzpBbsGhVp47bA%2BVoz624IBQif%2BxjA%3D&amp;reserved=0,
> > it doesn't look for the SevEs CPUID bit.
> >
> > } else {
> >    /* For SEV, check the SEV MSR */
> >    msr = __rdmsr(MSR_AMD64_SEV);
> >    if (!(msr & MSR_AMD64_SEV_ENABLED))
> >      return;
> >    /* SEV state cannot be controlled by a command line option */
> >    sme_me_mask = me_mask;
> >    sme_me_status |= SEV_ACTIVE;
> >    physical_mask &= ~sme_me_mask;
> > +
> > +  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
> > +    return;
> > +
> > +  sme_me_status |= SEV_ES_ACTIVE;
> >    return;
> > }
> >
> > }
> >
> >
> > On Fri, Nov 22, 2019 at 9:18 AM Jim Mattson <jmattson@google.com> wrote:
> >>
> >> Does SEV-ES indicate that SEV-ES guests are supported, or that the
> >> current (v)CPU is running with SEV-ES enabled, or both?
> >>
> >> On Fri, Nov 22, 2019 at 5:01 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
> >>>
> >>>
> >>> On 11/21/19 2:33 PM, Peter Gonda wrote:
> >>>> Only pass through guest relevant CPUID information: Cbit location and
> >>>> SEV bit. The kernel does not support nested SEV guests so the other data
> >>>> in this CPUID leaf is unneeded by the guest.
> >>>>
> >>>> Suggested-by: Jim Mattson <jmattson@google.com>
> >>>> Signed-off-by: Peter Gonda <pgonda@google.com>
> >>>> Reviewed-by: Jim Mattson <jmattson@google.com>
> >>>> ---
> >>>>   arch/x86/kvm/cpuid.c | 8 +++++++-
> >>>>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> >>>> index 946fa9cb9dd6..6439fb1dbe76 100644
> >>>> --- a/arch/x86/kvm/cpuid.c
> >>>> +++ b/arch/x86/kvm/cpuid.c
> >>>> @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
> >>>>                break;
> >>>>        /* Support memory encryption cpuid if host supports it */
> >>>>        case 0x8000001F:
> >>>> -             if (!boot_cpu_has(X86_FEATURE_SEV))
> >>>> +             if (boot_cpu_has(X86_FEATURE_SEV)) {
> >>>> +                     /* Expose only SEV bit and CBit location */
> >>>> +                     entry->eax &= F(SEV);
> >>>
> >>>
> >>> I know SEV-ES patches are not accepted yet, but can I ask to pass the
> >>> SEV-ES bit in eax?
> >>>
> >>>
> >>>> +                     entry->ebx &= GENMASK(5, 0);
> >>>> +                     entry->edx = entry->ecx = 0;
> >>>> +             } else {
> >>>>                        entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> >>>> +             }
> >>>>                break;
> >>>>        /*Add support for Centaur's CPUID instruction*/
> >>>>        case 0xC0000000:

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Brijesh Singh]

On 11/22/19 3:24 PM, Jim Mattson wrote:
> On Fri, Nov 22, 2019 at 1:22 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>>
>> Ah, I missed the fact that we don't need to pass the SevES
>> bit to the guest because guest actually does not need it.
>> It just needs the SevBit to make decision whether its
>> safe to call the RDMSR for SEV_STATUS. The SEV_STATUS
>> MSR will give information which SEV feature is enabled.
> 
> Why does it have to be safe to read the SEV_STATUS MSR? We read
> nonexistent MSRs all the time.
> 

The MSR access happens very early in the boot, IIRC calling this MSR on
non AMD platform may result in #GP. If OS is not ready to handle the
#GP so early then we will have problem.



>> thanks
>>
>> On 11/22/19 1:52 PM, Peter Gonda wrote:
>>> I am not sure that the SevEs CPUID bit has the same problem as the Sev
>>> bit. It seems the reason the Sev bit was to be passed to the guest was
>>> to prevent the guest from reading the SEV MSR if it did not exist. If
>>> the guest is running with SevEs it must be also running with Sev. So
>>> the guest  can safely read the SevStatus MSR to check the SevEsEnabled
>>> bit because the Sev CPUID bit will be set.
>>>
>>> If I look at the AMD patches for ES. I see just that,
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&amp;data=02%7C01%7Cbrijesh.singh%40amd.com%7C86545e99d62e4f8e8eb508d76f92720c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100547082927245&amp;sdata=5YsknSUmboS95T0OfLWvJ%2BcOQQk5sIllGfshNqf0j6Y%3D&amp;reserved=0,
>>> it doesn't look for the SevEs CPUID bit.
>>>
>>> } else {
>>>     /* For SEV, check the SEV MSR */
>>>     msr = __rdmsr(MSR_AMD64_SEV);
>>>     if (!(msr & MSR_AMD64_SEV_ENABLED))
>>>       return;
>>>     /* SEV state cannot be controlled by a command line option */
>>>     sme_me_mask = me_mask;
>>>     sme_me_status |= SEV_ACTIVE;
>>>     physical_mask &= ~sme_me_mask;
>>> +
>>> +  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
>>> +    return;
>>> +
>>> +  sme_me_status |= SEV_ES_ACTIVE;
>>>     return;
>>> }
>>>
>>> }
>>>
>>>
>>> On Fri, Nov 22, 2019 at 9:18 AM Jim Mattson <jmattson@google.com> wrote:
>>>>
>>>> Does SEV-ES indicate that SEV-ES guests are supported, or that the
>>>> current (v)CPU is running with SEV-ES enabled, or both?
>>>>
>>>> On Fri, Nov 22, 2019 at 5:01 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
>>>>>
>>>>>
>>>>> On 11/21/19 2:33 PM, Peter Gonda wrote:
>>>>>> Only pass through guest relevant CPUID information: Cbit location and
>>>>>> SEV bit. The kernel does not support nested SEV guests so the other data
>>>>>> in this CPUID leaf is unneeded by the guest.
>>>>>>
>>>>>> Suggested-by: Jim Mattson <jmattson@google.com>
>>>>>> Signed-off-by: Peter Gonda <pgonda@google.com>
>>>>>> Reviewed-by: Jim Mattson <jmattson@google.com>
>>>>>> ---
>>>>>>    arch/x86/kvm/cpuid.c | 8 +++++++-
>>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>>>>>> index 946fa9cb9dd6..6439fb1dbe76 100644
>>>>>> --- a/arch/x86/kvm/cpuid.c
>>>>>> +++ b/arch/x86/kvm/cpuid.c
>>>>>> @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>>>>>>                 break;
>>>>>>         /* Support memory encryption cpuid if host supports it */
>>>>>>         case 0x8000001F:
>>>>>> -             if (!boot_cpu_has(X86_FEATURE_SEV))
>>>>>> +             if (boot_cpu_has(X86_FEATURE_SEV)) {
>>>>>> +                     /* Expose only SEV bit and CBit location */
>>>>>> +                     entry->eax &= F(SEV);
>>>>>
>>>>>
>>>>> I know SEV-ES patches are not accepted yet, but can I ask to pass the
>>>>> SEV-ES bit in eax?
>>>>>
>>>>>
>>>>>> +                     entry->ebx &= GENMASK(5, 0);
>>>>>> +                     entry->edx = entry->ecx = 0;
>>>>>> +             } else {
>>>>>>                         entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>>>>>> +             }
>>>>>>                 break;
>>>>>>         /*Add support for Centaur's CPUID instruction*/
>>>>>>         case 0xC0000000:

Re: [PATCH 2/2] KVM x86: Mask memory encryption guest cpuid

[Jim Mattson]

On Fri, Nov 22, 2019 at 1:28 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
>
>
>
> On 11/22/19 3:24 PM, Jim Mattson wrote:
> > On Fri, Nov 22, 2019 at 1:22 PM Brijesh Singh <brijesh.singh@amd.com> wrote:
> >>
> >> Ah, I missed the fact that we don't need to pass the SevES
> >> bit to the guest because guest actually does not need it.
> >> It just needs the SevBit to make decision whether its
> >> safe to call the RDMSR for SEV_STATUS. The SEV_STATUS
> >> MSR will give information which SEV feature is enabled.
> >
> > Why does it have to be safe to read the SEV_STATUS MSR? We read
> > nonexistent MSRs all the time.
> >
>
> The MSR access happens very early in the boot, IIRC calling this MSR on
> non AMD platform may result in #GP. If OS is not ready to handle the
> #GP so early then we will have problem.

Ah. So, the SEV CPUID bit simply indicates the presence of the
SEV_STATUS MSR. Nothing more, nothing less.

>
>
> >> thanks
> >>
> >> On 11/22/19 1:52 PM, Peter Gonda wrote:
> >>> I am not sure that the SevEs CPUID bit has the same problem as the Sev
> >>> bit. It seems the reason the Sev bit was to be passed to the guest was
> >>> to prevent the guest from reading the SEV MSR if it did not exist. If
> >>> the guest is running with SevEs it must be also running with Sev. So
> >>> the guest  can safely read the SevStatus MSR to check the SevEsEnabled
> >>> bit because the Sev CPUID bit will be set.
> >>>
> >>> If I look at the AMD patches for ES. I see just that,
> >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Flinux%2Fcommit%2Fc19d84b803caf8e3130b1498868d0fcafc755da7&amp;data=02%7C01%7Cbrijesh.singh%40amd.com%7C86545e99d62e4f8e8eb508d76f92720c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100547082927245&amp;sdata=5YsknSUmboS95T0OfLWvJ%2BcOQQk5sIllGfshNqf0j6Y%3D&amp;reserved=0,
> >>> it doesn't look for the SevEs CPUID bit.
> >>>
> >>> } else {
> >>>     /* For SEV, check the SEV MSR */
> >>>     msr = __rdmsr(MSR_AMD64_SEV);
> >>>     if (!(msr & MSR_AMD64_SEV_ENABLED))
> >>>       return;
> >>>     /* SEV state cannot be controlled by a command line option */
> >>>     sme_me_mask = me_mask;
> >>>     sme_me_status |= SEV_ACTIVE;
> >>>     physical_mask &= ~sme_me_mask;
> >>> +
> >>> +  if (!(msr & MSR_AMD64_SEV_ES_ENABLED))
> >>> +    return;
> >>> +
> >>> +  sme_me_status |= SEV_ES_ACTIVE;
> >>>     return;
> >>> }
> >>>
> >>> }
> >>>
> >>>
> >>> On Fri, Nov 22, 2019 at 9:18 AM Jim Mattson <jmattson@google.com> wrote:
> >>>>
> >>>> Does SEV-ES indicate that SEV-ES guests are supported, or that the
> >>>> current (v)CPU is running with SEV-ES enabled, or both?
> >>>>
> >>>> On Fri, Nov 22, 2019 at 5:01 AM Brijesh Singh <brijesh.singh@amd.com> wrote:
> >>>>>
> >>>>>
> >>>>> On 11/21/19 2:33 PM, Peter Gonda wrote:
> >>>>>> Only pass through guest relevant CPUID information: Cbit location and
> >>>>>> SEV bit. The kernel does not support nested SEV guests so the other data
> >>>>>> in this CPUID leaf is unneeded by the guest.
> >>>>>>
> >>>>>> Suggested-by: Jim Mattson <jmattson@google.com>
> >>>>>> Signed-off-by: Peter Gonda <pgonda@google.com>
> >>>>>> Reviewed-by: Jim Mattson <jmattson@google.com>
> >>>>>> ---
> >>>>>>    arch/x86/kvm/cpuid.c | 8 +++++++-
> >>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> >>>>>> index 946fa9cb9dd6..6439fb1dbe76 100644
> >>>>>> --- a/arch/x86/kvm/cpuid.c
> >>>>>> +++ b/arch/x86/kvm/cpuid.c
> >>>>>> @@ -780,8 +780,14 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
> >>>>>>                 break;
> >>>>>>         /* Support memory encryption cpuid if host supports it */
> >>>>>>         case 0x8000001F:
> >>>>>> -             if (!boot_cpu_has(X86_FEATURE_SEV))
> >>>>>> +             if (boot_cpu_has(X86_FEATURE_SEV)) {
> >>>>>> +                     /* Expose only SEV bit and CBit location */
> >>>>>> +                     entry->eax &= F(SEV);
> >>>>>
> >>>>>
> >>>>> I know SEV-ES patches are not accepted yet, but can I ask to pass the
> >>>>> SEV-ES bit in eax?
> >>>>>
> >>>>>
> >>>>>> +                     entry->ebx &= GENMASK(5, 0);
> >>>>>> +                     entry->edx = entry->ecx = 0;
> >>>>>> +             } else {
> >>>>>>                         entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> >>>>>> +             }
> >>>>>>                 break;
> >>>>>>         /*Add support for Centaur's CPUID instruction*/
> >>>>>>         case 0xC0000000:

Re: [PATCH 1/2] KVM x86: Move kvm cpuid support out of svm

[Paolo Bonzini]

On 21/11/19 21:33, Peter Gonda wrote:
> Memory encryption support does not have module parameter dependencies
> and can be moved into the general x86 cpuid __do_cpuid_ent function.
> This changes maintains current behavior of passing through all of
> CPUID.8000001F.
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 5 +++++
>  arch/x86/kvm/svm.c   | 7 -------
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index f68c0c753c38..946fa9cb9dd6 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -778,6 +778,11 @@ static inline int __do_cpuid_func(struct kvm_cpuid_entry2 *entry, u32 function,
>  	case 0x8000001a:
>  	case 0x8000001e:
>  		break;
> +	/* Support memory encryption cpuid if host supports it */
> +	case 0x8000001F:
> +		if (!boot_cpu_has(X86_FEATURE_SEV))
> +			entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
> +		break;
>  	/*Add support for Centaur's CPUID instruction*/
>  	case 0xC0000000:
>  		/*Just support up to 0xC0000004 now*/
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index c5673bda4b66..79842329ebcd 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5936,13 +5936,6 @@ static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
>  		if (npt_enabled)
>  			entry->edx |= F(NPT);
>  
> -		break;
> -	case 0x8000001F:
> -		/* Support memory encryption cpuid if host supports it */
> -		if (boot_cpu_has(X86_FEATURE_SEV))
> -			cpuid(0x8000001f, &entry->eax, &entry->ebx,
> -				&entry->ecx, &entry->edx);
> -
>  	}
>  }
>  
> 

Queued patch 1, only.

Paolo