[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/5] xen/xsm: Improve fallback handling in xsm_fixup_ops()
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |