Re: [PATCH] xen/keyhandler: Move key_table[] into __ro_after_init

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Daniel Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> CC: Jason Andryuk <jandryuk@xxxxxxxxx>
>>
>> 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