Re: [PATCH v3 1/6] xen/arm: firmware: Add SCMI over SMC calls handling layer

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/12/2024 10:11 am, Andrei Cherechesu (OSS) wrote:
> diff --git a/xen/arch/arm/firmware/scmi-smc.c 
> b/xen/arch/arm/firmware/scmi-smc.c
> new file mode 100644
> index 0000000000..62657308d6
> --- /dev/null
> +++ b/xen/arch/arm/firmware/scmi-smc.c
> +bool scmi_handle_smc(struct cpu_user_regs *regs)
> +{
> +    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
> +    struct arm_smccc_res res;
> +
> +    if ( !scmi_is_valid_smc_id(fid) )
> +        return false;
> +
> +    /* Only the hardware domain should use SCMI calls */
> +    if ( !is_hardware_domain(current->domain) )
> +    {
> +        gprintk(XENLOG_ERR, "SCMI: Unprivileged %pd cannot use SCMI.\n",
> +                current->domain);

Minor points.

gprintk() already renders current at the start of the line, so you don't
need the extra %pd.

I'd suggest simply XENLOG_WARNING "SCMI: Unprivileged access attempt\n".

A double "SCMI", and trailing punctuation is not useful in the log.

However, I'd also suggest using gdprintk() because this is guest
trigger-able and not interesting to see in a release build.

> diff --git a/xen/arch/arm/include/asm/firmware/scmi-smc.h 
> b/xen/arch/arm/include/asm/firmware/scmi-smc.h
> new file mode 100644
> index 0000000000..57cc9eef86
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/firmware/scmi-smc.h
> +#ifndef __ASM_SCMI_SMC_H__
> +#define __ASM_SCMI_SMC_H__
> +
> +#include <xen/types.h>
> +#include <asm/regs.h>

You can remove the include of asm/regs.h, and simply declare:

struct cpu_user_regs;

here.  Nothing in this file needs to deference the pointer.

~Andrew