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

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

Posted by Xiaoyao Li 1 month ago

Set MADT.FLAGS[bit 0].PCAT_COMPAT based on x86ms->pic.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 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

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

Posted by Michael S. Tsirkin 1 month ago

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.

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

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

Posted by Xiaoyao Li 1 month ago

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)


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

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

Posted by Michael S. Tsirkin 1 month ago

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

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

Posted by Xiaoyao Li 1 month ago

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

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

Posted by Kirill A. Shutemov 1 month ago

On Wed, Apr 03, 2024 at 10:03:15AM +0800, Xiaoyao Li wrote:
> 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?

TDX guest cannot support PIC because the platform doesn't allow direct
interrupt injection, only posted interrupts.

For TDX guest kernel we had a patch[1] that forces no-PIC, but it is not
upstreamable as it is a hack.

I looked around to find The Right Way™ to archive the same effect and
discovered that we only have PIC ops hooked up because kernel bypasses[2]
PIC enumeration because PCAT_COMPAT is set. Which is wrong for TDX guest
or other platforms without PIC.

I am not aware about any user-visible issues due to it, but maybe they are
just not discovered yet.

[1] https://lore.kernel.org/linux-kernel/b29f00c1eb5cff585ec2b999b69923c13418ecc4.1619458733.git.sathyanarayanan.kuppuswamy@linux.intel.com/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/kernel/i8259.c#n322

-- 
  Kiryl Shutsemau / Kirill A. Shutemov