[Qemu-devel] [PATCH 0/5] intel_iommu: fix EIM

[Qemu-devel] [PATCH 0/5] intel_iommu: fix EIM

[Radim Krčmář]

intel_iommu exposed EIM (extended interrupt mode) feature, which in turn
made the guest think that using x2APIC is a good idea.  It was not:
QEMU clamped all addresses to 8 bits (effectively allowing only APIC IDs
below 8 in cluster mode) and 0xff was also interpreted as x2APIC
broadcast even in physical mode.

This series forbids EIM unless KVM is configured to use full 32 bit
addresses and doesn't have the broadcast quirk.

On top of this, it would be great if we had a mechanism that enabled EIM
whenever it can be used -- it is disabled by default now.


Peter Xu (1):
  intel_iommu: add "eim" property

Radim Krčmář (4):
  apic: add global apic_get_class()
  apic: add send_msi() to APICCommonClass
  intel_iommu: pass whole remapped addresses to apic
  intel_iommu: do not allow EIM without KVM support

 hw/i386/intel_iommu.c           | 41 +++++++++++++++++++++++++++++------------
 hw/i386/kvm/apic.c              | 19 +++++++++++++------
 hw/i386/xen/xen_apic.c          |  6 ++++++
 hw/intc/apic.c                  |  6 ++++++
 hw/intc/apic_common.c           | 14 ++++++++++++++
 include/hw/i386/apic_internal.h |  7 +++++++
 include/hw/i386/intel_iommu.h   |  1 +
 target-i386/kvm-stub.c          |  5 +++++
 target-i386/kvm.c               | 13 +++++++++++++
 target-i386/kvm_i386.h          |  1 +
 10 files changed, 95 insertions(+), 18 deletions(-)

-- 
2.10.0

[Qemu-devel] [PATCH 1/5] apic: add global apic_get_class()

[Radim Krčmář]

Every configuration has only up to one APIC class and we'll be extending
the class with a function that can be called without an instanced
object, so a direct access to the class is convenient.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/intc/apic_common.c           | 14 ++++++++++++++
 include/hw/i386/apic_internal.h |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 14ac43c18666..86ef9c43e6df 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -18,6 +18,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>
  */
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
 #include "cpu.h"
@@ -296,6 +297,13 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
 
 static const VMStateDescription vmstate_apic_common;
 
+APICCommonClass *apic_class;
+
+APICCommonClass *apic_get_class(void)
+{
+    return apic_class;
+}
+
 static void apic_common_realize(DeviceState *dev, Error **errp)
 {
     APICCommonState *s = APIC_COMMON(dev);
@@ -306,6 +314,12 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
     info = APIC_COMMON_GET_CLASS(s);
     info->realize(dev, errp);
 
+    if (apic_class && apic_class != info) {
+        error_report("All APICs must be of the same class.");
+        exit(1);
+    }
+    apic_class = info;
+
     /* Note: We need at least 1M to map the VAPIC option ROM */
     if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
         ram_size >= 1024 * 1024) {
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 06c4e9f6f95b..9ba8a5c87f90 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -222,4 +222,7 @@ static inline int apic_get_bit(uint32_t *tab, int index)
     return !!(tab[i] & mask);
 }
 
+
+APICCommonClass *apic_get_class(void);
+
 #endif /* QEMU_APIC_INTERNAL_H */
-- 
2.10.0

Re: [Qemu-devel] [PATCH 1/5] apic: add global apic_get_class()

[Peter Xu]

On Thu, Sep 22, 2016 at 11:04:28PM +0200, Radim Krčmář wrote:
> Every configuration has only up to one APIC class and we'll be extending
> the class with a function that can be called without an instanced
> object, so a direct access to the class is convenient.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  hw/intc/apic_common.c           | 14 ++++++++++++++
>  include/hw/i386/apic_internal.h |  3 +++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index 14ac43c18666..86ef9c43e6df 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -18,6 +18,7 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
> @@ -296,6 +297,13 @@ static int apic_load_old(QEMUFile *f, void *opaque, int version_id)
>  
>  static const VMStateDescription vmstate_apic_common;
>  
> +APICCommonClass *apic_class;
> +
> +APICCommonClass *apic_get_class(void)
> +{
> +    return apic_class;
> +}
> +
>  static void apic_common_realize(DeviceState *dev, Error **errp)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> @@ -306,6 +314,12 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>      info = APIC_COMMON_GET_CLASS(s);
>      info->realize(dev, errp);
>  
> +    if (apic_class && apic_class != info) {
> +        error_report("All APICs must be of the same class.");
> +        exit(1);
> +    }

Can user trigger this error? If not, I'd prefer:

  assert(!apic_class || apic_class == info);

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH 1/5] apic: add global apic_get_class()

[Radim Krčmář]

2016-09-23 17:17+0800, Peter Xu:
> On Thu, Sep 22, 2016 at 11:04:28PM +0200, Radim Krčmář wrote:
>> Every configuration has only up to one APIC class and we'll be extending
>> the class with a function that can be called without an instanced
>> object, so a direct access to the class is convenient.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> @@ -306,6 +314,12 @@ static void apic_common_realize(DeviceState *dev, Error **errp)
>>      info = APIC_COMMON_GET_CLASS(s);
>>      info->realize(dev, errp);
>>  
>> +    if (apic_class && apic_class != info) {
>> +        error_report("All APICs must be of the same class.");
>> +        exit(1);
>> +    }
> 
> Can user trigger this error? If not, I'd prefer:

Shouldn't be able to.

>   assert(!apic_class || apic_class == info);

I'll use that, thanks.

