[Qemu-devel] [RFC 0/4] APIC, IOMMU, KVM: add x2APIC interface

[Qemu-devel] [RFC 0/4] APIC, IOMMU, KVM: add x2APIC interface

[Radim Krčmář]

This series bases on Peter's IR v6 and depends on patches that were just
posted to kvm-list, "[RFC 0/9] KVM: x86: break the xAPIC barrier".

The kernel interface could use your comments, but internal QEMU one is
in dire need of them.  Please see [1/4].

I have tested the series and seems to work as well as it can.
Peter's IR v6 didn't boot on my setup, so I reverted to the latest
version I know was working, v4, and rebased paches for testing.
The setup from Igor's latest x2APIC QEMU patches creates two VCPUs,
first has id 0 and second has 280.  Edge IO-APIC and MSI interrupts
were being delivered to both of them, but level didn't work -- only
one interrupt was ever delivered, I blame EOI.

I didn't have enough time to look into IR, but will do so next week.


Radim Krčmář (4):
  apic: add deliver_msi to APICCommonClass
  intel_iommu: use deliver_msi APIC callback
  linux_headers: add MSI_X2APIC
  kvm: support MSI_X2APIC capability

 hw/i386/intel_iommu.c           | 29 ++++++++++++++++++-----------
 hw/i386/kvm/apic.c              | 21 ++++++++++++++-------
 hw/i386/xen/xen_apic.c          |  7 +++++++
 hw/intc/apic.c                  |  7 +++++++
 include/hw/i386/apic_internal.h |  5 +++++
 include/sysemu/kvm.h            |  1 +
 kvm-all.c                       | 14 +++++++++++++-
 linux-headers/linux/kvm.h       |  5 +++++
 target-i386/kvm.c               |  4 ++++
 9 files changed, 74 insertions(+), 19 deletions(-)

-- 
2.8.2

[Qemu-devel] [PATCH 1/4] apic: add deliver_msi to APICCommonClass

[Radim Krčmář]

The only way to send interrupts from outside of APIC devices is to use
the MSI-inspired memory mapped interface.  The interface is not
sufficient because interrupt remapping in extended mode can exceed 8 bit
destination ids.

The proper way to deliver interrupts would be to design a bus (QPI), but
that is a big undertaking.  Real IR unit stores excessive bits in the
high work of MSI address register, which would be bit [63:40] in memory
address space, so I was considering two options:
 - a new special address space for APIC to handle MSI-compatible writes
 - one callback to the APIC class that delivers extended MSIs

This patch uses latter option, because it was easier for me, but I think
the former one could be a tad nicer.

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

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 3c7c8fa00709..7881f13abb96 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -147,6 +147,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
     run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
 }
 
+static void kvm_deliver_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)
 {
@@ -157,13 +168,7 @@ 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_deliver_msi(&msg);
 }
 
 static const MemoryRegionOps kvm_apic_io_ops = {
@@ -202,6 +207,8 @@ 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->deliver_msi = kvm_deliver_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..0f34f63d449d 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_deliver_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,8 @@ 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->deliver_msi = xen_deliver_msi;
 }
 
 static const TypeInfo xen_apic_info = {
diff --git a/hw/intc/apic.c b/hw/intc/apic.c
index 28c2ea540608..b893942c6e73 100644
--- a/hw/intc/apic.c
+++ b/hw/intc/apic.c
@@ -877,6 +877,11 @@ static void apic_realize(DeviceState *dev, Error **errp)
     msi_nonbroken = true;
 }
 
+static void apic_deliver_msi(MSIMessage *msi)
+{
+    apic_send_msi(msi->address, msi->data);
+}
+
 static void apic_class_init(ObjectClass *klass, void *data)
 {
     APICCommonClass *k = APIC_COMMON_CLASS(klass);
@@ -889,6 +894,8 @@ 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->deliver_msi = apic_deliver_msi;
 }
 
 static const TypeInfo apic_info = {
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 74fe935e8eab..227ef30c5027 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -146,6 +146,11 @@ typedef struct APICCommonClass
     void (*pre_save)(APICCommonState *s);
     void (*post_load)(APICCommonState *s);
     void (*reset)(APICCommonState *s);
+
+    /* deliver_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 (*deliver_msi)(MSIMessage *msi);
 } APICCommonClass;
 
 struct APICCommonState {
-- 
2.8.2

[Qemu-devel] [PATCH 2/4] intel_iommu: use deliver_msi APIC callback

[Radim Krčmář]

The memory-mapped interface cannot express x2APIC destinations that are
a result of remapping.

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

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index bee85e469477..d10064289551 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -26,6 +26,7 @@
 #include "hw/pci/pci.h"
 #include "hw/boards.h"
 #include "hw/i386/x86-iommu.h"
+#include "hw/i386/apic_internal.h"
 
 /*#define DEBUG_INTEL_IOMMU*/
 #ifdef DEBUG_INTEL_IOMMU
@@ -268,24 +269,33 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
     g_hash_table_replace(s->iotlb, key, entry);
 }
 
