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

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

[Radim Krčmář]

v3: http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg08292.html

The x2APIC mode works on >=2.8 machine types with this series and <2.7
remain compatible (aka broken).  If the 2.7 compatibility layer, [7/8],
is deemed acceptable, then it should be squashed into [6/8] to avoid a
bisection breaker; see the discussion under [v2 7/7] for details
(http://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg07795.html).


Radim Krčmář (8):
  apic: add global apic_get_class()
  apic: add send_msi() to APICCommonClass
  intel_iommu: pass whole remapped addresses to apic
  intel_iommu: redo configuraton check in realize
  intel_iommu: add OnOffAuto intr_eim as "eim" property
  intel_iommu: reject broken EIM
  intel_iommu: keep buggy EIM enabled in 2.7 machine type
  target-i386/kvm: cache the return value of kvm_enable_x2apic()

 hw/i386/intel_iommu.c           | 81 ++++++++++++++++++++++++++++++-----------
 hw/i386/kvm/apic.c              | 19 +++++++---
 hw/i386/xen/xen_apic.c          |  6 +++
 hw/intc/apic.c                  |  8 +++-
 hw/intc/apic_common.c           |  1 +
 include/hw/compat.h             |  4 ++
 include/hw/i386/apic_internal.h |  6 +++
 include/hw/i386/intel_iommu.h   |  2 +
 target-i386/cpu.c               | 13 +++++--
 target-i386/kvm-stub.c          |  5 +++
 target-i386/kvm.c               | 26 +++++++++++++
 target-i386/kvm_i386.h          |  1 +
 12 files changed, 140 insertions(+), 32 deletions(-)

-- 
2.10.0

[Qemu-devel] [PATCH v4 1/8] 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.

This patch will break compilation if some code uses apic_get_class()
with CONFIG_USER_ONLY.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4: do not use private class attribute [Eduardo]
v3: completely rewrite the mechanism [Eduardo]
v2: assert() instead of error_report() and exit() [Peter]

It still looks horrible, so I'll be glad for any advice.
And what is CONFIG_USER_ONLY?
---
 hw/intc/apic_common.c           |  1 +
 include/hw/i386/apic_internal.h |  2 ++
 target-i386/cpu.c               | 13 ++++++++++---
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index 14ac43c18666..8d01c9c8750e 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"
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 06c4e9f6f95b..286684857e9f 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -222,4 +222,6 @@ 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 */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1c57fce81bcd..13505ab156e0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2848,9 +2848,8 @@ static void mce_init(X86CPU *cpu)
 }
 
 #ifndef CONFIG_USER_ONLY
-static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
+APICCommonClass *apic_get_class(void)
 {
-    APICCommonState *apic;
     const char *apic_type = "apic";
 
     if (kvm_apic_in_kernel()) {
@@ -2859,7 +2858,15 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
         apic_type = "xen-apic";
     }
 
-    cpu->apic_state = DEVICE(object_new(apic_type));
+    return APIC_COMMON_CLASS(object_class_by_name(apic_type));
+}
+
+static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
+{
+    APICCommonState *apic;
+    ObjectClass *apic_class = OBJECT_CLASS(apic_get_class());
+
+    cpu->apic_state = DEVICE(object_new(object_class_get_name(apic_class)));
 
     object_property_add_child(OBJECT(cpu), "lapic",
                               OBJECT(cpu->apic_state), &error_abort);
-- 
2.10.0

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

[Eduardo Habkost]

On Wed, Oct 05, 2016 at 03:06:50PM +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.
> 
> This patch will break compilation if some code uses apic_get_class()
> with CONFIG_USER_ONLY.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

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

[Peter Xu]

On Wed, Oct 05, 2016 at 03:06:50PM +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.
> 
> This patch will break compilation if some code uses apic_get_class()
> with CONFIG_USER_ONLY.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

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

[Qemu-devel] [PATCH v4 2/8] 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.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4: r-b Igor
v2: change apic_send_msi() to accept MSIMessage [Igor]
---
 hw/i386/kvm/apic.c              | 19 +++++++++++++------
 hw/i386/xen/xen_apic.c          |  6 ++++++
 hw/intc/apic.c                  |  8 ++++++--
 include/hw/i386/apic_internal.h |  4 ++++
 4 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index c016e63fc2ba..be55102c00ca 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -169,6 +169,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)
 {
@@ -179,13 +190,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 = {
@@ -232,6 +238,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..fe15fb602473 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -740,8 +740,10 @@ static uint32_t apic_mem_readl(void *opaque, hwaddr addr)
     return val;
 }
 
-static void apic_send_msi(hwaddr addr, uint32_t data)
+static void apic_send_msi(MSIMessage *msi)
 {
+    uint64_t addr = msi->address;
+    uint32_t data = msi->data;
     uint8_t dest = (addr & MSI_ADDR_DEST_ID_MASK) >> MSI_ADDR_DEST_ID_SHIFT;
     uint8_t vector = (data & MSI_DATA_VECTOR_MASK) >> MSI_DATA_VECTOR_SHIFT;
     uint8_t dest_mode = (addr >> MSI_ADDR_DEST_MODE_SHIFT) & 0x1;
@@ -762,7 +764,8 @@ static void apic_mem_writel(void *opaque, hwaddr addr, uint32_t val)
          * 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. */
-        apic_send_msi(addr, val);
+        MSIMessage msi = { .address = addr, .data = val };
+        apic_send_msi(&msi);
         return;
     }
 
@@ -913,6 +916,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;
 }
 
 static const TypeInfo apic_info = {
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 286684857e9f..cdd11fb0938f 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 v4 2/8] apic: add send_msi() to APICCommonClass

[Peter Xu]

On Wed, Oct 05, 2016 at 03:06:51PM +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.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

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

And...

> ---
> v4: r-b Igor
> v2: change apic_send_msi() to accept MSIMessage [Igor]
> ---
>  hw/i386/kvm/apic.c              | 19 +++++++++++++------
>  hw/i386/xen/xen_apic.c          |  6 ++++++
>  hw/intc/apic.c                  |  8 ++++++--
>  include/hw/i386/apic_internal.h |  4 ++++
>  4 files changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index c016e63fc2ba..be55102c00ca 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -169,6 +169,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));

Maybe use error_report() better? A nit not sufficient for a new spin
though.

And, this patch is assuming MSIMessage as host endianess (which is
good to me). Not sure whether we need fixes for the whole MSIMessage
cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
can do it afterwards since it won't break anything AFAIU.

-- peterx

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

[Michael S. Tsirkin]

On Sat, Oct 08, 2016 at 02:37:59PM +0800, Peter Xu wrote:
> On Wed, Oct 05, 2016 at 03:06:51PM +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.
> > 
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> And...
> 
> > ---
> > v4: r-b Igor
> > v2: change apic_send_msi() to accept MSIMessage [Igor]
> > ---
> >  hw/i386/kvm/apic.c              | 19 +++++++++++++------
> >  hw/i386/xen/xen_apic.c          |  6 ++++++
> >  hw/intc/apic.c                  |  8 ++++++--
> >  include/hw/i386/apic_internal.h |  4 ++++
> >  4 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> > index c016e63fc2ba..be55102c00ca 100644
> > --- a/hw/i386/kvm/apic.c
> > +++ b/hw/i386/kvm/apic.c
> > @@ -169,6 +169,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));
> 
> Maybe use error_report() better? A nit not sufficient for a new spin
> though.
> 
> And, this patch is assuming MSIMessage as host endianess (which is
> good to me). Not sure whether we need fixes for the whole MSIMessage
> cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
> can do it afterwards since it won't break anything AFAIU.
> 
> -- peterx

I think this is a bug really. Paolo?

We really need to start annotating le fields e.g. the way
Linux does this, to allow static checks with sparse.

-- 
MST

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

[Radim Krčmář]

2016-10-08 14:37+0800, Peter Xu:
> On Wed, Oct 05, 2016 at 03:06:51PM +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.
>> 
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks.

>> ---
>> v4: r-b Igor
>> v2: change apic_send_msi() to accept MSIMessage [Igor]
>> ---
>>  hw/i386/kvm/apic.c              | 19 +++++++++++++------
>>  hw/i386/xen/xen_apic.c          |  6 ++++++
>>  hw/intc/apic.c                  |  8 ++++++--
>>  include/hw/i386/apic_internal.h |  4 ++++
>>  4 files changed, 29 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
>> index c016e63fc2ba..be55102c00ca 100644
>> --- a/hw/i386/kvm/apic.c
>> +++ b/hw/i386/kvm/apic.c
>> @@ -169,6 +169,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));
> 
> Maybe use error_report() better? A nit not sufficient for a new spin
> though.

Yes, but it is a separate logical change, so I'd do that in a separate
cleaup patch that amends it in the whole file ...
(And I prefer to copy-paste code verbatim.)

> And, this patch is assuming MSIMessage as host endianess (which is
> good to me). Not sure whether we need fixes for the whole MSIMessage
> cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
> can do it afterwards since it won't break anything AFAIU.

This patch doesn't affect existing callers, and IR known to be broken,
so I think we can sort endianess out later ...

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

[Peter Xu]

On Mon, Oct 10, 2016 at 03:35:26PM +0200, Radim Krčmář wrote:

[...]

> Yes, but it is a separate logical change, so I'd do that in a separate
> cleaup patch that amends it in the whole file ...
> (And I prefer to copy-paste code verbatim.)

Sure.

> 
> > And, this patch is assuming MSIMessage as host endianess (which is
> > good to me). Not sure whether we need fixes for the whole MSIMessage
> > cleanup (after all, kvm_irqchip_send_msi() is taking it as LE). Or we
> > can do it afterwards since it won't break anything AFAIU.
> 
> This patch doesn't affect existing callers, and IR known to be broken,
> so I think we can sort endianess out later ...

Agree. Patch:

  [PATCH] Revert "KVM: MSI: Swap payload to native endianness"

is for the cleanup actually. Though I am not sure whether that's
enough and correct. Looking forward to any review comments.

Thanks,

-- peterx

[Qemu-devel] [PATCH v4 3/8] 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.

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4: r-b Igor
v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter]
---
 hw/i386/intel_iommu.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 9f4e64af1ad5..c39b62b898d8 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,17 @@ 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);
+    VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32,
+                msi.address, msi.data);
+    apic_get_class()->send_msi(&msi);
 }
 
 /* Generate a fault event to software via MSI if conditions are met.
@@ -2133,6 +2133,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;
@@ -2281,11 +2282,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 v4 3/8] intel_iommu: pass whole remapped addresses to apic

[Igor Mammedov]

On Wed,  5 Oct 2016 15:06:52 +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.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
I don't recall giving my RB to this patch but I do recall asking question,
see below.

> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> v4: r-b Igor
> v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter]
> ---
>  hw/i386/intel_iommu.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 9f4e64af1ad5..c39b62b898d8 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,17 @@ 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);
> +    VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32,
> +                msi.address, msi.data);
> +    apic_get_class()->send_msi(&msi);
>  }
>  
>  /* Generate a fault event to software via MSI if conditions are met.
> @@ -2133,6 +2133,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;

what about BE host? should it be:
 msg.__addr_hi = cpu_to_le32(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;
> @@ -2281,11 +2282,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;
>  }

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

[Radim Krčmář]

2016-10-07 15:05+0200, Igor Mammedov:
> On Wed,  5 Oct 2016 15:06:52 +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.
>> 
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> I don't recall giving my RB to this patch but I do recall asking question,
> see below.

Crap, sorry.

>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>> v4: r-b Igor
>> v2: fix build with enabled DEBUG_INTEL_IOMMU [Peter]
>> 
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> @@ -2133,6 +2133,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;
> 
> what about BE host?

KVM accepts the address in host endianess and QEMU/VTD code also uses
host endianess for internal representation of memory addresses, so this
hunk should be fine.

It is confusing, because the VTD is definitely broken with respect to
endianess -- it is even trying to swap the order of bits in a byte in
the definition of VTD_MSIMessage.
I don't believe that dma_memory_write() accepted LE address on BE hosts,
so the existing code for filling the address is wrong:

      msg.__addr_head = cpu_to_le32(0xfee);

>                     should it be:
>  msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00)

I don't think so.

Howewer, there are endianess bugs in this patch:

>> @@ -2281,11 +2282,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);

because dma_memory_write() does magic on data.

I don't understand how the code should have worked before this series,
because kvm_apic_mem_write() expects data in little endian and
apic_mem_writel() expects data in host endian, even though both of them
are DEVICE_NATIVE_ENDIAN and are called the same way, so one shouldn't
work.

And similarly, this hunk is wrong:

>> @@ -279,18 +280,17 @@ 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);
>> +    VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32,
>> +                msi.address, msi.data);
>> +    apic_get_class()->send_msi(&msi);
>>  }

It should have been wrong even before, because address_space_stl_le()
seems to accept the address in host endianess and not in LE ... UGH.

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

[Peter Xu]

On Fri, Oct 07, 2016 at 06:24:15PM +0200, Radim Krčmář wrote:

[...]

> KVM accepts the address in host endianess and QEMU/VTD code also uses
> host endianess for internal representation of memory addresses, so this
> hunk should be fine.
> 
> It is confusing, because the VTD is definitely broken with respect to
> endianess -- it is even trying to swap the order of bits in a byte in
> the definition of VTD_MSIMessage.
> I don't believe that dma_memory_write() accepted LE address on BE hosts,
> so the existing code for filling the address is wrong:
> 
>       msg.__addr_head = cpu_to_le32(0xfee);

Yeah. This is my fault. Sorry for the troubles.

I have a patch (as well...) to fix this in my local tree, but not
posted (as mst suggested). Maybe it's time to post some of them now (I
tried to make patches more into a bunch so that they won't be lost in
mailing list in case maintainer missed it). I agree that current code
is never tested on big endian machines yet.

Here it should be:

    msg.__addr_head = 0xfee;

> 
> >                     should it be:
> >  msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00)
> 
> I don't think so.
> 
> Howewer, there are endianess bugs in this patch:
> 
> >> @@ -2281,11 +2282,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);
> 
> because dma_memory_write() does magic on data.
> 
> I don't understand how the code should have worked before this series,
> because kvm_apic_mem_write() expects data in little endian and
> apic_mem_writel() expects data in host endian, even though both of them
> are DEVICE_NATIVE_ENDIAN and are called the same way, so one shouldn't
> work.

I guess that's because APIC is only used for x86? Then it does not
matter. And I agree with you that currently MSI is a little bit
confused on endianess.

First of all, I believe in the protocol MSI (along with PCI logics)
should be LE.

Instead, our MSIMessage struct looks more like to be for host
endianess. E.g. in msi_get_message() we are using:

    msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));

and pci_get_word() is converting LE to host endianess.

However, in all kvm_irqchip_*() APIs, we are assuming MSIMessage as
LE. E.g.:

int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
{
    struct kvm_msi msi;
    KVMMSIRoute *route;

    if (kvm_direct_msi_allowed) {
        ...
        msi.data = le32_to_cpu(msg.data);
        ...
    }
}

These things are conflicting.

Maybe we need to clean this up. And I prefer MSIMessage to be host
endianess.

> 
> And similarly, this hunk is wrong:
> 
> >> @@ -279,18 +280,17 @@ 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);
> >> +    VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32,
> >> +                msi.address, msi.data);
> >> +    apic_get_class()->send_msi(&msi);
> >>  }
> 
> It should have been wrong even before, because address_space_stl_le()
> seems to accept the address in host endianess and not in LE ... UGH.

Again, I guess no one is running VT-d in BE machines. So problems are
not exposed.

Thanks,

-- peterx

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

[Peter Xu]

On Sat, Oct 08, 2016 at 02:14:09PM +0800, Peter Xu wrote:
> On Fri, Oct 07, 2016 at 06:24:15PM +0200, Radim Krčmář wrote:
> 
> [...]
> 
> > KVM accepts the address in host endianess and QEMU/VTD code also uses
> > host endianess for internal representation of memory addresses, so this
> > hunk should be fine.
> > 
> > It is confusing, because the VTD is definitely broken with respect to
> > endianess -- it is even trying to swap the order of bits in a byte in
> > the definition of VTD_MSIMessage.
> > I don't believe that dma_memory_write() accepted LE address on BE hosts,
> > so the existing code for filling the address is wrong:
> > 
> >       msg.__addr_head = cpu_to_le32(0xfee);
> 
> Yeah. This is my fault. Sorry for the troubles.
> 
> I have a patch (as well...) to fix this in my local tree, but not
> posted (as mst suggested). Maybe it's time to post some of them now (I
> tried to make patches more into a bunch so that they won't be lost in
> mailing list in case maintainer missed it).

I'll send them after your series to avoid unecessary conflicts.

-- peterx

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

[Igor Mammedov]

On Sat, 8 Oct 2016 14:14:09 +0800
Peter Xu <peterx@redhat.com> wrote:

> On Fri, Oct 07, 2016 at 06:24:15PM +0200, Radim Krčmář wrote:
> 
> [...]
> 
> > KVM accepts the address in host endianess and QEMU/VTD code also uses
> > host endianess for internal representation of memory addresses, so this
> > hunk should be fine.
> > 
> > It is confusing, because the VTD is definitely broken with respect to
> > endianess -- it is even trying to swap the order of bits in a byte in
> > the definition of VTD_MSIMessage.
> > I don't believe that dma_memory_write() accepted LE address on BE hosts,
> > so the existing code for filling the address is wrong:
> > 
> >       msg.__addr_head = cpu_to_le32(0xfee);  
> 
> Yeah. This is my fault. Sorry for the troubles.
> 
> I have a patch (as well...) to fix this in my local tree, but not
> posted (as mst suggested). Maybe it's time to post some of them now (I
> tried to make patches more into a bunch so that they won't be lost in
> mailing list in case maintainer missed it). I agree that current code
> is never tested on big endian machines yet.
> 
> Here it should be:
> 
>     msg.__addr_head = 0xfee;
> 
> >   
> > >                     should it be:
> > >  msg.__addr_hi = cpu_to_le32(irq->dest & 0xffffff00)  
> > 
> > I don't think so.
> > 
> > Howewer, there are endianess bugs in this patch:
> >   
> > >> @@ -2281,11 +2282,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);  
> > 
> > because dma_memory_write() does magic on data.
> > 
> > I don't understand how the code should have worked before this series,
> > because kvm_apic_mem_write() expects data in little endian and
> > apic_mem_writel() expects data in host endian, even though both of them
> > are DEVICE_NATIVE_ENDIAN and are called the same way, so one shouldn't
> > work.  
> 
> I guess that's because APIC is only used for x86? Then it does not
> matter. And I agree with you that currently MSI is a little bit
> confused on endianess.
> 
> First of all, I believe in the protocol MSI (along with PCI logics)
> should be LE.
> 
> Instead, our MSIMessage struct looks more like to be for host
> endianess. E.g. in msi_get_message() we are using:
> 
>     msg.data = pci_get_word(dev->config + msi_data_off(dev, msi64bit));
> 
> and pci_get_word() is converting LE to host endianess.
> 
> However, in all kvm_irqchip_*() APIs, we are assuming MSIMessage as
> LE. E.g.:
> 
> int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
> {
>     struct kvm_msi msi;
>     KVMMSIRoute *route;
> 
>     if (kvm_direct_msi_allowed) {
>         ...
>         msi.data = le32_to_cpu(msg.data);
>         ...
>     }
> }
> 
> These things are conflicting.
> 
> Maybe we need to clean this up. And I prefer MSIMessage to be host
> endianess.
Mostly internal QEMU structures are in host order and converted
to guest byte-order on transfer, except for structures that are
memcpy-ed, in which case they are typically marked QEMU_PACKED
and that throws flag to reviewers to check if endianess is correct.

At least struct VTD_MSIMessage definition need a comment saying
what byte-order is expected.

> 
> > 
> > And similarly, this hunk is wrong:
> >   
> > >> @@ -279,18 +280,17 @@ 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);
> > >> +    VTD_DPRINTF(FLOG, "msi: addr 0x%"PRIx64 " data 0x%"PRIx32,
> > >> +                msi.address, msi.data);
> > >> +    apic_get_class()->send_msi(&msi);
> > >>  }  
> > 
> > It should have been wrong even before, because address_space_stl_le()
> > seems to accept the address in host endianess and not in LE ... UGH.  
> 
> Again, I guess no one is running VT-d in BE machines. So problems are
> not exposed.
> 
> Thanks,
> 
> -- peterx
> 

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

[Peter Xu]

On Wed, Oct 05, 2016 at 03:06:52PM +0200, Radim Krčmář 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.
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Again we are assuming MSIMessage as host endianess here.

If there is more spin for this one, maybe we can consider providing a
common function for:

    apic_get_class()->send_msi(&msi);

And call it in the two places. But this one is good enough for me, so:

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

Thanks,

-- peterx

[Qemu-devel] [PATCH v4 4/8] intel_iommu: redo configuraton check in realize

[Radim Krčmář]

* there no point in configuring the device if realization is going to
  fail, so move the check to the beginning,
* create a separate function for the check,
* use error_setg() instead error_report().

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4: r-b Igor
v3:
* use error_setg [Paolo]
* create a new function [Peter]
---
 hw/i386/intel_iommu.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c39b62b898d8..5b06b4091f36 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "intel_iommu_internal.h"
@@ -2448,6 +2449,18 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &vtd_as->as;
 }
 
+static bool vtd_check_config(X86IOMMUState *x86_iommu, Error **errp)
+{
+    /* Currently Intel IOMMU IR only support "kernel-irqchip={off|split}" */
+    if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
+        !kvm_irqchip_is_split()) {
+        error_setg(errp, "Intel Interrupt Remapping cannot work with "
+                         "kernel-irqchip=on, please use 'split|off'.");
+        return false;
+    }
+    return true;
+}
+
 static void vtd_realize(DeviceState *dev, Error **errp)
 {
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
@@ -2457,6 +2470,11 @@ static void vtd_realize(DeviceState *dev, Error **errp)
 
     VTD_DPRINTF(GENERAL, "");
     x86_iommu->type = TYPE_INTEL;
+
+    if (!vtd_check_config(x86_iommu, errp)) {
+        return;
+    }
+
     memset(s->vtd_as_by_bus_num, 0, sizeof(s->vtd_as_by_bus_num));
     memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
                           "intel_iommu", DMAR_REG_SIZE);