[Qemu-devel] [PATCH 2/5] apic: add send_msi() to APICCommonClass

[Radim Krčmář]

The MMIO based interface to APIC doesn't work well with MSIs that have
upper address bits set (remapped x2APIC MSIs).  A specialized interface
is a quick and dirty way to avoid the shortcoming.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/i386/kvm/apic.c              | 19 +++++++++++++------
 hw/i386/xen/xen_apic.c          |  6 ++++++
 hw/intc/apic.c                  |  6 ++++++
 include/hw/i386/apic_internal.h |  4 ++++
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index feb00024f20c..7cc1acd63d32 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -168,6 +168,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
     run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
 }
 
+static void kvm_send_msi(MSIMessage *msg)
+{
+    int ret;
+
+    ret = kvm_irqchip_send_msi(kvm_state, *msg);
+    if (ret < 0) {
+        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
+                strerror(-ret));
+    }
+}
+
 static uint64_t kvm_apic_mem_read(void *opaque, hwaddr addr,
                                   unsigned size)
 {
@@ -178,13 +189,8 @@ static void kvm_apic_mem_write(void *opaque, hwaddr addr,
                                uint64_t data, unsigned size)
 {
     MSIMessage msg = { .address = addr, .data = data };
-    int ret;
 
-    ret = kvm_irqchip_send_msi(kvm_state, msg);
-    if (ret < 0) {
-        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
-                strerror(-ret));
-    }
+    kvm_send_msi(&msg);
 }
 
 static const MemoryRegionOps kvm_apic_io_ops = {
@@ -231,6 +237,7 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
     k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting;
     k->vapic_base_update = kvm_apic_vapic_base_update;
     k->external_nmi = kvm_apic_external_nmi;
+    k->send_msi = kvm_send_msi;
 }
 
 static const TypeInfo kvm_apic_info = {
diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
index 21d68ee04b0a..55769eba7ede 100644
--- a/hw/i386/xen/xen_apic.c
+++ b/hw/i386/xen/xen_apic.c
@@ -68,6 +68,11 @@ static void xen_apic_external_nmi(APICCommonState *s)
 {
 }
 
+static void xen_send_msi(MSIMessage *msi)
+{
+    xen_hvm_inject_msi(msi->address, msi->data);
+}
+
 static void xen_apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
@@ -78,6 +83,7 @@ static void xen_apic_class_init(ObjectClass *klass, void *data)
     k->get_tpr = xen_apic_get_tpr;
     k->vapic_base_update = xen_apic_vapic_base_update;
     k->external_nmi = xen_apic_external_nmi;
+    k->send_msi = xen_send_msi;
 }
 
 static const TypeInfo xen_apic_info = {
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 7bd1d279c463..4f3fb44d05e4 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -900,6 +900,11 @@ static void apic_unrealize(DeviceState *dev, Error **errp)
     local_apics[s->id] = NULL;
 }
 
+static void apic_send_msi_struct(MSIMessage *msi)
+{
+    apic_send_msi(msi->address, msi->data);
+}
+
 static void apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
@@ -913,6 +918,7 @@ static void apic_class_init(ObjectClass *klass, void *data)
     k->external_nmi = apic_external_nmi;
     k->pre_save = apic_pre_save;
     k->post_load = apic_post_load;
