| Message ID | 20191121203344.156835-3-pgonda@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Limit memory encryption cpuid pass through | expand |
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:
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: >
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
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:
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:
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:
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&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:
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&data=02%7C01%7Cbrijesh.singh%40amd.com%7C86545e99d62e4f8e8eb508d76f92720c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100547082927245&sdata=5YsknSUmboS95T0OfLWvJ%2BcOQQk5sIllGfshNqf0j6Y%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:
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&data=02%7C01%7Cbrijesh.singh%40amd.com%7C86545e99d62e4f8e8eb508d76f92720c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637100547082927245&sdata=5YsknSUmboS95T0OfLWvJ%2BcOQQk5sIllGfshNqf0j6Y%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:
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: