Re: [RFC PATCH 00/25] Introduce xenbindgen to autogen hypercall structs

["Anthony PERARD" <anthony.perard@xxxxxxxxxx>]

Hi Alejandro,

Nice work :-).

On Fri, Nov 15, 2024 at 11:51:29AM +0000, Alejandro Vallejo wrote:
> This series is the result of my "Interfacing Rust with Xen" talk in Xen 
> Summit.
> It adds a hypercall ABI IDL parser and generator to the xen tree, replaces a
> couple of existing hypercalls, creates a Rust crate with autogenerated 
> contents
> an creates a CI job to ensure nothing goes out of sync.
>
> The changes are fairly invasive because the various autogenerated items appear
> in many places (specially the domaincreate flags). However, the changes to the
> hypervisor are all mechanical and not functional (not intentionally so, at
> least).

I tried to build QEMU with this series applied, and the build failed. In
this case nothing important, the "autogen" directory just need to be
installed. But I fear the changes introduce to the API (like change
of case for the flags) will also be done to other API that project
outside of the xen repo use, and thus introduce unneeded breakage.
Should the changes also introduce a compatibility with the previous API?

> I've split the generator in reasonably small pieces, but it's still not a 
> small
> tool. The Rust backend remains monolithic in a single patch until the RFC goes
> further. It mirrors the C backend for the most part.
> 
> The hypercall ABI is specified in a schema of TOML. Most of it should be 
> fairly
> obvious as to what it does and means, with the possible exception of the "typ"
> field. That has the look of a dictionary because that helps the deserializer 
> to
> automatically resolve the typ to a convenient Rust enum type (Typ). In time,
> that will become something nicer to write, but that's fairly far in my list of
> priorities at the moment.

Instead of creating your own IDL specification, did you look for
existing project that would do just that? That is been able to describe
the existing ABI in IDL and use an existing project to generate C and
Rust headers.

I kind of look into this, but there's quite a few project to explore and
I didn't really spend enough time. Also, there's probably quite a lot
that are for client-server interfaces rather than syscall/hypercalls, or
they impose a data format.


Next, on the file format choice, is TOML the best for describing an ABI,
or would other existing file format make it a bit easier to read, like
JSON or YAML? (I quite like using YAML so I have a bias toward it :-),
and that's the format used for the CI). I don't think it mater much for
Serde which file format is used.

> After the series sysctl::readconsole and domctl::createdomain are 
> autogenerated
> from their formalized forms. In the course of formalizing the ABI it became
> apparent readconsole has a different ABI in 32 and 64 bits. While benign in
> that particular case, it's yet one more reason to formalize the ABI in a
> language agnostic way and have it machine-checked.
> 
> ======== The Plan ===========
> 
> So, the idea of the series is to adjust 2 meaningful hypercalls to TOML-based
> specifications (sysctl::readconsole and domctl::createdomain). The series is
> organised in the following chunks of work
> 
>   1. Sanitise domctl::createdomain to remove packed subfields.
>   2. Introduce xenbindgen (IDL parser and generator for C).
>   3. Specify hypercalls in TOML, and replace the hand-crafted public bits.
>   4. Introduce Rust generator for xenbindgen.
>   5. Introduce a xen-sys crate, with the autogenerated Rust constructs.
>   6. Introduce CI checks for Rust linters, ABI validation and autogenerated
>      file consistency.
> 
> Future work involves migrating more hypercalls, in the same way patch 12 does.
> Most hypercalls should not take the amount of churn createdomain did. With the
> foundations laid down the involved work should be simple.
> 
> I have considered integrating the hypercall generation in the build process.
> That forces the Rust toolchain to be in the list of build dependencies for
> downstreams, which might be complicated or annoying. For the time being, I
> think checking in the autogenerated files and confirming in CI that they are
> in-sync is (imo) more than enough.

Having the generated header files been committed sound like a good idea
for now. For better or for worth we've got a few of those already, so
it isn't a first.

But the way the different pieces are spread out in the repository in
this patch series will make it difficult for future contributor to update
the hypercall ABI. They'll be meet with an "autogenerated file, don't
modify" with little clue as to how actually regenerate them. For that I
think it would be better to have the IDL description (TOML files) in
that "xen/public/include" directory or at the very least in "xen/".
Second, with "xenbindgen" been in in "tools/", this introduce a soft
dependency of "xen" on "tools", which should be avoided even if the
build system of "xen/" doesn't call into xenbindgen today. So I think it
would be better to have xenbindgen either live in "xen/" or in a
different directory at the root of the repo. There's already Kconfig in
"xen/" so xenbindgen isn't going to be the first parser/generator in
"xen/" directory.

> ======== Patch grouping ===========
> 
> Patches 1 and 2 remove packed subfields to allow encoding it in the TOML specs
> (e.g: see patch 13, replace hand-crafted altp2m_mode). It's non-functional
> changes aiming to reach simpler representability.
> 
>   Patch 1.  xen/domctl: Refine grant_opts into max_grant_version
>   Patch 2.  xen/domctl: Replace altp2m_opts with altp2m_mode
> 
> Patches 3 to 10 are xenbindgen (with the C generator backend only). The
> Cargo.lock file in patch 4 is required for build reproducibility and is
> recommended to have checked in the repo.
> 
>   Patch 3.  tools/xenbindgen: Introduce a Xen hypercall IDL generator
>   Patch 4.  tools/xenbindgen: Add a TOML spec reader
>   Patch 5.  tools/xenbindgen: Add basic plumbing for the C backend
>   Patch 6.  tools/xenbindgen: Add xenbindgen's Cargo.lock file
>   Patch 7.  tools/xenbindgen: Add support for structs in TOML specs
>   Patch 8.  tools/xenbindgen: Add support for enums in TOML specs
>   Patch 9.  tools/xenbindgen: Add support for bitmaps in TOML specs
>   Patch 10. tools/xenbindgen: Add support for includes in the TOML specs
> 
> Patch 11 goes a step beyond and validates the ABI has no implicit padding and
> that all names are unique. In the future, when we define rules for stable 
> ABIs,
> all of those can be checked here, at generation time.
> 
>   Patch 11. tools/xenbindgen: Validate ABI rules at generation time
> 
> Patches 12 to 19 replace current items in the C headers with autogenerated
> versions. They should all be mechanical translations.
> 
>   Patch 12. xen: Replace sysctl/readconsole with autogenerated version
>   Patch 13. xen: Replace hand-crafted altp2m_mode descriptions with
>             autogenerated ones
>   Patch 14. xen: Replace common bitmaps in domctl.createdomain with
>             autogenerated versions
>   Patch 15. xen/arm: Replace hand-crafted xen_arch_domainconfig with
>             autogenerated one

I feel like you write "hand-crafted" in those patch description as if it
is a bad thing. Yet, you replace the hand-crafted C headers by
hand-crafted IDL. I think a better title could be "Translate
xen_arch_domainconfig into IDL" to avoid what I feel like is a
pejorative term.

Also, would it be possible to separate changes to the existing API from
the patch that introduce the newly generated headers? I think it would
be much easier to review that the generated headers don't introduce
any changes over the current one.

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech