Re: [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled

[Xiaoyao Li]

On 4/2/2024 10:31 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 02, 2024 at 09:18:44PM +0800, Xiaoyao Li wrote:
> > On 4/2/2024 6:02 PM, Michael S. Tsirkin wrote:
> > > On Tue, Apr 02, 2024 at 04:25:16AM -0400, Xiaoyao Li wrote:
> > > > Set MADT.FLAGS[bit 0].PCAT_COMPAT based on x86ms->pic.
> > > >
> > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > >
> > > Please include more info in the commit log:
> > > what is the behaviour you observe, why it is wrong,
> > > how does the patch fix it, what is guest behaviour
> > > before and after.
> >
> > Sorry, I thought it was straightforward.
> >
> > A value 1 of PCAT_COMPAT (bit 0) of MADT.Flags indicates that the system
> > also has a PC-AT-compatible dual-8259 setup, i.e., the PIC.
> >
> > When PIC is not enabled for x86 machine, the PCAT_COMPAT bit needs to be
> > cleared. Otherwise, the guest thinks there is a present PIC even it is
> > booted with pic=off on QEMU.
> >
> > (I haven't seen real issue from Linux guest. The user of PIC inside guest
> > seems only the pit calibration. Whether pit calibration is triggered depends
> > on other things. But logically, current code is wrong, we need to fix it
> > anyway.
> >
> > @Isaku, please share more info if you have)

+ Kirill,

It seems to have issue with legacy irqs with PCAT_COMPAT set 1 while no 
PIC on QEMU side. Kirill, could you elaborate it?

> That's sufficient, thanks! Pls put this in commit log and resubmit.
>
> > > The commit log and the subject should not repeat
> > > what the diff already states.
> > >
> > > > ---
> > > >    hw/i386/acpi-common.c | 4 +++-
> > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> > > > index 20f19269da40..0cc2919bb851 100644
> > > > --- a/hw/i386/acpi-common.c
> > > > +++ b/hw/i386/acpi-common.c
> > > > @@ -107,7 +107,9 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> > > >        acpi_table_begin(&table, table_data);
> > > >        /* Local APIC Address */
> > > >        build_append_int_noprefix(table_data, APIC_DEFAULT_ADDRESS, 4);
> > > > -    build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */
> > > > +    /* Flags. bit 0: PCAT_COMPAT */
> > > > +    build_append_int_noprefix(table_data,
> > > > +                              x86ms->pic != ON_OFF_AUTO_OFF ? 1 : 0 , 4);
> > > >        for (i = 0; i < apic_ids->len; i++) {
> > > >            pc_madt_cpu_entry(i, apic_ids, table_data, false);
> > > > --
> > > > 2.34.1