Re: [PATCH] xen/watchdog: Identify which domain watchdog fired

[Jan Beulich <jbeulich@xxxxxxxx>]

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