+    k->send_msi = apic_send_msi_struct;
 }
 
 static const TypeInfo apic_info = {
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 9ba8a5c87f90..32b083ad2926 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -146,6 +146,10 @@ typedef struct APICCommonClass
     void (*pre_save)(APICCommonState *s);
     void (*post_load)(APICCommonState *s);
     void (*reset)(APICCommonState *s);
+    /* send_msi emulates an APIC bus and its proper place would be in a new
+     * device, but it's convenient to have it here for now.
+     */
+    void (*send_msi)(MSIMessage *msi);
 } APICCommonClass;
 
 struct APICCommonState {
-- 
2.10.0

Re: [Qemu-devel] [PATCH 2/5] apic: add send_msi() to APICCommonClass

[Peter Xu]

On Thu, Sep 22, 2016 at 11:04:29PM +0200, Radim Krčmář wrote:
> The MMIO based interface to APIC doesn't work well with MSIs that have
> upper address bits set (remapped x2APIC MSIs).  A specialized interface
> is a quick and dirty way to avoid the shortcoming.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- peterx

Re: [Qemu-devel] [PATCH 2/5] apic: add send_msi() to APICCommonClass

[Igor Mammedov]

On Thu, 22 Sep 2016 23:04:29 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> The MMIO based interface to APIC doesn't work well with MSIs that have
> upper address bits set (remapped x2APIC MSIs).  A specialized interface
> is a quick and dirty way to avoid the shortcoming.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  hw/i386/kvm/apic.c              | 19 +++++++++++++------
>  hw/i386/xen/xen_apic.c          |  6 ++++++
>  hw/intc/apic.c                  |  6 ++++++
>  include/hw/i386/apic_internal.h |  4 ++++
>  4 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index feb00024f20c..7cc1acd63d32 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -168,6 +168,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
>      run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
>  }
>  
> +static void kvm_send_msi(MSIMessage *msg)
> +{
> +    int ret;
> +
> +    ret = kvm_irqchip_send_msi(kvm_state, *msg);
> +    if (ret < 0) {
> +        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
> +                strerror(-ret));
> +    }
> +}
> +
>  static uint64_t kvm_apic_mem_read(void *opaque, hwaddr addr,
>                                    unsigned size)
>  {
> @@ -178,13 +189,8 @@ static void kvm_apic_mem_write(void *opaque, hwaddr addr,
>                                 uint64_t data, unsigned size)
>  {
>      MSIMessage msg = { .address = addr, .data = data };
> -    int ret;
>  
> -    ret = kvm_irqchip_send_msi(kvm_state, msg);
> -    if (ret < 0) {
> -        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
> -                strerror(-ret));
> -    }
> +    kvm_send_msi(&msg);
>  }
>  
>  static const MemoryRegionOps kvm_apic_io_ops = {
> @@ -231,6 +237,7 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
>      k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting;
>      k->vapic_base_update = kvm_apic_vapic_base_update;
>      k->external_nmi = kvm_apic_external_nmi;
> +    k->send_msi = kvm_send_msi;
>  }
>  
>  static const TypeInfo kvm_apic_info = {
> diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
> index 21d68ee04b0a..55769eba7ede 100644
> --- a/hw/i386/xen/xen_apic.c
> +++ b/hw/i386/xen/xen_apic.c
> @@ -68,6 +68,11 @@ static void xen_apic_external_nmi(APICCommonState *s)
>  {
>  }
>  
> +static void xen_send_msi(MSIMessage *msi)
> +{
> +    xen_hvm_inject_msi(msi->address, msi->data);
> +}
> +
>  static void xen_apic_class_init(ObjectClass *klass, void *data)
>  {
>      APICCommonClass *k = APIC_COMMON_CLASS(klass);
> @@ -78,6 +83,7 @@ static void xen_apic_class_init(ObjectClass *klass, void *data)
>      k->get_tpr = xen_apic_get_tpr;
>      k->vapic_base_update = xen_apic_vapic_base_update;
>      k->external_nmi = xen_apic_external_nmi;
> +    k->send_msi = xen_send_msi;
>  }
>  
>  static const TypeInfo xen_apic_info = {
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 7bd1d279c463..4f3fb44d05e4 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -900,6 +900,11 @@ static void apic_unrealize(DeviceState *dev, Error **errp)
>      local_apics[s->id] = NULL;
>  }
>  
> +static void apic_send_msi_struct(MSIMessage *msi)
> +{
> +    apic_send_msi(msi->address, msi->data);
> +}
why not to make apic_send_msi(MSIMessage *msi) instead of adding a wrapper?

Also when interface is switched to send_msi() in 3/5,
aren't you loosing following checks in apic_mem_writel():

    if (addr > 0xfff || !index) {                                                 


> +
>  static void apic_class_init(ObjectClass *klass, void *data)
>  {
>      APICCommonClass *k = APIC_COMMON_CLASS(klass);
> @@ -913,6 +918,7 @@ static void apic_class_init(ObjectClass *klass, void *data)
>      k->external_nmi = apic_external_nmi;
>      k->pre_save = apic_pre_save;
>      k->post_load = apic_post_load;
> +    k->send_msi = apic_send_msi_struct;
>  }
>  
>  static const TypeInfo apic_info = {
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 9ba8a5c87f90..32b083ad2926 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -146,6 +146,10 @@ typedef struct APICCommonClass
>      void (*pre_save)(APICCommonState *s);
>      void (*post_load)(APICCommonState *s);
>      void (*reset)(APICCommonState *s);
> +    /* send_msi emulates an APIC bus and its proper place would be in a new
> +     * device, but it's convenient to have it here for now.
> +     */
> +    void (*send_msi)(MSIMessage *msi);
>  } APICCommonClass;
>  
>  struct APICCommonState {

Re: [Qemu-devel] [PATCH 2/5] apic: add send_msi() to APICCommonClass

[Radim Krčmář]

2016-09-26 14:38+0200, Igor Mammedov:
> On Thu, 22 Sep 2016 23:04:29 +0200
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> 
>> The MMIO based interface to APIC doesn't work well with MSIs that have
>> upper address bits set (remapped x2APIC MSIs).  A specialized interface
>> is a quick and dirty way to avoid the shortcoming.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  hw/i386/kvm/apic.c              | 19 +++++++++++++------
>>  hw/i386/xen/xen_apic.c          |  6 ++++++
>>  hw/intc/apic.c                  |  6 ++++++
>>  include/hw/i386/apic_internal.h |  4 ++++
>>  4 files changed, 29 insertions(+), 6 deletions(-)
>> 
>> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
>> index feb00024f20c..7cc1acd63d32 100644
>> --- a/hw/i386/kvm/apic.c
>> +++ b/hw/i386/kvm/apic.c
>> @@ -168,6 +168,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
>>      run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
>>  }
>>  
>> +static void kvm_send_msi(MSIMessage *msg)
>> +{
>> +    int ret;
>> +
>> +    ret = kvm_irqchip_send_msi(kvm_state, *msg);
>> +    if (ret < 0) {
>> +        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
>> +                strerror(-ret));
>> +    }
>> +}
>> +
>>  static uint64_t kvm_apic_mem_read(void *opaque, hwaddr addr,
>>                                    unsigned size)
>>  {
>> @@ -178,13 +189,8 @@ static void kvm_apic_mem_write(void *opaque, hwaddr addr,
>>                                 uint64_t data, unsigned size)
>>  {
>>      MSIMessage msg = { .address = addr, .data = data };
>> -    int ret;
>>  
>> -    ret = kvm_irqchip_send_msi(kvm_state, msg);
>> -    if (ret < 0) {
>> -        fprintf(stderr, "KVM: injection failed, MSI lost (%s)\n",
>> -                strerror(-ret));
>> -    }
>> +    kvm_send_msi(&msg);
>>  }
>>  
>>  static const MemoryRegionOps kvm_apic_io_ops = {
>> @@ -231,6 +237,7 @@ static void kvm_apic_class_init(ObjectClass *klass, void *data)
>>      k->enable_tpr_reporting = kvm_apic_enable_tpr_reporting;
>>      k->vapic_base_update = kvm_apic_vapic_base_update;
>>      k->external_nmi = kvm_apic_external_nmi;
>> +    k->send_msi = kvm_send_msi;
>>  }
>>  
>>  static const TypeInfo kvm_apic_info = {
>> diff --git a/hw/i386/xen/xen_apic.c b/hw/i386/xen/xen_apic.c
>> index 21d68ee04b0a..55769eba7ede 100644
>> --- a/hw/i386/xen/xen_apic.c
>> +++ b/hw/i386/xen/xen_apic.c
>> @@ -68,6 +68,11 @@ static void xen_apic_external_nmi(APICCommonState *s)
>>  {
>>  }
>>  
>> +static void xen_send_msi(MSIMessage *msi)
>> +{
>> +    xen_hvm_inject_msi(msi->address, msi->data);
>> +}
>> +
>>  static void xen_apic_class_init(ObjectClass *klass, void *data)
>>  {
>>      APICCommonClass *k = APIC_COMMON_CLASS(klass);
>> @@ -78,6 +83,7 @@ static void xen_apic_class_init(ObjectClass *klass, void *data)
>>      k->get_tpr = xen_apic_get_tpr;
>>      k->vapic_base_update = xen_apic_vapic_base_update;
>>      k->external_nmi = xen_apic_external_nmi;
>> +    k->send_msi = xen_send_msi;
>>  }
>>  
>>  static const TypeInfo xen_apic_info = {
>> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
>> index 7bd1d279c463..4f3fb44d05e4 100644
>> --- a/hw/intc/apic.c
>> +++ b/hw/intc/apic.c
>> @@ -900,6 +900,11 @@ static void apic_unrealize(DeviceState *dev, Error **errp)
>>      local_apics[s->id] = NULL;
>>  }
>>  
>> +static void apic_send_msi_struct(MSIMessage *msi)
>> +{
>> +    apic_send_msi(msi->address, msi->data);
>> +}
> why not to make apic_send_msi(MSIMessage *msi) instead of adding a wrapper?

Good point, I'll change it.

> Also when interface is switched to send_msi() in 3/5,
> aren't you loosing following checks in apic_mem_writel():
> 
>     if (addr > 0xfff || !index) {

We don't need them.  addr <= 0xfff (the first page) was checked because of the
the APIC register page that overlaid the MSI space.  I think that the comment
in code explains it:

        /* MSI and MMIO APIC are at the same memory location,
         * but actually not on the global bus: MSI is on PCI bus
         * APIC is connected directly to the CPU.
         * Mapping them on the global bus happens to work because
         * MSI registers are reserved in APIC MMIO and vice versa. */

[Qemu-devel] [PATCH 3/5] intel_iommu: pass whole remapped addresses to apic

[Radim Krčmář]

The MMIO interface to APIC only allowed 8 bit addresses, which is not
enough for 32 bit addresses from EIM remapping.
Intel stored upper 24 bits in the high MSI address, so use the same
technique. The technique is also used in KVM MSI interface.
Other APICs are unlikely to handle those upper bits.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 hw/i386/intel_iommu.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 28c31a2cdfa3..1a0961e5cf6a 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -31,6 +31,7 @@
 #include "hw/i386/x86-iommu.h"
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
+#include "hw/i386/apic_internal.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -279,18 +280,16 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
 static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg,
                                    hwaddr mesg_data_reg)
 {
-    hwaddr addr;
-    uint32_t data;
+    MSIMessage msi;
 
     assert(mesg_data_reg < DMAR_REG_SIZE);
     assert(mesg_addr_reg < DMAR_REG_SIZE);
 
-    addr = vtd_get_long_raw(s, mesg_addr_reg);
-    data = vtd_get_long_raw(s, mesg_data_reg);
+    msi.address = vtd_get_long_raw(s, mesg_addr_reg);
+    msi.data = vtd_get_long_raw(s, mesg_data_reg);
 
     VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, addr, data);
-    address_space_stl_le(&address_space_memory, addr, data,
-                         MEMTXATTRS_UNSPECIFIED, NULL);
+    apic_get_class()->send_msi(&msi);
 }
 
 /* Generate a fault event to software via MSI if conditions are met.
@@ -2127,6 +2126,7 @@ static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out)
     msg.dest_mode = irq->dest_mode;
     msg.redir_hint = irq->redir_hint;
     msg.dest = irq->dest;
+    msg.__addr_hi = irq->dest & 0xffffff00;
     msg.__addr_head = cpu_to_le32(0xfee);
     /* Keep this from original MSI address bits */
     msg.__not_used = irq->msi_addr_last_bits;
@@ -2275,11 +2275,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
                 " for device sid 0x%04x",
                 to.address, to.data, sid);
 
-    if (dma_memory_write(&address_space_memory, to.address,
-                         &to.data, size)) {
-        VTD_DPRINTF(GENERAL, "error: fail to write 0x%"PRIx64
-                    " value 0x%"PRIx32, to.address, to.data);
-    }
+    apic_get_class()->send_msi(&to);
 
     return MEMTX_OK;
 }
-- 
2.10.0

Re: [Qemu-devel] [PATCH 3/5] intel_iommu: pass whole remapped addresses to apic

[Peter Xu]

On Thu, Sep 22, 2016 at 11:04:30PM +0200, Radim Krčmář wrote:

[...]

> @@ -279,18 +280,16 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
>  static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg,
>                                     hwaddr mesg_data_reg)
>  {
> -    hwaddr addr;
> -    uint32_t data;
> +    MSIMessage msi;
>  
>      assert(mesg_data_reg < DMAR_REG_SIZE);
>      assert(mesg_addr_reg < DMAR_REG_SIZE);
>  
> -    addr = vtd_get_long_raw(s, mesg_addr_reg);
> -    data = vtd_get_long_raw(s, mesg_data_reg);
> +    msi.address = vtd_get_long_raw(s, mesg_addr_reg);
> +    msi.data = vtd_get_long_raw(s, mesg_data_reg);
>  
>      VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, addr, data);

Need to fix addr/data as well?

-- peterx

Re: [Qemu-devel] [PATCH 3/5] intel_iommu: pass whole remapped addresses to apic

[Radim Krčmář]

2016-09-23 17:41+0800, Peter Xu:
> On Thu, Sep 22, 2016 at 11:04:30PM +0200, Radim Krčmář wrote:
> 
> [...]
> 
>> @@ -279,18 +280,16 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
>>  static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg,
>>                                     hwaddr mesg_data_reg)
>>  {
>> -    hwaddr addr;
>> -    uint32_t data;
>> +    MSIMessage msi;
>>  
>>      assert(mesg_data_reg < DMAR_REG_SIZE);
>>      assert(mesg_addr_reg < DMAR_REG_SIZE);
>>  
>> -    addr = vtd_get_long_raw(s, mesg_addr_reg);
>> -    data = vtd_get_long_raw(s, mesg_data_reg);
>> +    msi.address = vtd_get_long_raw(s, mesg_addr_reg);
>> +    msi.data = vtd_get_long_raw(s, mesg_data_reg);
>>  
>>      VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, addr, data);
> 
> Need to fix addr/data as well?

