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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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