| Message ID | 20240912125154.1708025-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/keyhandler: Move key_table[] into __ro_after_init | expand |
On Thu, Sep 12, 2024 at 01:51:54PM +0100, Andrew Cooper wrote: > All registration is done at boot. Almost... > > iommu_dump_page_tables() is registered in iommu_hwdom_init(), which is called > twice when LATE_HWDOM is in use. > > register_irq_keyhandler() has an ASSERT() guarding againt multiple > registration attempts, and the absence of bug reports hints at how many > configurations use LATE_HWDOM in practice. > > Move the registration into iommu_setup() just after printing the overall > status of the IOMMU. For starters, the hardware domain is specifically > excluded by iommu_dump_page_tables(). > > ept_dump_p2m_table is registered in setup_ept_dump() which is non-__init, but > whose sole caller, start_vmx(), is __init. Move setup_ept_dump() to match. > > With these two tweeks, all keyhandler reigstration is from __init functions, > so register_{,irq_}keyhandler() can move, and key_table[] can become > __ro_after_init. > > No practical change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Daniel Smith <dpsmith@apertussolutions.com> > CC: Jason Andryuk <jandryuk@gmail.com> > > CC'ing some OpenXT folks just FYI. > --- > xen/arch/x86/mm/p2m-ept.c | 2 +- > xen/common/keyhandler.c | 10 +++++----- > xen/drivers/passthrough/iommu.c | 4 ++-- > 3 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > index 2ea574ca6aef..21728397f9ac 100644 > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -1497,7 +1497,7 @@ static void cf_check ept_dump_p2m_table(unsigned char key) > rcu_read_unlock(&domlist_read_lock); > } > > -void setup_ept_dump(void) > +void __init setup_ept_dump(void) I would be tempted to just drop this function altogether and open-code the call to register_keyhandler(). Thanks, Roger.
On 12/09/2024 2:37 pm, Roger Pau Monné wrote: > On Thu, Sep 12, 2024 at 01:51:54PM +0100, Andrew Cooper wrote: >> All registration is done at boot. Almost... >> >> iommu_dump_page_tables() is registered in iommu_hwdom_init(), which is called >> twice when LATE_HWDOM is in use. >> >> register_irq_keyhandler() has an ASSERT() guarding againt multiple >> registration attempts, and the absence of bug reports hints at how many >> configurations use LATE_HWDOM in practice. >> >> Move the registration into iommu_setup() just after printing the overall >> status of the IOMMU. For starters, the hardware domain is specifically >> excluded by iommu_dump_page_tables(). >> >> ept_dump_p2m_table is registered in setup_ept_dump() which is non-__init, but >> whose sole caller, start_vmx(), is __init. Move setup_ept_dump() to match. >> >> With these two tweeks, all keyhandler reigstration is from __init functions, >> so register_{,irq_}keyhandler() can move, and key_table[] can become >> __ro_after_init. >> >> No practical change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. > >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Daniel Smith <dpsmith@apertussolutions.com> >> CC: Jason Andryuk <jandryuk@gmail.com> >> >> CC'ing some OpenXT folks just FYI. >> --- >> xen/arch/x86/mm/p2m-ept.c | 2 +- >> xen/common/keyhandler.c | 10 +++++----- >> xen/drivers/passthrough/iommu.c | 4 ++-- >> 3 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c >> index 2ea574ca6aef..21728397f9ac 100644 >> --- a/xen/arch/x86/mm/p2m-ept.c >> +++ b/xen/arch/x86/mm/p2m-ept.c >> @@ -1497,7 +1497,7 @@ static void cf_check ept_dump_p2m_table(unsigned char key) >> rcu_read_unlock(&domlist_read_lock); >> } >> >> -void setup_ept_dump(void) >> +void __init setup_ept_dump(void) > I would be tempted to just drop this function altogether and open-code > the call to register_keyhandler(). That would involve exporting ept_dump_p2m_table(), and quickly started detracting from the main purpose of the patch. There are a whole bunch of keyhandler registration which are unconditional out of an initcall. I'd like to do something better, but there's nothing completely obvious. ~Andrew
On 12.09.2024 14:51, Andrew Cooper wrote: > @@ -596,6 +594,8 @@ int __init iommu_setup(void) > } > else > { > + register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0); > + > if ( iommu_quarantine_init() ) > panic("Could not set up quarantine\n"); It's a little odd to have this immediately ahead of something that can panic(), but not a big deal of course. The line wants wrapping though, to stay within 80 cols. Jan
On 12/09/2024 3:20 pm, Jan Beulich wrote: > On 12.09.2024 14:51, Andrew Cooper wrote: >> @@ -596,6 +594,8 @@ int __init iommu_setup(void) >> } >> else >> { >> + register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0); >> + >> if ( iommu_quarantine_init() ) >> panic("Could not set up quarantine\n"); > It's a little odd to have this immediately ahead of something that can > panic(), but not a big deal of course. The line wants wrapping though, > to stay within 80 cols. I'm happy to put it somewhere else if you can suggest somewhere better. I was just looking for somewhere which was clearly behind an iommu_enabled check, as it was in iommu_hwdom_init(). ~Andrew
On 12.09.2024 16:22, Andrew Cooper wrote: > On 12/09/2024 3:20 pm, Jan Beulich wrote: >> On 12.09.2024 14:51, Andrew Cooper wrote: >>> @@ -596,6 +594,8 @@ int __init iommu_setup(void) >>> } >>> else >>> { >>> + register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0); >>> + >>> if ( iommu_quarantine_init() ) >>> panic("Could not set up quarantine\n"); >> It's a little odd to have this immediately ahead of something that can >> panic(), but not a big deal of course. The line wants wrapping though, >> to stay within 80 cols. > > I'm happy to put it somewhere else if you can suggest somewhere better. Just at the bottom of this very else block? Jan
On 12/09/2024 3:23 pm, Jan Beulich wrote: > On 12.09.2024 16:22, Andrew Cooper wrote: >> On 12/09/2024 3:20 pm, Jan Beulich wrote: >>> On 12.09.2024 14:51, Andrew Cooper wrote: >>>> @@ -596,6 +594,8 @@ int __init iommu_setup(void) >>>> } >>>> else >>>> { >>>> + register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0); >>>> + >>>> if ( iommu_quarantine_init() ) >>>> panic("Could not set up quarantine\n"); >>> It's a little odd to have this immediately ahead of something that can >>> panic(), but not a big deal of course. The line wants wrapping though, >>> to stay within 80 cols. >> I'm happy to put it somewhere else if you can suggest somewhere better. > Just at the bottom of this very else block? Done. ~Andrew
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index 2ea574ca6aef..21728397f9ac 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1497,7 +1497,7 @@ static void cf_check ept_dump_p2m_table(unsigned char key) rcu_read_unlock(&domlist_read_lock); } -void setup_ept_dump(void) +void __init setup_ept_dump(void) { register_keyhandler('D', ept_dump_p2m_table, "dump VT-x EPT tables", 0); } diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c index 674e7be39e9d..6da291b34ebc 100644 --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -40,7 +40,7 @@ static struct keyhandler { const char *desc; /* Description for help message. */ bool irq_callback, /* Call in irq context? if not, tasklet context. */ diagnostic; /* Include in 'dump all' handler. */ -} key_table[128] __read_mostly = +} key_table[128] __ro_after_init = { #define KEYHANDLER(k, f, desc, diag) \ [k] = { { .fn = (f) }, desc, 0, diag } @@ -99,8 +99,8 @@ void handle_keypress(unsigned char key, bool need_context) } } -void register_keyhandler(unsigned char key, keyhandler_fn_t *fn, - const char *desc, bool diagnostic) +void __init register_keyhandler(unsigned char key, keyhandler_fn_t *fn, + const char *desc, bool diagnostic) { BUG_ON(key >= ARRAY_SIZE(key_table)); /* Key in range? */ ASSERT(!key_table[key].fn); /* Clobbering something else? */ @@ -111,8 +111,8 @@ void register_keyhandler(unsigned char key, keyhandler_fn_t *fn, key_table[key].diagnostic = diagnostic; } -void register_irq_keyhandler(unsigned char key, irq_keyhandler_fn_t *fn, - const char *desc, bool diagnostic) +void __init register_irq_keyhandler(unsigned char key, irq_keyhandler_fn_t *fn, + const char *desc, bool diagnostic) { BUG_ON(key >= ARRAY_SIZE(key_table)); /* Key in range? */ ASSERT(!key_table[key].irq_fn); /* Clobbering something else? */ diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index 50bfd62553ae..1c567d441cd5 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -271,8 +271,6 @@ void __hwdom_init iommu_hwdom_init(struct domain *d) if ( !is_iommu_enabled(d) ) return; - register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0); - iommu_vcall(hd->platform_ops, hwdom_init, d); } @@ -596,6 +594,8 @@ int __init iommu_setup(void) } else { + register_keyhandler('o', &iommu_dump_page_tables, "dump iommu page tables", 0); + if ( iommu_quarantine_init() ) panic("Could not set up quarantine\n");
All registration is done at boot. Almost... iommu_dump_page_tables() is registered in iommu_hwdom_init(), which is called twice when LATE_HWDOM is in use. register_irq_keyhandler() has an ASSERT() guarding againt multiple registration attempts, and the absence of bug reports hints at how many configurations use LATE_HWDOM in practice. Move the registration into iommu_setup() just after printing the overall status of the IOMMU. For starters, the hardware domain is specifically excluded by iommu_dump_page_tables(). ept_dump_p2m_table is registered in setup_ept_dump() which is non-__init, but whose sole caller, start_vmx(), is __init. Move setup_ept_dump() to match. With these two tweeks, all keyhandler reigstration is from __init functions, so register_{,irq_}keyhandler() can move, and key_table[] can become __ro_after_init. No practical change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Daniel Smith <dpsmith@apertussolutions.com> CC: Jason Andryuk <jandryuk@gmail.com> CC'ing some OpenXT folks just FYI. --- xen/arch/x86/mm/p2m-ept.c | 2 +- xen/common/keyhandler.c | 10 +++++----- xen/drivers/passthrough/iommu.c | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) base-commit: 221f2748e8dabe8361b8cdfcffbeab9102c4c899