[PATCH v1 0/5] OCaml bindings for hvm_param_get and xc_evtchn_status

[PATCH v1 0/5] OCaml bindings for hvm_param_get and xc_evtchn_status

[Edwin Török]

Add bindings to xc_evtchn_status and hvm_param_get, useful for xenopsd
and for recovery from failed live updates.

.editorconfig helps me format the source code with the desired Xen
coding style (now that the reindent patch has switched it to spaces as
desired by the Xen project).
If you don't have an editor set up to use editorconfig this is a no-op.

.clang-format is an experiment for the OCaml subtree in slowly moving
its code to be closer to the Xen coding style. There is no Xen coding
style as such in clang-format, this takes GNU as a base and tweaks it to
be as close to CODING_STYLE as possible (there is just one different in
handling of do/while as far as I can tell).
It should be an improvement over the current situation where the OCaml C
bindings do not follow Xen coding style, and further bindings added that
follow the style of the code around them would not follow it either.
It doesn't yet reformat anything with it, just allows someone that
submits patches to use it if desired (e.g. on new code).

Edwin Török (5):
  CODING-STYLE: add .editorconfig to clarify indentation uses spaces
  tools/ocaml/libs/xc: add binding to xc_evtchn_status
  tools/ocaml/libs/xc: add hvm_param_get binding
  tools/ocaml/libs/xb: add missing stdint.h
  CODING_STYLE: add .clang-format

 .editorconfig                       | 20 +++++++
 tools/ocaml/.clang-format           |  9 ++++
 tools/ocaml/libs/xb/xenbus_stubs.c  |  1 +
 tools/ocaml/libs/xc/xenctrl.ml      | 58 +++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl.mli     | 60 +++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl_stubs.c | 81 +++++++++++++++++++++++++++++
 6 files changed, 229 insertions(+)
 create mode 100644 .editorconfig
 create mode 100644 tools/ocaml/.clang-format

-- 
2.34.1

[PATCH v1 1/5] CODING-STYLE: add .editorconfig to clarify indentation uses spaces

[Edwin Török]

Add an .editorconfig to make it easier to keep patches compatible with
Xen's coding style, and to reemphasize what Xen's coding style is.

I thought that Xen demands tabs rather than spaces (which is more
difficult with OCaml because indentation tools use spaces,
and the use of tabs requires changing editor settings),
however CODING-STYLE says it is spaces.

Document this explicitly by adding a .editorconfig file (see editorconfig.org),
which is an editor agnostic format for specifying basic style properties like
indentation, either with native support in editors or via plugins.

It is safer than modelines because it only supports controlling a
restricted set of editor properties and not arbitrary commands as Vim
modelines would have, and works with editors other than Vim too.
(Vim has a deny list for modeline sandboxing, which is error-prone
because every time a new command gets added it needs to be added to the
deny list, which has been the source of a few CVEs in the past
and I disable Vim modelines everywhere as a precaution).

This file is added as a convenience for those who might have an editor
that supports it, and its presence should have no impact on those that
do not (want to) use it.
It also won't cause re-indentation of existing files when edited, only
newly added lines would follow the convention.

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 .editorconfig | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 .editorconfig

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index 0000000000..cb2f27c581
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,20 @@
+# See ./CODING_STYLE
+root = true
+
+[*]
+end_of_line = lf
+indent_style = space
+charset = utf-8
+max_line_length = 79
+trim_trailing_whitespace = true
+insert_final_newline = true
+
+# Makefiles must use tabs, otherwise they don't work
+[{Makefile,*.mk,Makefile.rules}]
+indent_style = tabs
+
+[*.{c,h}]
+indent_size = 4
+
+[*.{ml,mli}]
+indent_size = 2
-- 
2.34.1

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

[Edwin Török]

There is no API or ioctl to query event channel status, it is only
present in xenctrl.h

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.
The information provided here is similar to 'lstevtchn', but rather than
parsing its output it queries the underlying API directly.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 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}
+
+type evtchn_stat =
+  | EVTCHNSTAT_unbound of domid
+  | EVTCHNSTAT_interdomain of evtchn_interdomain
+  | EVTCHNSTAT_pirq of int
+  | EVTCHNSTAT_virq of int
+  | EVTCHNSTAT_ipi
+
+type evtchn_status = { vcpu: int; status: evtchn_stat }
+
+external evtchn_status: handle -> domid -> int -> evtchn_status option =
+  "stub_xc_evtchn_status"
+
 external readconsolering: handle -> string = "stub_xc_readconsolering"
 
 external send_debug_keys: handle -> string -> unit = "stub_xc_send_debug_keys"
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 0f80aafea0..60e7902e66 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -206,6 +206,21 @@ external shadow_allocation_get : handle -> domid -> int
 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}
+
+type evtchn_stat =
+  | EVTCHNSTAT_unbound of domid
+  | EVTCHNSTAT_interdomain of evtchn_interdomain
+  | EVTCHNSTAT_pirq of int
+  | EVTCHNSTAT_virq of int
+  | EVTCHNSTAT_ipi
+
+type evtchn_status = { vcpu: int; status: evtchn_stat }
+
+external evtchn_status: handle -> domid -> int -> evtchn_status option =
+  "stub_xc_evtchn_status"
+
 external readconsolering : handle -> string = "stub_xc_readconsolering"
 external send_debug_keys : handle -> string -> unit = "stub_xc_send_debug_keys"
 external physinfo : handle -> physinfo = "stub_xc_physinfo"
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);
+
+    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
+    {
+        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");
+        }
+        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
+         */
+        result = caml_alloc_small(1, 0);
+        Store_field(result, 0, result_status);
+    }
+
+    CAMLreturn(result);
+}
 
 CAMLprim value stub_xc_readconsolering(value xch)
 {
-- 
2.34.1

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

[Andrew Cooper]

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@citrix.com>
> ---
>  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

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

[Edwin Torok]

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

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

[Andrew Cooper]

On 01/12/2022 13:35, Edwin Torok wrote:
>> On 1 Dec 2022, at 11:34, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>
>> On 30/11/2022 17:32, Edwin Török wrote:
>>> +
>>> +    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).

From the manual:

"The macros CAMLreturn, CAMLreturn0, and CAMLreturnT are used to replace
the C keyword return. Every occurrence of return x must be replaced by ..."

It is common in C to have multiple returns, and the manual does say
"Every occurrence" without stating any requirement for there to be a
single occurrence.

CAMLreturn can't syntactically work splitting a scope with CAMLparam,
because then this wouldn't compile:

CAMLprim value stub_xc_evtchn_status(value foo)
{
    CAMLparam1(foo);
    int bar = 0;

retry:
    if ( bar )
        CAMLreturn(foo);

    bar++
    goto retry;
}

CAMLreturn does use a normal "do { ... } while (0)" construct simply for
being a macro, and forces paring with CAMLparamX by referencing the
caml__frame local variable by name.


So I really do think that multiple CAMLreturns are fine and cannot
reasonably be broken in the future.

But if we do really still want to keep a single return, then a "goto
done" would be acceptable here and still better than the if/else.

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

Probably best, yes.

~Andrew

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

[Edwin Torok]

> On 1 Dec 2022, at 13:59, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 01/12/2022 13:35, Edwin Torok wrote:
>>> On 1 Dec 2022, at 11:34, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
>>> 
>>> On 30/11/2022 17:32, Edwin Török wrote:
>>>> +
>>>> +    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).
> 
> From the manual:
> 
> "The macros CAMLreturn, CAMLreturn0, and CAMLreturnT are used to replace
> the C keyword return. Every occurrence of return x must be replaced by ..."
> 
> It is common in C to have multiple returns, and the manual does say
> "Every occurrence" without stating any requirement for there to be a
> single occurrence.
> 
> CAMLreturn can't syntactically work splitting a scope with CAMLparam,
> because then this wouldn't compile:
> 
> CAMLprim value stub_xc_evtchn_status(value foo)
> {
>     CAMLparam1(foo);
>     int bar = 0;
> 
> retry:
>     if ( bar )
>         CAMLreturn(foo);
> 
>     bar++
>     goto retry;
> }
> 


I wouldn't expect it to, or at least I've never seen a C binding written that way (with CAMLreturn not as last statement),
but indeed nothing in the manual states that it wouldn't work.

> CAMLreturn does use a normal "do { ... } while (0)" construct simply for
> being a macro, and forces paring with CAMLparamX by referencing the
> caml__frame local variable by name.
> 
> 
> So I really do think that multiple CAMLreturns are fine and cannot
> reasonably be broken in the future.
> 
> But if we do really still want to keep a single return, then a "goto
> done" would be acceptable here and still better than the if/else.

I almost always used to use do/while(0) and break even in C as a replacement for 'goto',
especially if there are multiple nested resources that need cleaning up, do/while ensures you
unwind them in the correct order and don't accidentally skip one.
I think most code that is written using 'goto' can be rewritten not to use it, and might avoid some bugs in the process
(e.g. using goto might leave some local variables uninitialised).
I'm reluctant to introduce a goto just to decrease nesting level.

There might be another way to rewrite the code:
```
switch(status.status)
{
case EVTCHNSTAT_closed:
 stat = Val_none;
 break;
... other code that assigns to stat something other than None ...
}

if (Val_none == stat) {
   result = Val_none;
} else {
   .. code as it was before to construct a some ...
}

CAMLreturn(result);
```

This would then follow the logical order of how the values are constructed, and avoids the deep nesting of the switch.
(reading the code backwards from exit will show you how each piece is nested/constructed without jumps that alter control flow)

Val_none == is used instead of == Val_none to catch a typo where stat = Val_none would change stat, whereas Val_none = stat would be a compile error.

What do you think?

(might be slightly less efficient than the original version, but a reasonable C compiler should produce almost equal optimized code for both).

> 
>>> 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.
> 
> Probably best, yes.
> 
> ~Andrew

[PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Edwin Török]

Not to be confused which hvm_get_param, which also exists and has a
different, more error-prone interface.

This one always returns a 64-bit value, and that is retained in the
OCaml binding as well, returning 'int64' (and not int, or nativeint
which might have a different size).

The integer here is unsigned in the C API, however OCaml only has signed integers.

No bits are lost, it is just a matter of interpretation when printing
and for certain arithmetic operations, however in the cases where the
MSB is set it is very likely that the value is an address and no
arithmetic should be performed on the OCaml side on it.
(this is not a new problem with this binding, but worth mentioning given
the difference in types)

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/libs/xc/xenctrl.ml      | 44 ++++++++++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl.mli     | 45 +++++++++++++++++++++++++++++
 tools/ocaml/libs/xc/xenctrl_stubs.c | 16 ++++++++++
 3 files changed, 105 insertions(+)

diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index c21e391f98..1f8d927b0c 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -298,6 +298,50 @@ external map_foreign_range: handle -> domid -> int
   -> nativeint -> Xenmmap.mmap_interface
   = "stub_map_foreign_range"
 
+type hvm_param =
+  | HVM_PARAM_CALLBACK_IRQ
+  | HVM_PARAM_STORE_PFN
+  | HVM_PARAM_STORE_EVTCHN
+  | HVM_PARAM_UNDEFINED_3
+  | HVM_PARAM_PAE_ENABLED
+  | HVM_PARAM_IOREQ_PFN
+  | HVM_PARAM_BUFIOREQ_PFN
+  | HVM_PARAM_UNDEFINED_7
+  | HVM_PARAM_UNDEFINED_8
+  | HVM_PARAM_VIRIDIAN
+  | HVM_PARAM_TIMER_MODE0
+  | HVM_PARAM_HPET_ENABLED1
+  | HVM_PARAM_IDENT_PT2
+  | HVM_PARAM_UNDEFINED_13
+  | HVM_PARAM_ACPI_S_STATE4
+  | HVM_PARAM_VM86_TSS5
+  | HVM_PARAM_VPT_ALIGN6
+  | HVM_PARAM_CONSOLE_PFN7
+  | HVM_PARAM_CONSOLE_EVTCHN8
+  | HVM_PARAM_ACPI_IOPORTS_LOCATION9
+  | HVM_PARAM_MEMORY_EVENT_CR00
+  | HVM_PARAM_MEMORY_EVENT_CR31
+  | HVM_PARAM_MEMORY_EVENT_CR42
+  | HVM_PARAM_MEMORY_EVENT_INT33
+  | HVM_PARAM_NESTEDHVM4
+  | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5
+  | HVM_PARAM_UNDEFINED_26
+  | HVM_PARAM_PAGING_RING_PFN7
+  | HVM_PARAM_MONITOR_RING_PFN8
+  | HVM_PARAM_SHARING_RING_PFN9
+  | HVM_PARAM_MEMORY_EVENT_MSR0
+  | HVM_PARAM_TRIPLE_FAULT_REASON1
+  | HVM_PARAM_IOREQ_SERVER_PFN2
+  | HVM_PARAM_NR_IOREQ_SERVER_PAGES3
+  | HVM_PARAM_VM_GENERATION_ID_ADDR4
+  | HVM_PARAM_ALTP2M5
+  | HVM_PARAM_X87_FIP_WIDTH6
+  | HVM_PARAM_VM86_TSS_SIZED7
+  | HVM_PARAM_MCA_CAP8
+
+external hvm_param_get: handle -> domid -> hvm_param -> int64
+  = "stub_xc_hvm_param_get"
+
 external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit
   = "stub_xc_domain_assign_device"
 external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 60e7902e66..f6c7e5b553 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -236,6 +236,51 @@ external map_foreign_range :
   handle -> domid -> int -> nativeint -> Xenmmap.mmap_interface
   = "stub_map_foreign_range"
 
+(* needs to be sorted according to its numeric value, watch out for gaps! *)
+type hvm_param =
+  | HVM_PARAM_CALLBACK_IRQ
+  | HVM_PARAM_STORE_PFN
+  | HVM_PARAM_STORE_EVTCHN
+  | HVM_PARAM_UNDEFINED_3
+  | HVM_PARAM_PAE_ENABLED
+  | HVM_PARAM_IOREQ_PFN
+  | HVM_PARAM_BUFIOREQ_PFN
+  | HVM_PARAM_UNDEFINED_7
+  | HVM_PARAM_UNDEFINED_8
+  | HVM_PARAM_VIRIDIAN
+  | HVM_PARAM_TIMER_MODE0
+  | HVM_PARAM_HPET_ENABLED1
+  | HVM_PARAM_IDENT_PT2
+  | HVM_PARAM_UNDEFINED_13
+  | HVM_PARAM_ACPI_S_STATE4
+  | HVM_PARAM_VM86_TSS5
+  | HVM_PARAM_VPT_ALIGN6
+  | HVM_PARAM_CONSOLE_PFN7
+  | HVM_PARAM_CONSOLE_EVTCHN8
+  | HVM_PARAM_ACPI_IOPORTS_LOCATION9
+  | HVM_PARAM_MEMORY_EVENT_CR00
+  | HVM_PARAM_MEMORY_EVENT_CR31
+  | HVM_PARAM_MEMORY_EVENT_CR42
+  | HVM_PARAM_MEMORY_EVENT_INT33
+  | HVM_PARAM_NESTEDHVM4
+  | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5
+  | HVM_PARAM_UNDEFINED_26
+  | HVM_PARAM_PAGING_RING_PFN7
+  | HVM_PARAM_MONITOR_RING_PFN8
+  | HVM_PARAM_SHARING_RING_PFN9
+  | HVM_PARAM_MEMORY_EVENT_MSR0
+  | HVM_PARAM_TRIPLE_FAULT_REASON1
+  | HVM_PARAM_IOREQ_SERVER_PFN2
+  | HVM_PARAM_NR_IOREQ_SERVER_PAGES3
+  | HVM_PARAM_VM_GENERATION_ID_ADDR4
+  | HVM_PARAM_ALTP2M5
+  | HVM_PARAM_X87_FIP_WIDTH6
+  | HVM_PARAM_VM86_TSS_SIZED7
+  | HVM_PARAM_MCA_CAP8
+
+external hvm_param_get: handle -> domid -> hvm_param -> int64
+  = "stub_xc_hvm_param_get"
+
 external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit
   = "stub_xc_domain_assign_device"
 external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 67f3648391..b2df93d4f8 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -1176,6 +1176,22 @@ CAMLprim value stub_xc_domain_irq_permission(value xch, value domid,
     CAMLreturn(Val_unit);
 }
 
