| Message ID | 20240402082516.2921143-1-xiaoyao.li@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled | expand |
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
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 >
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 > >
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 >>> >
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
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);
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(-)