@@ -2471,14 +2489,6 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     pci_setup_iommu(bus, vtd_host_dma_iommu, dev);
     /* Pseudo address space under root PCI bus. */
     pcms->ioapic_as = vtd_host_dma_iommu(bus, s, Q35_PSEUDO_DEVFN_IOAPIC);
-
-    /* Currently Intel IOMMU IR only support "kernel-irqchip={off|split}" */
-    if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
-        !kvm_irqchip_is_split()) {
-        error_report("Intel Interrupt Remapping cannot work with "
-                     "kernel-irqchip=on, please use 'split|off'.");
-        exit(1);
-    }
 }
 
 static void vtd_class_init(ObjectClass *klass, void *data)
-- 
2.10.0

Re: [Qemu-devel] [PATCH v4 4/8] intel_iommu: redo configuraton check in realize

[Peter Xu]

On Wed, Oct 05, 2016 at 03:06:53PM +0200, Radim Krčmář wrote:
> * there no point in configuring the device if realization is going to
>   fail, so move the check to the beginning,
> * create a separate function for the check,
> * use error_setg() instead error_report().
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

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

[Qemu-devel] [PATCH v4 5/8] intel_iommu: add OnOffAuto intr_eim as "eim" property

[Radim Krčmář]

The default (auto) emulates the current behavior.
A user can now control EIM like
  -device intel-iommu,intremap=on,eim=off

Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4:
 * r-b Igor
 * added an example to the commit message
v3:
 * use error_setg [Paolo]
 * shorten the code [Peter]
---
 hw/i386/intel_iommu.c         | 24 +++++++++++++++++++++---
 include/hw/i386/intel_iommu.h |  1 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 5b06b4091f36..17892b8c336b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2012,6 +2012,8 @@ static const MemoryRegionOps vtd_mem_ops = {
 
 static Property vtd_properties[] = {
     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
+    DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
+                            ON_OFF_AUTO_AUTO),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2368,7 +2370,11 @@ 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->intr_eim == ON_OFF_AUTO_ON) {
+            s->ecap |= VTD_ECAP_EIM;
+        }
+        assert(s->intr_eim != ON_OFF_AUTO_AUTO);
     }
 
     vtd_reset_context_cache(s);
@@ -2449,8 +2455,10 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn)
     return &vtd_as->as;
 }
 
-static bool vtd_check_config(X86IOMMUState *x86_iommu, Error **errp)
+static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
 {
+    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
+
     /* Currently Intel IOMMU IR only support "kernel-irqchip={off|split}" */
     if (x86_iommu->intr_supported && kvm_irqchip_in_kernel() &&
         !kvm_irqchip_is_split()) {
@@ -2458,6 +2466,16 @@ static bool vtd_check_config(X86IOMMUState *x86_iommu, Error **errp)
                          "kernel-irqchip=on, please use 'split|off'.");
         return false;
     }
+    if (s->intr_eim == ON_OFF_AUTO_ON && !x86_iommu->intr_supported) {
+        error_setg(errp, "eim=on cannot be selected without intremap=on");
+        return false;
+    }
+
+    if (s->intr_eim == ON_OFF_AUTO_AUTO) {
+        s->intr_eim = x86_iommu->intr_supported ?
+                                              ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+    }
+
     return true;
 }
 
@@ -2471,7 +2489,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
     VTD_DPRINTF(GENERAL, "");
     x86_iommu->type = TYPE_INTEL;
 