+CAMLprim value stub_xc_hvm_param_get(value xch, value domid, value param)
+{
+    CAMLparam3(xch, domid, param);
+    uint64_t result;
+    int ret;
+
+    caml_enter_blocking_section();
+    ret = xc_hvm_param_get(_H(xch), _D(domid), Int_val(param), &result);
+    caml_leave_blocking_section();
+
+    if ( ret )
+        failwith_xc(_H(xch));
+
+    CAMLreturn(caml_copy_int64(result));
+}
+
 static uint32_t encode_sbdf(int domain, int bus, int dev, int func)
 {
     return  ((uint32_t)domain & 0xffff) << 16 |
-- 
2.34.1

Re: [PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Andrew Cooper]

On 30/11/2022 17:32, Edwin Török wrote:
> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
> index 60e7902e66..f6c7e5b553 100644
> --- a/tools/ocaml/libs/xc/xenctrl.mli
> +++ b/tools/ocaml/libs/xc/xenctrl.mli
> @@ -236,6 +236,51 @@ external map_foreign_range :
>    handle -> domid -> int -> nativeint -> Xenmmap.mmap_interface
>    = "stub_map_foreign_range"
>  
> +(* needs to be sorted according to its numeric value, watch out for gaps! *)
> +type hvm_param =
> +  | HVM_PARAM_CALLBACK_IRQ
> +  | HVM_PARAM_STORE_PFN
> +  | HVM_PARAM_STORE_EVTCHN
> +  | HVM_PARAM_UNDEFINED_3

Can we perhaps use

| _HVM_PARAM_UNDEF_3

with a leading underscore to highlight that its just a placeholder for a
hole ?

> +  | HVM_PARAM_PAE_ENABLED
> +  | HVM_PARAM_IOREQ_PFN
> +  | HVM_PARAM_BUFIOREQ_PFN
> +  | HVM_PARAM_UNDEFINED_7
> +  | HVM_PARAM_UNDEFINED_8
> +  | HVM_PARAM_VIRIDIAN
> +  | HVM_PARAM_TIMER_MODE0

From TIMER_MODE onwards, you appear to have a trailing digit on each
constant name.  It appears to be the final digit of the params numeric
value.

> +  | HVM_PARAM_HPET_ENABLED1
> +  | HVM_PARAM_IDENT_PT2
> +  | HVM_PARAM_UNDEFINED_13
> +  | HVM_PARAM_ACPI_S_STATE4
> +  | HVM_PARAM_VM86_TSS5
> +  | HVM_PARAM_VPT_ALIGN6
> +  | HVM_PARAM_CONSOLE_PFN7
> +  | HVM_PARAM_CONSOLE_EVTCHN8
> +  | HVM_PARAM_ACPI_IOPORTS_LOCATION9
> +  | HVM_PARAM_MEMORY_EVENT_CR00
> +  | HVM_PARAM_MEMORY_EVENT_CR31
> +  | HVM_PARAM_MEMORY_EVENT_CR42
> +  | HVM_PARAM_MEMORY_EVENT_INT33
> +  | HVM_PARAM_NESTEDHVM4
> +  | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5
> +  | HVM_PARAM_UNDEFINED_26
> +  | HVM_PARAM_PAGING_RING_PFN7
> +  | HVM_PARAM_MONITOR_RING_PFN8
> +  | HVM_PARAM_SHARING_RING_PFN9
> +  | HVM_PARAM_MEMORY_EVENT_MSR0
> +  | HVM_PARAM_TRIPLE_FAULT_REASON1
> +  | HVM_PARAM_IOREQ_SERVER_PFN2
> +  | HVM_PARAM_NR_IOREQ_SERVER_PAGES3
> +  | HVM_PARAM_VM_GENERATION_ID_ADDR4
> +  | HVM_PARAM_ALTP2M5
> +  | HVM_PARAM_X87_FIP_WIDTH6
> +  | HVM_PARAM_VM86_TSS_SIZED7
> +  | HVM_PARAM_MCA_CAP8

Similarly with EVENTSTAT_*, we ought to engage the build time ABI check.

But there isn't a suitable end delimiter, and these are only ever an
input into a binding (never a return), so it's not the end of the world
if new constants get missed.  (Not that new constants are likely. 
HVM_PARAMs are a gross bodge which I'm trying to phase out.)

> +
> +external hvm_param_get: handle -> domid -> hvm_param -> int64
> +  = "stub_xc_hvm_param_get"

IMO we should bind set at the same time.  It's trivial to do, and far
easier to do now than at some point in the future when we first need it.

> +
>  external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit
>    = "stub_xc_domain_assign_device"
>  external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 67f3648391..b2df93d4f8 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -1176,6 +1176,22 @@ CAMLprim value stub_xc_domain_irq_permission(value xch, value domid,
>      CAMLreturn(Val_unit);
>  }
>  
> +CAMLprim value stub_xc_hvm_param_get(value xch, value domid, value param)
> +{
> +    CAMLparam3(xch, domid, param);
> +    uint64_t result;

result is commonly a value in these bindings.  'val' would be the more
common name to use here.

~Andrew

Re: [PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Edwin Torok]

> On 1 Dec 2022, at 11:51, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> On 30/11/2022 17:32, Edwin Török wrote:
>> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
>> index 60e7902e66..f6c7e5b553 100644
>> --- a/tools/ocaml/libs/xc/xenctrl.mli
>> +++ b/tools/ocaml/libs/xc/xenctrl.mli
>> @@ -236,6 +236,51 @@ external map_foreign_range :
>>   handle -> domid -> int -> nativeint -> Xenmmap.mmap_interface
>>   = "stub_map_foreign_range"
>> 
>> +(* needs to be sorted according to its numeric value, watch out for gaps! *)
>> +type hvm_param =
>> +  | HVM_PARAM_CALLBACK_IRQ
>> +  | HVM_PARAM_STORE_PFN
>> +  | HVM_PARAM_STORE_EVTCHN
>> +  | HVM_PARAM_UNDEFINED_3
> 
> Can we perhaps use
> 
> | _HVM_PARAM_UNDEF_3
> 
> with a leading underscore to highlight that its just a placeholder for a
> hole ?

I tried this, but I get a compile error if I attempt to start a variant name with and underscore.

> 
>> +  | HVM_PARAM_PAE_ENABLED
>> +  | HVM_PARAM_IOREQ_PFN
>> +  | HVM_PARAM_BUFIOREQ_PFN
>> +  | HVM_PARAM_UNDEFINED_7
>> +  | HVM_PARAM_UNDEFINED_8
>> +  | HVM_PARAM_VIRIDIAN
>> +  | HVM_PARAM_TIMER_MODE0
> 
> From TIMER_MODE onwards, you appear to have a trailing digit on each
> constant name.  It appears to be the final digit of the params numeric
> value.
> 

I think I see how that happened (I had the numbers side by side to check that I filled in all the wholes, and then used the wrong regex to remove them,
which worked on single digit numbers, but not double).
I'm fixing this up in my tree now.

>> +  | HVM_PARAM_HPET_ENABLED1
>> +  | HVM_PARAM_IDENT_PT2
>> +  | HVM_PARAM_UNDEFINED_13
>> +  | HVM_PARAM_ACPI_S_STATE4
>> +  | HVM_PARAM_VM86_TSS5
>> +  | HVM_PARAM_VPT_ALIGN6
>> +  | HVM_PARAM_CONSOLE_PFN7
>> +  | HVM_PARAM_CONSOLE_EVTCHN8
>> +  | HVM_PARAM_ACPI_IOPORTS_LOCATION9
>> +  | HVM_PARAM_MEMORY_EVENT_CR00
>> +  | HVM_PARAM_MEMORY_EVENT_CR31
>> +  | HVM_PARAM_MEMORY_EVENT_CR42
>> +  | HVM_PARAM_MEMORY_EVENT_INT33
>> +  | HVM_PARAM_NESTEDHVM4
>> +  | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5
>> +  | HVM_PARAM_UNDEFINED_26
>> +  | HVM_PARAM_PAGING_RING_PFN7
>> +  | HVM_PARAM_MONITOR_RING_PFN8
>> +  | HVM_PARAM_SHARING_RING_PFN9
>> +  | HVM_PARAM_MEMORY_EVENT_MSR0
>> +  | HVM_PARAM_TRIPLE_FAULT_REASON1
>> +  | HVM_PARAM_IOREQ_SERVER_PFN2
>> +  | HVM_PARAM_NR_IOREQ_SERVER_PAGES3
>> +  | HVM_PARAM_VM_GENERATION_ID_ADDR4
>> +  | HVM_PARAM_ALTP2M5
>> +  | HVM_PARAM_X87_FIP_WIDTH6
>> +  | HVM_PARAM_VM86_TSS_SIZED7
>> +  | HVM_PARAM_MCA_CAP8
> 
> Similarly with EVENTSTAT_*, we ought to engage the build time ABI check.


It looks like we'd need to write a new ABI check, but I'm not familiar with the ABI checker script here,
and relying on a script to parse the OCaml source code and check the ABI seems brittle (but no less brittle than not having a check at all).

Should we instead switch to using ctypes to generate these constants? It can run at build time and produce a .ml file based on a build time test
by including the actual C header and getting the correct constant value. But it'd make cross-compilation (if Xen supports that?) more difficult.
Or we could run it ourselves by hand, and commit the result so that end-users do not need to have or run ctypes, just developers who change these bindings
(similar to how it is usual to commit the output from autoconf and automake into git to not require end-users to rerun these).

However a move to ctypes would require quite a lot of build time changes that I'd rather start only once we switched to Dune, it is not worthwhile doing in the current Makefile based
build system.

> 
> But there isn't a suitable end delimiter, and these are only ever an
> input into a binding (never a return), so it's not the end of the world
> if new constants get missed.  (Not that new constants are likely. 
> HVM_PARAMs are a gross bodge which I'm trying to phase out.)
> 
>> +
>> +external hvm_param_get: handle -> domid -> hvm_param -> int64
>> +  = "stub_xc_hvm_param_get"
> 
> IMO we should bind set at the same time.  It's trivial to do, and far
> easier to do now than at some point in the future when we first need it.
> 
>> +
>> external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit
>>   = "stub_xc_domain_assign_device"
>> external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit
>> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> index 67f3648391..b2df93d4f8 100644
>> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
>> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
>> @@ -1176,6 +1176,22 @@ CAMLprim value stub_xc_domain_irq_permission(value xch, value domid,
>>     CAMLreturn(Val_unit);
>> }
>> 
>> +CAMLprim value stub_xc_hvm_param_get(value xch, value domid, value param)
>> +{
>> +    CAMLparam3(xch, domid, param);
>> +    uint64_t result;
> 
> result is commonly a value in these bindings.  'val' would be the more
> common name to use here.
> 
> ~Andrew

Re: [PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Christian Lindig]

> On 1 Dec 2022, at 13:50, Edwin Torok <edvin.torok@citrix.com> wrote:
> 
> Should we instead switch to using ctypes to generate these constants?

I would not advocate this. Ctypes is the kind of meta programming that is great when it works but hell if it does not and it adds more dependencies. 

I just had a discussion with Andrew about other tricks how to bring C constants to the ML side in order to decouple them. I’m using it in my Polly library - it might not be the solution for Xen but worth knowing.

https://github.com/lindig/polly/blob/master/lib/polly_stubs.c#L23-L39

— C

Re: [PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Edwin Torok]

> On 1 Dec 2022, at 13:57, Christian Lindig <christian.lindig@citrix.com> wrote:
> 
> 
> 
>> On 1 Dec 2022, at 13:50, Edwin Torok <edvin.torok@citrix.com> wrote:
>> 
>> Should we instead switch to using ctypes to generate these constants?
> 
> I would not advocate this. Ctypes is the kind of meta programming that is great when it works but hell if it does not and it adds more dependencies. 

Perhaps use it to just generate the constant mappings?
Here is an example of how I used it elsewhere:
https://github.com/xapi-project/ocaml-dlm/blob/master/lib_gen/types/bindings_structs.ml#L30-L55

> 
> I just had a discussion with Andrew about other tricks how to bring C constants to the ML side in order to decouple them. I’m using it in my Polly library - it might not be the solution for Xen but worth knowing.
> 
> https://github.com/lindig/polly/blob/master/lib/polly_stubs.c#L23-L39


The disadvantage is that we can't pattern match on it anymore.

Although we could have some OCaml code that does the pattern matching on another type and maps it to these private integer types.
However at that point we've manually reinvented what ctypes would already do, and we have an additional mapping step (which may not matter from a performance point of view but would be nice if we could avoid it).

I'll have to do some experiments to see how the code generated by ctypes looks like in this case (actually the 'cstubs' part of it), and how different it would be from manually writing it
(i.e. can we still reasonably review the generated code, and would it look like something that we'd write ourselves?)

Best regards,
--Edwin

Re: [PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Christian Lindig]

> On 1 Dec 2022, at 14:16, Edwin Torok <edvin.torok@citrix.com> wrote:
> 
> The disadvantage is that we can't pattern match on it anymore.
> 
> Although we could have some OCaml code that does the pattern matching on another type and maps it to these private integer types.
> However at that point we've manually reinvented what ctypes would already do, and we have an additional mapping step (which may not matter from a performance point of view but would be nice if we could avoid it).

I agree that this is a severe disadvantage. My method is only useful if they are mostly passed around but not when they have an algebra defined over them where you want to combine and match them.

— C

Re: [PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Edwin Torok]

> On 1 Dec 2022, at 14:22, Christian Lindig <christian.lindig@citrix.com> wrote:
> 
> 
> 
>> On 1 Dec 2022, at 14:16, Edwin Torok <edvin.torok@citrix.com> wrote:
>> 
>> The disadvantage is that we can't pattern match on it anymore.
>> 
>> Although we could have some OCaml code that does the pattern matching on another type and maps it to these private integer types.
>> However at that point we've manually reinvented what ctypes would already do, and we have an additional mapping step (which may not matter from a performance point of view but would be nice if we could avoid it).
> 
> I agree that this is a severe disadvantage. My method is only useful if they are mostly passed around but not when they have an algebra defined over them where you want to combine and match them.


It might be possible to use your method to implement a pure-OCaml ABI checker though.

Consider:
```
external constant_A : unit -> int = "caml_constant_A"
external constant_B : unit -> int = "caml_constant_B"
external constant_C : unit -> int = "caml_constant_C"

let check_match = List.iter @@ fun (ocaml_variant, c_constant) ->
   let r = Obj.repr ocaml_variant in
   assert (Obj.is_int r);
   assert ((Obj.magic r : int) = c_constant ())

type t = A | B | C
let () =
   [A, constant_A
   ;B, constant_B
   ;C, constant_C]
   |> check_match
```

(although perhaps with the 2nd assertion replaced by something that includes the constant in the exception raised to aid debugging)

This'd only detect the ABI mismatch at runtime (although it would detect it right on startup), so it'd need to be accompanied by a small unit test
that would link just this module to make any mismatches a build time failure.

Then there would be no runtime overhead when using these variants in function calls, and we'd know they match 1:1.
Although there is still a possibility of mismatching on names (the bug I originally had in my patch, which although wouldn't cause any runtime issues,
would be good to catch at build time too).

I'll keep thinking about this to see whether there is another easy approach we can use that doesn't require using Cstubs and doesn't rely on manually keeping
code in different files in sync.

Best regards,
--Edwin

[PATCH v1 4/5] tools/ocaml/libs/xb: add missing stdint.h

[Edwin Török]

xs_wire.h fails to compile without this, and a slight rearrangement of
header includes (e.g. by clang-format) could cause the file to fail to
compile.

Be more robust and include the needed header file.
---
 tools/ocaml/libs/xb/xenbus_stubs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/ocaml/libs/xb/xenbus_stubs.c b/tools/ocaml/libs/xb/xenbus_stubs.c
index e5206f64d4..ce6d33b23e 100644
--- a/tools/ocaml/libs/xb/xenbus_stubs.c
+++ b/tools/ocaml/libs/xb/xenbus_stubs.c
@@ -15,6 +15,7 @@
  */
 
 #include <unistd.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <sys/mman.h>
 #include <string.h>
-- 
2.34.1

[PATCH v1 5/5] CODING_STYLE: add .clang-format

[Edwin Török]

Add a .clang-format configuration that tries to match CODING_STYLE where
possible.

I was not able to express the special casing of braces after 'do'
though, this can only be controlled generally for all control
statements.
It is imperfect, but should be better than the existing bindings, which
do not follow Xen coding style.

Add this to tools/ocaml first because:
* there are relatively few C files here, and it is a good place to start with
* it'd be useful to make these follow Xen's CODING_STYLE
(which they currently do not because they use tabs for example)
* they change relatively infrequently, so shouldn't cause issues with
  backporting security fixes (could either backport the reindentation
  patch too, or use git cherry-pick with `-Xignore-space-change`)

Once this is used it'll need inserting some '#include <stdint.h>', otherwise xs_wire.h
fails to compile due to the missing uint32_t define.

Does not yet reformat any code.

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/.clang-format | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644 tools/ocaml/.clang-format

diff --git a/tools/ocaml/.clang-format b/tools/ocaml/.clang-format
new file mode 100644
index 0000000000..7ff88ee043
--- /dev/null
+++ b/tools/ocaml/.clang-format
@@ -0,0 +1,9 @@
+BasedOnStyle: GNU
+IndentWidth: 4
+
+# override GNU to match Xen ../../CODING_STYLE more closely
+AlwaysBreakAfterDefinitionReturnType: None
+AlwaysBreakAfterReturnType: None
+SpacesInConditionalStatement: true
+SpaceBeforeParens: ControlStatements
+BreakBeforeBraces: Allman
-- 
2.34.1

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Julien Grall]

Hi Edwin,

The title should have "OCaml" to clarify that .clang-format is not added 
at the root level.

On 30/11/2022 17:32, Edwin Török wrote:
> Add a .clang-format configuration that tries to match CODING_STYLE where
> possible.
> 
> I was not able to express the special casing of braces after 'do'
> though, this can only be controlled generally for all control
> statements.
> It is imperfect, but should be better than the existing bindings, which
> do not follow Xen coding style.

Right, from previous discussion, I was under the impression that it 
requires some work to write a clang-format file for Xen.

I am hopeful that some day we will have a proper one. In fact, we have 
been discussing about this as part of MISRA (+ Stefano).

> 
> Add this to tools/ocaml first because:
> * there are relatively few C files here, and it is a good place to start with
> * it'd be useful to make these follow Xen's CODING_STYLE
> (which they currently do not because they use tabs for example)
> * they change relatively infrequently, so shouldn't cause issues with
>    backporting security fixes (could either backport the reindentation
>    patch too, or use git cherry-pick with `-Xignore-space-change`)
> 
> Once this is used it'll need inserting some '#include <stdint.h>', otherwise xs_wire.h
> fails to compile due to the missing uint32_t define.

At first I was a bit concerned with this paragraph because a coding 
style should not impact compilation. But I guess that's because the 
format will convert u32 to uint32_t. Is that correct?

If so, I would expand the paragraph to explicit say that, per the coding 
styel, u32 will be converted to uint32_t.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Edwin Torok]

> On 1 Dec 2022, at 09:11, Julien Grall <julien@xen.org> wrote:
> 
> Hi Edwin,
> 
> The title should have "OCaml" to clarify that .clang-format is not added at the root level.
> 

Sure, I'll update that when I resend.

> On 30/11/2022 17:32, Edwin Török wrote:
>> Add a .clang-format configuration that tries to match CODING_STYLE where
>> possible.
>> I was not able to express the special casing of braces after 'do'
>> though, this can only be controlled generally for all control
>> statements.
>> It is imperfect, but should be better than the existing bindings, which
>> do not follow Xen coding style.
> 
> Right, from previous discussion, I was under the impression that it requires some work to write a clang-format file for Xen.
> 
> I am hopeful that some day we will have a proper one. In fact, we have been discussing about this as part of MISRA (+ Stefano).
> 
>> Add this to tools/ocaml first because:
>> * there are relatively few C files here, and it is a good place to start with
>> * it'd be useful to make these follow Xen's CODING_STYLE
>> (which they currently do not because they use tabs for example)
>> * they change relatively infrequently, so shouldn't cause issues with
>>   backporting security fixes (could either backport the reindentation
>>   patch too, or use git cherry-pick with `-Xignore-space-change`)
>> Once this is used it'll need inserting some '#include <stdint.h>', otherwise xs_wire.h
>> fails to compile due to the missing uint32_t define.
> 
> At first I was a bit concerned with this paragraph because a coding style should not impact compilation. But I guess that's because the format will convert u32 to uint32_t. Is that correct?
> 
> If so, I would expand the paragraph to explicit say that, per the coding styel, u32 will be converted to uint32_t.


clang-format rearranges the order of '#include' in C files, it shouldn't convert types.
But rearranging (sorting) includes can sometimes reveal bugs where the code only happened to compile because the includes were done in a certain order
(e.g. we included something that included stdint.h, therefore the next include line worked, but if you swap the include order that no longer works), i.e.:

#include "c.h"
#include "b.h"

vs

/* post formatting */
#include "b.h"
#include "c.h"

Where c.h includes a.h, and b.h depends on declarations from a.h, then prior to reformat the code compiles, and afterwards it doesn't.

Which can be fixed by adding this to the C file (and then regardless of include order of the other 2 it compiles):
#include <a.h>

Or by fixing b.h to include a.h itself it it depends on it.


Perhaps this'd better be fixed in xs_wire.h itself to include all its dependencies, but that is a publicly installed header, and there might be a reason why it doesn't include stdint.h.
(neither u32, nor uint32_t is standard C, both require a header to be included for them to be available)

Best regards,
--Edwin
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Julien Grall]

(+ A few people)

On 01/12/2022 09:21, Edwin Torok wrote:
> 
> 
>> On 1 Dec 2022, at 09:11, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Edwin,
>>
>> The title should have "OCaml" to clarify that .clang-format is not added at the root level.
>>
> 
> Sure, I'll update that when I resend.
> 
>> On 30/11/2022 17:32, Edwin Török wrote:
>>> Add a .clang-format configuration that tries to match CODING_STYLE where
>>> possible.
>>> I was not able to express the special casing of braces after 'do'
>>> though, this can only be controlled generally for all control
>>> statements.
>>> It is imperfect, but should be better than the existing bindings, which
>>> do not follow Xen coding style.
>>
>> Right, from previous discussion, I was under the impression that it requires some work to write a clang-format file for Xen.
>>
>> I am hopeful that some day we will have a proper one. In fact, we have been discussing about this as part of MISRA (+ Stefano).
>>
>>> Add this to tools/ocaml first because:
>>> * there are relatively few C files here, and it is a good place to start with
>>> * it'd be useful to make these follow Xen's CODING_STYLE
>>> (which they currently do not because they use tabs for example)
>>> * they change relatively infrequently, so shouldn't cause issues with
>>>    backporting security fixes (could either backport the reindentation
>>>    patch too, or use git cherry-pick with `-Xignore-space-change`)
>>> Once this is used it'll need inserting some '#include <stdint.h>', otherwise xs_wire.h
>>> fails to compile due to the missing uint32_t define.
>>
>> At first I was a bit concerned with this paragraph because a coding style should not impact compilation. But I guess that's because the format will convert u32 to uint32_t. Is that correct?
>>
>> If so, I would expand the paragraph to explicit say that, per the coding styel, u32 will be converted to uint32_t.
> 
> 
> clang-format rearranges the order of '#include' in C files, it shouldn't convert types.
> But rearranging (sorting) includes can sometimes reveal bugs where the code only happened to compile because the includes were done in a certain order
> (e.g. we included something that included stdint.h, therefore the next include line worked, but if you swap the include order that no longer works), i.e.:
> 
> #include "c.h"
> #include "b.h"
> 
> vs
> 
> /* post formatting */
> #include "b.h"
> #include "c.h"
> 
> Where c.h includes a.h, and b.h depends on declarations from a.h, then prior to reformat the code compiles, and afterwards it doesn't.
Thanks for the detailed information, I think some of it needs to be 
summarized in the commmit message.

> 
> Which can be fixed by adding this to the C file (and then regardless of include order of the other 2 it compiles):
> #include <a.h>
This would not work if the file were called "d.h" rather than "a.h" 
because the clang format would re-order it. So...

> 
> Or by fixing b.h to include a.h itself it it depends on it.

... this is the proper way to fix it.

> 
> Perhaps this'd better be fixed in xs_wire.h itself to include all its dependencies, but that is a publicly installed header, and there might be a reason why it doesn't include stdint.h.

I am not aware of any restrictions and at least one public headers 
already include <stdint.h>. I am CCed a few more people to get an opinion.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 3896 bytes --]

On 01.12.22 10:31, Julien Grall wrote:
> (+ A few people)
> 
> On 01/12/2022 09:21, Edwin Torok wrote:
>>
>>
>>> On 1 Dec 2022, at 09:11, Julien Grall <julien@xen.org> wrote:
>>>
>>> Hi Edwin,
>>>
>>> The title should have "OCaml" to clarify that .clang-format is not added at 
>>> the root level.
>>>
>>
>> Sure, I'll update that when I resend.
>>
>>> On 30/11/2022 17:32, Edwin Török wrote:
>>>> Add a .clang-format configuration that tries to match CODING_STYLE where
>>>> possible.
>>>> I was not able to express the special casing of braces after 'do'
>>>> though, this can only be controlled generally for all control
>>>> statements.
>>>> It is imperfect, but should be better than the existing bindings, which
>>>> do not follow Xen coding style.
>>>
>>> Right, from previous discussion, I was under the impression that it requires 
>>> some work to write a clang-format file for Xen.
>>>
>>> I am hopeful that some day we will have a proper one. In fact, we have been 
>>> discussing about this as part of MISRA (+ Stefano).
>>>
>>>> Add this to tools/ocaml first because:
>>>> * there are relatively few C files here, and it is a good place to start with
>>>> * it'd be useful to make these follow Xen's CODING_STYLE
>>>> (which they currently do not because they use tabs for example)
>>>> * they change relatively infrequently, so shouldn't cause issues with
>>>>    backporting security fixes (could either backport the reindentation
>>>>    patch too, or use git cherry-pick with `-Xignore-space-change`)
>>>> Once this is used it'll need inserting some '#include <stdint.h>', otherwise 
>>>> xs_wire.h
>>>> fails to compile due to the missing uint32_t define.
>>>
>>> At first I was a bit concerned with this paragraph because a coding style 
>>> should not impact compilation. But I guess that's because the format will 
>>> convert u32 to uint32_t. Is that correct?
>>>
>>> If so, I would expand the paragraph to explicit say that, per the coding 
>>> styel, u32 will be converted to uint32_t.
>>
>>
>> clang-format rearranges the order of '#include' in C files, it shouldn't 
>> convert types.
>> But rearranging (sorting) includes can sometimes reveal bugs where the code 
>> only happened to compile because the includes were done in a certain order
>> (e.g. we included something that included stdint.h, therefore the next include 
>> line worked, but if you swap the include order that no longer works), i.e.:
>>
>> #include "c.h"
>> #include "b.h"
>>
>> vs
>>
>> /* post formatting */
>> #include "b.h"
>> #include "c.h"
>>
>> Where c.h includes a.h, and b.h depends on declarations from a.h, then prior 
>> to reformat the code compiles, and afterwards it doesn't.
> Thanks for the detailed information, I think some of it needs to be summarized 
> in the commmit message.
> 
>>
>> Which can be fixed by adding this to the C file (and then regardless of 
>> include order of the other 2 it compiles):
>> #include <a.h>
> This would not work if the file were called "d.h" rather than "a.h" because the 
> clang format would re-order it. So...
> 
>>
>> Or by fixing b.h to include a.h itself it it depends on it.
> 
> ... this is the proper way to fix it.
> 
>>
>> Perhaps this'd better be fixed in xs_wire.h itself to include all its 
>> dependencies, but that is a publicly installed header, and there might be a 
>> reason why it doesn't include stdint.h.
> 
> I am not aware of any restrictions and at least one public headers already 
> include <stdint.h>. I am CCed a few more people to get an opinion.

I don't think xs_wire.h should include stdint.h. This will result in a conflict
e.g. in the Linux kernel.

We might want to add a comment to xs_wire.h like the one in ring.h in order to
document the requirement of the type definition of uint32_t.


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Julien Grall]

On 01/12/2022 10:08, Juergen Gross wrote:
> On 01.12.22 10:31, Julien Grall wrote:
>> (+ A few people)
>>
>> On 01/12/2022 09:21, Edwin Torok wrote:
>>>
>>>
>>>> On 1 Dec 2022, at 09:11, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi Edwin,
>>>>
>>>> The title should have "OCaml" to clarify that .clang-format is not 
>>>> added at the root level.
>>>>
>>>
>>> Sure, I'll update that when I resend.
>>>
>>>> On 30/11/2022 17:32, Edwin Török wrote:
>>>>> Add a .clang-format configuration that tries to match CODING_STYLE 
>>>>> where
>>>>> possible.
>>>>> I was not able to express the special casing of braces after 'do'
>>>>> though, this can only be controlled generally for all control
>>>>> statements.
>>>>> It is imperfect, but should be better than the existing bindings, 
>>>>> which
>>>>> do not follow Xen coding style.
>>>>
>>>> Right, from previous discussion, I was under the impression that it 
>>>> requires some work to write a clang-format file for Xen.
>>>>
>>>> I am hopeful that some day we will have a proper one. In fact, we 
>>>> have been discussing about this as part of MISRA (+ Stefano).
>>>>
>>>>> Add this to tools/ocaml first because:
>>>>> * there are relatively few C files here, and it is a good place to 
>>>>> start with
>>>>> * it'd be useful to make these follow Xen's CODING_STYLE
>>>>> (which they currently do not because they use tabs for example)
>>>>> * they change relatively infrequently, so shouldn't cause issues with
>>>>>    backporting security fixes (could either backport the reindentation
>>>>>    patch too, or use git cherry-pick with `-Xignore-space-change`)
>>>>> Once this is used it'll need inserting some '#include <stdint.h>', 
>>>>> otherwise xs_wire.h
>>>>> fails to compile due to the missing uint32_t define.
>>>>
>>>> At first I was a bit concerned with this paragraph because a coding 
>>>> style should not impact compilation. But I guess that's because the 
>>>> format will convert u32 to uint32_t. Is that correct?
>>>>
>>>> If so, I would expand the paragraph to explicit say that, per the 
>>>> coding styel, u32 will be converted to uint32_t.
>>>
>>>
>>> clang-format rearranges the order of '#include' in C files, it 
>>> shouldn't convert types.
>>> But rearranging (sorting) includes can sometimes reveal bugs where 
>>> the code only happened to compile because the includes were done in a 
>>> certain order
>>> (e.g. we included something that included stdint.h, therefore the 
>>> next include line worked, but if you swap the include order that no 
>>> longer works), i.e.:
>>>
>>> #include "c.h"
>>> #include "b.h"
>>>
>>> vs
>>>
>>> /* post formatting */
>>> #include "b.h"
>>> #include "c.h"
>>>
>>> Where c.h includes a.h, and b.h depends on declarations from a.h, 
>>> then prior to reformat the code compiles, and afterwards it doesn't.
>> Thanks for the detailed information, I think some of it needs to be 
>> summarized in the commmit message.
>>
>>>
>>> Which can be fixed by adding this to the C file (and then regardless 
>>> of include order of the other 2 it compiles):
>>> #include <a.h>
>> This would not work if the file were called "d.h" rather than "a.h" 
>> because the clang format would re-order it. So...
>>
>>>
>>> Or by fixing b.h to include a.h itself it it depends on it.
>>
>> ... this is the proper way to fix it.
>>
>>>
>>> Perhaps this'd better be fixed in xs_wire.h itself to include all its 
>>> dependencies, but that is a publicly installed header, and there 
>>> might be a reason why it doesn't include stdint.h.
>>
>> I am not aware of any restrictions and at least one public headers 
>> already include <stdint.h>. I am CCed a few more people to get an 
>> opinion.
> 
> I don't think xs_wire.h should include stdint.h. This will result in a 
> conflict
> e.g. in the Linux kernel.

AFAIU, Linux has its own copy of the headers. Is that correct?

> 
> We might want to add a comment to xs_wire.h like the one in ring.h in 
> order to
> document the requirement of the type definition of uint32_t.

The problem with this approach is you made more difficult for any 
userspace application to use the headers. So I would argue that the 
Linux copy can remove "stdint.h" if needed.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4982 bytes --]

