| Message ID | 20230926160637.27995-1-minhquangbui99@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 9/26/23 23:06, Bui Quang Minh wrote: > Version 8 changes, > - Patch 2, 4: > + Rebase to master and resolve conflicts in these 2 patches The conflicts when rebasing is due to the commit 9926cf34de5fa15da ("target/i386: Allow elision of kvm_enable_x2apic()"). AFAIK, this commit adds kvm_enabled() before kvm_enable_x2apic() in the and (&&) expression so that when kvm_enabled() is known to be false at the compile time (CONFIG_KVM_IS_POSSIBLE is undefined), the compiler can omit the kvm_enable_x2apic() in the and expression. In patch 2, I simply combine the change logic in patch 2 with logic in the commit 9926cf34de5fa15da. In patch 4, the end result of version 8 is the same as version 7. I don't think we need to add the kvm_enabled() to make the expression become if (kvm_enabled() && kvm_irqchip_is_split() && !kvm_enable_x2apic()) Because when CONFIG_KVM_IS_POSSIBLE is undefined, kvm_irqchip_is_split() is known to be false at the compile time too so just keep the expression as if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) is enough. > git range-diff feat/tcg-x2apic-v7~5..feat/tcg-x2apic-v7 feat/tcg-x2apic-v8~5..feat/tcg-x2apic-v8 1: c1d197a230 = 1: f6e3918e0f i386/tcg: implement x2APIC registers MSR access 2: dd96cb0238 ! 2: 54d44a15b6 apic: add support for x2APIC mode @@ Commit message ## hw/i386/x86.c ## @@ hw/i386/x86.c: void x86_cpus_init(X86MachineState *x86ms, int default_cpu_version) - * Can we support APIC ID 255 or higher? - * - * Under Xen: yes. -- * With userspace emulated lapic: no -+ * With userspace emulated lapic: checked later in apic_common_set_id. - * With KVM's in-kernel lapic: only if X2APIC API is enabled. + * both in-kernel lapic and X2APIC userspace API. */ - if (x86ms->apic_id_limit > 255 && !xen_enabled() && + if (x86ms->apic_id_limit > 255 && kvm_enabled() && - (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) { + kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { error_report("current -smp configuration requires kernel " 3: 31a5c555a6 = 3: eb080d1e2c apic, i386/tcg: add x2apic transitions 4: d78b5c43b4 ! 4: 59f028f119 intel_iommu: allow Extended Interrupt Mode when using userspace APIC @@ hw/i386/intel_iommu.c: static bool vtd_decide_config(IntelIOMMUState *s, Error * - error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split"); - return false; - } -- if (!kvm_enable_x2apic()) { +- if (kvm_enabled() && !kvm_enable_x2apic()) { + if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) { error_setg(errp, "eim=on requires support on the KVM side" "(X2APIC_API, first shipped in v4.7)"); 5: 51f558035d = 5: bc95c3cb60 amd_iommu: report x2APIC support to the operating system As the change is minor and does not change the main logic, I keep the Reviewed-by and Acked-by tags. Thank you, Quang Minh.
On Tue, Sep 26, 2023 at 11:23:53PM +0700, Bui Quang Minh wrote: > On 9/26/23 23:06, Bui Quang Minh wrote: > > > Version 8 changes, > > - Patch 2, 4: > > + Rebase to master and resolve conflicts in these 2 patches > > The conflicts when rebasing is due to the commit 9926cf34de5fa15da > ("target/i386: Allow elision of kvm_enable_x2apic()"). AFAIK, this commit > adds kvm_enabled() before kvm_enable_x2apic() in the and (&&) expression so > that when kvm_enabled() is known to be false at the compile time > (CONFIG_KVM_IS_POSSIBLE is undefined), the compiler can omit the > kvm_enable_x2apic() in the and expression. > > In patch 2, I simply combine the change logic in patch 2 with logic in the > commit 9926cf34de5fa15da. > > In patch 4, the end result of version 8 is the same as version 7. I don't > think we need to add the kvm_enabled() to make the expression become > > if (kvm_enabled() && kvm_irqchip_is_split() && !kvm_enable_x2apic()) > > Because when CONFIG_KVM_IS_POSSIBLE is undefined, kvm_irqchip_is_split() is > known to be false at the compile time too so just keep the expression as > > if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) > > is enough. > > > git range-diff feat/tcg-x2apic-v7~5..feat/tcg-x2apic-v7 > feat/tcg-x2apic-v8~5..feat/tcg-x2apic-v8 > > 1: c1d197a230 = 1: f6e3918e0f i386/tcg: implement x2APIC registers MSR > access > 2: dd96cb0238 ! 2: 54d44a15b6 apic: add support for x2APIC mode > @@ Commit message > > ## hw/i386/x86.c ## > @@ hw/i386/x86.c: void x86_cpus_init(X86MachineState *x86ms, int > default_cpu_version) > - * Can we support APIC ID 255 or higher? > - * > - * Under Xen: yes. > -- * With userspace emulated lapic: no > -+ * With userspace emulated lapic: checked later in > apic_common_set_id. > - * With KVM's in-kernel lapic: only if X2APIC API is enabled. > + * both in-kernel lapic and X2APIC userspace API. > */ > - if (x86ms->apic_id_limit > 255 && !xen_enabled() && > + if (x86ms->apic_id_limit > 255 && kvm_enabled() && > - (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) { > + kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { > error_report("current -smp configuration requires kernel " > 3: 31a5c555a6 = 3: eb080d1e2c apic, i386/tcg: add x2apic transitions > 4: d78b5c43b4 ! 4: 59f028f119 intel_iommu: allow Extended Interrupt Mode > when using userspace APIC > @@ hw/i386/intel_iommu.c: static bool vtd_decide_config(IntelIOMMUState > *s, Error * > - error_setg(errp, "eim=on requires > accel=kvm,kernel-irqchip=split"); > - return false; > - } > -- if (!kvm_enable_x2apic()) { > +- if (kvm_enabled() && !kvm_enable_x2apic()) { > + if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) { > error_setg(errp, "eim=on requires support on the KVM side" > "(X2APIC_API, first shipped in v4.7)"); > 5: 51f558035d = 5: bc95c3cb60 amd_iommu: report x2APIC support to the > operating system > > As the change is minor and does not change the main logic, I keep the > Reviewed-by and Acked-by tags. > > Thank you, > Quang Minh. Causes some build failures: https://gitlab.com/mstredhat/qemu/-/jobs/5216377483 /builds/mstredhat/qemu/build/../hw/intc/apic.c:1023: undefined reference to `raise_exception_ra' checkpatch warnings: https://gitlab.com/mstredhat/qemu/-/jobs/5216377552
On 10/4/23 13:51, Michael S. Tsirkin wrote: > On Tue, Sep 26, 2023 at 11:23:53PM +0700, Bui Quang Minh wrote: >> On 9/26/23 23:06, Bui Quang Minh wrote: >> >>> Version 8 changes, >>> - Patch 2, 4: >>> + Rebase to master and resolve conflicts in these 2 patches >> >> The conflicts when rebasing is due to the commit 9926cf34de5fa15da >> ("target/i386: Allow elision of kvm_enable_x2apic()"). AFAIK, this commit >> adds kvm_enabled() before kvm_enable_x2apic() in the and (&&) expression so >> that when kvm_enabled() is known to be false at the compile time >> (CONFIG_KVM_IS_POSSIBLE is undefined), the compiler can omit the >> kvm_enable_x2apic() in the and expression. >> >> In patch 2, I simply combine the change logic in patch 2 with logic in the >> commit 9926cf34de5fa15da. >> >> In patch 4, the end result of version 8 is the same as version 7. I don't >> think we need to add the kvm_enabled() to make the expression become >> >> if (kvm_enabled() && kvm_irqchip_is_split() && !kvm_enable_x2apic()) >> >> Because when CONFIG_KVM_IS_POSSIBLE is undefined, kvm_irqchip_is_split() is >> known to be false at the compile time too so just keep the expression as >> >> if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) >> >> is enough. >> >>> git range-diff feat/tcg-x2apic-v7~5..feat/tcg-x2apic-v7 >> feat/tcg-x2apic-v8~5..feat/tcg-x2apic-v8 >> >> 1: c1d197a230 = 1: f6e3918e0f i386/tcg: implement x2APIC registers MSR >> access >> 2: dd96cb0238 ! 2: 54d44a15b6 apic: add support for x2APIC mode >> @@ Commit message >> >> ## hw/i386/x86.c ## >> @@ hw/i386/x86.c: void x86_cpus_init(X86MachineState *x86ms, int >> default_cpu_version) >> - * Can we support APIC ID 255 or higher? >> - * >> - * Under Xen: yes. >> -- * With userspace emulated lapic: no >> -+ * With userspace emulated lapic: checked later in >> apic_common_set_id. >> - * With KVM's in-kernel lapic: only if X2APIC API is enabled. >> + * both in-kernel lapic and X2APIC userspace API. >> */ >> - if (x86ms->apic_id_limit > 255 && !xen_enabled() && >> + if (x86ms->apic_id_limit > 255 && kvm_enabled() && >> - (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) { >> + kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { >> error_report("current -smp configuration requires kernel " >> 3: 31a5c555a6 = 3: eb080d1e2c apic, i386/tcg: add x2apic transitions >> 4: d78b5c43b4 ! 4: 59f028f119 intel_iommu: allow Extended Interrupt Mode >> when using userspace APIC >> @@ hw/i386/intel_iommu.c: static bool vtd_decide_config(IntelIOMMUState >> *s, Error * >> - error_setg(errp, "eim=on requires >> accel=kvm,kernel-irqchip=split"); >> - return false; >> - } >> -- if (!kvm_enable_x2apic()) { >> +- if (kvm_enabled() && !kvm_enable_x2apic()) { >> + if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) { >> error_setg(errp, "eim=on requires support on the KVM side" >> "(X2APIC_API, first shipped in v4.7)"); >> 5: 51f558035d = 5: bc95c3cb60 amd_iommu: report x2APIC support to the >> operating system >> >> As the change is minor and does not change the main logic, I keep the >> Reviewed-by and Acked-by tags. >> >> Thank you, >> Quang Minh. > > > > Causes some build failures: > > https://gitlab.com/mstredhat/qemu/-/jobs/5216377483 > /builds/mstredhat/qemu/build/../hw/intc/apic.c:1023: undefined reference to `raise_exception_ra' raise_exception_ra is tcg specific so the builds are failed as tcg is disabled. I will remove the use of raise_exception_ra, the invalid register read just returns 0, invalid register write has no effect without raising the exception anymore. The APIC state invalid transition does not raise exception either, just don't change the APIC state. As a side effect, we fail some more KVM unit test of invalid APIC state transition, as they expect to catch exception in these cases. I think it's not a big problem. What's your opinion? Thank you, Quang Minh.
On Wed, Oct 04, 2023 at 11:40:43PM +0700, Bui Quang Minh wrote: > On 10/4/23 13:51, Michael S. Tsirkin wrote: > > On Tue, Sep 26, 2023 at 11:23:53PM +0700, Bui Quang Minh wrote: > > > On 9/26/23 23:06, Bui Quang Minh wrote: > > > > > > > Version 8 changes, > > > > - Patch 2, 4: > > > > + Rebase to master and resolve conflicts in these 2 patches > > > > > > The conflicts when rebasing is due to the commit 9926cf34de5fa15da > > > ("target/i386: Allow elision of kvm_enable_x2apic()"). AFAIK, this commit > > > adds kvm_enabled() before kvm_enable_x2apic() in the and (&&) expression so > > > that when kvm_enabled() is known to be false at the compile time > > > (CONFIG_KVM_IS_POSSIBLE is undefined), the compiler can omit the > > > kvm_enable_x2apic() in the and expression. > > > > > > In patch 2, I simply combine the change logic in patch 2 with logic in the > > > commit 9926cf34de5fa15da. > > > > > > In patch 4, the end result of version 8 is the same as version 7. I don't > > > think we need to add the kvm_enabled() to make the expression become > > > > > > if (kvm_enabled() && kvm_irqchip_is_split() && !kvm_enable_x2apic()) > > > > > > Because when CONFIG_KVM_IS_POSSIBLE is undefined, kvm_irqchip_is_split() is > > > known to be false at the compile time too so just keep the expression as > > > > > > if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) > > > > > > is enough. > > > > > > > git range-diff feat/tcg-x2apic-v7~5..feat/tcg-x2apic-v7 > > > feat/tcg-x2apic-v8~5..feat/tcg-x2apic-v8 > > > > > > 1: c1d197a230 = 1: f6e3918e0f i386/tcg: implement x2APIC registers MSR > > > access > > > 2: dd96cb0238 ! 2: 54d44a15b6 apic: add support for x2APIC mode > > > @@ Commit message > > > > > > ## hw/i386/x86.c ## > > > @@ hw/i386/x86.c: void x86_cpus_init(X86MachineState *x86ms, int > > > default_cpu_version) > > > - * Can we support APIC ID 255 or higher? > > > - * > > > - * Under Xen: yes. > > > -- * With userspace emulated lapic: no > > > -+ * With userspace emulated lapic: checked later in > > > apic_common_set_id. > > > - * With KVM's in-kernel lapic: only if X2APIC API is enabled. > > > + * both in-kernel lapic and X2APIC userspace API. > > > */ > > > - if (x86ms->apic_id_limit > 255 && !xen_enabled() && > > > + if (x86ms->apic_id_limit > 255 && kvm_enabled() && > > > - (!kvm_irqchip_in_kernel() || !kvm_enable_x2apic())) { > > > + kvm_irqchip_in_kernel() && !kvm_enable_x2apic()) { > > > error_report("current -smp configuration requires kernel " > > > 3: 31a5c555a6 = 3: eb080d1e2c apic, i386/tcg: add x2apic transitions > > > 4: d78b5c43b4 ! 4: 59f028f119 intel_iommu: allow Extended Interrupt Mode > > > when using userspace APIC > > > @@ hw/i386/intel_iommu.c: static bool vtd_decide_config(IntelIOMMUState > > > *s, Error * > > > - error_setg(errp, "eim=on requires > > > accel=kvm,kernel-irqchip=split"); > > > - return false; > > > - } > > > -- if (!kvm_enable_x2apic()) { > > > +- if (kvm_enabled() && !kvm_enable_x2apic()) { > > > + if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) { > > > error_setg(errp, "eim=on requires support on the KVM side" > > > "(X2APIC_API, first shipped in v4.7)"); > > > 5: 51f558035d = 5: bc95c3cb60 amd_iommu: report x2APIC support to the > > > operating system > > > > > > As the change is minor and does not change the main logic, I keep the > > > Reviewed-by and Acked-by tags. > > > > > > Thank you, > > > Quang Minh. > > > > > > > > Causes some build failures: > > > > https://gitlab.com/mstredhat/qemu/-/jobs/5216377483 > > /builds/mstredhat/qemu/build/../hw/intc/apic.c:1023: undefined reference to `raise_exception_ra' > > raise_exception_ra is tcg specific so the builds are failed as tcg is > disabled. I will remove the use of raise_exception_ra, the invalid register > read just returns 0, invalid register write has no effect without raising > the exception anymore. The APIC state invalid transition does not raise > exception either, just don't change the APIC state. As a side effect, we > fail some more KVM unit test of invalid APIC state transition, as they > expect to catch exception in these cases. I think it's not a big problem. > What's your opinion? > > Thank you, > Quang Minh. Hmm. I think this will have to be addressed somehow so people and ci systems are not confused. Paolo any feedback?
On 9/26/23 23:06, Bui Quang Minh wrote: > Hi everyone, > > This series implements x2APIC mode in userspace local APIC and the > RDMSR/WRMSR helper to access x2APIC registers in x2APIC mode. Intel iommu > and AMD iommu are adjusted to support x2APIC interrupt remapping. With this > series, we can now boot Linux kernel into x2APIC mode with TCG accelerator > using either Intel or AMD iommu. > > Testing to boot my own built Linux 6.3.0-rc2, the kernel successfully boot > with enabled x2APIC and can enumerate CPU with APIC ID 257 > > Using Intel IOMMU > > qemu/build/qemu-system-x86_64 \ > -smp 2,maxcpus=260 \ > -cpu qemu64,x2apic=on \ > -machine q35 \ > -device intel-iommu,intremap=on,eim=on \ > -device qemu64-x86_64-cpu,x2apic=on,core-id=257,socket-id=0,thread-id=0 \ > -m 2G \ > -kernel $KERNEL_DIR \ > -append "nokaslr console=ttyS0 root=/dev/sda earlyprintk=serial net.ifnames=0" \ > -drive file=$IMAGE_DIR,format=raw \ > -nographic \ > -s > > Using AMD IOMMU > > qemu/build/qemu-system-x86_64 \ > -smp 2,maxcpus=260 \ > -cpu qemu64,x2apic=on \ > -machine q35 \ > -device amd-iommu,intremap=on,xtsup=on \ > -device qemu64-x86_64-cpu,x2apic=on,core-id=257,socket-id=0,thread-id=0 \ > -m 2G \ > -kernel $KERNEL_DIR \ > -append "nokaslr console=ttyS0 root=/dev/sda earlyprintk=serial net.ifnames=0" \ > -drive file=$IMAGE_DIR,format=raw \ > -nographic \ > -s > > Testing the emulated userspace APIC with kvm-unit-tests, disable test > device with this patch > > diff --git a/lib/x86/fwcfg.c b/lib/x86/fwcfg.c > index 1734afb..f56fe1c 100644 > --- a/lib/x86/fwcfg.c > +++ b/lib/x86/fwcfg.c > @@ -27,6 +27,7 @@ static void read_cfg_override(void) > > if ((str = getenv("TEST_DEVICE"))) > no_test_device = !atol(str); > + no_test_device = true; > > if ((str = getenv("MEMLIMIT"))) > fw_override[FW_CFG_MAX_RAM] = atol(str) * 1024 * 1024; > > ~ env QEMU=/home/minh/Desktop/oss/qemu/build/qemu-system-x86_64 ACCEL=tcg \ > ./run_tests.sh -v -g apic > > TESTNAME=apic-split TIMEOUT=90s ACCEL=tcg ./x86/run x86/apic.flat -smp 2 > -cpu qemu64,+x2apic,+tsc-deadline -machine kernel_irqchip=split FAIL > apic-split (54 tests, 8 unexpected failures, 1 skipped) > TESTNAME=ioapic-split TIMEOUT=90s ACCEL=tcg ./x86/run x86/ioapic.flat -smp > 1 -cpu qemu64 -machine kernel_irqchip=split PASS ioapic-split (19 tests) > TESTNAME=x2apic TIMEOUT=30 ACCEL=tcg ./x86/run x86/apic.flat -smp 2 -cpu > qemu64,+x2apic,+tsc-deadline FAIL x2apic (54 tests, 8 unexpected failures, > 1 skipped) TESTNAME=xapic TIMEOUT=60 ACCEL=tcg ./x86/run x86/apic.flat -smp > 2 -cpu qemu64,-x2apic,+tsc-deadline -machine pit=off FAIL xapic (43 tests, > 6 unexpected failures, 2 skipped) > > FAIL: apic_disable: *0xfee00030: 50014 > FAIL: apic_disable: *0xfee00080: f0 > FAIL: apic_disable: *0xfee00030: 50014 > FAIL: apic_disable: *0xfee00080: f0 > FAIL: apicbase: relocate apic > > These errors are because we don't disable MMIO region when switching to > x2APIC and don't support relocate MMIO region yet. This is a problem > because, MMIO region is the same for all CPUs, in order to support these we > need to figure out how to allocate and manage different MMIO regions for > each CPUs. This can be an improvement in the future. I've tried to address these failed tests with the idea of creating separate APIC MMIO region per CPU. I've created a working patch with this approach and will send it in reply to this message, you can see the detail in the patch. However, it has a big drawback, it breaks MSI handler. With that patch, device needs to call apic_send_msi directly instead of writing to 0xfee00000 in system memory. Furthermore, I think APIC MMIO relocation is a very unusual use case and APIC MMIO disable is not much important for normal system software. I'm pleased to receive any comments on that patch. Thank you, Quang Minh.
diff --git a/lib/x86/fwcfg.c b/lib/x86/fwcfg.c index 1734afb..f56fe1c 100644 --- a/lib/x86/fwcfg.c +++ b/lib/x86/fwcfg.c @@ -27,6 +27,7 @@ static void read_cfg_override(void) if ((str = getenv("TEST_DEVICE"))) no_test_device = !atol(str); + no_test_device = true; if ((str = getenv("MEMLIMIT"))) fw_override[FW_CFG_MAX_RAM] = atol(str) * 1024 * 1024;