Re: [Xen-devel] [PATCH v2] tools/ocaml: Fix xenctrl ABI and introduce build-time checks

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Andrew Cooper writes ("[PATCH v2] tools/ocaml: Fix xenctrl ABI and introduce 
build-time checks"):
> c/s f089fddd941 broke the Ocaml ABI by renumering XEN_SYSCTL_PHYSCAP_directio
> without adjusting the Ocaml physinfo_cap_flag enumeration.  Fix this by
> inserting CAP_PV between CAP_HVM and CAP_DirectIO.
...
>  type physinfo_cap_flag =
>       | CAP_HVM
> +     | CAP_PV
>       | CAP_DirectIO

It is surely scandalous that we had this open-coding here of a
duplication of a Xen ABI list.  Thanks for trying to fix it.

> +/*
> + * Various fields which are a bitmap in the C ABI are converted to lists of
> + * integers in the Ocaml ABI for more idiomatic handling.

Err, I don't think you mean lists of integers.  I think you mean
lists enums, which happen to be enums.

> +      * emulation_flags: x86_arch_emulation_flags list;
> +      *
> +      * These BUILD_BUG_ON()'s map the C ABI to the Ocaml ABI.  If they
> +      * trip, xenctrl.ml{,i} need updating to match.
> +      */
> +     BUILD_BUG_ON(XEN_X86_EMU_LAPIC    != (1u <<  0));
> +     BUILD_BUG_ON(XEN_X86_EMU_HPET     != (1u <<  1));
> +     BUILD_BUG_ON(XEN_X86_EMU_PM       != (1u <<  2));
> +     BUILD_BUG_ON(XEN_X86_EMU_RTC      != (1u <<  3));
> +     BUILD_BUG_ON(XEN_X86_EMU_IOAPIC   != (1u <<  4));
> +     BUILD_BUG_ON(XEN_X86_EMU_PIC      != (1u <<  5));
> +     BUILD_BUG_ON(XEN_X86_EMU_VGA      != (1u <<  6));
> +     BUILD_BUG_ON(XEN_X86_EMU_IOMMU    != (1u <<  7));
> +     BUILD_BUG_ON(XEN_X86_EMU_PIT      != (1u <<  8));
> +     BUILD_BUG_ON(XEN_X86_EMU_USE_PIRQ != (1u <<  9));
> +     BUILD_BUG_ON(XEN_X86_EMU_VPCI     != (1u << 10));

I really don't like this approach.  Instead of automatically deriving
the ocaml enum from the Xen ABI, or automatically checking that the
ocaml ABI agrees with the Xen one, you are instead adding a new list
which duplicates the ocaml ABI.

I suggest we do something in the build system - a new script or shell
rune, which is given the strings `x86_arch_emulation_flags' and
`X86_EMU' (and correspondingly for the other enums).

The new thing would search xenctrl.ml[i] for the type and read the
enum list there with an ad-hoc shoddy ocaml parser and then do one or
more of:

(a) synthesise the enum conversion function to map the flag
    numbers back and forth (ie the numbers in ocaml would no
    longer need to match)

(b) synthesise the BUILD_BUG_ON list you have above

(c) search the Xen headers itself and check the value correspondences

Ideally it would have been better to automatically generate
xenctrl.ml[i] from the Xen headers but I rejected that as being too
much annoying interaction with the ocaml build.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel