* [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled @ 2024-04-02 8:25 Xiaoyao Li 2024-04-02 10:02 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Xiaoyao Li @ 2024-04-02 8:25 UTC (permalink / raw) To: Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost Cc: qemu-devel, xiaoyao.li, isaku.yamahata 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled 2024-04-02 8:25 [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled Xiaoyao Li @ 2024-04-02 10:02 ` Michael S. Tsirkin 2024-04-02 13:18 ` Xiaoyao Li 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2024-04-02 10:02 UTC (permalink / raw) To: Xiaoyao Li Cc: Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost, qemu-devel, isaku.yamahata 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled 2024-04-02 10:02 ` Michael S. Tsirkin @ 2024-04-02 13:18 ` Xiaoyao Li 2024-04-02 14:31 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Xiaoyao Li @ 2024-04-02 13:18 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost, qemu-devel, isaku.yamahata 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled 2024-04-02 13:18 ` Xiaoyao Li @ 2024-04-02 14:31 ` Michael S. Tsirkin 2024-04-03 2:03 ` Xiaoyao Li 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2024-04-02 14:31 UTC (permalink / raw) To: Xiaoyao Li Cc: Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost, qemu-devel, isaku.yamahata 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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled 2024-04-02 14:31 ` Michael S. Tsirkin @ 2024-04-03 2:03 ` Xiaoyao Li 2024-04-03 13:37 ` Kirill A. Shutemov 0 siblings, 1 reply; 6+ messages in thread From: Xiaoyao Li @ 2024-04-03 2:03 UTC (permalink / raw) To: Michael S. Tsirkin, Kirill A. Shutemov Cc: Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost, qemu-devel, isaku.yamahata 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 >>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled 2024-04-03 2:03 ` Xiaoyao Li @ 2024-04-03 13:37 ` Kirill A. Shutemov 0 siblings, 0 replies; 6+ messages in thread From: Kirill A. Shutemov @ 2024-04-03 13:37 UTC (permalink / raw) To: Xiaoyao Li Cc: Michael S. Tsirkin, Marcel Apfelbaum, Paolo Bonzini, Richard Henderson, Eduardo Habkost, qemu-devel, isaku.yamahata 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-03 13:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-02 8:25 [PATCH] hw/i386/acpi: Set PCAT_COMPAT bit only when pic is not disabled Xiaoyao Li 2024-04-02 10:02 ` Michael S. Tsirkin 2024-04-02 13:18 ` Xiaoyao Li 2024-04-02 14:31 ` Michael S. Tsirkin 2024-04-03 2:03 ` Xiaoyao Li 2024-04-03 13:37 ` Kirill A. Shutemov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).