Re: [PATCH v8 0/5] Support x2APIC mode with TCG accelerator

[Michael S. Tsirkin]

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