Re: [Qemu-devel] [PATCH v2] x86: ioapic: conditionally upgrade IOAPIC version

[Paolo Bonzini]

On 26/07/2016 05:28, Peter Xu wrote:
> IOMMU IR and IOAPIC legacy devices (e.g., e1000) cannot work well
> together with some old Linux kernels (upstream before v4.0, or any
> released RHEL kernels). This patch fixes it.
> 
> The problem is that: some old linux kernels (with IR enabled) only
> support IOAPIC chips with version 0x20. New kernels after commit d32932d
> ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces")
> fixed this problem. To make sure we can work with even old kernels,
> let's upgrade our IOAPIC to version 0x20.
> 
> To make sure we have minimum impact on old systems, IOAPIC version is
> only boosted to 0x20 when vIOMMU IR is enabled. Besides that, the old
> version 0x11 is used.

I'd rather do this conditional only on the machine type, but as you
pointed out it's a bit late to change the default in QEMU 2.7.
Therefore, for 2.7 you could add a property to the IOAPIC device, and
document that you need "-global ioapic.version=0x20" for interrupt
remapping to work with Linux <= 4.0.  QEMU 2.8 can then flip the default
for the new machine types.

Thanks,

Paolo

> Signed-off-by: Peter Xu <address@hidden>
> ---
>  hw/intc/ioapic.c                  | 25 ++++++++++++++++++++++++-
>  include/hw/i386/ioapic_internal.h |  3 +--
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 2d3282a..cd3f283 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -244,6 +244,23 @@ void ioapic_dump_state(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +static uint32_t ioapic_get_version(void)
> +{
> +    X86IOMMUState *iommu = x86_iommu_get_default();
> +
> +    /*
> +     * If we enabled vIOMMU IR, we provide a upgraded version of
> +     * IOAPIC 0x20, which support explicit EOI request from guest.
> +     * This fixes a bug that IR not working on some old upstream
> +     * kernels (before v4.0, commit d32932d) or most RHEL ones.
> +     */
> +    if (iommu && iommu->intr_supported) {
> +        return 0x20;
> +    }
> +
> +    return 0x11;
> +}
> +
>  static uint64_t
>  ioapic_mem_read(void *opaque, hwaddr addr, unsigned int size)
>  {
> @@ -265,7 +282,7 @@ ioapic_mem_read(void *opaque, hwaddr addr, unsigned int 
> size)
>              val = s->id << IOAPIC_ID_SHIFT;
>              break;
>          case IOAPIC_REG_VER:
> -            val = IOAPIC_VERSION |
> +            val = ioapic_get_version() |
>                  ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT);
>              break;
>          default:
> @@ -354,6 +371,12 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
>              }
>          }
>          break;
> +    case IOAPIC_EOI:
> +        if (size != 4 || ioapic_get_version() != 0x20) {
> +            break;
> +        }
> +        ioapic_eoi_broadcast(val);
> +        break;
>      }
>  
>      ioapic_update_kvm_routes(s);
> diff --git a/include/hw/i386/ioapic_internal.h 
> b/include/hw/i386/ioapic_internal.h
> index d89ea1b..6f266c3 100644
> --- a/include/hw/i386/ioapic_internal.h
> +++ b/include/hw/i386/ioapic_internal.h
> @@ -29,8 +29,6 @@
>  
>  #define MAX_IOAPICS                     1
>  
> -#define IOAPIC_VERSION                  0x11
> -
>  #define IOAPIC_LVT_DEST_SHIFT           56
>  #define IOAPIC_LVT_DEST_IDX_SHIFT       48
>  #define IOAPIC_LVT_MASKED_SHIFT         16
> @@ -71,6 +69,7 @@
>  
>  #define IOAPIC_IOREGSEL                 0x00
>  #define IOAPIC_IOWIN                    0x10
> +#define IOAPIC_EOI                      0x40
>  
>  #define IOAPIC_REG_ID                   0x00
>  #define IOAPIC_REG_VER                  0x01
>