Some use cases of get_global_virq_handler() didn't account for the
case of running without hardware domain.

Fix that by testing get_global_virq_handler() returning NULL where
needed (e.g. when directly dereferencing the result).

Fixes: 980822c5edd1 ("xen/events: allow setting of global virq handler only for unbound virqs")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/event_channel.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 4ee6b6b4ce..fa83d9dd1a 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1036,7 +1036,9 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
     {
         old = global_virq_handlers[virq];
         hdl = get_global_virq_handler(virq);
-        if ( hdl != d )
+        if ( !hdl )
+            global_virq_handlers[virq] = d;
+        else if ( hdl != d )
         {
             read_lock(&hdl->event_lock);
 
@@ -1091,7 +1093,7 @@ struct domain *lock_dom_exc_handler(void)
     struct domain *d;
 
     d = get_global_virq_handler(VIRQ_DOM_EXC);
-    if ( unlikely(!get_domain(d)) )
+    if ( unlikely(!d || !get_domain(d)) )
         return NULL;
 
     read_lock(&d->event_lock);
-- 
2.43.0

On 06.03.2025 16:54, Juergen Gross wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1036,7 +1036,9 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
>      {
>          old = global_virq_handlers[virq];
>          hdl = get_global_virq_handler(virq);
> -        if ( hdl != d )
> +        if ( !hdl )
> +            global_virq_handlers[virq] = d;
> +        else if ( hdl != d )
>          {
>              read_lock(&hdl->event_lock);
>  
> @@ -1091,7 +1093,7 @@ struct domain *lock_dom_exc_handler(void)
>      struct domain *d;
>  
>      d = get_global_virq_handler(VIRQ_DOM_EXC);
> -    if ( unlikely(!get_domain(d)) )
> +    if ( unlikely(!d || !get_domain(d)) )

According to my understanding of how unlikely() works this wants to be

    if ( unlikely(!d) || unlikely(!get_domain(d)) )

Of course I could also live with being given an explanation of why my
understanding is wrong.

>          return NULL;
>  
>      read_lock(&d->event_lock);

There's one more change needed: get_domain_state() needs to avoid calling
unlock_dom_exc_handler() when "hdl" is NULL.

Jan

On 06.03.25 17:00, Jan Beulich wrote:
> On 06.03.2025 16:54, Juergen Gross wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -1036,7 +1036,9 @@ int set_global_virq_handler(struct domain *d, uint32_t virq)
>>       {
>>           old = global_virq_handlers[virq];
>>           hdl = get_global_virq_handler(virq);
>> -        if ( hdl != d )
>> +        if ( !hdl )
>> +            global_virq_handlers[virq] = d;
>> +        else if ( hdl != d )
>>           {
>>               read_lock(&hdl->event_lock);
>>   
>> @@ -1091,7 +1093,7 @@ struct domain *lock_dom_exc_handler(void)
>>       struct domain *d;
>>   
>>       d = get_global_virq_handler(VIRQ_DOM_EXC);
>> -    if ( unlikely(!get_domain(d)) )
>> +    if ( unlikely(!d || !get_domain(d)) )
> 
> According to my understanding of how unlikely() works this wants to be
> 
>      if ( unlikely(!d) || unlikely(!get_domain(d)) )
> 
> Of course I could also live with being given an explanation of why my
> understanding is wrong.

I was unsure, so I looked through the code until I found a case where
2 conditions were tested as "unlikely". I might have found a wrong example.
I'll change it as you are suggesting.

> 
>>           return NULL;
>>   
>>       read_lock(&d->event_lock);
> 
> There's one more change needed: get_domain_state() needs to avoid calling
> unlock_dom_exc_handler() when "hdl" is NULL.

Oh, right. Will add.


Juergen