| Message ID | 20231120151006.168773-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ioapic: Check 1k alignment for IO-APIC physical addresses | expand |
On 20.11.2023 16:10, Andrew Cooper wrote: > The MP spec requires a minimum of 1k alignment. So too does Xen's use of a > single fixmap page to map the IO-APIC. > > Reject out-of-spec values so we don't end up in a position where a bad > firmware value causes Xen to use the wrong mapping. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> I'm curious though: Was this observed in practice? Jan
On 20/11/2023 4:06 pm, Jan Beulich wrote: > On 20.11.2023 16:10, Andrew Cooper wrote: >> The MP spec requires a minimum of 1k alignment. So too does Xen's use of a >> single fixmap page to map the IO-APIC. >> >> Reject out-of-spec values so we don't end up in a position where a bad >> firmware value causes Xen to use the wrong mapping. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > I'm curious though: Was this observed in practice? No. I had some prototype cleanup which collided with MISRA, and I realised I hadn't posted this part of the series. The other part of the cleanup still has work to do, and I don't have time to unpick it right now. ~Andrew
On Mon, Nov 20, 2023 at 03:10:06PM +0000, Andrew Cooper wrote: > The MP spec requires a minimum of 1k alignment. So too does Xen's use of a > single fixmap page to map the IO-APIC. > > Reject out-of-spec values so we don't end up in a position where a bad > firmware value causes Xen to use the wrong mapping. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/io_apic.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c > index 0ef61fb2f167..db1ea6e2f2ca 100644 > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -2589,10 +2589,11 @@ static void __init ioapic_init_mappings(void) > union IO_APIC_reg_01 reg_01; > paddr_t ioapic_phys = mp_ioapics[i].mpc_apicaddr; > > - if ( !ioapic_phys ) > + if ( !ioapic_phys || (ioapic_phys & (KB(1) - 1)) ) Might be clearer to use !IS_ALIGNED(), otherwise: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> > { > printk(KERN_ERR > - "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling IO/APIC support!\n"); > + "WARNING: bogus IO-APIC address %08lx found in MPTABLE, disabling IO/APIC support!\n", FWIW, I think the '!' at the end is not useful, and I would adjust the last 'IO/APIC' to 'IO-APIC' if already modifying the line. Thanks, Roger.
On 20/11/2023 5:36 pm, Roger Pau Monné wrote: > On Mon, Nov 20, 2023 at 03:10:06PM +0000, Andrew Cooper wrote: >> The MP spec requires a minimum of 1k alignment. So too does Xen's use of a >> single fixmap page to map the IO-APIC. >> >> Reject out-of-spec values so we don't end up in a position where a bad >> firmware value causes Xen to use the wrong mapping. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Wei Liu <wl@xen.org> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> --- >> xen/arch/x86/io_apic.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c >> index 0ef61fb2f167..db1ea6e2f2ca 100644 >> --- a/xen/arch/x86/io_apic.c >> +++ b/xen/arch/x86/io_apic.c >> @@ -2589,10 +2589,11 @@ static void __init ioapic_init_mappings(void) >> union IO_APIC_reg_01 reg_01; >> paddr_t ioapic_phys = mp_ioapics[i].mpc_apicaddr; >> >> - if ( !ioapic_phys ) >> + if ( !ioapic_phys || (ioapic_phys & (KB(1) - 1)) ) > Might be clearer to use !IS_ALIGNED() Hmm, yeah. Will change. > , otherwise: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > >> { >> printk(KERN_ERR >> - "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling IO/APIC support!\n"); >> + "WARNING: bogus IO-APIC address %08lx found in MPTABLE, disabling IO/APIC support!\n", > FWIW, I think the '!' at the end is not useful, and I would adjust > the last 'IO/APIC' to 'IO-APIC' if already modifying the line. Ok. will do. ~Andrew
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c index 0ef61fb2f167..db1ea6e2f2ca 100644 --- a/xen/arch/x86/io_apic.c +++ b/xen/arch/x86/io_apic.c @@ -2589,10 +2589,11 @@ static void __init ioapic_init_mappings(void) union IO_APIC_reg_01 reg_01; paddr_t ioapic_phys = mp_ioapics[i].mpc_apicaddr; - if ( !ioapic_phys ) + if ( !ioapic_phys || (ioapic_phys & (KB(1) - 1)) ) { printk(KERN_ERR - "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling IO/APIC support!\n"); + "WARNING: bogus IO-APIC address %08lx found in MPTABLE, disabling IO/APIC support!\n", + ioapic_phys); smp_found_config = false; skip_ioapic_setup = true; break;
The MP spec requires a minimum of 1k alignment. So too does Xen's use of a single fixmap page to map the IO-APIC. Reject out-of-spec values so we don't end up in a position where a bad firmware value causes Xen to use the wrong mapping. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/io_apic.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)