On Thu, Jun 13, 2024 at 05:50:08PM +0800, Xiaoyao Li wrote:
> On 6/8/2024 4:34 PM, Paolo Bonzini wrote:
> > From: John Allen <john.al...@amd.com>
> >
> > Add cpuid bit definition for the SUCCOR feature. This cpuid bit is required
> > to
> > be exposed to guests to allow them to handle machine check exceptions on AMD
> > hosts.
> >
> > ----
> > v2:
> > - Add "succor" feature word.
> > - Add case to kvm_arch_get_supported_cpuid for the SUCCOR feature.
> >
> > Reported-by: William Roche <william.ro...@oracle.com>
> > Reviewed-by: Joao Martins <joao.m.mart...@oracle.com>
> > Signed-off-by: John Allen <john.al...@amd.com>
> > Message-ID: <20240603193622.47156-3-john.al...@amd.com>
> > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
>
> [snip]
> ...
>
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index 55a9e8a70cf..56d8e2a89ec 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -532,6 +532,8 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s,
> > uint32_t function,
> > */
> > cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
> > ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
> > + } else if (function == 0x80000007 && reg == R_EBX) {
> > + ret |= CPUID_8000_0007_EBX_SUCCOR;
>
> IMO, this is incorrect.
>
> Why make it supported unconditionally? Shouldn't there be a KVM patch to
> report it in KVM_GET_SUPPORTED_CPUID?
>
> If make it supported unconditionally, all VMs boot with "-cpu host/max" will
> see the CPUID bits, even if it is Intel VMs.
Paolo,
This change in kvm_arch_get_supported_cpuid was added based on your
suggestion here:
https://lore.kernel.org/all/d4c1bb9b-8438-ed00-c79d-e8ad2a7e4...@redhat.com/
Is there something missing from the patch needed to prevent the bits
from being seen on Intel VMs?
I am planning to send a patch early this week to report the bits for KVM
and a patch that removes the above change for qemu. Is there another way
you would prefer to handle it?
Thanks,
John
>
>
> > } else if (function == KVM_CPUID_FEATURES && reg == R_EAX) {
> > /* kvm_pv_unhalt is reported by GET_SUPPORTED_CPUID, but it can't
> > * be enabled without the in-kernel irqchip
>
