Re: [PATCH v2 6/8] i386/memory: Define GRUB_PAGE_MASK constant and GRUB_PAGE_{UP, DOWN} macros

[Frediano Ziglio]

On Thu, Sep 19, 2024 at 11:03 PM Sergii Dmytruk
<sergii.dmytruk@3mdeb.com> wrote:
>
> From: Krystian Hebel <krystian.hebel@3mdeb.com>
>
> Subsequent patches will use those macros and constant.
>

Minor, but "Define GRUB_PAGE_MASK constant and GRUB_PAGE_{UP, DOWN}
macros" subject sounds a bit confusing to me. I mean, at the end they
are all defined as macros so why mixing constant and macros?

> Signed-off-by: Krystian Hebel <krystian.hebel@3mdeb.com>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
> ---
>  include/grub/i386/memory.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/grub/i386/memory.h b/include/grub/i386/memory.h
> index c64529630..56f64855b 100644
> --- a/include/grub/i386/memory.h
> +++ b/include/grub/i386/memory.h
> @@ -22,6 +22,7 @@
>
>  #define GRUB_PAGE_SHIFT                12
>  #define GRUB_PAGE_SIZE         (1UL << GRUB_PAGE_SHIFT)
> +#define GRUB_PAGE_MASK         (~(GRUB_PAGE_SIZE - 1))
>

Why not use signed numbers so it could extend as needed?

>  /* The flag for protected mode.  */
>  #define GRUB_MEMORY_CPU_CR0_PE_ON              0x1
> @@ -43,8 +44,12 @@
>
>  #define GRUB_MMAP_MALLOC_LOW 1
>
> +#include <grub/misc.h>
>  #include <grub/types.h>
>
> +#define GRUB_PAGE_UP(p)                ALIGN_UP (p, GRUB_PAGE_SIZE)
> +#define GRUB_PAGE_DOWN(p)      ALIGN_DOWN (p, GRUB_PAGE_SIZE)
> +
>  struct grub_e820_mmap_entry
>  {
>    grub_uint64_t addr;

Frediano