Re: [PATCH v3 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2025 15:17, Sergii Dmytruk wrote:
> The file provides constants, structures and several helper functions for
> parsing SLRT.
> 
> The data described by the structures is passed to Xen by a bootloader
> which initiated DRTM.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
> ---
>  xen/include/xen/slr-table.h | 276 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 276 insertions(+)
>  create mode 100644 xen/include/xen/slr-table.h

Btw, please don't forget to Cc maintainers of code you're changing / adding.

> +/*
> + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
> + */
> +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);

I keep wondering why this ...

> +/*
> + * DRTM Dynamic Launch Configuration
> + */
> +struct slr_entry_dl_info
> +{
> +    struct slr_entry_hdr hdr;
> +    uint64_t dce_size;
> +    uint64_t dce_base;
> +    uint64_t dlme_size;
> +    uint64_t dlme_base;
> +    uint64_t dlme_entry;
> +    struct slr_bl_context bl_context;
> +    uint64_t dl_handler;

... then isn't used right here, instead requiring a cast somewhere (presumably,
as code using this isn't visible in this patch).

> +} __packed;

I similarly keep wondering why there are all these packed attributes here, when
(afaics) all of the structures are defined in a way that any padding is explicit
anyway.

> +static inline const struct slr_entry_hdr *
> +slr_next_entry(const struct slr_table *table, const struct slr_entry_hdr 
> *curr)
> +{
> +    const struct slr_entry_hdr *next = (void *)curr + curr->size;
> +
> +    if ( (void *)next + sizeof(*next) > slr_end_of_entries(table) )
> +        return NULL;
> +    if ( next->tag == SLR_ENTRY_END )
> +        return NULL;
> +    if ( (void *)next + next->size > slr_end_of_entries(table) )
> +        return NULL;
> +
> +    return next;
> +}
> +
> +static inline const struct slr_entry_hdr *
> +slr_next_entry_by_tag(const struct slr_table *table,
> +                      const struct slr_entry_hdr *entry,
> +                      uint16_t tag)
> +{
> +    if ( !entry ) /* Start from the beginning */
> +        entry = (void *)table + sizeof(*table);
> +
> +    for ( ; ; )
> +    {
> +        if ( entry->tag == tag )
> +            return entry;
> +
> +        entry = slr_next_entry(table, entry);
> +        if ( !entry )
> +            return NULL;
> +    }
> +
> +    return NULL;
> +}

For both of the functions, again: Please don't cast away const-ness.

Jan