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

[Michael S. Tsirkin]

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)
> 


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