Re: [PATCH 06/15] nx: add memory attribute get/set API

From: "Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com>
To: The development of GNU GRUB <grub-devel@gnu.org>
Subject: Re: [PATCH 06/15] nx: add memory attribute get/set API
Date: Fri, 24 May 2024 21:03:08 +0300	[thread overview]
Message-ID: <CAEaD8JPsbsCZeSSVcp-xJpdb9FQfU_dWp2njMFAzvRLP=GqGqQ@mail.gmail.com> (raw)
In-Reply-To: <20240524110402.203880-7-mate.kukri@canonical.com>

> +  if (!proto)
> +    return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
Here and in other places you return an error without using grub_error.
Also NOT_IMPLEMENTED_YET means that *GRUB* doesn't implement it yet,
not firmware. Also given that you ignore the return here anyway a
single bool or even void return might be more appropriate


> +  if (physaddr & 0xfff || size & 0xfff || size == 0 || attrs == NULL)
Can we not put 0xfff/0x1000 everywhere and instead use defines?
> +    {
> +      grub_dprintf ("nx", "%s called on 0x%"PRIxGRUB_ADDR"-0x%"PRIxGRUB_ADDR" and attrs %p\n",
> +                   __func__, physaddr, physaddr+size-1, attrs);
> +      return 0;
> +    }
> +
> +  efi_status = proto->get_memory_attributes(proto, physaddr, size, attrs);
> +  *attrs = uefi_mem_attrs_to_grub_mem_attrs (*attrs);
> +
> +  return grub_efi_status_to_err (efi_status);
> +}
> +
> +grub_err_t
> +grub_update_mem_attrs (grub_addr_t addr, grub_size_t size,
> +                      grub_uint64_t set_attrs, grub_uint64_t clear_attrs)
> +{
> +  grub_efi_memory_attribute_protocol_t *proto;
> +  grub_efi_physical_address_t physaddr = addr;
> +  grub_guid_t protocol_guid = GRUB_EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID;
> +  grub_efi_status_t efi_status = GRUB_EFI_SUCCESS;
> +  grub_uint64_t before = 0, after = 0, uefi_set_attrs, uefi_clear_attrs;
> +  grub_err_t err;
> +
> +  proto = grub_efi_locate_protocol (&protocol_guid, 0);
> +  if (!proto)
> +    return GRUB_ERR_NONE;
> +
> +  err = grub_get_mem_attrs (addr, size, &before);
> +  if (err)
> +    grub_dprintf ("nx", "grub_get_mem_attrs(0x%"PRIxGRUB_ADDR", %"PRIuGRUB_SIZE", %p) -> 0x%x\n",
> +                 addr, size, &before, err);
> +
> +  if (physaddr & 0xfff || size & 0xfff || size == 0)
> +    {
> +      grub_dprintf ("nx", "%s called on 0x%"PRIxGRUB_ADDR"-0x%"PRIxGRUB_ADDR" +%s%s%s -%s%s%s\n",
> +                   __func__, physaddr, physaddr + size - 1,
> +                   (set_attrs & GRUB_MEM_ATTR_R) ? "r" : "",
> +                   (set_attrs & GRUB_MEM_ATTR_W) ? "w" : "",
> +                   (set_attrs & GRUB_MEM_ATTR_X) ? "x" : "",
> +                   (clear_attrs & GRUB_MEM_ATTR_R) ? "r" : "",
> +                   (clear_attrs & GRUB_MEM_ATTR_W) ? "w" : "",
> +                   (clear_attrs & GRUB_MEM_ATTR_X) ? "x" : "");
> +      return 0;
> +    }
> +
> +  uefi_set_attrs = grub_mem_attrs_to_uefi_mem_attrs (set_attrs);
> +  grub_dprintf ("nx", "translating set_attrs from 0x%lx to 0x%lx\n", set_attrs, uefi_set_attrs);
> +  uefi_clear_attrs = grub_mem_attrs_to_uefi_mem_attrs (clear_attrs);
> +  grub_dprintf ("nx", "translating clear_attrs from 0x%lx to 0x%lx\n", clear_attrs, uefi_clear_attrs);
> +  if (uefi_set_attrs)
> +    efi_status = proto->set_memory_attributes(proto, physaddr, size, uefi_set_attrs);
> +  if (efi_status == GRUB_EFI_SUCCESS && uefi_clear_attrs)
> +    efi_status = proto->clear_memory_attributes(proto, physaddr, size, uefi_clear_attrs);
> +
> +  err = grub_get_mem_attrs (addr, size, &after);
> +  if (err)
> +    grub_dprintf ("nx", "grub_get_mem_attrs(0x%"PRIxGRUB_ADDR", %"PRIuGRUB_SIZE", %p) -> 0x%x\n",
> +                 addr, size, &after, err);
> +
> +  grub_dprintf ("nx", "set +%s%s%s -%s%s%s on 0x%"PRIxGRUB_ADDR"-0x%"PRIxGRUB_ADDR" before:%c%c%c after:%c%c%c\n",
> +               (set_attrs & GRUB_MEM_ATTR_R) ? "r" : "",
> +               (set_attrs & GRUB_MEM_ATTR_W) ? "w" : "",
> +               (set_attrs & GRUB_MEM_ATTR_X) ? "x" : "",
> +               (clear_attrs & GRUB_MEM_ATTR_R) ? "r" : "",
> +               (clear_attrs & GRUB_MEM_ATTR_W) ? "w" : "",
> +               (clear_attrs & GRUB_MEM_ATTR_X) ? "x" : "",
> +               addr, addr + size - 1,
> +               (before & GRUB_MEM_ATTR_R) ? 'r' : '-',
> +               (before & GRUB_MEM_ATTR_W) ? 'w' : '-',
> +               (before & GRUB_MEM_ATTR_X) ? 'x' : '-',
> +               (after & GRUB_MEM_ATTR_R) ? 'r' : '-',
> +               (after & GRUB_MEM_ATTR_W) ? 'w' : '-',
> +               (after & GRUB_MEM_ATTR_X) ? 'x' : '-');
> +
> +  return grub_efi_status_to_err (efi_status);
> +}
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index d44d00ad7..b686e8afe 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -379,6 +379,11 @@
>      {0xb6, 0xc7, 0x44, 0x0b, 0x29, 0xbb, 0x8c, 0x4f } \
>    }
>
> +#define GRUB_EFI_MEMORY_ATTRIBUTE_PROTOCOL_GUID \
> +  { 0xf4560cf6, 0x40ec, 0x4b4a, \
> +    { 0xa1, 0x92, 0xbf, 0x1d, 0x57, 0xd0, 0xb1, 0x89 } \
> +  }
> +
>  struct grub_efi_sal_system_table
>  {
>    grub_uint32_t signature;
> @@ -1815,4 +1820,24 @@ struct initrd_media_device_path {
>  } GRUB_PACKED;
>  typedef struct initrd_media_device_path initrd_media_device_path_t;
>
> +struct grub_efi_memory_attribute_protocol
> +{
> +  grub_efi_status_t (__grub_efi_api *get_memory_attributes) (
> +                           struct grub_efi_memory_attribute_protocol *this,
> +                           grub_efi_physical_address_t base_address,
> +                           grub_efi_uint64_t length,
> +                           grub_efi_uint64_t *attributes);
> +  grub_efi_status_t (__grub_efi_api *set_memory_attributes) (
> +                           struct grub_efi_memory_attribute_protocol *this,
> +                           grub_efi_physical_address_t base_address,
> +                           grub_efi_uint64_t length,
> +                           grub_efi_uint64_t attributes);
> +  grub_efi_status_t (__grub_efi_api *clear_memory_attributes) (
> +                           struct grub_efi_memory_attribute_protocol *this,
> +                           grub_efi_physical_address_t base_address,
> +                           grub_efi_uint64_t length,
> +                           grub_efi_uint64_t attributes);
> +};
> +typedef struct grub_efi_memory_attribute_protocol grub_efi_memory_attribute_protocol_t;
> +
>  #endif /* ! GRUB_EFI_API_HEADER */
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index a5cd99e5a..194adc1f7 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -151,4 +151,6 @@ struct grub_net_card;
>  grub_efi_handle_t
>  grub_efinet_get_device_handle (struct grub_net_card *card);
>
> +grub_err_t EXPORT_FUNC(grub_efi_status_to_err) (grub_efi_status_t status);
> +
>  #endif /* ! GRUB_EFI_EFI_HEADER */
> diff --git a/include/grub/mm.h b/include/grub/mm.h
> index f3bf87fa0..8ee1fc717 100644
> --- a/include/grub/mm.h
> +++ b/include/grub/mm.h
> @@ -23,6 +23,7 @@
>  #include <grub/err.h>
>  #include <grub/types.h>
>  #include <grub/symbol.h>
> +#include <grub/err.h>
>  #include <config.h>
>
>  #ifndef NULL
> @@ -56,6 +57,37 @@ void *EXPORT_FUNC(grub_realloc) (void *ptr, grub_size_t size);
>  void *EXPORT_FUNC(grub_memalign) (grub_size_t align, grub_size_t size);
>  #endif
>
> +#define GRUB_MEM_ATTR_R        0x0000000000000004LLU
> +#define GRUB_MEM_ATTR_W        0x0000000000000002LLU
> +#define GRUB_MEM_ATTR_X        0x0000000000000001LLU
> +
> +#ifdef GRUB_MACHINE_EFI
> +grub_err_t EXPORT_FUNC(grub_get_mem_attrs) (grub_addr_t addr,
> +                                           grub_size_t size,
> +                                           grub_uint64_t *attrs);
> +grub_err_t EXPORT_FUNC(grub_update_mem_attrs) (grub_addr_t addr,
> +                                              grub_size_t size,
> +                                              grub_uint64_t set_attrs,
> +                                              grub_uint64_t clear_attrs);
> +#else /* !GRUB_MACHINE_EFI */
> +static inline grub_err_t
> +grub_get_mem_attrs (grub_addr_t addr __attribute__((__unused__)),
> +                   grub_size_t size __attribute__((__unused__)),
> +                   grub_uint64_t *attrs __attribute__((__unused__)))
> +{
> +  return GRUB_ERR_NONE;
> +}
> +
> +static inline grub_err_t
> +grub_update_mem_attrs (grub_addr_t addr __attribute__((__unused__)),
> +                      grub_size_t size __attribute__((__unused__)),
> +                      grub_uint64_t set_attrs __attribute__((__unused__)),
> +                      grub_uint64_t clear_attrs __attribute__((__unused__)))
> +{
> +  return GRUB_ERR_NONE;
> +}
> +#endif /* GRUB_MACHINE_EFI */
> +
>  void grub_mm_check_real (const char *file, int line);
>  #define grub_mm_check() grub_mm_check_real (GRUB_FILE, __LINE__);
>
> --
> 2.39.2
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'phcoder' Serbinenko

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel