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

[Joao Martins <joao.m.martins@oracle.com>]

+x86 qemu folks

On 06/07/2023 20:40, 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 f25837f63f..63bd7a7d3a 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -530,16 +530,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;
>      }
>  

I was gonna say that we should only handle BUS_MCEERR_AR for AMD, but the way
you came up with, does seem to work from quick testing. And it's better to log
an error that silently ignore it obviously.

>      flags = cpu_x86_support_mca_broadcast(env) ? MCE_INJECT_BROADCAST : 0;