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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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