[XEN PATCH v2 0/2] Enumerate all allocated evtchns in lsevtchn

[XEN PATCH v2 0/2] Enumerate all allocated evtchns in lsevtchn

[Matthew Barnes]

Currently, lsevtchn aborts its event channel enumeration when it hits
its first hypercall error, namely:
* When an event channel doesn't exist at the specified port
* When the event channel is owned by Xen

This results in lsevtchn missing potential relevant event channels with
higher port numbers, since lsevtchn cannot determine the cause of
hypercall errors.

This patch adds error status indicators for the evtchn_status hypercall
for when no further event channels will be yielded for higher port
numbers, allowing lsevtchn to terminate when all event channels have
been enumerated over.

Matthew Barnes (2):
  evtchn: Add error status indicators for evtchn_status hypercall
  tools/lsevtchn: Use new status identifiers in loop

 tools/xcutils/lsevtchn.c           | 11 ++++++++++-
 xen/common/event_channel.c         | 12 +++++++++++-
 xen/include/public/event_channel.h |  2 ++
 3 files changed, 23 insertions(+), 2 deletions(-)

-- 
2.34.1

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

[Matthew Barnes]

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.

Because of this, lsevtchn is unable to determine whether to continue
event channel enumeration upon an evtchn_status hypercall error.

Add error status indicators for the eventchn_status hypercall for
lsevtchn to terminate its loop on, and keep other errors as failed
hypercalls.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 xen/common/event_channel.c         | 12 +++++++++++-
 xen/include/public/event_channel.h |  2 ++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index aceee0695f9f..0f11e71c3e6f 100644
--- 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;
+    }
+
+    if ( !port_is_valid(d, port) )
+    {
+        status->status = EVTCHNSTAT_port_invalid;
+        rcu_unlock_domain(d);
+        return 0;
+    }
 
     chn = _evtchn_from_port(d, port);
     if ( !chn )
diff --git a/xen/include/public/event_channel.h b/xen/include/public/event_channel.h
index 0d91a1c4afab..29cbf43945b3 100644
--- 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 {
-- 
2.34.1

[XEN PATCH v2 2/2] tools/lsevtchn: Use new status identifiers in loop

[Matthew Barnes]

lsevtchn terminates the loop when the hypercall returns an error, even
if there are still event channels with higher port numbers to be
enumerated over.

Continue the loop even on hypercall errors, and use the new status
identifiers for the evtchn_status hypercall, namely "port invalid" and
"domain invalid", to determine when to break the loop.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 tools/xcutils/lsevtchn.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/xcutils/lsevtchn.c b/tools/xcutils/lsevtchn.c
index d1710613ddc5..4a48620cd72a 100644
--- a/tools/xcutils/lsevtchn.c
+++ b/tools/xcutils/lsevtchn.c
@@ -24,11 +24,20 @@ int main(int argc, char **argv)
         status.port = port;
         rc = xc_evtchn_status(xch, &status);
         if ( rc < 0 )
-            break;
+            continue;
 
         if ( status.status == EVTCHNSTAT_closed )
             continue;
 
+        if ( status.status == EVTCHNSTAT_dom_invalid )
+        {
+            printf("Domain ID '%d' does not correspond to valid domain.\n", domid);
+            break;
+        }
+
+        if ( status.status == EVTCHNSTAT_port_invalid )
+            break;
+
         printf("%4d: VCPU %u: ", port, status.vcpu);
 
         switch ( status.status )
-- 
2.34.1

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

[Jan Beulich]

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