-    if (!vtd_check_config(x86_iommu, errp)) {
+    if (!vtd_decide_config(s, errp)) {
         return;
     }
 
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index a42dbd745a70..b5ac60927b1f 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 */
+    OnOffAuto intr_eim;             /* Toggle for EIM cabability */
 };
 
 /* Find the VTD Address space associated with the given bus pointer,
-- 
2.10.0

Re: [Qemu-devel] [PATCH v4 5/8] intel_iommu: add OnOffAuto intr_eim as "eim" property

[Peter Xu]

On Wed, Oct 05, 2016 at 03:06:54PM +0200, Radim Krčmář wrote:
> The default (auto) emulates the current behavior.
> A user can now control EIM like
>   -device intel-iommu,intremap=on,eim=off
> 
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

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

[Qemu-devel] [PATCH v4 6/8] intel_iommu: reject broken EIM

[Radim Krčmář]

Cluster x2APIC cannot work without KVM's x2apic API when the maximal
APIC ID is greater than 8 and only KVM's LAPIC can support x2APIC, so we
forbid other APICs and also the old KVM case with less than 9, to
simplify the code.

There is no point in enabling EIM in forbidden APICs, so we keep it
enabled only for the KVM APIC;  unconditionally, because making the
option depend on KVM version would be a maintanance burden.

Old QEMUs would enable eim whenever intremap was on, which would trick
guests into thinking that they can enable cluster x2APIC even if any
interrupt destination would get clamped to 8 bits.
Depending on your configuration, QEMU could notice that the destination
LAPIC is not present and report it with a very non-obvious:

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

Or the guest could say something about unexpected interrupts, because
clamping leads to aliasing so interrupts were being delivered to
incorrect VCPUs.

KVM_X2APIC_API is the feature that allows us to enable eim for KVM.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4: be more specific in the comment [Igor]
v3:
 * use error_setg [Paolo]
 * shorten the code [Peter]
v2:
 * adapt to new intr_eim parameter
 * provide first linux version that has x2apic api
 * disable QEMU's LAPIC
---
 hw/i386/intel_iommu.c  | 15 ++++++++++++++-
 target-i386/kvm-stub.c |  5 +++++
 target-i386/kvm.c      | 13 +++++++++++++
 target-i386/kvm_i386.h |  1 +
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 17892b8c336b..efb018b85544 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -33,6 +33,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
@@ -2472,10 +2473,22 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
     }
 
     if (s->intr_eim == ON_OFF_AUTO_AUTO) {
-        s->intr_eim = x86_iommu->intr_supported ?
+        s->intr_eim = x86_iommu->intr_supported && kvm_irqchip_in_kernel() ?
                                               ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
 
+    if (s->intr_eim == ON_OFF_AUTO_ON) {
+        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
+            error_setg(errp, "eim=on requires support on the KVM side"
+                             "(X2APIC_API, first shipped in v4.7)");
+            return false;
+        }
+        if (!kvm_irqchip_in_kernel()) {
+            error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
+            return false;
+        }
+    }
+
     return true;
 }
 
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 ee1f53e56953..0fd664648665 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -122,6 +122,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 36407e0a5dc7..5c369b1c5b40 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -43,4 +43,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 
 void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
 
+bool kvm_enable_x2apic(void);
 #endif
-- 
2.10.0

Re: [Qemu-devel] [PATCH v4 6/8] intel_iommu: reject broken EIM

[Peter Xu]

On Wed, Oct 05, 2016 at 03:06:55PM +0200, Radim Krčmář wrote:

[...]

> @@ -2472,10 +2473,22 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>      }
>  
>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> -        s->intr_eim = x86_iommu->intr_supported ?
> +        s->intr_eim = x86_iommu->intr_supported && kvm_irqchip_in_kernel() ?
>                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
>  
> +    if (s->intr_eim == ON_OFF_AUTO_ON) {
> +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> +            error_setg(errp, "eim=on requires support on the KVM side"
> +                             "(X2APIC_API, first shipped in v4.7)");
> +            return false;
> +        }
> +        if (!kvm_irqchip_in_kernel()) {
> +            error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
> +            return false;
> +        }

I would prefer:

  if (kvm_irqchip_in_kernel()) {
      if (!kvm_enable_x2apic()) {
          error("enable x2apic failed");
          return false;
      }
  } else {
      error("need split irqchip");
      return false;
  }

But that's really a matter of taste. So:

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

Re: [Qemu-devel] [PATCH v4 6/8] intel_iommu: reject broken EIM

[Radim Krčmář]

2016-10-08 15:21+0800, Peter Xu:
> On Wed, Oct 05, 2016 at 03:06:55PM +0200, Radim Krčmář wrote:
> 
> [...]
> 
>> @@ -2472,10 +2473,22 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>      }
>>  
>>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
>> -        s->intr_eim = x86_iommu->intr_supported ?
>> +        s->intr_eim = x86_iommu->intr_supported && kvm_irqchip_in_kernel() ?
>>                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>>      }
>>  
>> +    if (s->intr_eim == ON_OFF_AUTO_ON) {
>> +        if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>> +            error_setg(errp, "eim=on requires support on the KVM side"
>> +                             "(X2APIC_API, first shipped in v4.7)");
>> +            return false;
>> +        }
>> +        if (!kvm_irqchip_in_kernel()) {
>> +            error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
>> +            return false;
>> +        }
> 
> I would prefer:
> 
>   if (kvm_irqchip_in_kernel()) {
>       if (!kvm_enable_x2apic()) {
>           error("enable x2apic failed");
>           return false;
>       }
>   } else {
>       error("need split irqchip");
>       return false;
>   }

Yeah, it looks better, thanks.

> But that's really a matter of taste. So:

I'll currently go for an implicit else: (because 4 levels of indentation
are getting helper-function worthy and it has less curly braces)

        if (!kvm_irqchip_in_kernel()) {
            error("need split irqchip");
            return false;
        }
        if (!kvm_enable_x2apic()) {
            error("enable x2apic failed");
            return false;
        }

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

I squashed [7/8] into this patch in v5 and the second one didn't have
your r-b, so I made the change as I'd have to drop the r-b anyway.

Re: [Qemu-devel] [PATCH v4 6/8] intel_iommu: reject broken EIM

[Peter Xu]

On Mon, Oct 10, 2016 at 05:11:19PM +0200, Radim Krčmář wrote:

[...]

> > But that's really a matter of taste. So:
> 
> I'll currently go for an implicit else: (because 4 levels of indentation
> are getting helper-function worthy and it has less curly braces)
> 
>         if (!kvm_irqchip_in_kernel()) {
>             error("need split irqchip");
>             return false;
>         }
>         if (!kvm_enable_x2apic()) {
>             error("enable x2apic failed");
>             return false;
>         }

Good to me.

> 
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> I squashed [7/8] into this patch in v5 and the second one didn't have
> your r-b, so I made the change as I'd have to drop the r-b anyway.

Sure. Thanks,

-- peterx

[Qemu-devel] [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type

[Radim Krčmář]

QEMU 2.7 allowed EIM even in configurations that were forbidden in the
last patch because they were not working, like old KVM or userspace
APIC.  In order to keep backward compatibility, we again allow guests to
misbehave in non-obvious ways, and make it the default for old machine
types.

A user can enable the buggy mode it with "buggy_eim=on", which is weird,
but I don't know how to add a private property.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4:
 * use a device property [Igor]
 * clarify the last sentence of the commit message
v3: shorten the code [Peter]
---
 hw/i386/intel_iommu.c         | 7 ++++---
 include/hw/compat.h           | 4 ++++
 include/hw/i386/intel_iommu.h | 1 +
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index efb018b85544..fe257e8357b4 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2015,6 +2015,7 @@ static Property vtd_properties[] = {
     DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
     DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
                             ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_BOOL("buggy_eim", IntelIOMMUState, buggy_eim, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2473,11 +2474,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
     }
 
     if (s->intr_eim == ON_OFF_AUTO_AUTO) {
-        s->intr_eim = x86_iommu->intr_supported && kvm_irqchip_in_kernel() ?
+        s->intr_eim = (kvm_irqchip_in_kernel() || s->buggy_eim)
+                      && x86_iommu->intr_supported ?
                                               ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
     }
-
-    if (s->intr_eim == ON_OFF_AUTO_ON) {
+    if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
         if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
             error_setg(errp, "eim=on requires support on the KVM side"
                              "(X2APIC_API, first shipped in v4.7)");
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 46412b229a70..43b50065e082 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -10,6 +10,10 @@
         .driver   = "ioapic",\
         .property = "version",\
         .value    = "0x11",\
+    },{\
+        .driver   = "intel-iommu",\
+        .property = "buggy_eim",\
+        .value    = "true",\
     },
 
 #define HW_COMPAT_2_6 \
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index b5ac60927b1f..1989c1eec10a 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -290,6 +290,7 @@ struct IntelIOMMUState {
     uint32_t intr_size;             /* Number of IR table entries */
     bool intr_eime;                 /* Extended interrupt mode enabled */
     OnOffAuto intr_eim;             /* Toggle for EIM cabability */
