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

[Bui Quang Minh <minhquangbui99@gmail.com>]

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.