Re: [Qemu-devel] [PATCH v2 10/14] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Igor Mammedov]

On Mon, 26 Sep 2016 11:47:38 +0200
Igor Mammedov  wrote:

> On Thu, 22 Sep 2016 21:57:39 +0200
> Radim Krčmář  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

Re: [Qemu-devel] [PATCH v2 10/14] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Igor Mammedov]

On Thu, 22 Sep 2016 21:57:39 +0200
Radim Krčmář  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 ...

Re: [Qemu-devel] [PATCH v2 10/14] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Radim Krčmář]

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 ...

Re: [Qemu-devel] [PATCH v2 10/14] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[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?

Paolo

[Qemu-devel] [PATCH v2 10/14] pc: kvm_apic: pass APIC ID depending on xAPIC/x2APIC mode

[Igor Mammedov]

Signed-off-by: Igor Mammedov 
---
 target-i386/kvm_i386.h |  1 +
 hw/i386/kvm/apic.c | 13 +++--
 target-i386/kvm.c  | 14 ++
 3 files changed, 26 insertions(+), 2 deletions(-)

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 | 0x0fff);
@@ -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;
-- 
2.7.4