On 01.12.22 11:12, Julien Grall wrote:
> 
> 
> On 01/12/2022 10:08, Juergen Gross wrote:
>> On 01.12.22 10:31, Julien Grall wrote:
>>> (+ A few people)
>>>
>>> On 01/12/2022 09:21, Edwin Torok wrote:
>>>>
>>>>
>>>>> On 1 Dec 2022, at 09:11, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>> Hi Edwin,
>>>>>
>>>>> The title should have "OCaml" to clarify that .clang-format is not added at 
>>>>> the root level.
>>>>>
>>>>
>>>> Sure, I'll update that when I resend.
>>>>
>>>>> On 30/11/2022 17:32, Edwin Török wrote:
>>>>>> Add a .clang-format configuration that tries to match CODING_STYLE where
>>>>>> possible.
>>>>>> I was not able to express the special casing of braces after 'do'
>>>>>> though, this can only be controlled generally for all control
>>>>>> statements.
>>>>>> It is imperfect, but should be better than the existing bindings, which
>>>>>> do not follow Xen coding style.
>>>>>
>>>>> Right, from previous discussion, I was under the impression that it 
>>>>> requires some work to write a clang-format file for Xen.
>>>>>
>>>>> I am hopeful that some day we will have a proper one. In fact, we have been 
>>>>> discussing about this as part of MISRA (+ Stefano).
>>>>>
>>>>>> Add this to tools/ocaml first because:
>>>>>> * there are relatively few C files here, and it is a good place to start with
>>>>>> * it'd be useful to make these follow Xen's CODING_STYLE
>>>>>> (which they currently do not because they use tabs for example)
>>>>>> * they change relatively infrequently, so shouldn't cause issues with
>>>>>>    backporting security fixes (could either backport the reindentation
>>>>>>    patch too, or use git cherry-pick with `-Xignore-space-change`)
>>>>>> Once this is used it'll need inserting some '#include <stdint.h>', 
>>>>>> otherwise xs_wire.h
>>>>>> fails to compile due to the missing uint32_t define.
>>>>>
>>>>> At first I was a bit concerned with this paragraph because a coding style 
>>>>> should not impact compilation. But I guess that's because the format will 
>>>>> convert u32 to uint32_t. Is that correct?
>>>>>
>>>>> If so, I would expand the paragraph to explicit say that, per the coding 
>>>>> styel, u32 will be converted to uint32_t.
>>>>
>>>>
>>>> clang-format rearranges the order of '#include' in C files, it shouldn't 
>>>> convert types.
>>>> But rearranging (sorting) includes can sometimes reveal bugs where the code 
>>>> only happened to compile because the includes were done in a certain order
>>>> (e.g. we included something that included stdint.h, therefore the next 
>>>> include line worked, but if you swap the include order that no longer 
>>>> works), i.e.:
>>>>
>>>> #include "c.h"
>>>> #include "b.h"
>>>>
>>>> vs
>>>>
>>>> /* post formatting */
>>>> #include "b.h"
>>>> #include "c.h"
>>>>
>>>> Where c.h includes a.h, and b.h depends on declarations from a.h, then prior 
>>>> to reformat the code compiles, and afterwards it doesn't.
>>> Thanks for the detailed information, I think some of it needs to be 
>>> summarized in the commmit message.
>>>
>>>>
>>>> Which can be fixed by adding this to the C file (and then regardless of 
>>>> include order of the other 2 it compiles):
>>>> #include <a.h>
>>> This would not work if the file were called "d.h" rather than "a.h" because 
>>> the clang format would re-order it. So...
>>>
>>>>
>>>> Or by fixing b.h to include a.h itself it it depends on it.
>>>
>>> ... this is the proper way to fix it.
>>>
>>>>
>>>> Perhaps this'd better be fixed in xs_wire.h itself to include all its 
>>>> dependencies, but that is a publicly installed header, and there might be a 
>>>> reason why it doesn't include stdint.h.
>>>
>>> I am not aware of any restrictions and at least one public headers already 
>>> include <stdint.h>. I am CCed a few more people to get an opinion.
>>
>> I don't think xs_wire.h should include stdint.h. This will result in a conflict
>> e.g. in the Linux kernel.
> 
> AFAIU, Linux has its own copy of the headers. Is that correct?