+static void apic_deliver_msi(MSIMessage *msi)
+{
+    /* Conjure apic-bound msi delivery out of thin air. */
+    X86CPU *cpu = X86_CPU(first_cpu);
+    APICCommonState *apic_state = APIC_COMMON(cpu->apic_state);
+    APICCommonClass *apic_class = APIC_COMMON_GET_CLASS(apic_state);
+
+    apic_class->deliver_msi(msi);
+}
+
 /* Given the reg addr of both the message data and address, generate an
  * interrupt via MSI.
  */
 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_quad_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_deliver_msi(&msi);
 }
 
 /* Generate a fault event to software via MSI if conditions are met.
@@ -2113,6 +2123,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 = 0xfee;
     /* Keep this from original MSI address bits */
     msg.__not_used = irq->msi_addr_last_bits;
@@ -2262,11 +2273,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
     VTD_DPRINTF(IR, "delivering MSI 0x%"PRIx64":0x%"PRIx32,
                 to.address, to.data);
 
-    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_deliver_msi(&to);
 
     return MEMTX_OK;
 }
-- 
2.8.2

[Qemu-devel] [PATCH 3/4] linux_headers: add MSI_X2APIC

[Radim Krčmář]

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 linux-headers/linux/kvm.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 3bae71a8743e..3d9ca622bec9 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -865,6 +865,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_SPAPR_TCE_64 125
 #define KVM_CAP_ARM_PMU_V3 126
 #define KVM_CAP_VCPU_ATTRIBUTES 127
