Re: [PATCH v1 2/5] tools/ocaml/libs/xc: add binding to xc_evtchn_status

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 30/11/2022 17:32, Edwin Török wrote:
> There is no API or ioctl to query event channel status, it is only
> present in xenctrl.h

Yeah, this is very unfortunate, because it really wanted to be part of
the xenevtchn stable API/ABI.

> The C union is mapped to an OCaml variant exposing just the value from the
> correct union tag.
>
> Querying event channel status is useful when analyzing Windows VMs that
> may have reset and changed the xenstore event channel port number from
> what it initially got booted with.

This paragraph is why we need it now, but it's not really relevant for
the upstream commit.  I'd drop this sentence, and simply how the lower
one noting the similarity to lsevtchn.

> The information provided here is similar to 'lstevtchn', but rather than

"lsevtchn".

> parsing its output it queries the underlying API directly.
>
> Signed-off-by: Edwin Török <edvin.torok@xxxxxxxxxx>
> ---
>  tools/ocaml/libs/xc/xenctrl.ml      | 14 +++++++
>  tools/ocaml/libs/xc/xenctrl.mli     | 15 +++++++
>  tools/ocaml/libs/xc/xenctrl_stubs.c | 65 +++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+)
>
> diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
> index 2ed7454b16..c21e391f98 100644
> --- a/tools/ocaml/libs/xc/xenctrl.ml
> +++ b/tools/ocaml/libs/xc/xenctrl.ml
> @@ -267,6 +267,20 @@ external evtchn_alloc_unbound: handle -> domid -> domid 
> -> int
>    = "stub_xc_evtchn_alloc_unbound"
>  external evtchn_reset: handle -> domid -> unit = "stub_xc_evtchn_reset"
>  
> +type evtchn_interdomain = { dom: domid; port: int}

Strictly speaking, port needs to be int32.

ABI-wise, it can be configured as large as 2^32-2 during domain creation.

However, FIFO currently tops out at 2^17 and has a theoretical maximum
at 2^28, so perhaps int ought to enough for now.

> +
> +type evtchn_stat =
> +  | EVTCHNSTAT_unbound of domid
> +  | EVTCHNSTAT_interdomain of evtchn_interdomain
> +  | EVTCHNSTAT_pirq of int
> +  | EVTCHNSTAT_virq of int

Similar comment.  A vcpu id should in principle be int32

> +  | EVTCHNSTAT_ipi

Normally when having an enumeration like this, we want to hook up the
build-time ABI check.

But in this case, it's produced by the bindings (not consumed by them),
and there's an exception raised in the default case, so I don't think we
need the build-time ABI check for any kind of safety (and therefore
shouldn't go to the reasonably-invasive effort of adding the check).

> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index d30585f21c..67f3648391 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -641,6 +641,71 @@ CAMLprim value stub_xc_evtchn_reset(value xch, value 
> domid)
>      CAMLreturn(Val_unit);
>  }
>  
> +CAMLprim value stub_xc_evtchn_status(value xch, value domid, value port)
> +{
> +    CAMLparam3(xch, domid, port);
> +    CAMLlocal4(result, result_status, stat, interdomain);
> +    xc_evtchn_status_t status;
> +    int rc;
> +
> +    memset(&status, 0, sizeof(status));
> +    status.dom = _D(domid);
> +    status.port = Int_val(port);

xc_evtchn_status_t status = {
    .dom = _D(domid),
    .port = Int_val(port),
};

is the marginally preferred way of doing this.  It removes potential
issues with typo-ing the memset().

> +
> +    caml_enter_blocking_section();
> +    rc = xc_evtchn_status(_H(xch), &status);
> +    caml_leave_blocking_section();
> +
> +    if ( rc < 0 )
> +        failwith_xc(_H(xch));
> +
> +    if ( status.status == EVTCHNSTAT_closed )
> +        result = Val_none;
> +    else
> +    {

This is actually one example where using a second CAMLreturn would
simply things substantially.

switch ( status.status )
{
case EVTCHNSTAT_closed:
    CAMLreturn(Val_none);

case EVTCHNSTAT_unbound:
    ...

Would remove the need for the outer if/else.


> +        switch ( status.status )
> +        {
> +        case EVTCHNSTAT_unbound:
> +            stat = caml_alloc(1, 0); /* 1st non-constant constructor */
> +            Store_field(stat, 0, Val_int(status.u.unbound.dom));
> +            break;
> +
> +        case EVTCHNSTAT_interdomain:
> +            interdomain = caml_alloc_tuple(2);
> +            Store_field(interdomain, 0, Val_int(status.u.interdomain.dom));
> +            Store_field(interdomain, 1, Val_int(status.u.interdomain.port));
> +            stat = caml_alloc(1, 1); /*  2nd non-constant constructor */
> +            Store_field(stat, 0, interdomain);
> +            break;
> +        case EVTCHNSTAT_pirq:
> +            stat = caml_alloc(1, 2); /* 3rd non-constant constructor */
> +            Store_field(stat, 0, Val_int(status.u.pirq));
> +            break;
> +
> +        case EVTCHNSTAT_virq:
> +            stat = caml_alloc(1, 3); /* 4th non-constant constructor */
> +            Store_field(stat, 0, Val_int(status.u.virq));
> +            break;
> +
> +        case EVTCHNSTAT_ipi:
> +            stat = Val_int(0); /* 1st constant constructor */
> +            break;
> +
> +        default:
> +            caml_failwith("Unkown evtchn status");
> +        }

We'd normally have a blank line here.

> +        result_status = caml_alloc_tuple(2);
> +        Store_field(result_status, 0, Val_int(status.vcpu));
> +        Store_field(result_status, 1, stat);
> +
> +        /* Tag_some and caml_alloc_some are missing in older versions of 
> OCaml
> +         */

Can we do the usual

#ifndef Tag_some
# define Tag_some ...
#endif

at the top, and use it unconditionally here?

caml_alloc_some() is perhaps less interesting as it only appeared in
Ocaml 4.12 AFAICT, but we could also have some ifdefary for that at the
top of the file.

I don't know whether we have opencoded options elsewhere in the
bindings, but it certainly would be reduce the amount opencoding that
exists for standard patterns.

~Andrew