Re: [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops()

[Andrew Cooper <amc96@srcf.net>]

On 08/11/2021 09:04, Jan Beulich wrote:
> On 05.11.2021 14:55, Andrew Cooper wrote:
>> +void __init xsm_fixup_ops(struct xsm_ops *ops)
>> +{
>> +    /*
>> +     * We make some simplifying assumptions about struct xsm_ops; that it is
>> +     * made exclusively of function pointers to non-init text.
>> +     *
>> +     * This allows us to walk over struct xsm_ops as if it were an array of
>> +     * unsigned longs.
>> +     */
>> +    unsigned long *dst = _p(ops);
>> +    unsigned long *src = _p(&dummy_ops);
> I'm afraid I consider this an abuse of _p(): It hides casting when
> that would better not be hidden (and there's then also a pointless
> step through "unsigned long" in the casting). I suppose this is
> also why "src" didn't end up "const unsigned long *" - with spelled
> out casts the casting away of const might have been more noticable.

I've changed to a const pointer, but opencoding _p() wouldn't make it 
any more likely for me to have spotted that it ought to have been const 
to begin with.

But ultimately it comes down to neatness/clarity.  This:

unsigned long *dst = _p(ops);
const unsigned long *src = _p(&dummy_ops);

is easier to read than this:

unsigned long *dst = (unsigned long *)ops;
const unsigned long *src = (const unsigned long *)&dummy_ops;

Fundamentally, I can do either, but I have a preference for the one 
which is easier to follow.

>> +    for ( ; dst < (unsigned long *)(ops + 1); src++, dst++ )
>> +    {
>> +        /*
>> +         * If you encounter this BUG(), then you've most likely added a new
>> +         * XSM hook but failed to provide the default implementation in
>> +         * dummy_ops.
>> +         *
>> +         * If not, then perhaps a function pointer to an init function, or
>> +         * something which isn't a function pointer at all.
>> +         */
>> +        BUG_ON(!is_kernel_text(*src));
> Just as a remark, not a request to change anything: A cause of this
> triggering may also be is_kernel_text() not covering all text
> sections. Some of what recently we've been talking about informally
> may lead to new text section variants appearing, and whether those
> would sensibly end up inside [_stext,_etext) is uncertain.

I'm afraid that I'm not aware of what you're referring to here.  But I 
don't think any good will come from having is_kernel_text() not covering 
suitable things.

~Andrew