+#define KVM_CAP_MSI_X2APIC 128
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -898,6 +899,7 @@ struct kvm_irq_routing_hv_sint {
 #define KVM_IRQ_ROUTING_MSI 2
 #define KVM_IRQ_ROUTING_S390_ADAPTER 3
 #define KVM_IRQ_ROUTING_HV_SINT 4
+#define KVM_IRQ_ROUTING_MSI_X2APIC 5 /* KVM_CAP_MSI_X2APIC */
 
 struct kvm_irq_routing_entry {
 	__u32 gsi;
@@ -1023,6 +1025,9 @@ struct kvm_one_reg {
 	__u64 addr;
 };
 
+#define KVM_SIGNAL_MSI_X2APIC  (1 <<  0) /* KVM_CAP_X2APIC */
+#define KVM_SIGNAL_MSI_FLAGS   KVM_SIGNAL_MSI_X2APIC
+
 struct kvm_msi {
 	__u32 address_lo;
 	__u32 address_hi;
-- 
2.8.2

[Qemu-devel] [PATCH 4/4] kvm: support MSI_X2APIC capability

[Radim Krčmář]

The capability alows us to express x2APIC destinations.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 include/sysemu/kvm.h |  1 +
 kvm-all.c            | 14 +++++++++++++-
 target-i386/kvm.c    |  4 ++++
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index b7a20eb6ae69..e356438fdf5f 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -213,6 +213,7 @@ int kvm_has_pit_state2(void);
 int kvm_has_many_ioeventfds(void);
 int kvm_has_gsi_routing(void);
 int kvm_has_intx_set_mask(void);
+bool kvm_has_msi_x2apic(void);
 
 int kvm_init_vcpu(CPUState *cpu);
 int kvm_cpu_exec(CPUState *cpu);
diff --git a/kvm-all.c b/kvm-all.c
index 8106efb19519..270152615fc2 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -81,6 +81,7 @@ struct KVMState
 #endif
     int many_ioeventfds;
     int intx_set_mask;
+    bool msi_x2apic;
     /* The man page (and posix) say ioctl numbers are signed int, but
      * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
      * unsigned, and treating them as signed here can break things */
@@ -1143,6 +1144,9 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
         msi.address_hi = msg.address >> 32;
         msi.data = le32_to_cpu(msg.data);
         msi.flags = 0;
+        if (kvm_has_msi_x2apic()) {
+            msi.flags |= KVM_SIGNAL_MSI_X2APIC;
+        }
         memset(msi.pad, 0, sizeof(msi.pad));
 
         return kvm_vm_ioctl(s, KVM_SIGNAL_MSI, &msi);
@@ -1159,7 +1163,8 @@ int kvm_irqchip_send_msi(KVMState *s, MSIMessage msg)
 
         route = g_malloc0(sizeof(KVMMSIRoute));
         route->kroute.gsi = virq;
-        route->kroute.type = KVM_IRQ_ROUTING_MSI;
+        route->kroute.type = kvm_has_msi_x2apic() ? KVM_IRQ_ROUTING_MSI_X2APIC
+                                                  : KVM_IRQ_ROUTING_MSI;
         route->kroute.flags = 0;
         route->kroute.u.msi.address_lo = (uint32_t)msg.address;
         route->kroute.u.msi.address_hi = msg.address >> 32;
@@ -1623,6 +1628,8 @@ static int kvm_init(MachineState *ms)
 
     s->intx_set_mask = kvm_check_extension(s, KVM_CAP_PCI_2_3);
 
+    s->msi_x2apic = kvm_check_extension(s, KVM_CAP_MSI_X2APIC);
+
     s->irq_set_ioctl = KVM_IRQ_LINE;
     if (kvm_check_extension(s, KVM_CAP_IRQ_INJECT_STATUS)) {
         s->irq_set_ioctl = KVM_IRQ_LINE_STATUS;
@@ -2104,6 +2111,11 @@ int kvm_has_intx_set_mask(void)
     return kvm_state->intx_set_mask;
 }
 
+bool kvm_has_msi_x2apic(void)
+{
+    return kvm_state->msi_x2apic;
+}
+
 void kvm_setup_guest_memory(void *start, size_t size)
 {
     if (!kvm_has_sync_mmu()) {
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 80b325146a5e..4c9c53ad3e76 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -3351,6 +3351,10 @@ int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
         route->u.msi.address_hi = dst.address >> VTD_MSI_ADDR_HI_SHIFT;
         route->u.msi.address_lo = dst.address & VTD_MSI_ADDR_LO_MASK;
         route->u.msi.data = dst.data;
+
+        if (kvm_has_msi_x2apic()) {
+            route->type = KVM_IRQ_ROUTING_MSI_X2APIC;
+        }
     }
 
     return 0;
-- 
2.8.2

Re: [Qemu-devel] [PATCH 1/4] apic: add deliver_msi to APICCommonClass

[Jan Kiszka]

On 2016-05-06 22:53, Radim Krčmář wrote:
> The only way to send interrupts from outside of APIC devices is to use
> the MSI-inspired memory mapped interface.  The interface is not
> sufficient because interrupt remapping in extended mode can exceed 8 bit
> destination ids.
> 
> The proper way to deliver interrupts would be to design a bus (QPI), but
> that is a big undertaking.  Real IR unit stores excessive bits in the
> high work of MSI address register, which would be bit [63:40] in memory

s/work/word/

> address space, so I was considering two options:
>  - a new special address space for APIC to handle MSI-compatible writes
>  - one callback to the APIC class that delivers extended MSIs
> 
> This patch uses latter option, because it was easier for me, but I think
> the former one could be a tad nicer.

Makes sense.

Eventually, we should also finally untangle the MSI DMA request handling
from the xAPIC MMIO processing. The former could become pretty generic
(instead of being reimplemented by each APIC flavour), just potentially
redirected to an IOMMU when one is present. But that could come as
separate patches.

Jan

Re: [Qemu-devel] [PATCH 4/4] kvm: support MSI_X2APIC capability

[Jan Kiszka]

On 2016-05-06 22:53, Radim Krčmář wrote:
> The capability alows us to express x2APIC destinations.

"allows"

Will the possibility to create >254 CPUs be indirectly coupled to this
capability, or should userland check for it explicitly then?

Will the kernel handle the case gracefully that AMD CPUs will report the
capability as well, although there is no x2APIC (at least so far) with
those CPU types, and we still inject MSI messages with that flag set
(just to double-check if the case was thought through)?

Jan

Re: [Qemu-devel] [RFC 0/4] APIC, IOMMU, KVM: add x2APIC interface

[Peter Xu]

On Fri, May 06, 2016 at 10:53:44PM +0200, Radim Krčmář wrote:
> This series bases on Peter's IR v6 and depends on patches that were just
> posted to kvm-list, "[RFC 0/9] KVM: x86: break the xAPIC barrier".
> 
> The kernel interface could use your comments, but internal QEMU one is
> in dire need of them.  Please see [1/4].
> 
> I have tested the series and seems to work as well as it can.
> Peter's IR v6 didn't boot on my setup, so I reverted to the latest
> version I know was working, v4, and rebased paches for testing.

Radim,

Would you please provide your test setup? So that I can try to
reproduce it on my machine and debug it. 

> The setup from Igor's latest x2APIC QEMU patches creates two VCPUs,
> first has id 0 and second has 280.  Edge IO-APIC and MSI interrupts
> were being delivered to both of them, but level didn't work -- only
> one interrupt was ever delivered, I blame EOI.

Not sure whether this is related to the v5 fix on kernel EOI hack
(the two patches that you have reviewed)? That two patches should
solve the level-triggered interrupt issue that I have encountered,
like e1000 cards.

-- peterx

Re: [Qemu-devel] [PATCH 3/4] linux_headers: add MSI_X2APIC

[Peter Xu]

On Fri, May 06, 2016 at 10:53:47PM +0200, Radim Krčmář wrote:
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  linux-headers/linux/kvm.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 3bae71a8743e..3d9ca622bec9 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -865,6 +865,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_SPAPR_TCE_64 125
>  #define KVM_CAP_ARM_PMU_V3 126
>  #define KVM_CAP_VCPU_ATTRIBUTES 127
> +#define KVM_CAP_MSI_X2APIC 128
>  
>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -898,6 +899,7 @@ struct kvm_irq_routing_hv_sint {
>  #define KVM_IRQ_ROUTING_MSI 2
>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>  #define KVM_IRQ_ROUTING_HV_SINT 4
> +#define KVM_IRQ_ROUTING_MSI_X2APIC 5 /* KVM_CAP_MSI_X2APIC */
>  
>  struct kvm_irq_routing_entry {
>  	__u32 gsi;
> @@ -1023,6 +1025,9 @@ struct kvm_one_reg {
>  	__u64 addr;
>  };
>  
> +#define KVM_SIGNAL_MSI_X2APIC  (1 <<  0) /* KVM_CAP_X2APIC */
                                               ^^^^^^^^^^^^^^
                                             KVM_CAP_MSI_X2APIC?

Re: [Qemu-devel] [RFC 0/4] APIC, IOMMU, KVM: add x2APIC interface

[Radim Krčmář]

2016-05-09 13:36+0800, Peter Xu:
> On Fri, May 06, 2016 at 10:53:44PM +0200, Radim Krčmář wrote:
>> This series bases on Peter's IR v6 and depends on patches that were just
>> posted to kvm-list, "[RFC 0/9] KVM: x86: break the xAPIC barrier".
>> 
>> The kernel interface could use your comments, but internal QEMU one is
>> in dire need of them.  Please see [1/4].
>> 
>> I have tested the series and seems to work as well as it can.
>> Peter's IR v6 didn't boot on my setup, so I reverted to the latest
>> version I know was working, v4, and rebased paches for testing.
> 
> Radim,
> 
> Would you please provide your test setup? So that I can try to
> reproduce it on my machine and debug it. 

I could reproduce with a kernel based off kvm/queue (basically 4.6-rc3),
that was make olconfig with fedora rawhide config for 4.6-rc3 and
qemu/master (53db932604d) after pulling your vtd-intr-v6 and doing the
minimum to make ./configure happy.

The bug was caused by pci-bridge, which I didn't remove from a
copy-pasted qemu line ... Linux boot hangs if QEMU is ran with
 -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
 -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
 -drive file=/home/rhel7.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
 -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \

but the following works
 -drive file=/home/rhel7.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
 -device virtio-blk-pci,scsi=off,bus=pcie.0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \

I can give you ssh access to the machine too.

>> The setup from Igor's latest x2APIC QEMU patches creates two VCPUs,
>> first has id 0 and second has 280.  Edge IO-APIC and MSI interrupts
>> were being delivered to both of them, but level didn't work -- only
>> one interrupt was ever delivered, I blame EOI.
> 
> Not sure whether this is related to the v5 fix on kernel EOI hack
> (the two patches that you have reviewed)? That two patches should
> solve the level-triggered interrupt issue that I have encountered,
> like e1000 cards.

I didn't try the R/O preserving patch, but the other one on top of v4
changed nothing.  (There might be some kernel bugs too, because I
expected that EOI would start working with it ...)

Re: [Qemu-devel] [PATCH 4/4] kvm: support MSI_X2APIC capability

[Radim Krčmář]

2016-05-07 16:03+0200, Jan Kiszka:
> On 2016-05-06 22:53, Radim Krčmář wrote:
>> The capability alows us to express x2APIC destinations.
> 
> "allows"

Thanks. :)

> Will the possibility to create >254 CPUs be indirectly coupled to this
> capability, or should userland check for it explicitly then?

Yes, for now, so I would prefer if userspace checked explicitly.
Greater amount of VCPUs needs to be checked with KVM_CAP_MAX_VCPUS, but
it's not guaranteed that all kernels with maximum above 255 will have
KVM_CAP_MSI_X2APIC as there are other methods of sending an interrupt.

> Will the kernel handle the case gracefully that AMD CPUs will report the
> capability as well, although there is no x2APIC (at least so far) with
> those CPU types, and we still inject MSI messages with that flag set
> (just to double-check if the case was thought through)?

Yes, it's perfectly backward compatible.  We allowed x2APIC on emulated
AMD cpus in the past and this just extends the address space to 32 bits.
Maybe the capability should be called MSI_EXTENDED?

(I though about using the same GSI type and just using those upper bits
 + notifiing QEMU with a capability, but userspace could have passed
 garbage in those bits, so it had non-zero risk.)

Re: [Qemu-devel] [RFC 0/4] APIC, IOMMU, KVM: add x2APIC interface

[Peter Xu]

On Mon, May 09, 2016 at 04:35:24PM +0200, Radim Krčmář wrote:
> 2016-05-09 13:36+0800, Peter Xu:
> > On Fri, May 06, 2016 at 10:53:44PM +0200, Radim Krčmář wrote:
> >> This series bases on Peter's IR v6 and depends on patches that were just
> >> posted to kvm-list, "[RFC 0/9] KVM: x86: break the xAPIC barrier".
> >> 
> >> The kernel interface could use your comments, but internal QEMU one is
> >> in dire need of them.  Please see [1/4].
> >> 
> >> I have tested the series and seems to work as well as it can.
> >> Peter's IR v6 didn't boot on my setup, so I reverted to the latest
> >> version I know was working, v4, and rebased paches for testing.
> > 
> > Radim,
> > 
> > Would you please provide your test setup? So that I can try to
> > reproduce it on my machine and debug it. 
> 
> I could reproduce with a kernel based off kvm/queue (basically 4.6-rc3),
> that was make olconfig with fedora rawhide config for 4.6-rc3 and
> qemu/master (53db932604d) after pulling your vtd-intr-v6 and doing the
> minimum to make ./configure happy.
> 
> The bug was caused by pci-bridge, which I didn't remove from a
> copy-pasted qemu line ... Linux boot hangs if QEMU is ran with
>  -device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \
>  -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x0 \
>  -drive file=/home/rhel7.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
>  -device virtio-blk-pci,scsi=off,bus=pci.2,addr=0x3,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
> 
> but the following works
>  -drive file=/home/rhel7.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
>  -device virtio-blk-pci,scsi=off,bus=pcie.0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 \
> 
> I can give you ssh access to the machine too.

I got it reproduced on my host with another random kernel as well.
Will let you know if got any progress.

Thanks!

-- peterx

Re: [Qemu-devel] [PATCH 1/4] apic: add deliver_msi to APICCommonClass

[Eduardo Habkost]

On Fri, May 06, 2016 at 10:53:45PM +0200, Radim Krčmář wrote:
> The only way to send interrupts from outside of APIC devices is to use
> the MSI-inspired memory mapped interface.  The interface is not
> sufficient because interrupt remapping in extended mode can exceed 8 bit
> destination ids.
> 
> The proper way to deliver interrupts would be to design a bus (QPI), but
> that is a big undertaking.  Real IR unit stores excessive bits in the
> high work of MSI address register, which would be bit [63:40] in memory
> address space, so I was considering two options:
>  - a new special address space for APIC to handle MSI-compatible writes
>  - one callback to the APIC class that delivers extended MSIs
> 
> This patch uses latter option, because it was easier for me, but I think
> the former one could be a tad nicer.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

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

But I have a suggestion below:

> ---
>  hw/i386/kvm/apic.c              | 21 ++++++++++++++-------
>  hw/i386/xen/xen_apic.c          |  7 +++++++
>  hw/intc/apic.c                  |  7 +++++++
>  include/hw/i386/apic_internal.h |  5 +++++
>  4 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index 3c7c8fa00709..7881f13abb96 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -147,6 +147,17 @@ static void kvm_apic_external_nmi(APICCommonState *s)
>      run_on_cpu(CPU(s->cpu), do_inject_external_nmi, s);
>  }
>  
> +static void kvm_deliver_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)
>  {
> @@ -157,13 +168,7 @@ 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_deliver_msi(&msg);
>  }

You could call APICCommonClass::deliver_msi() here. Then (in a
follow-up patch?) both kvm_apic_mem_write() and
xen_apic_mem_write() could be replaced by a common function like:

  void apic_msi_write(void *opaque, hwaddr addr, uint64_t data, unsigned size)
  {
      APICommonState *s = opaque;
      APICCommonClass *k = APIC_COMMON_GET_CLASS(s);
      MSIMessage msg = { .address = addr, .data = data };
      k->deliver_msi(&msg);
  }

(apic_mem_writel() is more complex, but maybe it could call the
common function instead of calling apic_send_msi() directly)

>  
>  static const MemoryRegionOps kvm_apic_io_ops = {
> @@ -202,6 +207,8 @@ 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->deliver_msi = kvm_deliver_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..0f34f63d449d 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_deliver_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,8 @@ 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->deliver_msi = xen_deliver_msi;
>  }
>  
>  static const TypeInfo xen_apic_info = {
> diff --git a/hw/intc/apic.c b/hw/intc/apic.c
> index 28c2ea540608..b893942c6e73 100644
> --- a/hw/intc/apic.c
> +++ b/hw/intc/apic.c
> @@ -877,6 +877,11 @@ static void apic_realize(DeviceState *dev, Error **errp)
>      msi_nonbroken = true;
>  }
>  
> +static void apic_deliver_msi(MSIMessage *msi)
> +{
> +    apic_send_msi(msi->address, msi->data);
> +}
> +
>  static void apic_class_init(ObjectClass *klass, void *data)
>  {
>      APICCommonClass *k = APIC_COMMON_CLASS(klass);
> @@ -889,6 +894,8 @@ 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->deliver_msi = apic_deliver_msi;
>  }
>  
>  static const TypeInfo apic_info = {
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 74fe935e8eab..227ef30c5027 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -146,6 +146,11 @@ typedef struct APICCommonClass
>      void (*pre_save)(APICCommonState *s);
>      void (*post_load)(APICCommonState *s);
>      void (*reset)(APICCommonState *s);
> +
> +    /* deliver_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 (*deliver_msi)(MSIMessage *msi);
>  } APICCommonClass;
>  
>  struct APICCommonState {
> -- 
> 2.8.2
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] intel_iommu: use deliver_msi APIC callback

[Eduardo Habkost]

On Fri, May 06, 2016 at 10:53:46PM +0200, Radim Krčmář wrote:
> The memory-mapped interface cannot express x2APIC destinations that are
> a result of remapping.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index bee85e469477..d10064289551 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -26,6 +26,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/boards.h"
>  #include "hw/i386/x86-iommu.h"
> +#include "hw/i386/apic_internal.h"
>  
>  /*#define DEBUG_INTEL_IOMMU*/
>  #ifdef DEBUG_INTEL_IOMMU
> @@ -268,24 +269,33 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
>      g_hash_table_replace(s->iotlb, key, entry);
>  }
>  
> +static void apic_deliver_msi(MSIMessage *msi)
> +{
> +    /* Conjure apic-bound msi delivery out of thin air. */
> +    X86CPU *cpu = X86_CPU(first_cpu);
> +    APICCommonState *apic_state = APIC_COMMON(cpu->apic_state);
> +    APICCommonClass *apic_class = APIC_COMMON_GET_CLASS(apic_state);

I see a pattern here:

hw/i386/kvmvapic.c-static void do_vapic_enable(void *data)
hw/i386/kvmvapic.c-{
[...]
hw/i386/kvmvapic.c-    X86CPU *cpu = X86_CPU(first_cpu);
[...]
hw/i386/kvmvapic.c:    apic_enable_vapic(cpu->apic_state, s->vapic_paddr);
[...]

hw/i386/pc.c-static void pic_irq_request(void *opaque, int irq, int level)
hw/i386/pc.c-{
hw/i386/pc.c-    CPUState *cs = first_cpu;
hw/i386/pc.c-    X86CPU *cpu = X86_CPU(cs);
[...]
hw/i386/pc.c:    if (cpu->apic_state) {
[...]


Time to write a common pc_get_first_apic() helper, or provide a
PCMachineState::first_apic field?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] intel_iommu: use deliver_msi APIC callback

[Paolo Bonzini]

On 13/05/2016 19:33, Eduardo Habkost wrote:
> On Fri, May 06, 2016 at 10:53:46PM +0200, Radim Krčmář wrote:
>> The memory-mapped interface cannot express x2APIC destinations that are
>> a result of remapping.
>>
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  hw/i386/intel_iommu.c | 29 ++++++++++++++++++-----------
>>  1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index bee85e469477..d10064289551 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -26,6 +26,7 @@
>>  #include "hw/pci/pci.h"
>>  #include "hw/boards.h"
>>  #include "hw/i386/x86-iommu.h"
>> +#include "hw/i386/apic_internal.h"
>>  
>>  /*#define DEBUG_INTEL_IOMMU*/
>>  #ifdef DEBUG_INTEL_IOMMU
>> @@ -268,24 +269,33 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
>>      g_hash_table_replace(s->iotlb, key, entry);
>>  }
>>  
>> +static void apic_deliver_msi(MSIMessage *msi)
>> +{
>> +    /* Conjure apic-bound msi delivery out of thin air. */
>> +    X86CPU *cpu = X86_CPU(first_cpu);
>> +    APICCommonState *apic_state = APIC_COMMON(cpu->apic_state);
>> +    APICCommonClass *apic_class = APIC_COMMON_GET_CLASS(apic_state);
> 
> I see a pattern here:
> 
> hw/i386/kvmvapic.c-static void do_vapic_enable(void *data)
> hw/i386/kvmvapic.c-{
> [...]
> hw/i386/kvmvapic.c-    X86CPU *cpu = X86_CPU(first_cpu);
> [...]
> hw/i386/kvmvapic.c:    apic_enable_vapic(cpu->apic_state, s->vapic_paddr);
> [...]

Here first_cpu is just the CPU that do_vapic_enable is being called on
(via run_on_cpu).  So this one can use the posted patch that passes a
CPUState* to run_on_cpu callbacks.


> hw/i386/pc.c-static void pic_irq_request(void *opaque, int irq, int level)
> hw/i386/pc.c-{
> hw/i386/pc.c-    CPUState *cs = first_cpu;
> hw/i386/pc.c-    X86CPU *cpu = X86_CPU(cs);
> [...]
> hw/i386/pc.c:    if (cpu->apic_state) {
> [...]
> 
> 
> Time to write a common pc_get_first_apic() helper, or provide a
> PCMachineState::first_apic field?

For this one, we could add a pc_get_apic_class() helper that returns
NULL if there is no APIC on the first_cpu.  It wouldn't be a change for
this code and would be nicer for Radim's use case.

Paolo

Re: [Qemu-devel] [PATCH 4/4] kvm: support MSI_X2APIC capability

[Paolo Bonzini]

On 06/05/2016 22:53, Radim Krčmář wrote:
> +        route->kroute.type = kvm_has_msi_x2apic() ? KVM_IRQ_ROUTING_MSI_X2APIC
> +                                                  : KVM_IRQ_ROUTING_MSI;
>          route->kroute.flags = 0;

Perhaps using flags here instead of a new route type gives simpler code
in the kernel?

It's a pity that the padding field of struct kvm_irq_routing_msi was not
checked against zero. :(

Paolo

>          route->kroute.u.msi.address_lo = (uint32_t)msg.address;
>          route->kroute.u.msi.address_hi = msg.address >> 32;

Re: [Qemu-devel] [PATCH 2/4] intel_iommu: use deliver_msi APIC callback

[Eduardo Habkost]

On Tue, May 17, 2016 at 03:09:49PM +0200, Paolo Bonzini wrote:
> 
> 
> On 13/05/2016 19:33, Eduardo Habkost wrote:
> > On Fri, May 06, 2016 at 10:53:46PM +0200, Radim Krčmář wrote:
> >> The memory-mapped interface cannot express x2APIC destinations that are
> >> a result of remapping.
> >>
> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> >> ---
> >>  hw/i386/intel_iommu.c | 29 ++++++++++++++++++-----------
> >>  1 file changed, 18 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index bee85e469477..d10064289551 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -26,6 +26,7 @@
> >>  #include "hw/pci/pci.h"
> >>  #include "hw/boards.h"
> >>  #include "hw/i386/x86-iommu.h"
> >> +#include "hw/i386/apic_internal.h"
> >>  
> >>  /*#define DEBUG_INTEL_IOMMU*/
> >>  #ifdef DEBUG_INTEL_IOMMU
> >> @@ -268,24 +269,33 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
> >>      g_hash_table_replace(s->iotlb, key, entry);
> >>  }
> >>  
> >> +static void apic_deliver_msi(MSIMessage *msi)
> >> +{
> >> +    /* Conjure apic-bound msi delivery out of thin air. */
> >> +    X86CPU *cpu = X86_CPU(first_cpu);
> >> +    APICCommonState *apic_state = APIC_COMMON(cpu->apic_state);
> >> +    APICCommonClass *apic_class = APIC_COMMON_GET_CLASS(apic_state);
> > 
> > I see a pattern here:
> > 
> > hw/i386/kvmvapic.c-static void do_vapic_enable(void *data)
> > hw/i386/kvmvapic.c-{
> > [...]
> > hw/i386/kvmvapic.c-    X86CPU *cpu = X86_CPU(first_cpu);
> > [...]
> > hw/i386/kvmvapic.c:    apic_enable_vapic(cpu->apic_state, s->vapic_paddr);
> > [...]
> 
> Here first_cpu is just the CPU that do_vapic_enable is being called on
> (via run_on_cpu).  So this one can use the posted patch that passes a
> CPUState* to run_on_cpu callbacks.
> 
> 
> > hw/i386/pc.c-static void pic_irq_request(void *opaque, int irq, int level)
> > hw/i386/pc.c-{
> > hw/i386/pc.c-    CPUState *cs = first_cpu;
> > hw/i386/pc.c-    X86CPU *cpu = X86_CPU(cs);
> > [...]
> > hw/i386/pc.c:    if (cpu->apic_state) {
> > [...]
> > 
> > 
> > Time to write a common pc_get_first_apic() helper, or provide a
> > PCMachineState::first_apic field?
> 
> For this one, we could add a pc_get_apic_class() helper that returns
> NULL if there is no APIC on the first_cpu.  It wouldn't be a change for
> this code and would be nicer for Radim's use case.

Returning just the APIC class would be even nicer, yes. We could
just move the type name logic in x86_cpu_apic_create() to
pc_get_apic_class().

About returning NULL if there is no APIC on first_cpu: I would
like to avoid making more code depend on first_cpu if possible.
pc_get_first_apic() wouldn't even need to use
first_cpu/pc_get_apic_class() if we just do this:

    CPU_FOREACH(cs) {
        cpu = X86_CPU(cs);
        if (!cpu->apic_state) {
            if (level) {
                cpu_interrupt(cs, CPU_INTERRUPT_HARD);
            } else {
                cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
            }
            break;
        }
        if (apic_accept_pic_intr(cpu->apic_state)) {
            apic_deliver_pic_intr(cpu->apic_state, level);
        }
    }

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] intel_iommu: use deliver_msi APIC callback

[Paolo Bonzini]

On 17/05/2016 15:53, Eduardo Habkost wrote:
> On Tue, May 17, 2016 at 03:09:49PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 13/05/2016 19:33, Eduardo Habkost wrote:
>>> On Fri, May 06, 2016 at 10:53:46PM +0200, Radim Krčmář wrote:
>>>> The memory-mapped interface cannot express x2APIC destinations that are
>>>> a result of remapping.
>>>>
>>>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>>>> ---
>>>>  hw/i386/intel_iommu.c | 29 ++++++++++++++++++-----------
>>>>  1 file changed, 18 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index bee85e469477..d10064289551 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -26,6 +26,7 @@
>>>>  #include "hw/pci/pci.h"
>>>>  #include "hw/boards.h"
>>>>  #include "hw/i386/x86-iommu.h"
>>>> +#include "hw/i386/apic_internal.h"
>>>>  
>>>>  /*#define DEBUG_INTEL_IOMMU*/
>>>>  #ifdef DEBUG_INTEL_IOMMU
>>>> @@ -268,24 +269,33 @@ static void vtd_update_iotlb(IntelIOMMUState *s, uint16_t source_id,
>>>>      g_hash_table_replace(s->iotlb, key, entry);
>>>>  }
>>>>  
>>>> +static void apic_deliver_msi(MSIMessage *msi)
>>>> +{
>>>> +    /* Conjure apic-bound msi delivery out of thin air. */
>>>> +    X86CPU *cpu = X86_CPU(first_cpu);
>>>> +    APICCommonState *apic_state = APIC_COMMON(cpu->apic_state);
>>>> +    APICCommonClass *apic_class = APIC_COMMON_GET_CLASS(apic_state);
>>>
>>> I see a pattern here:
>>>
>>> hw/i386/kvmvapic.c-static void do_vapic_enable(void *data)
>>> hw/i386/kvmvapic.c-{
>>> [...]
>>> hw/i386/kvmvapic.c-    X86CPU *cpu = X86_CPU(first_cpu);
>>> [...]
>>> hw/i386/kvmvapic.c:    apic_enable_vapic(cpu->apic_state, s->vapic_paddr);
>>> [...]
>>
>> Here first_cpu is just the CPU that do_vapic_enable is being called on
>> (via run_on_cpu).  So this one can use the posted patch that passes a
>> CPUState* to run_on_cpu callbacks.
>>
>>
>>> hw/i386/pc.c-static void pic_irq_request(void *opaque, int irq, int level)
>>> hw/i386/pc.c-{
>>> hw/i386/pc.c-    CPUState *cs = first_cpu;
>>> hw/i386/pc.c-    X86CPU *cpu = X86_CPU(cs);
>>> [...]
>>> hw/i386/pc.c:    if (cpu->apic_state) {
>>> [...]
>>>
>>>
>>> Time to write a common pc_get_first_apic() helper, or provide a
>>> PCMachineState::first_apic field?
>>
>> For this one, we could add a pc_get_apic_class() helper that returns
>> NULL if there is no APIC on the first_cpu.  It wouldn't be a change for
>> this code and would be nicer for Radim's use case.
> 
> Returning just the APIC class would be even nicer, yes. We could
> just move the type name logic in x86_cpu_apic_create() to
> pc_get_apic_class().
> 
> About returning NULL if there is no APIC on first_cpu: I would
> like to avoid making more code depend on first_cpu if possible.
> pc_get_first_apic() wouldn't even need to use
> first_cpu/pc_get_apic_class() if we just do this:
> 
>     CPU_FOREACH(cs) {
>         cpu = X86_CPU(cs);
>         if (!cpu->apic_state) {
>             if (level) {
>                 cpu_interrupt(cs, CPU_INTERRUPT_HARD);
>             } else {
>                 cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
>             }
>             break;
>         }
>         if (apic_accept_pic_intr(cpu->apic_state)) {
>             apic_deliver_pic_intr(cpu->apic_state, level);
>         }
>     }
> 

Sounds good.

Paolo