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

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

Posted by Xiaoyao Li 1 year, 10 months ago

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 (pic=off) for x86 machine, the PCAT_COMPAT bit
needs to be cleared. Otherwise, the guest thinks there is a present PIC.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
changes in v2:
- Clarify more in commit message;
---
 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 v2] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled

Posted by Paolo Bonzini 1 year, 9 months ago

Queued, thanks.

Paolo

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

Posted by Michael S. Tsirkin 1 year, 9 months ago

On Mon, Apr 15, 2024 at 11:44:50AM +0200, Paolo Bonzini wrote:
> Queued, thanks.
> 
> Paolo

I actually had it queued too - it's not 9.0 material though.
If you queue it don't forget to tweak the commit log manually.

-- 
MST

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

Posted by Michael S. Tsirkin 1 year, 9 months ago

On Mon, Apr 15, 2024 at 05:51:56AM -0400, Michael S. Tsirkin wrote:
> On Mon, Apr 15, 2024 at 11:44:50AM +0200, Paolo Bonzini wrote:
> > Queued, thanks.
> > 
> > Paolo
> 
> I actually had it queued too - it's not 9.0 material though.
> If you queue it don't forget to tweak the commit log manually.

oh and then you can add my

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> -- 
> MST

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

Posted by Igor Mammedov 1 year, 10 months ago

On Wed,  3 Apr 2024 10:59:53 -0400
Xiaoyao Li <xiaoyao.li@intel.com> wrote:

> 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 (pic=off) for x86 machine, the PCAT_COMPAT bit
> needs to be cleared. Otherwise, the guest thinks there is a present PIC.

Can you add to commit message reproducer (aka qemu CLI and relevant
logs/symptoms observed on guest side)?

> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
> changes in v2:
> - Clarify more in commit message;
> ---
>  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);

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

Posted by Xiaoyao Li 1 year, 10 months ago

On 4/3/2024 11:12 PM, Igor Mammedov wrote:
> On Wed,  3 Apr 2024 10:59:53 -0400
> Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> 
>> 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 (pic=off) for x86 machine, the PCAT_COMPAT bit
>> needs to be cleared. Otherwise, the guest thinks there is a present PIC.
> 
> Can you add to commit message reproducer (aka qemu CLI and relevant
> logs/symptoms observed on guest side)?

When booting a VM with "-machine xx,pic=off", there is supposed to be no 
PIC for the guest. When guest probes PIC, it should find nothing and log 
of below should be printed:

   [    0.155970] Using NULL legacy PIC

However, the fact is that no such log printed in guest kernel, with the 
VM created with "pic=off". This is because guest think there is a 
present due to pcat_compat is reporte as 1 in MADT. See Linux code of 
probe_8259A() in arch/x86/kernel/i8259.c 


> 
>>    
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>                                  
>> ---
>> changes in v2:
>> - Clarify more in commit message;
>> ---
>>   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);
>