[PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes

[hchauhan at ventanamicro.com]

-----Original Message-----
From: Anup Patel <anup at brainfault.org> 
Sent: 06 January 2023 23:06
To: Himanshu Chauhan <hchauhan at ventanamicro.com>
Cc: opensbi at lists.infradead.org
Subject: Re: [PATCH 1/9] include: sbi: Fine grain the permissions for M and SU modes

On Tue, Dec 20, 2022 at 4:16 PM Himanshu Chauhan <hchauhan at ventanamicro.com> wrote:
>
> Split the permissions for M-mode and SU-mode. This would help if 
> different sections of OpenSBI need to be given different permissions 
> and if M-mode has different permisssions than the SU-mode over a 
> region.
>
> Signed-off-by: Himanshu Chauhan <hchauhan at ventanamicro.com>
> ---
>  include/sbi/sbi_domain.h | 40 
> +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/include/sbi/sbi_domain.h b/include/sbi/sbi_domain.h index 
> 5553d21..955ffa3 100644
> --- a/include/sbi/sbi_domain.h
> +++ b/include/sbi/sbi_domain.h
> @@ -36,11 +36,41 @@ struct sbi_domain_memregion {
>          */
>         unsigned long base;
>         /** Flags representing memory region attributes */
> -#define SBI_DOMAIN_MEMREGION_READABLE          (1UL << 0)
> -#define SBI_DOMAIN_MEMREGION_WRITEABLE         (1UL << 1)
> -#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (1UL << 2)
> -#define SBI_DOMAIN_MEMREGION_MMODE             (1UL << 3)
> -#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0xfUL)
> +#define SBI_DOMAIN_MEMREGION_M_READABLE                (1UL << 0)
> +#define SBI_DOMAIN_MEMREGION_M_WRITABLE                (1UL << 1)
> +#define SBI_DOMAIN_MEMREGION_M_EXECUTABLE      (1UL << 2)
> +#define SBI_DOMAIN_MEMREGION_SU_READABLE       (1UL << 3)
> +#define SBI_DOMAIN_MEMREGION_SU_WRITABLE       (1UL << 4)
> +#define SBI_DOMAIN_MEMREGION_SU_EXECUTABLE     (1UL << 5)
> +
> +       /** Bit to control if permissions are enforced on all modes */
> +#define SBI_DOMAIN_MEMREGION_ENF_PERMISSIONS   (1UL << 6)
> +
> +#define SBI_DOMAIN_MEMREGION_M_RWX             (SBI_DOMAIN_MEMREGION_M_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE | \
> +                                                
> +SBI_DOMAIN_MEMREGION_M_EXECUTABLE)
> +
> +       /* Unrestricted M-mode accesses but enfoced on SU-mode */
> +#define SBI_DOMAIN_MEMREGION_READABLE          (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> +#define SBI_DOMAIN_MEMREGION_WRITEABLE         (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_RWX)
> +#define SBI_DOMAIN_MEMREGION_EXECUTABLE                (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> +                                                
> +SBI_DOMAIN_MEMREGION_M_RWX)
> +
> +       /* Enforced accesses across all modes */
> +#define SBI_DOMAIN_MEMREGION_ENF_READABLE      (SBI_DOMAIN_MEMREGION_SU_READABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_READABLE)
> +#define SBI_DOMAIN_MEMREGION_ENF_WRITABLE      (SBI_DOMAIN_MEMREGION_SU_WRITABLE | \
> +                                                SBI_DOMAIN_MEMREGION_M_WRITABLE)
> +#define SBI_DOMAIN_MEMREGION_ENF_EXECUTABLE    (SBI_DOMAIN_MEMREGION_SU_EXECUTABLE | \
> +                                                
> +SBI_DOMAIN_MEMREGION_M_EXECUTABLE)

Small nit: Please try to keep each line within 80 characters

It becomes difficult to build upon existing macros having large names and keep the column restrictions. Only way to honour the column restriction is  to shorten the new and exiting names. I believe it would be too much change. Moreover, 80 column limit comes from very old terminal types. So it may be relaxed to a more sane value like 100 or so. Just my thoughts.

I can make the changes if you think restricting them to 80 column makes more sense. But that would take change to existing macro names.

Regards
Himanshu

> +
> +#define SBI_DOMAIN_MEMREGION_ACCESS_MASK       (0x3fUL)
> +#define SBI_DOMAIN_MEMREGION_M_ACCESS_MASK     (0x7UL)
> +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_MASK    (0x38UL)
> +
> +#define SBI_DOMAIN_MEMREGION_SU_ACCESS_SHIFT   (3)
>
>  #define SBI_DOMAIN_MEMREGION_MMIO              (1UL << 31)
>         unsigned long flags;
> --
> 2.39.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

Otherwise, it looks good to me.

Reviewed-by: Anup Patel <anup at brainfault.org>

Regards,
Anup