On 14.02.2025 16:09, Andrew Cooper wrote:
> On 13/02/2025 5:00 pm, Jan Beulich wrote:
>> On 13.02.2025 17:46, Andrew Cooper wrote:
>>> @@ -1593,7 +1598,17 @@ void watchdog_domain_init(struct domain *d)
>>> d->watchdog_inuse_map = 0;
>>>
>>> for ( i = 0; i < NR_DOMAIN_WATCHDOG_TIMERS; i++ )
>>> - init_timer(&d->watchdog_timer[i], domain_watchdog_timeout, d, 0);
>>> + {
>>> + void *data = d;
>>> +
>>> + BUILD_BUG_ON(NR_DOMAIN_WATCHDOG_TIMERS >= PAGE_SIZE);
>>> +
>>> + /*
>>> + * For the timer callback parameter, encode the watchdog id in
>>> + * the low bits of the domain pointer.
>>> + */
>>> + init_timer(&d->watchdog_timer[i], domain_watchdog_timeout, data +
>>> i, 0);
>>> + }
>> This way we'll be promising to ourselves that we're never going to alter
>> the allocation mechanism of struct domain instances, always requiring
>> them to have at least page alignment. If someone wanted to change that,
>> they'll have a hard time spotting the logic here. Sadly I have no good
>> suggestion towards improving the situation.
>
> I wasn't terribly happy either, but something has occurred to me.
>
> For both struct domain and vcpu, we could have an __aligned(PAGE_SIZE)
> attribute. It's accurate and unlikely to change (and helps x86 code
> generation at least).
>
> Then, we can use:
>
> BUILD_BUG_ON((NR_DOMAIN_WATCHDOG_TIMERS > alignof(d));
>
> which should trigger cleanly if the precondition is violated.
Hmm, yes, why not. That would establish the missing link.
> For watchdog specifically, we only actually need uint16_t alignment to
> be safe, and there's no way that's going to break in practice.
Right.
Jan
