diff mbox

[v2,10/14] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

Message ID 1474548655-157373-11-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Sept. 22, 2016, 12:50 p.m. UTC 
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/kvm_i386.h |  1 +
 hw/i386/kvm/apic.c     | 13 +++++++++++--
 target-i386/kvm.c      | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

Comments

Paolo Bonzini Sept. 22, 2016, 2:36 p.m. UTC | #1 
On 22/09/2016 14:50, Igor Mammedov wrote:
> +#ifdef KVM_CAP_X2APIC_API
> +    if (kvm_check_extension(s, KVM_CAP_X2APIC_API)) {
> +        has_x2apic_ids = !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0,
> +                                            KVM_X2APIC_API_USE_32BIT_IDS);
> +    }
> +#endif
> +

Radim, whose patches are going to set
KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK?

Paolo
Radim Krčmář Sept. 22, 2016, 7:57 p.m. UTC | #2 
2016-09-22 16:36+0200, Paolo Bonzini:
> On 22/09/2016 14:50, Igor Mammedov wrote:
>> +#ifdef KVM_CAP_X2APIC_API
>> +    if (kvm_check_extension(s, KVM_CAP_X2APIC_API)) {
>> +        has_x2apic_ids = !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0,
>> +                                            KVM_X2APIC_API_USE_32BIT_IDS);
>> +    }
>> +#endif
>> +
> 
> Radim, whose patches are going to set
> KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK?

I added kvm_enable_x2apic() helper for intel_iommu that enables both,
because we really want to make sure that both are enabled before
allowing EIM.  (And then I didn't post those patches ... ameding that
after a rebase and a quick retest.)

We'd better forbid APIC IDs above 255 without "intel_iommu,eim=on", so
reusing kvm_enable_x2apic() and enabling both in Igor's patches would be
just a bit nicer.

Having separate KVM_X2APIC_API_USE_32BIT_IDS and
KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK isn't as useful as I thought it
would be ...
Igor Mammedov Sept. 26, 2016, 9:47 a.m. UTC | #3 
On Thu, 22 Sep 2016 21:57:39 +0200
Radim Krčmář <rkrcmar@redhat.com> wrote:

> 2016-09-22 16:36+0200, Paolo Bonzini:
> > On 22/09/2016 14:50, Igor Mammedov wrote:  
> >> +#ifdef KVM_CAP_X2APIC_API
> >> +    if (kvm_check_extension(s, KVM_CAP_X2APIC_API)) {
> >> +        has_x2apic_ids = !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0,
> >> +                                            KVM_X2APIC_API_USE_32BIT_IDS);
> >> +    }
> >> +#endif
> >> +  
> > 
> > Radim, whose patches are going to set
> > KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK?  
> 
> I added kvm_enable_x2apic() helper for intel_iommu that enables both,
> because we really want to make sure that both are enabled before
> allowing EIM.  (And then I didn't post those patches ... ameding that
> after a rebase and a quick retest.)
I can include those patches along with this series

 
> We'd better forbid APIC IDs above 255 without "intel_iommu,eim=on", so
> reusing kvm_enable_x2apic() and enabling both in Igor's patches would be
> just a bit nicer.
Is it possible on real hw to start system with APIC IDs above 255 but
with EIM or even whole IOMMU disabled?


Btw:
Are EIM patches merged, current master says:
 -device intel-iommu,intremap=on,eim=on: Property '.eim' not found

we can do eim check at pc_machine_done() time and as well check for
correct(supported) IOMMU type /split/.
Maybe it should be part of EIM patches.


