On 5/14/20 8:50 PM, Michael Walle wrote: > Am 2020-05-14 20:35, schrieb Heinrich Schuchardt: >> On 5/14/20 2:38 PM, Michael Walle wrote: >>> The first argument has to be aligned with EFI_PAGE_SIZE. This alignment >>> is already checked for external callers but it is not checked for >>> internal callers. Unfortunately, most of the time the return value is >>> not checked, so scream loud and clear. >> >> Why do you mention the return value here? > > most callers just ignore the return value. so if not aligned this will > silently fail. > >>> >>> Signed-off-by: Michael Walle <[email protected]> >>> --- >>> lib/efi_loader/efi_memory.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>> index fd79178da9..b56e19cb30 100644 >>> --- a/lib/efi_loader/efi_memory.c >>> +++ b/lib/efi_loader/efi_memory.c >>> @@ -248,6 +248,9 @@ efi_status_t efi_add_memory_map(uint64_t start, >>> uint64_t pages, int memory_type, >>> EFI_PRINT("%s: 0x%llx 0x%llx %d %s\n", __func__, >>> start, pages, memory_type, overlap_only_ram ? "yes" : "no"); >>> >>> + if (start & EFI_PAGE_MASK) >>> + panic("%s: start not aligned\n", __func__); >>> + >> >> Did you find any internal caller that has a problem? > See next patch. > >> We do not want to increase code size. > Mh, even within the efi_loader? Well I could do a > > if (start & EFI_PAGE_MASK) { > debug("%s: start not aligned\n", __func__); > return EFI_INVALID_PARAMETER; > } > > but as I said, nobody checks the return value.
The Linux kernel has this nice code comment: "Don't use BUG() or BUG_ON() unless there's really no way out;" Looking at the different internal callers, some of these actively ensure alignment others don't. Wouldn't it make more sense to move all rounding into a common function taking a start address and a size in bytes as arguments. Best regards Heinrich > >