Yes.

> 
>>
>> We might want to add a comment to xs_wire.h like the one in ring.h in order to
>> document the requirement of the type definition of uint32_t.
> 
> The problem with this approach is you made more difficult for any userspace 
> application to use the headers. So I would argue that the Linux copy can remove 
> "stdint.h" if needed.

Today there is exactly one public header including stdint.h, and I'd argue
that this was a mistake.

xs_wire.h is especially rather uninteresting for any user space application
but a Xenstore implementation. All consumers of xs_wire.h are probably
either in the Xen tree, or operating system kernels. User space applications
should use libxenstore for accessing the Xenstore, so I don't see an
advantage in breaking the usual philosophy of the Xen public headers NOT
including external headers like stdint.h.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Julien Grall]

On 01/12/2022 10:44, Juergen Gross wrote:
> On 01.12.22 11:12, Julien Grall wrote:
>>> We might want to add a comment to xs_wire.h like the one in ring.h in 
>>> order to
>>> document the requirement of the type definition of uint32_t.
>>
>> The problem with this approach is you made more difficult for any 
>> userspace application to use the headers. So I would argue that the 
>> Linux copy can remove "stdint.h" if needed.
> 
> Today there is exactly one public header including stdint.h, and I'd argue
> that this was a mistake.
> 
> xs_wire.h is especially rather uninteresting for any user space application
> but a Xenstore implementation. All consumers of xs_wire.h are probably
> either in the Xen tree, or operating system kernels. User space 
> applications
> should use libxenstore for accessing the Xenstore, so I don't see an
> advantage in breaking the usual philosophy of the Xen public headers NOT
> including external headers like stdint.h.

I think Edwin example is a pretty good justification for including 
stdint.h.

If you have a coding style requiring to order header alphabetically, 
then the developer may not even be able to include stdint.h without any 
hackery (e.g. introducing a header that will always be before the Xen 
public headers).

Cheers,

-- 
Julien Grall

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Jan Beulich]

On 01.12.2022 11:47, Julien Grall wrote:
> 
> 
> On 01/12/2022 10:44, Juergen Gross wrote:
>> On 01.12.22 11:12, Julien Grall wrote:
>>>> We might want to add a comment to xs_wire.h like the one in ring.h in 
>>>> order to
>>>> document the requirement of the type definition of uint32_t.
>>>
>>> The problem with this approach is you made more difficult for any 
>>> userspace application to use the headers. So I would argue that the 
>>> Linux copy can remove "stdint.h" if needed.
>>
>> Today there is exactly one public header including stdint.h, and I'd argue
>> that this was a mistake.

I think so, too.

>> xs_wire.h is especially rather uninteresting for any user space application
>> but a Xenstore implementation. All consumers of xs_wire.h are probably
>> either in the Xen tree, or operating system kernels. User space 
>> applications
>> should use libxenstore for accessing the Xenstore, so I don't see an
>> advantage in breaking the usual philosophy of the Xen public headers NOT
>> including external headers like stdint.h.
> 
> I think Edwin example is a pretty good justification for including 
> stdint.h.

