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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/09/2019 15:25, Ian Jackson wrote:
> 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.

Well - worse is having two of them (xenctrl.ml and .mli)

I think it is a side effect of trying to express it in more idiomatic Ocaml.

>
>> +/*
>> + * 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.

The distinction between enums and integers in Ocaml is fuzzy, because
enums are defined in terms of unboxed integers, hence their construction
with "Val_int(i);".

The return value from this function *is* a list of integers, which will
either be interpreted by Ocaml as "x86_arch_emulation_flags list" or
"physinfo_cap_flag list" as appropriate.

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

Nor me, but it was the only thing which came to mind which didn't
involve rewriting part of the build.

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

The current code depends on the Ocaml constants being the logarithm of
the C constants so that the conversion is easy.

While this isn't a hard requirement, anything more complicated should
have a compelling reason to use.

> (b) synthesise the BUILD_BUG_ON list you have above
>
> (c) search the Xen headers itself and check the value correspondences

This would then result in two ad-hoc shoddy parsers, which I don't think
is a direction we want to go.

Option (b) seems to be the least bad alternative.

The common case needing catching is someone adding a constant to the C
ABI.  This is why the BUILD_BUG_ON()'s against XEN_X86_EMU_ALL and
XEN_SYSCTL_PHYSCAP_MAX exist.  The list of individual constants was to
catch the renumbering, which ought to be a much rarer issue.

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

The interface here is a subset of some headers, and superset of others. 
Automatically generating it isn't feasible IMO, especially as we have
inconsistent ways of doing similar things.

~Andrew

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