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(-)

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");
 

base-commit: 221f2748e8dabe8361b8cdfcffbeab9102c4c899
-- 
2.39.2

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

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