Re: [RFC PATCH 12/25] xen: Replace sysctl/readconsole with autogenerated version

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.11.2024 12:51, Alejandro Vallejo wrote:
> Describe sysctl/readconsole as a TOML specification, remove old
> hand-coded version and replace it with autogenerated file.
> 
> While at it, transform the console driver to use uint8_t rather than
> char in order to mandate the type to be unsigned and ensure the ABI is
> not defined with regards to C-specific types.

Yet the derived C representation imo then should still be using char, not
uint8_t. In particular it would be a good sign if the Xen sources wouldn't
need to change, unlike happens here (altering types of a few internals of
the console machinery).

> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>  stubdom/Makefile                              |  2 +-
>  tools/rust/Makefile                           | 19 ++++++++
>  .../xenbindgen/extra/sysctl/readconsole.toml  | 43 +++++++++++++++++++
>  xen/drivers/char/console.c                    | 12 +++---
>  xen/include/public/autogen/sysctl.h           | 35 +++++++++++++++

In the build tree, having an autogen subdir under public/ _may_ be okay
(personally I dislike even that). I didn't manage to spot adjustments to
how files are eventually installed, yet at that point there clearly
shouldn't be any autogen subdir(s) anymore. How the individual files come
into existence is, imo, nothing consumers of the interface ought to (need
to) care about.

> --- /dev/null
> +++ b/tools/rust/xenbindgen/extra/sysctl/readconsole.toml
> @@ -0,0 +1,43 @@
> +[[structs]]
> +name = "xen_sysctl_readconsole"
> +description = "Read console content from Xen buffer ring."
> +
> +[[structs.fields]]
> +name = "clear"
> +description = "IN: Non-zero -> clear after reading."
> +typ = { tag = "u8" }
> +
> +[[structs.fields]]
> +name = "incremental"
> +description = "IN: Non-zero -> start index specified by `index` field."
> +typ = { tag = "u8" }
> +
> +[[structs.fields]]
> +name = "_pad"
> +description = "Unused."
> +typ = { tag = "u16" }
> +
> +[[structs.fields]]
> +name = "index"
> +description = """
> +IN:  Start index for consuming from ring buffer (if @incremental);
> +OUT: End index after consuming from ring buffer."""
> +typ = { tag = "u32" }
> +
> +[[structs.fields]]
> +name = "buffer"
> +description = """
> +IN: Virtual address to write console data.
> +
> +NOTE: The pointer itself is IN, but the contents of the buffer are OUT."""
> +typ = { tag = "ptr", args = { tag = "u8" } }
> +
> +[[structs.fields]]
> +name = "count"
> +description = "IN: Size of buffer; OUT: Bytes written to buffer."
> +typ = { tag = "u32" }
> +
> +[[structs.fields]]
> +name = "rsvd0_a"
> +description = "Tail padding reserved to zero."
> +typ = { tag = "u32" }

Up to here I wasn't able to spot any documentation on what it to be written
in which way. I already struggle with the double square brackets. The TOML
doc I found when searching the web doesn't have such. Taking just the example
above also doesn't allow me to conclude how e.g. nested structures would be
specified. Really, when talk was of some form of IDL, I expected to see
something IDLish (im particular closer to typical programming languages we
use). Whereas TOML, aiui, is more an easy language for config files of all
sorts.

What I have in mind wouldn't allow for descriptions, yet I'm not sure that's
relevant. The description ought to, first of all, live in the primary source
(i.e. the IDL itself) anyway. Commentary there might be possible to extract
into proper (machine generated/derived) documentation.

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -42,6 +42,8 @@
>  #include <asm/vpl011.h>
>  #endif
>  
> +#include <public/xen.h>

Why would this be needed all of the sudden?

> --- /dev/null
> +++ b/xen/include/public/autogen/sysctl.h
> @@ -0,0 +1,35 @@
> +/*
> + * sysctl
> + *
> + * AUTOGENERATED. DO NOT MODIFY
> + */
> +#ifndef __XEN_AUTOGEN_SYSCTL_H
> +#define __XEN_AUTOGEN_SYSCTL_H
> +
> +/* Read console content from Xen buffer ring. */
> +struct xen_sysctl_readconsole {
> +    /* IN: Non-zero -> clear after reading. */
> +    uint8_t clear;
> +    /* IN: Non-zero -> start index specified by `index` field. */
> +    uint8_t incremental;
> +    /* Unused. */
> +    uint16_t _pad;
> +    /*
> +     * IN:  Start index for consuming from ring buffer (if @incremental);
> +     * OUT: End index after consuming from ring buffer.
> +     */
> +    uint32_t index;
> +    /*
> +     * IN: Virtual address to write console data.
> +     *
> +     * NOTE: The pointer itself is IN, but the contents of the buffer are 
> OUT.
> +     */
> +    XEN_GUEST_HANDLE_64(uint8) buffer;
> +    /* IN: Size of buffer; OUT: Bytes written to buffer. */
> +    uint32_t count;
> +    /* Tail padding reserved to zero. */
> +    uint32_t rsvd0_a;
> +};
> +
> +#endif /* __XEN_AUTOGEN_SYSCTL_H */
> +

If this file is auto-generated, why would it need committing? And yes, there
is the connected question: Will everyone then need to have a Rust compiler
available?

Nit: For anything that is committed, it would be nice if those files were as
tidy as possible style-wise. Most of the above looks entirely okay, just
that there is an unnecessary trailing blank line.

Jan