I disagree. The intention has always been for consumers to provide the
basic C99 types by whatever suitable means they have. Note that in Xen
we also have no stdint.h.

> If you have a coding style requiring to order header alphabetically, 
> then the developer may not even be able to include stdint.h without any 
> hackery (e.g. introducing a header that will always be before the Xen 
> public headers).

Just to indicate that commonly style requirements may be weaker than
"fully alphabetic" - we don't request full ordering. What we request is
that groups (xen/, asm/, public/) be ordered within any group, but we
do not (afaia) demand ordering across groups (and indeed commonly we
have asm/ after xen/).

Jan

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Julien Grall]

On 01/12/2022 11:30, Jan Beulich wrote:
> On 01.12.2022 11:47, Julien Grall wrote:
>>
>>
>> On 01/12/2022 10:44, Juergen Gross wrote:
>>> On 01.12.22 11:12, Julien Grall wrote:
>>>>> We might want to add a comment to xs_wire.h like the one in ring.h in
>>>>> order to
>>>>> document the requirement of the type definition of uint32_t.
>>>>
>>>> The problem with this approach is you made more difficult for any
>>>> userspace application to use the headers. So I would argue that the
>>>> Linux copy can remove "stdint.h" if needed.
>>>
>>> Today there is exactly one public header including stdint.h, and I'd argue
>>> that this was a mistake.
> 
> I think so, too.
> 
>>> xs_wire.h is especially rather uninteresting for any user space application
>>> but a Xenstore implementation. All consumers of xs_wire.h are probably
>>> either in the Xen tree, or operating system kernels. User space
>>> applications
>>> should use libxenstore for accessing the Xenstore, so I don't see an
>>> advantage in breaking the usual philosophy of the Xen public headers NOT
>>> including external headers like stdint.h.
>>
>> I think Edwin example is a pretty good justification for including
>> stdint.h.
> 
> I disagree. The intention has always been for consumers to provide the
> basic C99 types by whatever suitable means they have. Note that in Xen
> we also have no stdint.h.

