On Mon, 2021-11-29 at 15:14 +0100, Claudio Fontana wrote:
> On 11/29/21 12:39 PM, Woodhouse, David wrote:
> > On Fri, 2021-07-23 at 13:29 +0200, Claudio Fontana wrote:
> > > static void kvm_cpu_instance_init(CPUState *cs)
> > > {
> > > X86CPU *cpu = X86_CPU(cs);
> > > + X86CPUClass *xcc = X86_CPU_GET_CLASS(cpu);
> > >
> > > host_cpu_instance_init(cpu);
> > >
> > > - if (!kvm_irqchip_in_kernel()) {
> > > - x86_cpu_change_kvm_default("x2apic", "off");
> > > - } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
> > > - x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
> > > - }
> > > -
> > > - /* Special cases not set in the X86CPUDefinition structs: */
> > > + if (xcc->model) {
> > > + /* only applies to builtin_x86_defs cpus */
> > > + if (!kvm_irqchip_in_kernel()) {
> > > + x86_cpu_change_kvm_default("x2apic", "off");
> > > + } else if (kvm_irqchip_is_split() && kvm_enable_x2apic()) {
> > > + x86_cpu_change_kvm_default("kvm-msi-ext-dest-id", "on");
> > > + }
> > >
> > > - x86_cpu_apply_props(cpu, kvm_default_props);
> > > + /* Special cases not set in the X86CPUDefinition structs: */
> > > + x86_cpu_apply_props(cpu, kvm_default_props);
> > > + }
> > >
> >
> > I think this causes a regression in x2apic and kvm-msi-ext-dest-id
> > support. If you start qemu thus:
>
> If I recall correctly, this change just tries to restore the behavior prior to
> commit f5cc5a5c168674f84bf061cdb307c2d25fba5448 ,
>
> fixing the issue introduced with the refactoring at that time.
>
> Can you try bisecting prior to
> f5cc5a5c168674f84bf061cdb307c2d25fba5448 , to see if the actual
> breakage comes from somewhere else?
Hm, so it looks like it never worked for '-cpu host' *until* commit
f5cc5a5c16.
It didn't matter before c1bb5418e3 because you couldn't enable that
many vCPUs without an IOMMU, and the *IOMMU* setup would call
kvm_enable_x2apic().
But after that, nothing ever called kvm_enable_x2apic() in the '-cpu
host' case until commit f5cc5a5c16, which fixed it... until you
restored the previous behaviour :)
This "works" to fix this case, but presumably isn't correct:
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -161,7 +161,7 @@ static void kvm_cpu_instance_init(CPUState *cs)
host_cpu_instance_init(cpu);
- if (xcc->model) {
+ if (1 || xcc->model) {
/* only applies to builtin_x86_defs cpus */
if (!kvm_irqchip_in_kernel()) {
x86_cpu_change_kvm_default("x2apic", "off");
> > Any image to specifically test out? Would an actual 9 sockets machine be
> > required to reproduce this?
No, but the more CPUs you have in the host the less you have to wait
for 288 vCPUs to spin up :)
My test is:
./qemu-system-x86_64 -machine q35,accel=kvm,usb=off,kernel_irqchip=split -cpu
host -m 2G -smp sockets=9,cores=16,threads=2 -drive
file=/var/lib/libvirt/images/fedora.qcow2,if=virtio -serial mon:stdio -display
none -kernel ~/git/linux/arch/x86/boot/bzImage -append "console=ttyS0,115200
root=/dev/vda1"
I then play with the affinity of the AHCI MSI. Pointing it at CPU 255
should show the problem.
[root@localhost ~]# cd /proc/irq/313
[root@localhost 313]# echo 255 > smp_affinity_list
[root@localhost 313]#
[ 65.365821] Composed MSI for APIC 255 vector 0x22: 0/feeff000 22
[root@localhost 313]# grep ahci /proc/interrupts
I also added some debugging into host and guest kernels to be a little
more explicit:
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344bf6600..53191db5145d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1866,6 +1866,7 @@ static __init void try_to_enable_x2apic(int remap_mode)
* used for non-remapped IRQ domains.
*/
if (x86_init.hyper.msi_ext_dest_id()) {
+ pr_info("x2apic: support extended destination ID\n");
virt_ext_dest_id = 1;
apic_limit = 32767;
}
@@ -2539,6 +2540,7 @@ void __irq_msi_compose_msg(struct irq_cfg *cfg, struct
msi_msg *msg,
msg->arch_addr_lo.virt_destid_8_14 = cfg->dest_apicid >> 8;
else
WARN_ON_ONCE(cfg->dest_apicid > 0xFF);
+ printk("Composed MSI for APIC %d vector 0x%x: %x/%x %x\n",
cfg->dest_apicid, cfg->vector, msg->address_hi, msg->address_lo, msg->data);
}
u32 x86_msi_msg_get_destid(struct msi_msg *msg, bool extid)
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 59abbdad7729..f0a7715763a2 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -856,6 +856,8 @@ static void __init kvm_apic_init(void)
static bool __init kvm_msi_ext_dest_id(void)
{
+ printk("dest id? %d (%x)\n",
kvm_para_has_feature(KVM_FEATURE_MSI_EXT_DEST_ID),
+ kvm_arch_para_features());
return kvm_para_has_feature(KVM_FEATURE_MSI_EXT_DEST_ID);
}
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 759952dd1222..defe6a843780 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -894,15 +894,21 @@ static bool kvm_apic_is_broadcast_dest(struct kvm *kvm,
struct kvm_lapic **src,
{
if (kvm->arch.x2apic_broadcast_quirk_disabled) {
if ((irq->dest_id == APIC_BROADCAST &&
- map->mode != KVM_APIC_MODE_X2APIC))
+ map->mode != KVM_APIC_MODE_X2APIC)) {
+ printk("dest %d mode %d makes bcast\n", irq->dest_id,
map->mode);
return true;
- if (irq->dest_id == X2APIC_BROADCAST)
+ }
+ if (irq->dest_id == X2APIC_BROADCAST) {
+ printk("Sent to X2APIC bcast\n");
return true;
+ }
} else {
bool x2apic_ipi = src && *src && apic_x2apic_mode(*src);
if (irq->dest_id == (x2apic_ipi ?
- X2APIC_BROADCAST : APIC_BROADCAST))
+ X2APIC_BROADCAST : APIC_BROADCAST)) {
+ printk("no quirk dest %x\n", irq->dest_id);
return true;
+ }
}
return false;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d8f1d2169b45..5b0fd6d37a7e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5714,6 +5714,7 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
if (cap->args[0] & KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
kvm->arch.x2apic_broadcast_quirk_disabled = true;
+ printk("X2APIC API: %x\n", cap->args[0]);
r = 0;
break;
case KVM_CAP_X86_DISABLE_EXITS:
smime.p7s
Description: S/MIME cryptographic signature
