[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] x86: ioapic: add support for explicit EOI
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v3] x86: ioapic: add support for explicit EOI |
Date: |
Mon, 1 Aug 2016 16:43:26 +0200 |
On Mon, 1 Aug 2016 21:59:19 +0800
Peter Xu <address@hidden> wrote:
> Some old Linux kernels (upstream before v4.0), or any released RHEL
> kernels has problem in sending APIC EOI when IR is enabled. Meanwhile,
> many of them only support explicit EOI for IOAPIC, which is only
> introduced in IOAPIC version 0x20. This patch provide a way to boost
s/provide/provides/
> QEMU IOAPIC to version 0x20, in order for QEMU to correctly receive EOI
> messages.
>
> Without boosting IOAPIC version to 0x20, kernels before commit d32932d
> ("x86/irq: Convert IOAPIC to use hierarchical irqdomain interfaces")
> will have trouble enabling both IR and level-triggered interrupt devices
> (like e1000).
>
> To upgrade IOAPIC to version 0x20, we need to specify:
>
> -global ioapic.version=0x20
>
> To be compatible with old systems, 0x11 will still be the default IOAPIC
> version. Here 0x11 and 0x20 are the only versions to be supported.
Is there a reason not to default to 0x20 for new machine types and
set 2.6 and older machine types to 0x11?
>
> One thing to mention: this patch only applies to emulated IOAPIC. It
> does not affect kernel IOAPIC behavior.
^^^ which is .... ?
>
> Signed-off-by: Peter Xu <address@hidden>
> ---
> hw/intc/ioapic.c | 22 +++++++++++++++++++++-
> include/hw/i386/ioapic_internal.h | 4 ++--
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 2d3282a..e8568d2 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -21,6 +21,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> #include "monitor/monitor.h"
> #include "hw/hw.h"
> #include "hw/i386/pc.h"
> @@ -265,7 +266,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 = s->version |
> ((IOAPIC_NUM_PINS - 1) << IOAPIC_VER_ENTRIES_SHIFT);
> break;
> default:
> @@ -354,6 +355,13 @@ ioapic_mem_write(void *opaque, hwaddr addr, uint64_t val,
> }
> }
> break;
> + case IOAPIC_EOI:
> + /* Explicit EOI is only supported for IOAPIC version 0x20 */
> + if (size != 4 || s->version != 0x20) {
> + break;
> + }
> + ioapic_eoi_broadcast(val);
> + break;
> }
>
> ioapic_update_kvm_routes(s);
> @@ -387,6 +395,12 @@ static void ioapic_realize(DeviceState *dev, Error
> **errp)
> {
> IOAPICCommonState *s = IOAPIC_COMMON(dev);
>
> + if (s->version != 0x11 && s->version != 0x20) {
> + error_report("IOAPIC only supports version 0x11 or 0x20 "
> + "(default: 0x11).");
probably no need to say what's default here.
> + exit(1);
For erro handling realize() typically calls
error_setg() + error_propagate()
instead of directly error_report() + exit()
It should work in this case as well as the caller
ioapic_init_gsi()->qdev_init_nofail() will terminate
QEMU is "realize" = true fails.
> + }
> +
> memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
> "ioapic", 0x1000);
>
> @@ -397,6 +411,11 @@ static void ioapic_realize(DeviceState *dev, Error
> **errp)
> qemu_add_machine_init_done_notifier(&s->machine_done);
> }
>
> +static Property ioapic_properties[] = {
> + DEFINE_PROP_UINT8("version", IOAPICCommonState, version, 0x11),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> static void ioapic_class_init(ObjectClass *klass, void *data)
> {
> IOAPICCommonClass *k = IOAPIC_COMMON_CLASS(klass);
> @@ -404,6 +423,7 @@ static void ioapic_class_init(ObjectClass *klass, void
> *data)
>
> k->realize = ioapic_realize;
> dc->reset = ioapic_reset_common;
> + dc->props = ioapic_properties;
> }
>
> static const TypeInfo ioapic_info = {
> diff --git a/include/hw/i386/ioapic_internal.h
> b/include/hw/i386/ioapic_internal.h
> index d89ea1b..a11d86d 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
> @@ -109,6 +108,7 @@ struct IOAPICCommonState {
> uint32_t irr;
> uint64_t ioredtbl[IOAPIC_NUM_PINS];
> Notifier machine_done;
> + uint8_t version;
> };
>
> void ioapic_reset_common(DeviceState *dev);