I really dislike when I have to find the dependency of an header. This 
is really a waste of time...

If other disagree with that, then the strict minimum would be for this 
dependency to be recorded if it hasn't been done (I couldn't find anywhere).

> 
>> If you have a coding style requiring to order header alphabetically,
>> then the developer may not even be able to include stdint.h without any
>> hackery (e.g. introducing a header that will always be before the Xen
>> public headers).
> 
> Just to indicate that commonly style requirements may be weaker than
> "fully alphabetic" - we don't request full ordering. What we request is
> that groups (xen/, asm/, public/) be ordered within any group, but we
> do not (afaia) demand ordering across groups (and indeed commonly we
> have asm/ after xen/).
Right, but that's **our** coding style. You don't know what's going to 
be the coding style for other project.

Cheers,

-- 
Julien Grall

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Jan Beulich]

On 01.12.2022 12:35, Julien Grall wrote:
> On 01/12/2022 11:30, Jan Beulich wrote:
>> On 01.12.2022 11:47, Julien Grall wrote:
>>> On 01/12/2022 10:44, Juergen Gross wrote:
>>>> On 01.12.22 11:12, Julien Grall wrote:
>>>>>> We might want to add a comment to xs_wire.h like the one in ring.h in
>>>>>> order to
>>>>>> document the requirement of the type definition of uint32_t.
>>>>>
>>>>> The problem with this approach is you made more difficult for any
>>>>> userspace application to use the headers. So I would argue that the
>>>>> Linux copy can remove "stdint.h" if needed.
>>>>
>>>> Today there is exactly one public header including stdint.h, and I'd argue
>>>> that this was a mistake.
>>
>> I think so, too.
>>
>>>> xs_wire.h is especially rather uninteresting for any user space application
>>>> but a Xenstore implementation. All consumers of xs_wire.h are probably
>>>> either in the Xen tree, or operating system kernels. User space
>>>> applications
>>>> should use libxenstore for accessing the Xenstore, so I don't see an
>>>> advantage in breaking the usual philosophy of the Xen public headers NOT
>>>> including external headers like stdint.h.
>>>
>>> I think Edwin example is a pretty good justification for including
>>> stdint.h.
>>
>> I disagree. The intention has always been for consumers to provide the
>> basic C99 types by whatever suitable means they have. Note that in Xen
>> we also have no stdint.h.
> 
> I really dislike when I have to find the dependency of an header. This 
> is really a waste of time...

I can see your point, but for Xen's public headers it has always been
that way. Plus, as said, adding (unguarded) #include <stdint.h> would
even break the building of Xen itself.

> If other disagree with that, then the strict minimum would be for this 
> dependency to be recorded if it hasn't been done (I couldn't find anywhere).

It is kind of recorded in xen/include/Makefile, with the three
"-include stdint.h" that are used there (of which one probably really
ought to be -include cstdint). But I agree this can't really count as
documentation.

>>> If you have a coding style requiring to order header alphabetically,
>>> then the developer may not even be able to include stdint.h without any
>>> hackery (e.g. introducing a header that will always be before the Xen
>>> public headers).
>>
>> Just to indicate that commonly style requirements may be weaker than
>> "fully alphabetic" - we don't request full ordering. What we request is
>> that groups (xen/, asm/, public/) be ordered within any group, but we
>> do not (afaia) demand ordering across groups (and indeed commonly we
>> have asm/ after xen/).
> Right, but that's **our** coding style. You don't know what's going to 
> be the coding style for other project.

Sure, hence me having said "may be" in my reply.

Jan

Re: [PATCH v1 5/5] CODING_STYLE: add .clang-format

[Christian Lindig]

> On 30 Nov 2022, at 17:32, Edwin Török <edvin.torok@citrix.com> wrote:
> 
> Add a .clang-format configuration that tries to match CODING_STYLE where
> possible

> [..]
> Does not yet reformat any code.
> 
> No functional change.
> 
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> ---
> tools/ocaml/.clang-format | 9 +++++++++
> 1 file changed, 9 insertions(+)
> create mode 100644 tools/ocaml/.clang-format

Acked-by: Christian Lindig <christian.lindig@citrix.com>

I support this kind of automation.  I also agree with the previous comment that the title should indicate that this is only going into a subtree.

— C