| Message ID | 20160929112329.2408-8-rkrcmar@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 29/09/2016 13:23, 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. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> Ugh, I misremembered that VTD_ECAP_EIM was not set in 2.7. :( Perhaps it's better to drop this patch... Paolo > --- > hw/i386/intel_iommu.c | 6 +++++- > hw/i386/pc_q35.c | 2 ++ > include/hw/i386/pc.h | 2 ++ > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index b1afe5b133e0..d6657a361ff9 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -2458,6 +2458,7 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) > static void vtd_realize(DeviceState *dev, Error **errp) > { > PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); > + PCMachineClass *pcmc = PC_MACHINE_CLASS(MACHINE_GET_CLASS(pcms)); > PCIBus *bus = pcms->bus; > IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); > X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); > @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp) > if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) { > s->intr_eim = ON_OFF_AUTO_OFF; > } > + if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) { > + s->intr_eim = ON_OFF_AUTO_ON; > + } > if (s->intr_eim == ON_OFF_AUTO_AUTO) { > s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON > : ON_OFF_AUTO_OFF; > } > - if (s->intr_eim == ON_OFF_AUTO_ON) { > + if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) { > if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { > error_report("intel-iommu,eim=on requires support on the KVM side " > "(X2APIC_API, first shipped in v4.7)."); > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 0b214f24c977..97f419fbf4dd 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -304,8 +304,10 @@ DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL, > > static void pc_q35_2_7_machine_options(MachineClass *m) > { > + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); > pc_q35_2_8_machine_options(m); > m->alias = NULL; > + pcmc->buggy_intel_iommu_eim = true; > SET_MACHINE_COMPAT(m, PC_COMPAT_2_7); > } > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 47bdf10cfd9b..4bd435f8fa5c 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -143,6 +143,8 @@ struct PCMachineClass { > bool save_tsc_khz; > /* generate legacy CPU hotplug AML */ > bool legacy_cpu_hotplug; > + /* enable buggy Intel-IOMMU EIM by default */ > + bool buggy_intel_iommu_eim; > }; > > #define TYPE_PC_MACHINE "generic-pc-machine" >
On Thu, Sep 29, 2016 at 01:23:29PM +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. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> If you break compatibility and fix it in separate patches, you break bisectability (even for people that are bisecting bugs unrelated to EIM). (But I still don't understand if patch 6/7 really breaks anything, or not.)
2016-09-29 15:19+0200, Paolo Bonzini: > On 29/09/2016 13:23, 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. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > Ugh, I misremembered that VTD_ECAP_EIM was not set in 2.7. :( Perhaps > it's better to drop this patch... I think that adding this backward compatibility hack into code that is supposed to be developed is not a good idea. 2016-09-29 12:01-0300, Eduardo Habkost: > If you break compatibility and fix it in separate patches, you > break bisectability (even for people that are bisecting bugs > unrelated to EIM). I'd keep it as a separate patch and let maintainers decide whether they want to squish or drop it. > (But I still don't understand if patch 6/7 really breaks > anything, or not.) Nothing useful. It "breaks" three cases: 1) If user configured -machine kernel_irqchip=off -device intel_iommu,intremap=on QEMU 2.7 pc-q35-2.7 enabled (broken) EIM, but 2.8 wouldn't, leading to a different machine. (The same with new KVM and split irqchip.) 2) If user had old KVM and configured -machine kernel_irqchip=split -device intel_iommu,intremap=on QEMU 2.7 pc-q35-2.7 enabled (broken) EIM, but after offline migration to 2.8, QEMU would refuse to start. 3) If user started a pc-q35-2.7 with QEMU 2.8 on a new KVM, then they could use cluster x2APIC without a problem, but the guest wouldn't work after offline migration to QEMU 2.7 (I'm not sure if this case is supported). Luckily, the intel-iommu device doesn't support live migration. :)
On Thu, Sep 29, 2016 at 01:23:29PM +0200, Radim Krčmář wrote: [...] > @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp) > if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) { > s->intr_eim = ON_OFF_AUTO_OFF; > } > + if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) { > + s->intr_eim = ON_OFF_AUTO_ON; > + } > if (s->intr_eim == ON_OFF_AUTO_AUTO) { > s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON > : ON_OFF_AUTO_OFF; > } > - if (s->intr_eim == ON_OFF_AUTO_ON) { > + if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) { > if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { > error_report("intel-iommu,eim=on requires support on the KVM side " > "(X2APIC_API, first shipped in v4.7)."); No matter how we would treat this patch, I see that we are stacking up if clauses here. So IMHO maybe it's time to award EIM a new routine: int vtd_eim_detect(IntelIOMMUState *, Error **errp); And squash all these conditions in. Then in vtd_realize(): if (vtd_eim_detect(s, errp)) { return; } Thanks, -- peterx
2016-09-30 13:40+0800, Peter Xu: > On Thu, Sep 29, 2016 at 01:23:29PM +0200, Radim Krčmář wrote: > > [...] > >> @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp) >> if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) { >> s->intr_eim = ON_OFF_AUTO_OFF; >> } >> + if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) { >> + s->intr_eim = ON_OFF_AUTO_ON; >> + } >> if (s->intr_eim == ON_OFF_AUTO_AUTO) { >> s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON >> : ON_OFF_AUTO_OFF; >> } >> - if (s->intr_eim == ON_OFF_AUTO_ON) { >> + if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) { >> if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { >> error_report("intel-iommu,eim=on requires support on the KVM side " >> "(X2APIC_API, first shipped in v4.7)."); > > No matter how we would treat this patch, I see that we are stacking up > if clauses here. So IMHO maybe it's time to award EIM a new routine: > > int vtd_eim_detect(IntelIOMMUState *, Error **errp); > > And squash all these conditions in. Then in vtd_realize(): > > if (vtd_eim_detect(s, errp)) { > return; > } Yeah, thanks.
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index b1afe5b133e0..d6657a361ff9 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -2458,6 +2458,7 @@ static AddressSpace *vtd_host_dma_iommu(PCIBus *bus, void *opaque, int devfn) static void vtd_realize(DeviceState *dev, Error **errp) { PCMachineState *pcms = PC_MACHINE(qdev_get_machine()); + PCMachineClass *pcmc = PC_MACHINE_CLASS(MACHINE_GET_CLASS(pcms)); PCIBus *bus = pcms->bus; IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev); X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev); @@ -2481,11 +2482,14 @@ static void vtd_realize(DeviceState *dev, Error **errp) if (s->intr_eim == ON_OFF_AUTO_AUTO && !x86_iommu->intr_supported) { s->intr_eim = ON_OFF_AUTO_OFF; } + if (s->intr_eim == ON_OFF_AUTO_AUTO && pcmc->buggy_intel_iommu_eim) { + s->intr_eim = ON_OFF_AUTO_ON; + } if (s->intr_eim == ON_OFF_AUTO_AUTO) { s->intr_eim = kvm_irqchip_in_kernel() ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF; } - if (s->intr_eim == ON_OFF_AUTO_ON) { + if (s->intr_eim == ON_OFF_AUTO_ON && !pcmc->buggy_intel_iommu_eim) { if (kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { error_report("intel-iommu,eim=on requires support on the KVM side " "(X2APIC_API, first shipped in v4.7)."); diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c index 0b214f24c977..97f419fbf4dd 100644 --- a/hw/i386/pc_q35.c +++ b/hw/i386/pc_q35.c @@ -304,8 +304,10 @@ DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL, static void pc_q35_2_7_machine_options(MachineClass *m) { + PCMachineClass *pcmc = PC_MACHINE_CLASS(m); pc_q35_2_8_machine_options(m); m->alias = NULL; + pcmc->buggy_intel_iommu_eim = true; SET_MACHINE_COMPAT(m, PC_COMPAT_2_7); } diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 47bdf10cfd9b..4bd435f8fa5c 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -143,6 +143,8 @@ struct PCMachineClass { bool save_tsc_khz; /* generate legacy CPU hotplug AML */ bool legacy_cpu_hotplug; + /* enable buggy Intel-IOMMU EIM by default */ + bool buggy_intel_iommu_eim; }; #define TYPE_PC_MACHINE "generic-pc-machine"
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. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- hw/i386/intel_iommu.c | 6 +++++- hw/i386/pc_q35.c | 2 ++ include/hw/i386/pc.h | 2 ++ 3 files changed, 9 insertions(+), 1 deletion(-)