On Fri, Nov 22, 2019 at 1:28 PM Brijesh Singh <brijesh.singh@xxxxxxx> wrote: > > > > On 11/22/19 3:24 PM, Jim Mattson wrote: > > On Fri, Nov 22, 2019 at 1:22 PM Brijesh Singh <brijesh.singh@xxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxx> > >>>>>> Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx> > >>>>>> Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx> > >>>>>> --- > >>>>>> 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:
