Re: [XEN PATCH v2 1/2] evtchn: Add error status indicators for evtchn_status hypercall

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2024 15:42, Matthew Barnes wrote:
> When the evtchn_status hypercall fails, it is not possible to determine
> the cause of the error, since the library call returns -1 to the tool
> and not the errno.

That's normal behavior for such library functions. If you want to know the
specific error number, you ought to look at the errno variable. Or are
you saying that errno isn't set correctly in this case (I can't spot such
an issue when looking at do_evtchn_op(), backing xc_evtchn_status())?

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1030,7 +1030,17 @@ int evtchn_status(evtchn_status_t *status)
>  
>      d = rcu_lock_domain_by_any_id(dom);
>      if ( d == NULL )
> -        return -ESRCH;
> +    {
> +        status->status = EVTCHNSTAT_dom_invalid;
> +        return 0;

This surely ought to remain -ESRCH. You may not break existing callers.

> +    }
> +
> +    if ( !port_is_valid(d, port) )
> +    {
> +        status->status = EVTCHNSTAT_port_invalid;
> +        rcu_unlock_domain(d);
> +        return 0;
> +    }

I can see that for the purpose of patch 2 this wants distinguishing from

>      chn = _evtchn_from_port(d, port);
>      if ( !chn )

... the -EINVAL returned here. Yet "success" doesn't look correct there
either. -ENOENT, -EBADF, -ENFILE, or -EDOM maybe?

> --- a/xen/include/public/event_channel.h
> +++ b/xen/include/public/event_channel.h
> @@ -200,6 +200,8 @@ struct evtchn_status {
>  #define EVTCHNSTAT_pirq         3  /* Channel is bound to a phys IRQ line.   
> */
>  #define EVTCHNSTAT_virq         4  /* Channel is bound to a virtual IRQ line 
> */
>  #define EVTCHNSTAT_ipi          5  /* Channel is bound to a virtual IPI line 
> */
> +#define EVTCHNSTAT_dom_invalid  6  /* Given domain ID is not a valid domain  
> */
> +#define EVTCHNSTAT_port_invalid 7  /* Given port is not within valid range   
> */
>      uint32_t status;
>      uint32_t vcpu;                 /* VCPU to which this channel is bound.   
> */
>      union {

If such indicators are to be added, I'm pretty sure they want to be 
discontiguous
from the presently used range. Sadly, with status having unsigned type, using
negative values wouldn't feel quite right.

Jan