+    bool buggy_eim;                 /* Force buggy EIM unless eim=off */
 };
 
 /* Find the VTD Address space associated with the given bus pointer,
-- 
2.10.0

Re: [Qemu-devel] [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type

[Eduardo Habkost]

On Wed, Oct 05, 2016 at 03:06:56PM +0200, Radim Krčmář wrote:
> QEMU 2.7 allowed EIM even in configurations that were forbidden in the
> last patch because they were not working, like old KVM or userspace
> APIC.  In order to keep backward compatibility, we again allow guests to
> misbehave in non-obvious ways, and make it the default for old machine
> types.
> 
> A user can enable the buggy mode it with "buggy_eim=on", which is weird,
> but I don't know how to add a private property.

Properties et by compat_props are always user-visible. I believe
that's a feature (this way, it will be possible to let management
software and other tools know what exactly is behind a
machine-type).

Additional comment below:

> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
> v4:
>  * use a device property [Igor]
>  * clarify the last sentence of the commit message
> v3: shorten the code [Peter]
> ---
>  hw/i386/intel_iommu.c         | 7 ++++---
>  include/hw/compat.h           | 4 ++++
>  include/hw/i386/intel_iommu.h | 1 +
>  3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index efb018b85544..fe257e8357b4 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2015,6 +2015,7 @@ static Property vtd_properties[] = {
>      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>      DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
>                              ON_OFF_AUTO_AUTO),
> +    DEFINE_PROP_BOOL("buggy_eim", IntelIOMMUState, buggy_eim, false),

I suggest "buggy-eim", to follow the usual style for QOM property
names.

Assuming the name gets changed:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2473,11 +2474,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>      }
>  
>      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> -        s->intr_eim = x86_iommu->intr_supported && kvm_irqchip_in_kernel() ?
> +        s->intr_eim = (kvm_irqchip_in_kernel() || s->buggy_eim)
> +                      && x86_iommu->intr_supported ?
>                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
> -
> -    if (s->intr_eim == ON_OFF_AUTO_ON) {
> +    if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
>          if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
>              error_setg(errp, "eim=on requires support on the KVM side"
>                               "(X2APIC_API, first shipped in v4.7)");
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 46412b229a70..43b50065e082 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -10,6 +10,10 @@
>          .driver   = "ioapic",\
>          .property = "version",\
>          .value    = "0x11",\
> +    },{\
> +        .driver   = "intel-iommu",\
> +        .property = "buggy_eim",\
> +        .value    = "true",\
>      },
>  
>  #define HW_COMPAT_2_6 \
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index b5ac60927b1f..1989c1eec10a 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -290,6 +290,7 @@ struct IntelIOMMUState {
>      uint32_t intr_size;             /* Number of IR table entries */
>      bool intr_eime;                 /* Extended interrupt mode enabled */
>      OnOffAuto intr_eim;             /* Toggle for EIM cabability */
> +    bool buggy_eim;                 /* Force buggy EIM unless eim=off */
>  };
>  
>  /* Find the VTD Address space associated with the given bus pointer,
> -- 
> 2.10.0
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type

[Michael S. Tsirkin]

On Thu, Oct 06, 2016 at 11:51:42AM -0300, Eduardo Habkost wrote:
> On Wed, Oct 05, 2016 at 03:06:56PM +0200, Radim Krčmář wrote:
> > QEMU 2.7 allowed EIM even in configurations that were forbidden in the
> > last patch because they were not working, like old KVM or userspace
> > APIC.  In order to keep backward compatibility, we again allow guests to
> > misbehave in non-obvious ways, and make it the default for old machine
> > types.
> > 
> > A user can enable the buggy mode it with "buggy_eim=on", which is weird,
> > but I don't know how to add a private property.
> 
> Properties et by compat_props are always user-visible. I believe
> that's a feature (this way, it will be possible to let management
> software and other tools know what exactly is behind a
> machine-type).

There's a rule that any name beginning with x- is deemed
experimental. See docs/qmp-spec.txt.
It is a good idea to always use this for compat properties as
we want to be able to drop them when we drop the old
machine type.


> Additional comment below:
> 
> > 
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> > v4:
> >  * use a device property [Igor]
> >  * clarify the last sentence of the commit message
> > v3: shorten the code [Peter]
> > ---
> >  hw/i386/intel_iommu.c         | 7 ++++---
> >  include/hw/compat.h           | 4 ++++
> >  include/hw/i386/intel_iommu.h | 1 +
> >  3 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index efb018b85544..fe257e8357b4 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2015,6 +2015,7 @@ static Property vtd_properties[] = {
> >      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> >      DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
> >                              ON_OFF_AUTO_AUTO),
> > +    DEFINE_PROP_BOOL("buggy_eim", IntelIOMMUState, buggy_eim, false),
> 
> I suggest "buggy-eim", to follow the usual style for QOM property
> names.
> 
> Assuming the name gets changed:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -2473,11 +2474,11 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
> >      }
> >  
> >      if (s->intr_eim == ON_OFF_AUTO_AUTO) {
> > -        s->intr_eim = x86_iommu->intr_supported && kvm_irqchip_in_kernel() ?
> > +        s->intr_eim = (kvm_irqchip_in_kernel() || s->buggy_eim)
> > +                      && x86_iommu->intr_supported ?
> >                                                ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> >      }
> > -
> > -    if (s->intr_eim == ON_OFF_AUTO_ON) {
> > +    if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
> >          if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) {
> >              error_setg(errp, "eim=on requires support on the KVM side"
> >                               "(X2APIC_API, first shipped in v4.7)");
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 46412b229a70..43b50065e082 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -10,6 +10,10 @@
> >          .driver   = "ioapic",\
> >          .property = "version",\
> >          .value    = "0x11",\
> > +    },{\
> > +        .driver   = "intel-iommu",\
> > +        .property = "buggy_eim",\
> > +        .value    = "true",\
> >      },
> >  
> >  #define HW_COMPAT_2_6 \
> > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> > index b5ac60927b1f..1989c1eec10a 100644
> > --- a/include/hw/i386/intel_iommu.h
> > +++ b/include/hw/i386/intel_iommu.h
> > @@ -290,6 +290,7 @@ struct IntelIOMMUState {
> >      uint32_t intr_size;             /* Number of IR table entries */
> >      bool intr_eime;                 /* Extended interrupt mode enabled */
> >      OnOffAuto intr_eim;             /* Toggle for EIM cabability */
> > +    bool buggy_eim;                 /* Force buggy EIM unless eim=off */
> >  };
> >  
> >  /* Find the VTD Address space associated with the given bus pointer,
> > -- 
> > 2.10.0
> > 
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type

[Radim Krčmář]

2016-10-06 18:33+0300, Michael S. Tsirkin:
> On Thu, Oct 06, 2016 at 11:51:42AM -0300, Eduardo Habkost wrote:
>> On Wed, Oct 05, 2016 at 03:06:56PM +0200, Radim Krčmář wrote:
>> > QEMU 2.7 allowed EIM even in configurations that were forbidden in the
>> > last patch because they were not working, like old KVM or userspace
>> > APIC.  In order to keep backward compatibility, we again allow guests to
>> > misbehave in non-obvious ways, and make it the default for old machine
>> > types.
>> > 
>> > A user can enable the buggy mode it with "buggy_eim=on", which is weird,
>> > but I don't know how to add a private property.
>> 
>> Properties et by compat_props are always user-visible. I believe
>> that's a feature (this way, it will be possible to let management
>> software and other tools know what exactly is behind a
>> machine-type).
> 
> There's a rule that any name beginning with x- is deemed
> experimental. See docs/qmp-spec.txt.
> It is a good idea to always use this for compat properties as
> we want to be able to drop them when we drop the old
> machine type.

"x-buggy-eim" should deter most users, thanks.

pc-0.10 seems to be the first machine type ever (2009), is there already
a plan to deprecate it?

[Qemu-devel] Deprecating old machine-types (was Re: [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type)

[Eduardo Habkost]

On Thu, Oct 06, 2016 at 05:55:25PM +0200, Radim Krčmář wrote:
[...]
> pc-0.10 seems to be the first machine type ever (2009), is there already
> a plan to deprecate it?

I don't think we have a plan, but I would support deprecating and
removing very old machine-types. The question is: how old is too
old?

For reference, the commits and dates when each machine-type was
added are below:

machine   commit    commit date  release  release date
pc-0.10   e8b2a1c6  Jul 8 2009   v0.11.0  Sep 22 2009
pc-0.13   95747581  Jul 22 2009  v0.12.0  Dec 19 2009
pc-0.12   2cae6f5e  Jan 8 2010   v0.13.0  Oct 14 2010
pc-0.13   d76fa62d  Feb 15 2010  v0.13.0  Oct 14 2010
pc-0.14   b903a0f7  Nov 11 2010  v0.14.0  Feb 16 2011
pc-0.15   ce01a508  Dec 18 2011  v1.1.0   Jun 1 2012
pc-1.0    19857e62  Nov 7 2011   v1.0     Dec 1 2011
pc-1.1    382b3a68  Feb 21 2012  v1.1.0   Jun 1 2012
pc-1.2    f1dacf1c  Jun 11 2012  v1.2.0   Sep 5 2012
pc-1.3    f4306941  Sep 13 2012  v1.3.0   Dec 3 2012
pc-*-1.4  f1ae2e38  Dec 4 2012   v1.4.0   Feb 15 2013
pc-*-1.5  bf3caa3d  Feb 8 2013   v1.5.0   May 20 2013
pc-*-1.6  45053fde  May 27 2013  v1.6.0   Aug 15 2013
pc-*-1.7  e9845f09  Aug 2 2013   v1.7.0   Nov 27 2013
pc-*-2.0  aeca6e8d  Dec 2 2013   v2.0.0   Apr 17 2014
pc-*-2.1  3458b2b0  Apr 24 2014  v2.1.0   Aug 1 2014
pc-*-2.2  f9f21873  Jul 30 2014  v2.2.0   Dec 9 2014
pc-*-2.3  64bbd372  Dec 5 2014   v2.3.0   Apr 24 2015
pc-*-2.4  5cb50e0a  Apr 23 2015  v2.4.0   Aug 11 2015
pc-*-2.5  87e896ab  Sep 11 2015  v2.5.0   Dec 16 2015
pc-*-2.6  240240d5  Nov 30 2015  v2.6.0   May 11 2016
pc-*-2.7  d86c1451  May 17 2016  v2.7.0   Sep 2 2016
pc-*-2.8  a4d3c834  Sep 7 2016   -        -

-- 
Eduardo

Re: [Qemu-devel] Deprecating old machine-types (was Re: [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type)

[Paolo Bonzini]

On 10/10/2016 19:46, Eduardo Habkost wrote:
> I don't think we have a plan, but I would support deprecating and
> removing very old machine-types. The question is: how old is too
> old?
> 
> For reference, the commits and dates when each machine-type was
> added are below:
> 
> machine   commit    commit date  release  release date
> pc-0.10   e8b2a1c6  Jul 8 2009   v0.11.0  Sep 22 2009
> pc-0.13   95747581  Jul 22 2009  v0.12.0  Dec 19 2009
> pc-0.12   2cae6f5e  Jan 8 2010   v0.13.0  Oct 14 2010
> pc-0.13   d76fa62d  Feb 15 2010  v0.13.0  Oct 14 2010
> pc-0.14   b903a0f7  Nov 11 2010  v0.14.0  Feb 16 2011
> pc-0.15   ce01a508  Dec 18 2011  v1.1.0   Jun 1 2012
> pc-1.0    19857e62  Nov 7 2011   v1.0     Dec 1 2011
> pc-1.1    382b3a68  Feb 21 2012  v1.1.0   Jun 1 2012
> pc-1.2    f1dacf1c  Jun 11 2012  v1.2.0   Sep 5 2012
> pc-1.3    f4306941  Sep 13 2012  v1.3.0   Dec 3 2012

Anything before pc-1.3 has issues with migration due to the introduction
of the memory API.  Basically, 0xf0000-0xfffff is not migrated
correctly, and the result is that rebooting after migration causes the
guest to crash.  So that could be a reasonable place to draw the line at.

Paolo

Re: [Qemu-devel] Deprecating old machine-types (was Re: [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type)

[Daniel P. Berrange]

On Tue, Oct 11, 2016 at 09:36:29AM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/10/2016 19:46, Eduardo Habkost wrote:
> > I don't think we have a plan, but I would support deprecating and
> > removing very old machine-types. The question is: how old is too
> > old?
> > 
> > For reference, the commits and dates when each machine-type was
> > added are below:
> > 
> > machine   commit    commit date  release  release date
> > pc-0.10   e8b2a1c6  Jul 8 2009   v0.11.0  Sep 22 2009
> > pc-0.13   95747581  Jul 22 2009  v0.12.0  Dec 19 2009
> > pc-0.12   2cae6f5e  Jan 8 2010   v0.13.0  Oct 14 2010
> > pc-0.13   d76fa62d  Feb 15 2010  v0.13.0  Oct 14 2010
> > pc-0.14   b903a0f7  Nov 11 2010  v0.14.0  Feb 16 2011
> > pc-0.15   ce01a508  Dec 18 2011  v1.1.0   Jun 1 2012
> > pc-1.0    19857e62  Nov 7 2011   v1.0     Dec 1 2011
> > pc-1.1    382b3a68  Feb 21 2012  v1.1.0   Jun 1 2012
> > pc-1.2    f1dacf1c  Jun 11 2012  v1.2.0   Sep 5 2012
> > pc-1.3    f4306941  Sep 13 2012  v1.3.0   Dec 3 2012
> 
> Anything before pc-1.3 has issues with migration due to the introduction
> of the memory API.  Basically, 0xf0000-0xfffff is not migrated
> correctly, and the result is that rebooting after migration causes the
> guest to crash.  So that could be a reasonable place to draw the line at.

That is a one-off special case - I think it would be desirable to come up
with a general rule we can follow indefinitely, which we can apply at the
start of each release cycle to purge old stuff.

If we wanted to pick pc-1.3 as the starting point and generalize it, we
choose declare we'll support machine types for 4 years. Or we could do
it in terms of number of releases - eg we'll support the last N releases
(for 3/releases per year cadence, that'd be N == 12)

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] Deprecating old machine-types (was Re: [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type)

[Paolo Bonzini]

On 11/10/2016 10:23, Daniel P. Berrange wrote:
> On Tue, Oct 11, 2016 at 09:36:29AM +0200, Paolo Bonzini wrote:
>>
>>
>> On 10/10/2016 19:46, Eduardo Habkost wrote:
>>> I don't think we have a plan, but I would support deprecating and
>>> removing very old machine-types. The question is: how old is too
>>> old?
>>>
>>> For reference, the commits and dates when each machine-type was
>>> added are below:
>>>
>>> machine   commit    commit date  release  release date
>>> pc-0.10   e8b2a1c6  Jul 8 2009   v0.11.0  Sep 22 2009
>>> pc-0.13   95747581  Jul 22 2009  v0.12.0  Dec 19 2009
>>> pc-0.12   2cae6f5e  Jan 8 2010   v0.13.0  Oct 14 2010
>>> pc-0.13   d76fa62d  Feb 15 2010  v0.13.0  Oct 14 2010
>>> pc-0.14   b903a0f7  Nov 11 2010  v0.14.0  Feb 16 2011
>>> pc-0.15   ce01a508  Dec 18 2011  v1.1.0   Jun 1 2012
>>> pc-1.0    19857e62  Nov 7 2011   v1.0     Dec 1 2011
>>> pc-1.1    382b3a68  Feb 21 2012  v1.1.0   Jun 1 2012
>>> pc-1.2    f1dacf1c  Jun 11 2012  v1.2.0   Sep 5 2012
>>> pc-1.3    f4306941  Sep 13 2012  v1.3.0   Dec 3 2012
>>
>> Anything before pc-1.3 has issues with migration due to the introduction
>> of the memory API.  Basically, 0xf0000-0xfffff is not migrated
>> correctly, and the result is that rebooting after migration causes the
>> guest to crash.  So that could be a reasonable place to draw the line at.
> 
> That is a one-off special case - I think it would be desirable to come up
> with a general rule we can follow indefinitely, which we can apply at the
> start of each release cycle to purge old stuff.
> 
> If we wanted to pick pc-1.3 as the starting point and generalize it, we
> choose declare we'll support machine types for 4 years. Or we could do
> it in terms of number of releases - eg we'll support the last N releases
> (for 3/releases per year cadence, that'd be N == 12)

I don't know, it's already boring to create a new machine every time...
I would hate to have to remove one or more machine types every three
months.  Consider that adding new machine types will hardly introduce
bugs; what causes bugs is removing them.

Paolo

Re: [Qemu-devel] Deprecating old machine-types (was Re: [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type)

[Eduardo Habkost]

On Tue, Oct 11, 2016 at 10:47:43AM +0200, Paolo Bonzini wrote:
> 
> 
> On 11/10/2016 10:23, Daniel P. Berrange wrote:
> > On Tue, Oct 11, 2016 at 09:36:29AM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 10/10/2016 19:46, Eduardo Habkost wrote:
> >>> I don't think we have a plan, but I would support deprecating and
> >>> removing very old machine-types. The question is: how old is too
> >>> old?
> >>>
> >>> For reference, the commits and dates when each machine-type was
> >>> added are below:
> >>>
> >>> machine   commit    commit date  release  release date
> >>> pc-0.10   e8b2a1c6  Jul 8 2009   v0.11.0  Sep 22 2009
> >>> pc-0.13   95747581  Jul 22 2009  v0.12.0  Dec 19 2009
> >>> pc-0.12   2cae6f5e  Jan 8 2010   v0.13.0  Oct 14 2010
> >>> pc-0.13   d76fa62d  Feb 15 2010  v0.13.0  Oct 14 2010
> >>> pc-0.14   b903a0f7  Nov 11 2010  v0.14.0  Feb 16 2011
> >>> pc-0.15   ce01a508  Dec 18 2011  v1.1.0   Jun 1 2012
> >>> pc-1.0    19857e62  Nov 7 2011   v1.0     Dec 1 2011
> >>> pc-1.1    382b3a68  Feb 21 2012  v1.1.0   Jun 1 2012
> >>> pc-1.2    f1dacf1c  Jun 11 2012  v1.2.0   Sep 5 2012
> >>> pc-1.3    f4306941  Sep 13 2012  v1.3.0   Dec 3 2012
> >>
> >> Anything before pc-1.3 has issues with migration due to the introduction
> >> of the memory API.  Basically, 0xf0000-0xfffff is not migrated
> >> correctly, and the result is that rebooting after migration causes the
> >> guest to crash.  So that could be a reasonable place to draw the line at.
> > 
> > That is a one-off special case - I think it would be desirable to come up
> > with a general rule we can follow indefinitely, which we can apply at the
> > start of each release cycle to purge old stuff.
> > 
> > If we wanted to pick pc-1.3 as the starting point and generalize it, we
> > choose declare we'll support machine types for 4 years. Or we could do
> > it in terms of number of releases - eg we'll support the last N releases
> > (for 3/releases per year cadence, that'd be N == 12)
> 
> I don't know, it's already boring to create a new machine every time...
> I would hate to have to remove one or more machine types every three
> months.  Consider that adding new machine types will hardly introduce
> bugs; what causes bugs is removing them.

I wouldn't like to _have_ to remove them, but I would love to
have a clear policy that would set user expectations and allow us
to remove some of them once in a while.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type

[Radim Krčmář]

2016-10-06 11:51-0300, Eduardo Habkost:
> On Wed, Oct 05, 2016 at 03:06:56PM +0200, Radim Krčmář wrote:
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> @@ -2015,6 +2015,7 @@ static Property vtd_properties[] = {
>>      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
>>      DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
>>                              ON_OFF_AUTO_AUTO),
>> +    DEFINE_PROP_BOOL("buggy_eim", IntelIOMMUState, buggy_eim, false),
> 
> I suggest "buggy-eim", to follow the usual style for QOM property
> names.
> 
> Assuming the name gets changed:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I'll change the name to "x-buggy-eim" and also squash the patch with
[6/8] as the property doesn't seem to be hated too much.

It's going to be a different patch, so I'll drop the r-b by default,
sorry.

Re: [Qemu-devel] [PATCH v4 7/8] intel_iommu: keep buggy EIM enabled in 2.7 machine type

[Michael S. Tsirkin]

On Thu, Oct 06, 2016 at 06:00:52PM +0200, Radim Krčmář wrote:
> 2016-10-06 11:51-0300, Eduardo Habkost:
> > On Wed, Oct 05, 2016 at 03:06:56PM +0200, Radim Krčmář wrote:
> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> >> ---
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> @@ -2015,6 +2015,7 @@ static Property vtd_properties[] = {
> >>      DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0),
> >>      DEFINE_PROP_ON_OFF_AUTO("eim", IntelIOMMUState, intr_eim,
> >>                              ON_OFF_AUTO_AUTO),
> >> +    DEFINE_PROP_BOOL("buggy_eim", IntelIOMMUState, buggy_eim, false),
> > 
> > I suggest "buggy-eim", to follow the usual style for QOM property
> > names.
> > 
> > Assuming the name gets changed:
> > 
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> I'll change the name to "x-buggy-eim" and also squash the patch with
> [6/8] as the property doesn't seem to be hated too much.
> 
> It's going to be a different patch, so I'll drop the r-b by default,
> sorry.

You don't have to drop the r-b if the change is mechanical or
minor. Anyway, waiting for the next version.

-- 
MST

[Qemu-devel] [PATCH v4 8/8] target-i386/kvm: cache the return value of kvm_enable_x2apic()

[Radim Krčmář]

Assume that KVM would have returned the same on subsequent runs.
Abstract the memoizaiton pattern into macros and call it memorize as
adding the r makes it less obscure.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
v4:
 * changed the name to memorize [Igor]
 * remove useless underscores in macro arguments
 * merge the two macros, as it seems that the deleted one wouldn't get
   other users anytime soon
---
 target-i386/kvm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 0fd664648665..0472f45fd092 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -129,10 +129,23 @@ static bool kvm_x2apic_api_set_flags(uint64_t flags)
     return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
 }
 
+#define MEMORIZE(fn) \
+    ({ \
+        static typeof(fn) _result; \
+        static bool _memorized; \
+        \
+        if (_memorized) { \
+            return _result; \
+        } \
+        _memorized = true; \
+        _result = fn; \
+    })
+
 bool kvm_enable_x2apic(void)
 {
-    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
-                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
+    return MEMORIZE(
+             kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
+                                      KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK));
 }
 
 static int kvm_get_tsc(CPUState *cs)
-- 
2.10.0

Re: [Qemu-devel] [PATCH v4 8/8] target-i386/kvm: cache the return value of kvm_enable_x2apic()

[Igor Mammedov]

On Wed,  5 Oct 2016 15:06:57 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> Assume that KVM would have returned the same on subsequent runs.
> Abstract the memoizaiton pattern into macros and call it memorize as
> adding the r makes it less obscure.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> v4:
>  * changed the name to memorize [Igor]
>  * remove useless underscores in macro arguments
>  * merge the two macros, as it seems that the deleted one wouldn't get
>    other users anytime soon
> ---
>  target-i386/kvm.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 0fd664648665..0472f45fd092 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -129,10 +129,23 @@ static bool kvm_x2apic_api_set_flags(uint64_t flags)
>      return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
>  }
>  
> +#define MEMORIZE(fn) \
> +    ({ \
> +        static typeof(fn) _result; \
> +        static bool _memorized; \
> +        \
> +        if (_memorized) { \
> +            return _result; \
> +        } \
> +        _memorized = true; \
> +        _result = fn; \
> +    })
> +
>  bool kvm_enable_x2apic(void)
>  {
> -    return kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> -                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
> +    return MEMORIZE(
> +             kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS |
> +                                      KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK));
>  }
>  
>  static int kvm_get_tsc(CPUState *cs)