> Having separate KVM_X2APIC_API_USE_32BIT_IDS and
> KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK isn't as useful as I thought it
> would be ...
Igor Mammedov Sept. 27, 2016, 2:13 p.m. UTC | #4 
On Mon, 26 Sep 2016 11:47:38 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 22 Sep 2016 21:57:39 +0200
> Radim Krčmář <rkrcmar@redhat.com> wrote:
> 
> > 2016-09-22 16:36+0200, Paolo Bonzini:  
> > > On 22/09/2016 14:50, Igor Mammedov wrote:    
> > >> +#ifdef KVM_CAP_X2APIC_API
> > >> +    if (kvm_check_extension(s, KVM_CAP_X2APIC_API)) {
> > >> +        has_x2apic_ids = !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0,
> > >> +                                            KVM_X2APIC_API_USE_32BIT_IDS);
> > >> +    }
> > >> +#endif
> > >> +    
> > > 
> > > Radim, whose patches are going to set
> > > KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK?    
> > 
> > I added kvm_enable_x2apic() helper for intel_iommu that enables both,
> > because we really want to make sure that both are enabled before
> > allowing EIM.  (And then I didn't post those patches ... ameding that
> > after a rebase and a quick retest.)  
I'll rebase these series on top of your EIM fixes
diff mbox

Patch

diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
index 42b00af..dad0dcf 100644
--- a/target-i386/kvm_i386.h
+++ b/target-i386/kvm_i386.h
@@ -41,4 +41,5 @@  int kvm_device_msix_set_vector(KVMState *s, uint32_t dev_id, uint32_t vector,
 int kvm_device_msix_assign(KVMState *s, uint32_t dev_id);
 int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
 
+bool kvm_has_x2apic_ids(void);
 #endif
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index feb0002..34326d9 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -15,6 +15,7 @@ 
 #include "hw/i386/apic_internal.h"
 #include "hw/pci/msi.h"
 #include "sysemu/kvm.h"
+#include "kvm_i386.h"
 
 static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
                                     int reg_id, uint32_t val)
@@ -33,7 +34,11 @@  static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic
     int i;
 
     memset(kapic, 0, sizeof(*kapic));
-    kvm_apic_set_reg(kapic, 0x2, s->id << 24);
+    if (kvm_has_x2apic_ids() && s->apicbase & MSR_IA32_APICBASE_EXTD) {
+        kvm_apic_set_reg(kapic, 0x2, s->initial_apic_id);
+    } else {
+        kvm_apic_set_reg(kapic, 0x2, s->id << 24);
+    }
     kvm_apic_set_reg(kapic, 0x8, s->tpr);
     kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24);
     kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff);
@@ -58,7 +63,11 @@  void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic)
     APICCommonState *s = APIC_COMMON(dev);
     int i, v;
 
-    s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
+    if (kvm_has_x2apic_ids() && s->apicbase & MSR_IA32_APICBASE_EXTD) {
+        assert(kvm_apic_get_reg(kapic, 0x2) == s->initial_apic_id);
+    } else {
+        s->id = kvm_apic_get_reg(kapic, 0x2) >> 24;
+    }
     s->tpr = kvm_apic_get_reg(kapic, 0x8);
     s->arb_id = kvm_apic_get_reg(kapic, 0x9);
     s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index f1ad805..0a307c2 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -111,8 +111,15 @@  static int has_pit_state2;
 
 static bool has_msr_mcg_ext_ctl;
 
+static bool has_x2apic_ids;
+
 static struct kvm_cpuid2 *cpuid_cache;
 
+bool kvm_has_x2apic_ids(void)
+{
+    return has_x2apic_ids;
+}
+
 int kvm_has_pit_state2(void)
 {
     return has_pit_state2;
@@ -1162,6 +1169,13 @@  int kvm_arch_init(MachineState *ms, KVMState *s)
     has_pit_state2 = kvm_check_extension(s, KVM_CAP_PIT_STATE2);
 #endif
 
+#ifdef KVM_CAP_X2APIC_API
+    if (kvm_check_extension(s, KVM_CAP_X2APIC_API)) {
+        has_x2apic_ids = !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0,
+                                            KVM_X2APIC_API_USE_32BIT_IDS);
+    }
+#endif
+
     ret = kvm_get_supported_msrs(s);
     if (ret < 0) {
         return ret;