Oops, thanks.

Re: [Qemu-devel] [PATCH 3/5] intel_iommu: pass whole remapped addresses to apic

[Igor Mammedov]

On Thu, 22 Sep 2016 23:04:30 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> The MMIO interface to APIC only allowed 8 bit addresses, which is not
> enough for 32 bit addresses from EIM remapping.
> Intel stored upper 24 bits in the high MSI address, so use the same
> technique. The technique is also used in KVM MSI interface.
> Other APICs are unlikely to handle those upper bits.

this series applied upto here fixes:

KVM: injection failed, MSI lost (Operation not permitted)
KVM: injection failed, MSI lost (Operation not permitted)
KVM: injection failed, MSI lost (Operation not permitted)

with EMI enabled by default that I've reported
https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05459.html

> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 28c31a2cdfa3..1a0961e5cf6a 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -31,6 +31,7 @@
>  #include "hw/i386/x86-iommu.h"
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
> +#include "hw/i386/apic_internal.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -279,18 +280,16 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
>  static void vtd_generate_interrupt(IntelIOMMUState *s, hwaddr mesg_addr_reg,
>                                     hwaddr mesg_data_reg)
>  {
> -    hwaddr addr;
> -    uint32_t data;
> +    MSIMessage msi;
>  
>      assert(mesg_data_reg < DMAR_REG_SIZE);
>      assert(mesg_addr_reg < DMAR_REG_SIZE);
>  
> -    addr = vtd_get_long_raw(s, mesg_addr_reg);
> -    data = vtd_get_long_raw(s, mesg_data_reg);
> +    msi.address = vtd_get_long_raw(s, mesg_addr_reg);
> +    msi.data = vtd_get_long_raw(s, mesg_data_reg);
>  
>      VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32, addr, data);
> -    address_space_stl_le(&address_space_memory, addr, data,
> -                         MEMTXATTRS_UNSPECIFIED, NULL);
> +    apic_get_class()->send_msi(&msi);
>  }
>  
>  /* Generate a fault event to software via MSI if conditions are met.
> @@ -2127,6 +2126,7 @@ static void vtd_generate_msi_message(VTDIrq *irq, MSIMessage *msg_out)
>      msg.dest_mode = irq->dest_mode;
>      msg.redir_hint = irq->redir_hint;
>      msg.dest = irq->dest;
> +    msg.__addr_hi = irq->dest & 0xffffff00;
>      msg.__addr_head = cpu_to_le32(0xfee);
>      /* Keep this from original MSI address bits */
>      msg.__not_used = irq->msi_addr_last_bits;
> @@ -2275,11 +2275,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
>                  " for device sid 0x%04x",
>                  to.address, to.data, sid);
>  
> -    if (dma_memory_write(&address_space_memory, to.address,
> -                         &to.data, size)) {
> -        VTD_DPRINTF(GENERAL, "error: fail to write 0x%"PRIx64
> -                    " value 0x%"PRIx32, to.address, to.data);
> -    }
> +    apic_get_class()->send_msi(&to);
>  
>      return MEMTX_OK;
>  }

[Qemu-devel] [PATCH 4/5] intel_iommu: add "eim" property

[Radim Krčmář]

From: Peter Xu <peterx@redhat.com>

Adding one extra property for intel-iommu device to decide whether we
should support EIM bit for IR.

Now we are throwing high 24 bits of dest_id away directly. This will
cause interrupt issues with guests that:

- enabled x2apic with cluster mode
- have more than 8 vcpus (so dest_id[31:8] might be nonzero)

Let's make xapic the default one, and for the brave people who would
like to try EIM and know the side effects, we can do it by explicitly
enabling EIM using:

  -device intel-iommu,intremap=on,eim=on

Even after we have x2apic support, it'll still be good if we can provide
a way to switch xapic/x2apic from QEMU side for e.g. debugging purpose,
which is an alternative for tuning guest kernel boot parameters.

