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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 1 Dec 2022, at 11:34, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> wrote:
> 
> 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).

Yes, I was looking for how to add an ABI check there, but other places like the 
featureset enum doesn't have it either.
The ABI check only seems to exist for the case where the values are used as bit 
flags.

> 
>> 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.


CAMLreturn has some macro magic to ensure it gets paired with the toplevel 
CAMLparam correctly (one of them opens a { scope and the other closes it, or 
something like that),
so I'd avoid putting it into the middle of other syntactic elements, it might 
just cause the build to fail (either now or in the future).

> 
> 
>> +        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?


Yes to the other suggestions.

> 
> 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.


perhaps we can look into doing that cleanup as a separate patch.

Best regards,
--Edwin