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?
--
MST
