Re: [PATCH v2 2/2] i386: Fix MCE support for AMD hosts

[John Allen]

On Thu, Aug 31, 2023 at 11:54:56PM +0200, William Roche wrote:
> John,
> 
> I also noticed something important about this specific code:
> 
> Qemu commit cb48748af7dfd7654323eb839d1f853ffa873652 introduced the use of
> the MCG_STATUS_RIPV in the case of a BUS_MCEERR_AR error, but it looks like
> your reference code is not having this flag.
> 
> According to me, we should keep this flag in the case of a non-AMD machine
> with a BUS_MCEERR_AR.
> 
> The patch should look something like that:
> 
> [...]
> -    if (code == BUS_MCEERR_AR) {
> -        status |= MCI_STATUS_AR | 0x134;
> -        mcg_status |= MCG_STATUS_RIPV | MCG_STATUS_EIPV;
> +    if (!IS_AMD_CPU(env)) {
> +        status |= MCI_STATUS_S;
> +        if (code == BUS_MCEERR_AR) {
> +            status |= MCI_STATUS_AR | 0x134;
> +            mcg_status |= MCG_STATUS_RIPV | MCG_STATUS_EIPV;
> [...]

Yes, that looks correct. I will fix this in the next version of the
series.

Thanks,
John

> 
> 
> Cheers,
> William.
> 
> 
> On 7/26/23 22:41, John Allen wrote:
> > For the most part, AMD hosts can use the same MCE injection code as Intel 
> > but,
> > there are instances where the qemu implementation is Intel specific. First, 
> > MCE
> > deliviery works differently on AMD and does not support broadcast. Second,
> > kvm_mce_inject generates MCEs that include a number of Intel specific status
> > bits. Modify kvm_mce_inject to properly generate MCEs on AMD platforms.
> > 
> > Reported-by: William Roche <william.roche@oracle.com>
> > Signed-off-by: John Allen <john.allen@amd.com>
> > ---
> >   target/i386/helper.c  |  4 ++++
> >   target/i386/kvm/kvm.c | 17 +++++++++++------
> >   2 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/target/i386/helper.c b/target/i386/helper.c
> > index 533b29cb91..a6523858e0 100644
> > --- a/target/i386/helper.c
> > +++ b/target/i386/helper.c
> > @@ -76,6 +76,10 @@ int cpu_x86_support_mca_broadcast(CPUX86State *env)
> >       int family = 0;
> >       int model = 0;
> > +    if (IS_AMD_CPU(env)) {
> > +        return 0;
> > +    }
> > +
> >       cpu_x86_version(env, &family, &model);
> >       if ((family == 6 && model >= 14) || family > 6) {
> >           return 1;
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index 4b62138459..87a50c8aaf 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -532,16 +532,21 @@ static void kvm_mce_inject(X86CPU *cpu, hwaddr paddr, 
> > int code)
> >       CPUState *cs = CPU(cpu);
> >       CPUX86State *env = &cpu->env;
> >       uint64_t status = MCI_STATUS_VAL | MCI_STATUS_UC | MCI_STATUS_EN |
> > -                      MCI_STATUS_MISCV | MCI_STATUS_ADDRV | MCI_STATUS_S;
> > +                      MCI_STATUS_MISCV | MCI_STATUS_ADDRV;
> >       uint64_t mcg_status = MCG_STATUS_MCIP;
> >       int flags = 0;
> > -    if (code == BUS_MCEERR_AR) {
> > -        status |= MCI_STATUS_AR | 0x134;
> > -        mcg_status |= MCG_STATUS_EIPV;
> > +    if (!IS_AMD_CPU(env)) {
> > +        status |= MCI_STATUS_S;
> > +        if (code == BUS_MCEERR_AR) {
> > +            status |= MCI_STATUS_AR | 0x134;
> > +            mcg_status |= MCG_STATUS_EIPV;
> > +        } else {
> > +            status |= 0xc0;
> > +            mcg_status |= MCG_STATUS_RIPV;
> > +        }
> >       } else {
> > -        status |= 0xc0;
> > -        mcg_status |= MCG_STATUS_RIPV;
> > +        mcg_status |= MCG_STATUS_EIPV | MCG_STATUS_RIPV;
> >       }
> >       flags = cpu_x86_support_mca_broadcast(env) ? MCE_INJECT_BROADCAST : 0;