We can switch the default to "on" after x2apic fully supported.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c         | 16 +++++++++++++++-
 include/hw/i386/intel_iommu.h |  1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 1a0961e5cf6a..269e37e71af4 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2004,6 +2004,11 @@ static const MemoryRegionOps vtd_mem_ops = {
 
 static Property vtd_properties[] = {
     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
+    /*
+     * TODO: currently EIM is disabled by default. We can enable this
+     * after fully support x2apic.
+     */
+    DEFINE_PROP_BOOL("eim", IntelIOMMUState, eim_supported, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2360,7 +2365,10 @@ static void vtd_init(IntelIOMMUState *s)
     s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
 
     if (x86_iommu->intr_supported) {
-        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM | VTD_ECAP_MHMV;
+        s->ecap |= VTD_ECAP_IR | VTD_ECAP_MHMV;
+        if (s->eim_supported) {
+            s->ecap |= VTD_ECAP_EIM;
+        }
     }
 
     vtd_reset_context_cache(s);
@@ -2464,6 +2472,12 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     /* Pseudo address space under root PCI bus. */
     pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
 
+    /* EIM bit requires IR */
+    if (s->eim_supported && !x86_iommu->intr_supported) {
+        error_report("EIM (Extended Interrupt Mode) bit requires intremap=on");
+        exit(1);
+    }
+
     /* Currently Intel IOMMU IR only support "kernel-irqchip={off|split}" */
     if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
         !kvm_irqchip_is_split()) {
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index a42dbd745a70..b1bc76895deb 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -289,6 +289,7 @@ struct IntelIOMMUState {
     dma_addr_t intr_root;           /* Interrupt remapping table pointer */
     uint32_t intr_size;             /* Number of IR table entries */
     bool intr_eime;                 /* Extended interrupt mode enabled */
+    bool eim_supported;             /* Whether to allow EIM bit */
 };
 
 /* Find the VTD Address space associated with the given bus pointer,
-- 
2.10.0

[Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Radim Krčmář]

Cluster x2APIC cannot work without KVM's x2apic API when the maximal
APIC ID is > 8.  Make the code simpler by completely forbidding EIM
without KVM's x2apic API.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
  I think it the dependency would be nicer in the eim setter, but the
  other dependency, for interrupt remapping, isn't there and I didn't
  venture for reasons.
---
 hw/i386/intel_iommu.c  |  7 +++++++
 target-i386/kvm-stub.c |  5 +++++
 target-i386/kvm.c      | 13 +++++++++++++
 target-i386/kvm_i386.h |  1 +
 4 files changed, 26 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 269e37e71af4..0304a1bf6f1d 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -32,6 +32,7 @@
 #include "hw/pci-host/q35.h"
 #include "sysemu/kvm.h"
 #include "hw/i386/apic_internal.h"
+#include "kvm_i386.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -2485,6 +2486,12 @@ static void vtd_realize(DeviceState *dev, Error **errp)
                      "kernel-irqchip=on, please use 'split|off'.");
         exit(1);
     }
+
+    if (s->eim_supported && kvm_irqchip_in_kernel() &&
+        !kvm_enable_x2apic()) {
+        error_report("EIM requires support from the KVM side (X2APIC_API).");
+        exit(1);
+    }
 }
 
 static void vtd_class_init(ObjectClass *klass, void *data)
diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
index cdf15061091d..bda4dc2f0c57 100644
--- a/target-i386/kvm-stub.c
+++ b/target-i386/kvm-stub.c
@@ -25,6 +25,11 @@ bool kvm_has_smm(void)
     return 1;
 }
 
+bool kvm_enable_x2apic(void)
+{
+    return false;
+}
+
 /* This function is only called inside conditionals which we
  * rely on the compiler to optimize out when CONFIG_KVM is not
  * defined.
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f1ad805665ad..4c0a4df5eaea 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -128,6 +128,19 @@ bool kvm_allows_irq0_override(void)
     return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
 }
 
+static bool kvm_x2apic_api_set_flags(uint64_t flags)
+{
+    KVMState *s = KVM_STATE(current_machine->accelerator);
+
+    return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
+}
+
+bool kvm_enable_x2apic(void)
+{
+    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
+                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
+}
+
 static int kvm_get_tsc(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 42b00af1b1c3..559dd8b5acd2 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -41,4 +41,5 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector,
 int kvm_device_msix_assign(KVMState *s, uint32_t dev_id);
 int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 
+bool kvm_enable_x2apic(void);
 #endif
-- 
2.10.0

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Paolo Bonzini]

On 22/09/2016 23:04, Radim Krčmář wrote:
> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> APIC ID is > 8.  Make the code simpler by completely forbidding EIM
> without KVM's x2apic API.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>   I think it the dependency would be nicer in the eim setter, but the
>   other dependency, for interrupt remapping, isn't there and I didn't
>   venture for reasons.
> ---
>  hw/i386/intel_iommu.c  |  7 +++++++
>  target-i386/kvm-stub.c |  5 +++++
>  target-i386/kvm.c      | 13 +++++++++++++
>  target-i386/kvm_i386.h |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 269e37e71af4..0304a1bf6f1d 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -32,6 +32,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> +#include "kvm_i386.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2485,6 +2486,12 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>                       "kernel-irqchip=on, please use 'split|off'.");
>          exit(1);
>      }
> +
> +    if (s->eim_supported && kvm_irqchip_in_kernel() &&
> +        !kvm_enable_x2apic()) {
> +        error_report("EIM requires support from the KVM side (X2APIC_API).");
> +        exit(1);
> +    }
>  }
>  
>  static void vtd_class_init(ObjectClass *klass, void *data)
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> index cdf15061091d..bda4dc2f0c57 100644
> --- a/target-i386/kvm-stub.c
> +++ b/target-i386/kvm-stub.c
> @@ -25,6 +25,11 @@ bool kvm_has_smm(void)
>      return 1;
>  }
>  
> +bool kvm_enable_x2apic(void)
> +{
> +    return false;
> +}
> +
>  /* This function is only called inside conditionals which we
>   * rely on the compiler to optimize out when CONFIG_KVM is not
>   * defined.
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index f1ad805665ad..4c0a4df5eaea 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -128,6 +128,19 @@ bool kvm_allows_irq0_override(void)
>      return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
>  }
>  
> +static bool kvm_x2apic_api_set_flags(uint64_t flags)
> +{
> +    KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +    return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
> +}
> +
> +bool kvm_enable_x2apic(void)
> +{
> +    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> +                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
> +}
> +
>  static int kvm_get_tsc(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 42b00af1b1c3..559dd8b5acd2 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -41,4 +41,5 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector,
>  int kvm_device_msix_assign(KVMState *s, uint32_t dev_id);
>  int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
>  
> +bool kvm_enable_x2apic(void);
>  #endif
> 

Since the whole IOMMU feature is new and somewhat experimental, I think
it's okay to just make EIM the default for >=2.8 machine types if KVM is
on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and
false otherwise, and pc-2.7 would set eim=off). It means requiring
kernel 4.8 by default, but I don't think it's a big deal.

Paolo

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Peter Xu]

On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote:
> Since the whole IOMMU feature is new and somewhat experimental, I think
> it's okay to just make EIM the default for >=2.8 machine types if KVM is
> on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and
> false otherwise, and pc-2.7 would set eim=off). It means requiring
> kernel 4.8 by default, but I don't think it's a big deal.

I think the problem is, even we have KVM support for x2apic, we are
still losing QEMU part. And guests with cluster x2apic and >8 vcpus
will not working properly on device interrupts, which can be very
confusing to people (it can boot, but some devices just don't work
properly, and they won't see useful information in guest dmesg).

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Paolo Bonzini]

On 23/09/2016 12:02, Peter Xu wrote:
> On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote:
>> > Since the whole IOMMU feature is new and somewhat experimental, I think
>> > it's okay to just make EIM the default for >=2.8 machine types if KVM is
>> > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and
>> > false otherwise, and pc-2.7 would set eim=off). It means requiring
>> > kernel 4.8 by default, but I don't think it's a big deal.
> I think the problem is, even we have KVM support for x2apic, we are
> still losing QEMU part. And guests with cluster x2apic and >8 vcpus
> will not working properly on device interrupts, which can be very
> confusing to people (it can boot, but some devices just don't work
> properly, and they won't see useful information in guest dmesg).

Yes, that's why I suggested EIM=on by default.

Paolo

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Peter Xu]

On Fri, Sep 23, 2016 at 12:03:26PM +0200, Paolo Bonzini wrote:
> 
> 
> On 23/09/2016 12:02, Peter Xu wrote:
> > On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote:
> >> > Since the whole IOMMU feature is new and somewhat experimental, I think
> >> > it's okay to just make EIM the default for >=2.8 machine types if KVM is
> >> > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and
> >> > false otherwise, and pc-2.7 would set eim=off). It means requiring
> >> > kernel 4.8 by default, but I don't think it's a big deal.
> > I think the problem is, even we have KVM support for x2apic, we are
> > still losing QEMU part. And guests with cluster x2apic and >8 vcpus
> > will not working properly on device interrupts, which can be very
> > confusing to people (it can boot, but some devices just don't work
> > properly, and they won't see useful information in guest dmesg).
> 
> Yes, that's why I suggested EIM=on by default.

I am confused. :(

Why not we just keep people from that wrong configuration by default,
until we have x2apic in QEMU?

-- peterx

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Paolo Bonzini]

On 23/09/2016 12:12, Peter Xu wrote:
> On Fri, Sep 23, 2016 at 12:03:26PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 23/09/2016 12:02, Peter Xu wrote:
>>> On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote:
>>>>> Since the whole IOMMU feature is new and somewhat experimental, I think
>>>>> it's okay to just make EIM the default for >=2.8 machine types if KVM is
>>>>> on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and
>>>>> false otherwise, and pc-2.7 would set eim=off). It means requiring
>>>>> kernel 4.8 by default, but I don't think it's a big deal.
>>> I think the problem is, even we have KVM support for x2apic, we are
>>> still losing QEMU part. And guests with cluster x2apic and >8 vcpus
>>> will not working properly on device interrupts, which can be very
>>> confusing to people (it can boot, but some devices just don't work
>>> properly, and they won't see useful information in guest dmesg).
>>
>> Yes, that's why I suggested EIM=on by default.
> 
> I am confused. :(
> 
> Why not we just keep people from that wrong configuration by default,
> until we have x2apic in QEMU?

Do you mean Igor's patches?  I expect that they will go in pretty much
at the same time as Radim's.

Paolo

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Peter Xu]

On Fri, Sep 23, 2016 at 12:39:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 23/09/2016 12:12, Peter Xu wrote:
> > On Fri, Sep 23, 2016 at 12:03:26PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 23/09/2016 12:02, Peter Xu wrote:
> >>> On Fri, Sep 23, 2016 at 11:27:09AM +0200, Paolo Bonzini wrote:
> >>>>> Since the whole IOMMU feature is new and somewhat experimental, I think
> >>>>> it's okay to just make EIM the default for >=2.8 machine types if KVM is
> >>>>> on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and
> >>>>> false otherwise, and pc-2.7 would set eim=off). It means requiring
> >>>>> kernel 4.8 by default, but I don't think it's a big deal.
> >>> I think the problem is, even we have KVM support for x2apic, we are
> >>> still losing QEMU part. And guests with cluster x2apic and >8 vcpus
> >>> will not working properly on device interrupts, which can be very
> >>> confusing to people (it can boot, but some devices just don't work
> >>> properly, and they won't see useful information in guest dmesg).
> >>
> >> Yes, that's why I suggested EIM=on by default.
> > 
> > I am confused. :(
> > 
> > Why not we just keep people from that wrong configuration by default,
> > until we have x2apic in QEMU?
> 
> Do you mean Igor's patches?  I expect that they will go in pretty much
> at the same time as Radim's.

Ah! Yes we have x2apic all here... So I totally agree we should set it
on as default.

(My mistake of not noticing the truth)

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Radim Krčmář]

2016-09-23 11:27+0200, Paolo Bonzini:
> Since the whole IOMMU feature is new and somewhat experimental, I think
> it's okay to just make EIM the default for >=2.8 machine types if KVM is
> on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and

Sounds good.

> false otherwise, and pc-2.7 would set eim=off).

What about eim=on in pc-2.7, to avoid breaking migration?

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Paolo Bonzini]

----- Original Message -----
> From: "Radim Krčmář" <rkrcmar@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, "Peter Xu" <peterx@redhat.com>, "Igor Mammedov" <imammedo@redhat.com>, "Richard Henderson"
> <rth@twiddle.net>, "Eduardo Habkost" <ehabkost@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>
> Sent: Tuesday, September 27, 2016 4:01:39 PM
> Subject: Re: [PATCH 5/5] intel_iommu: do not allow EIM without KVM support
> 
> 2016-09-23 11:27+0200, Paolo Bonzini:
> > Since the whole IOMMU feature is new and somewhat experimental, I think
> > it's okay to just make EIM the default for >=2.8 machine types if KVM is
> > on (using DEFINE_PROP_ON_OFF_AUTO; auto means true if KVM is on and
> 
> Sounds good.

BTW this also means KVM+vIOMMU requires 4.8.  Let's remember to document
it in the release notes.

> > false otherwise, and pc-2.7 would set eim=off).
> 
> What about eim=on in pc-2.7, to avoid breaking migration?

Yup, just a thinko.

Paolo

Re: [Qemu-devel] [PATCH 5/5] intel_iommu: do not allow EIM without KVM support

[Igor Mammedov]

On Thu, 22 Sep 2016 23:04:32 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> Cluster x2APIC cannot work without KVM's x2apic API when the maximal
> APIC ID is > 8.  Make the code simpler by completely forbidding EIM
> without KVM's x2apic API.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>   I think it the dependency would be nicer in the eim setter, but the
>   other dependency, for interrupt remapping, isn't there and I didn't
>   venture for reasons.
> ---
>  hw/i386/intel_iommu.c  |  7 +++++++
>  target-i386/kvm-stub.c |  5 +++++
>  target-i386/kvm.c      | 13 +++++++++++++
>  target-i386/kvm_i386.h |  1 +
>  4 files changed, 26 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 269e37e71af4..0304a1bf6f1d 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -32,6 +32,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> +#include "kvm_i386.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -2485,6 +2486,12 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>                       "kernel-irqchip=on, please use 'split|off'.");
>          exit(1);
>      }
> +
> +    if (s->eim_supported && kvm_irqchip_in_kernel() &&
> +        !kvm_enable_x2apic()) {
> +        error_report("EIM requires support from the KVM side (X2APIC_API).");
> +        exit(1);
> +    }
>  }
>  
>  static void vtd_class_init(ObjectClass *klass, void *data)
> diff --git a/target-i386/kvm-stub.c b/target-i386/kvm-stub.c
> index cdf15061091d..bda4dc2f0c57 100644
> --- a/target-i386/kvm-stub.c
> +++ b/target-i386/kvm-stub.c
> @@ -25,6 +25,11 @@ bool kvm_has_smm(void)
>      return 1;
>  }
>  
> +bool kvm_enable_x2apic(void)
> +{
> +    return false;
> +}
> +
>  /* This function is only called inside conditionals which we
>   * rely on the compiler to optimize out when CONFIG_KVM is not
>   * defined.
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index f1ad805665ad..4c0a4df5eaea 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -128,6 +128,19 @@ bool kvm_allows_irq0_override(void)
>      return !kvm_irqchip_in_kernel() || kvm_has_gsi_routing();
>  }
>  
> +static bool kvm_x2apic_api_set_flags(uint64_t flags)
> +{
> +    KVMState *s = KVM_STATE(current_machine->accelerator);
> +
> +    return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
> +}
> +
> +bool kvm_enable_x2apic(void)
> +{
maybe it should be:

static bool enabled_x2apic;
if (!enabled_x2apic && kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
                                                KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)) {
    enabled_x2apic = true;
}

return enabled_x2apic;

so that it could be called from multiple sites.

> +    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> +                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
> +}
> +


>  static int kvm_get_tsc(CPUState *cs)
>  {
>      X86CPU *cpu = X86_CPU(cs);
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 42b00af1b1c3..559dd8b5acd2 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -41,4 +41,5 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector,
>  int kvm_device_msix_assign(KVMState *s, uint32_t dev_id);
>  int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
>  
> +bool kvm_enable_x2apic(void);
>  #endif