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

[Sergii Dmytruk]

On Fri, Sep 20, 2024 at 02:40:22PM +0100, Frediano Ziglio via Grub-devel wrote:
> 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?

The latter two are function-like macros while the first one isn't and
always expands to the same value.  That's how I interpreted the title.

> > 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?

I don't think signed numbers were avoided on purpose, masks just tend to
be unsigned (e.g., to avoid UB on shifts).  Can do

    #define GRUB_PAGE_MASK         (~((1L << GRUB_PAGE_SHIFT) - 1))

instead.

> >  /* 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

Sergii