The EFI System Resource Table (ESRT) is necessary for fwupd to identify
firmware updates to install.  According to the UEFI specification §23.4,
the ESRT shall be stored in memory of type EfiBootServicesData.  However,
memory of type EfiBootServicesData is considered general-purpose memory
by Xen, so the ESRT needs to be moved somewhere where Xen will not
overwrite it.  Copy the ESRT to memory of type EfiRuntimeServicesData,
which Xen will not reuse.  dom0 can use the ESRT if (and only if) it is
in memory of type EfiRuntimeServicesData.

Earlier versions of this patch reserved the memory in which the ESRT was
located.  This created awkward alignment problems, and required either
splitting the E820 table or wasting memory.  It also would have required
a new platform op for dom0 to use to indicate if the ESRT is reserved.
By copying the ESRT into EfiRuntimeServicesData memory, the E820 table
does not need to be modified, and dom0 can just check the type of the
memory region containing the ESRT.  The copy is only done if the ESRT is
not already in EfiRuntimeServicesData memory, avoiding memory leaks on
repeated kexec.

See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
for details.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/common/efi/boot.c | 134 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 134 insertions(+)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index a25e1d29f1..f6f34aa816 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -39,6 +39,26 @@
   { 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }
 #define APPLE_PROPERTIES_PROTOCOL_GUID \
   { 0x91bd12fe, 0xf6c3, 0x44fb, { 0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} }
+#define EFI_SYSTEM_RESOURCE_TABLE_GUID    \
+  { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
+#define EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION 1
+
+typedef struct {
+    EFI_GUID FwClass;
+    UINT32 FwType;
+    UINT32 FwVersion;
+    UINT32 LowestSupportedFwVersion;
+    UINT32 CapsuleFlags;
+    UINT32 LastAttemptVersion;
+    UINT32 LastAttemptStatus;
+} EFI_SYSTEM_RESOURCE_ENTRY;
+
+typedef struct {
+    UINT32 FwResourceCount;
+    UINT32 FwResourceCountMax;
+    UINT64 FwResourceVersion;
+    EFI_SYSTEM_RESOURCE_ENTRY Entries[];
+} EFI_SYSTEM_RESOURCE_TABLE;
 
 typedef EFI_STATUS
 (/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) (
@@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
 }
 #endif
 
+static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
+
+static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
+{
+    size_t available_len, len;
+    const UINTN physical_start = desc->PhysicalStart;
+    const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
+
+    len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+    if ( esrt == EFI_INVALID_TABLE_ADDR )
+        return 0;
+    if ( physical_start > esrt || esrt - physical_start >= len )
+        return 0;
+    /*
+     * The specification requires EfiBootServicesData, but accept
+     * EfiRuntimeServicesData, which is a more logical choice.
+     */
+    if ( (desc->Type != EfiRuntimeServicesData) &&
+         (desc->Type != EfiBootServicesData) )
+        return 0;
+    available_len = len - (esrt - physical_start);
+    if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
+        return 0;
+    available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
+    esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
+    if ( (esrt_ptr->FwResourceVersion !=
+          EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION) ||
+         !esrt_ptr->FwResourceCount )
+        return 0;
+    if ( esrt_ptr->FwResourceCount > available_len / sizeof(esrt_ptr->Entries[0]) )
+        return 0;
+
+    return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
+}
+
 /*
  * Include architecture specific implementation here, which references the
  * static globals defined above.
@@ -845,6 +900,8 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
     return gop_mode;
 }
 
+static EFI_GUID __initdata esrt_guid = EFI_SYSTEM_RESOURCE_TABLE_GUID;
+
 static void __init efi_tables(void)
 {
     unsigned int i;
@@ -868,6 +925,8 @@ static void __init efi_tables(void)
             efi.smbios = (unsigned long)efi_ct[i].VendorTable;
         if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) )
             efi.smbios3 = (unsigned long)efi_ct[i].VendorTable;
+        if ( match_guid(&esrt_guid, &efi_ct[i].VendorGuid) )
+            esrt = (UINTN)efi_ct[i].VendorTable;
     }
 
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
@@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
 #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
 
+static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
+{
+    EFI_STATUS status;
+    UINTN info_size = 0, map_key, mdesc_size;
+    void *memory_map = NULL;
+    UINT32 ver;
+    unsigned int i;
+
+    for ( ; ; ) {
+        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
+                                      &mdesc_size, &ver);
+        if ( status == EFI_SUCCESS && memory_map != NULL )
+            break;
+        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
+        {
+            info_size += 8 * efi_mdesc_size;
+            if ( memory_map != NULL )
+                efi_bs->FreePool(memory_map);
+            memory_map = NULL;
+            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
+            if ( status == EFI_SUCCESS )
+                continue;
+            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
+        }
+        else
+            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
+        return;
+    }
+
+    /* Try to obtain the ESRT.  Errors are not fatal. */
+    for ( i = 0; i < info_size; i += efi_mdesc_size )
+    {
+        /*
+         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
+         * so that the memory it is in will not be used for other purposes.
+         */
+        void *new_esrt = NULL;
+        size_t esrt_size = get_esrt_size(efi_memmap + i);
+
+        if ( !esrt_size )
+            continue;
+        if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
+             EfiRuntimeServicesData )
+            break; /* ESRT already safe from reuse */
+        status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
+                                      &new_esrt);
+        if ( status == EFI_SUCCESS && new_esrt )
+        {
+            memcpy(new_esrt, (void *)esrt, esrt_size);
+            status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
+            if ( status != EFI_SUCCESS )
+            {
+                PrintErr(L"Cannot install new ESRT\r\n");
+                efi_bs->FreePool(new_esrt);
+            }
+        }
+        else
+            PrintErr(L"Cannot allocate memory for ESRT\r\n");
+        break;
+    }
+
+    efi_bs->FreePool(memory_map);
+}
+
 static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 {
     EFI_STATUS status;
@@ -1413,6 +1536,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     if ( gop )
         efi_set_gop_mode(gop, gop_mode);
 
+    efi_relocate_esrt(SystemTable);
+
     efi_exit_boot(ImageHandle, SystemTable);
 
     efi_arch_post_exit_boot(); /* Doesn't return. */
@@ -1753,3 +1878,12 @@ void __init efi_init_memory(void)
     unmap_domain_page(efi_l4t);
 }
 #endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

+ CC Julien Grall

> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
> 
> The EFI System Resource Table (ESRT) is necessary for fwupd to identify
> firmware updates to install.  According to the UEFI specification §23.4,
> the ESRT shall be stored in memory of type EfiBootServicesData.  However,
> memory of type EfiBootServicesData is considered general-purpose memory
> by Xen, so the ESRT needs to be moved somewhere where Xen will not
> overwrite it.  Copy the ESRT to memory of type EfiRuntimeServicesData,
> which Xen will not reuse.  dom0 can use the ESRT if (and only if) it is
> in memory of type EfiRuntimeServicesData.
> 
> Earlier versions of this patch reserved the memory in which the ESRT was
> located.  This created awkward alignment problems, and required either
> splitting the E820 table or wasting memory.  It also would have required
> a new platform op for dom0 to use to indicate if the ESRT is reserved.
> By copying the ESRT into EfiRuntimeServicesData memory, the E820 table
> does not need to be modified, and dom0 can just check the type of the
> memory region containing the ESRT.  The copy is only done if the ESRT is
> not already in EfiRuntimeServicesData memory, avoiding memory leaks on
> repeated kexec.
> 
> See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
> for details.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
> xen/common/efi/boot.c | 134 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 134 insertions(+)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index a25e1d29f1..f6f34aa816 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -39,6 +39,26 @@
>   { 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }
> #define APPLE_PROPERTIES_PROTOCOL_GUID \
>   { 0x91bd12fe, 0xf6c3, 0x44fb, { 0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} }
> +#define EFI_SYSTEM_RESOURCE_TABLE_GUID    \
> +  { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
> +#define EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION 1
> +
> +typedef struct {
> +    EFI_GUID FwClass;
> +    UINT32 FwType;
> +    UINT32 FwVersion;
> +    UINT32 LowestSupportedFwVersion;
> +    UINT32 CapsuleFlags;
> +    UINT32 LastAttemptVersion;
> +    UINT32 LastAttemptStatus;
> +} EFI_SYSTEM_RESOURCE_ENTRY;
> +
> +typedef struct {
> +    UINT32 FwResourceCount;
> +    UINT32 FwResourceCountMax;
> +    UINT64 FwResourceVersion;
> +    EFI_SYSTEM_RESOURCE_ENTRY Entries[];
> +} EFI_SYSTEM_RESOURCE_TABLE;
> 
> typedef EFI_STATUS
> (/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) (
> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> }
> #endif
> 
> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
> +
> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> +{
> +    size_t available_len, len;
> +    const UINTN physical_start = desc->PhysicalStart;

Hi,

From my tests on an arm64 machine so far I can tell that desc is NULL here,
for this reason we have the problem.

I’ll have a further look on the cause of this, but if you are faster than me you are
welcome to give your opinion on that.

Cheers,
Luca

> +    const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
> +
> +    len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> +    if ( esrt == EFI_INVALID_TABLE_ADDR )
> +        return 0;
> +    if ( physical_start > esrt || esrt - physical_start >= len )
> +        return 0;
> +    /*
> +     * The specification requires EfiBootServicesData, but accept
> +     * EfiRuntimeServicesData, which is a more logical choice.
> +     */
> +    if ( (desc->Type != EfiRuntimeServicesData) &&
> +         (desc->Type != EfiBootServicesData) )
> +        return 0;
> +    available_len = len - (esrt - physical_start);
> +    if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
> +        return 0;
> +    available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
> +    esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
> +    if ( (esrt_ptr->FwResourceVersion !=
> +          EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION) ||
> +         !esrt_ptr->FwResourceCount )
> +        return 0;
> +    if ( esrt_ptr->FwResourceCount > available_len / sizeof(esrt_ptr->Entries[0]) )
> +        return 0;
> +
> +    return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
> +}
> +
> /*
>  * Include architecture specific implementation here, which references the
>  * static globals defined above.
> @@ -845,6 +900,8 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>     return gop_mode;
> }
> 
> +static EFI_GUID __initdata esrt_guid = EFI_SYSTEM_RESOURCE_TABLE_GUID;
> +
> static void __init efi_tables(void)
> {
>     unsigned int i;
> @@ -868,6 +925,8 @@ static void __init efi_tables(void)
>             efi.smbios = (unsigned long)efi_ct[i].VendorTable;
>         if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) )
>             efi.smbios3 = (unsigned long)efi_ct[i].VendorTable;
> +        if ( match_guid(&esrt_guid, &efi_ct[i].VendorGuid) )
> +            esrt = (UINTN)efi_ct[i].VendorTable;
>     }
> 
> #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
> 
> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> +{
> +    EFI_STATUS status;
> +    UINTN info_size = 0, map_key, mdesc_size;
> +    void *memory_map = NULL;
> +    UINT32 ver;
> +    unsigned int i;
> +
> +    for ( ; ; ) {
> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
> +                                      &mdesc_size, &ver);
> +        if ( status == EFI_SUCCESS && memory_map != NULL )
> +            break;
> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
> +        {
> +            info_size += 8 * efi_mdesc_size;
> +            if ( memory_map != NULL )
> +                efi_bs->FreePool(memory_map);
> +            memory_map = NULL;
> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
> +            if ( status == EFI_SUCCESS )
> +                continue;
> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
> +        }
> +        else
> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
> +        return;
> +    }
> +
> +    /* Try to obtain the ESRT.  Errors are not fatal. */
> +    for ( i = 0; i < info_size; i += efi_mdesc_size )
> +    {
> +        /*
> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
> +         * so that the memory it is in will not be used for other purposes.
> +         */
> +        void *new_esrt = NULL;
> +        size_t esrt_size = get_esrt_size(efi_memmap + i);
> +
> +        if ( !esrt_size )
> +            continue;
> +        if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
> +             EfiRuntimeServicesData )
> +            break; /* ESRT already safe from reuse */
> +        status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> +                                      &new_esrt);
> +        if ( status == EFI_SUCCESS && new_esrt )
> +        {
> +            memcpy(new_esrt, (void *)esrt, esrt_size);
> +            status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
> +            if ( status != EFI_SUCCESS )
> +            {
> +                PrintErr(L"Cannot install new ESRT\r\n");
> +                efi_bs->FreePool(new_esrt);
> +            }
> +        }
> +        else
> +            PrintErr(L"Cannot allocate memory for ESRT\r\n");
> +        break;
> +    }
> +
> +    efi_bs->FreePool(memory_map);
> +}
> +
> static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
> {
>     EFI_STATUS status;
> @@ -1413,6 +1536,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>     if ( gop )
>         efi_set_gop_mode(gop, gop_mode);
> 
> +    efi_relocate_esrt(SystemTable);
> +
>     efi_exit_boot(ImageHandle, SystemTable);
> 
>     efi_arch_post_exit_boot(); /* Doesn't return. */
> @@ -1753,3 +1878,12 @@ void __init efi_init_memory(void)
>     unmap_domain_page(efi_l4t);
> }
> #endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
> 

On 06/07/2022 11:32, Luca Fancellu wrote:
>> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
>>
>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>> index a25e1d29f1..f6f34aa816 100644
>> --- a/xen/common/efi/boot.c
>> +++ b/xen/common/efi/boot.c
>> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
>> }
>> #endif
>>
>> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
>> +
>> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
>> +{
>> +    size_t available_len, len;
>> +    const UINTN physical_start = desc->PhysicalStart;
> Hi,
>
> From my tests on an arm64 machine so far I can tell that desc is NULL here,
> for this reason we have the problem.
>
> I’ll have a further look on the cause of this, but if you are faster than me you are
> welcome to give your opinion on that.

Given this observation, there's clearly ...

> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>
> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> +{
> +    EFI_STATUS status;
> +    UINTN info_size = 0, map_key, mdesc_size;
> +    void *memory_map = NULL;
> +    UINT32 ver;
> +    unsigned int i;
> +
> +    for ( ; ; ) {
> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
> +                                      &mdesc_size, &ver);
> +        if ( status == EFI_SUCCESS && memory_map != NULL )
> +            break;
> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
> +        {
> +            info_size += 8 * efi_mdesc_size;
> +            if ( memory_map != NULL )
> +                efi_bs->FreePool(memory_map);
> +            memory_map = NULL;
> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
> +            if ( status == EFI_SUCCESS )
> +                continue;
> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
> +        }
> +        else
> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
> +        return;
> +    }
> +
> +    /* Try to obtain the ESRT.  Errors are not fatal. */
> +    for ( i = 0; i < info_size; i += efi_mdesc_size )
> +    {
> +        /*
> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
> +         * so that the memory it is in will not be used for other purposes.
> +         */
> +        void *new_esrt = NULL;
> +        size_t esrt_size = get_esrt_size(efi_memmap + i);

... a NULL pointer here.  And the only way that could happen is if
efi_memmap is NULL.

Which perhaps isn't surprising because presumably ARM gets memory
information from the DT, not EFI?

~Andrew

Hi Andrew,

On 06/07/2022 11:44, Andrew Cooper wrote:
> On 06/07/2022 11:32, Luca Fancellu wrote:
>> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>>                                   (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>>
>> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
>> +{
>> +    EFI_STATUS status;
>> +    UINTN info_size = 0, map_key, mdesc_size;
>> +    void *memory_map = NULL;
>> +    UINT32 ver;
>> +    unsigned int i;
>> +
>> +    for ( ; ; ) {
>> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
>> +                                      &mdesc_size, &ver);
>> +        if ( status == EFI_SUCCESS && memory_map != NULL )
>> +            break;
>> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
>> +        {
>> +            info_size += 8 * efi_mdesc_size;
>> +            if ( memory_map != NULL )
>> +                efi_bs->FreePool(memory_map);
>> +            memory_map = NULL;
>> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
>> +            if ( status == EFI_SUCCESS )
>> +                continue;
>> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
>> +        }
>> +        else
>> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
>> +        return;
>> +    }
>> +
>> +    /* Try to obtain the ESRT.  Errors are not fatal. */
>> +    for ( i = 0; i < info_size; i += efi_mdesc_size )
>> +    {
>> +        /*
>> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
>> +         * so that the memory it is in will not be used for other purposes.
>> +         */
>> +        void *new_esrt = NULL;
>> +        size_t esrt_size = get_esrt_size(efi_memmap + i);
> 
> ... a NULL pointer here.  And the only way that could happen is if
> efi_memmap is NULL.
> 
> Which perhaps isn't surprising because presumably ARM gets memory
> information from the DT, not EFI?

We are always using the EFI memory map on Arm. The information from the 
DT used to be removed, but now kept just to get the NUMA information 
outside of the EFI stub.

Cheers,

-- 
Julien Grall

On 06.07.2022 12:44, Andrew Cooper wrote:
> On 06/07/2022 11:32, Luca Fancellu wrote:
>>> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
>>>
>>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>>> index a25e1d29f1..f6f34aa816 100644
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
>>> }
>>> #endif
>>>
>>> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
>>> +
>>> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
>>> +{
>>> +    size_t available_len, len;
>>> +    const UINTN physical_start = desc->PhysicalStart;
>> Hi,
>>
>> From my tests on an arm64 machine so far I can tell that desc is NULL here,
>> for this reason we have the problem.
>>
>> I’ll have a further look on the cause of this, but if you are faster than me you are
>> welcome to give your opinion on that.
> 
> Given this observation, there's clearly ...
> 
>> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>>                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>>
>> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
>> +{
>> +    EFI_STATUS status;
>> +    UINTN info_size = 0, map_key, mdesc_size;
>> +    void *memory_map = NULL;
>> +    UINT32 ver;
>> +    unsigned int i;
>> +
>> +    for ( ; ; ) {
>> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
>> +                                      &mdesc_size, &ver);
>> +        if ( status == EFI_SUCCESS && memory_map != NULL )
>> +            break;
>> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
>> +        {
>> +            info_size += 8 * efi_mdesc_size;
>> +            if ( memory_map != NULL )
>> +                efi_bs->FreePool(memory_map);
>> +            memory_map = NULL;
>> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
>> +            if ( status == EFI_SUCCESS )
>> +                continue;
>> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
>> +        }
>> +        else
>> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
>> +        return;
>> +    }
>> +
>> +    /* Try to obtain the ESRT.  Errors are not fatal. */
>> +    for ( i = 0; i < info_size; i += efi_mdesc_size )
>> +    {
>> +        /*
>> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
>> +         * so that the memory it is in will not be used for other purposes.
>> +         */
>> +        void *new_esrt = NULL;
>> +        size_t esrt_size = get_esrt_size(efi_memmap + i);
> 
> ... a NULL pointer here.  And the only way that could happen is if
> efi_memmap is NULL.

Incomplete refactoring - this needs to be memory_map, not efi_memmap.
Of course efi_mdesc_size also needs to be mdesc_size. Didn't check
for further leftover from the earlier patch version. I'm going to
revert the commit.

Jan

On Wed, Jul 06, 2022 at 12:55:50PM +0200, Jan Beulich wrote:
> On 06.07.2022 12:44, Andrew Cooper wrote:
> > On 06/07/2022 11:32, Luca Fancellu wrote:
> >>> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
> >>>
> >>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> >>> index a25e1d29f1..f6f34aa816 100644
> >>> --- a/xen/common/efi/boot.c
> >>> +++ b/xen/common/efi/boot.c
> >>> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> >>> }
> >>> #endif
> >>>
> >>> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
> >>> +
> >>> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> >>> +{
> >>> +    size_t available_len, len;
> >>> +    const UINTN physical_start = desc->PhysicalStart;
> >> Hi,
> >>
> >> From my tests on an arm64 machine so far I can tell that desc is NULL here,
> >> for this reason we have the problem.
> >>
> >> I’ll have a further look on the cause of this, but if you are faster than me you are
> >> welcome to give your opinion on that.
> > 
> > Given this observation, there's clearly ...
> > 
> >> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
> >> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
> >>                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
> >>
> >> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> >> +{
> >> +    EFI_STATUS status;
> >> +    UINTN info_size = 0, map_key, mdesc_size;
> >> +    void *memory_map = NULL;
> >> +    UINT32 ver;
> >> +    unsigned int i;
> >> +
> >> +    for ( ; ; ) {
> >> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
> >> +                                      &mdesc_size, &ver);
> >> +        if ( status == EFI_SUCCESS && memory_map != NULL )
> >> +            break;
> >> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
> >> +        {
> >> +            info_size += 8 * efi_mdesc_size;
> >> +            if ( memory_map != NULL )
> >> +                efi_bs->FreePool(memory_map);
> >> +            memory_map = NULL;
> >> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
> >> +            if ( status == EFI_SUCCESS )
> >> +                continue;
> >> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
> >> +        }
> >> +        else
> >> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
> >> +        return;
> >> +    }
> >> +
> >> +    /* Try to obtain the ESRT.  Errors are not fatal. */
> >> +    for ( i = 0; i < info_size; i += efi_mdesc_size )
> >> +    {
> >> +        /*
> >> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
> >> +         * so that the memory it is in will not be used for other purposes.
> >> +         */
> >> +        void *new_esrt = NULL;
> >> +        size_t esrt_size = get_esrt_size(efi_memmap + i);
> > 
> > ... a NULL pointer here.  And the only way that could happen is if
> > efi_memmap is NULL.
> 
> Incomplete refactoring - this needs to be memory_map, not efi_memmap.
> Of course efi_mdesc_size also needs to be mdesc_size. Didn't check
> for further leftover from the earlier patch version. I'm going to
> revert the commit.

Sorry about that.  Want me to submit a v9?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

On 06.07.2022 12:58, Demi Marie Obenour wrote:
> On Wed, Jul 06, 2022 at 12:55:50PM +0200, Jan Beulich wrote:
>> On 06.07.2022 12:44, Andrew Cooper wrote:
>>> On 06/07/2022 11:32, Luca Fancellu wrote:
>>>>> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
>>>>>
>>>>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>>>>> index a25e1d29f1..f6f34aa816 100644
>>>>> --- a/xen/common/efi/boot.c
>>>>> +++ b/xen/common/efi/boot.c
>>>>> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
>>>>> }
>>>>> #endif
>>>>>
>>>>> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
>>>>> +
>>>>> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
>>>>> +{
>>>>> +    size_t available_len, len;
>>>>> +    const UINTN physical_start = desc->PhysicalStart;
>>>> Hi,
>>>>
>>>> From my tests on an arm64 machine so far I can tell that desc is NULL here,
>>>> for this reason we have the problem.
>>>>
>>>> I’ll have a further look on the cause of this, but if you are faster than me you are
>>>> welcome to give your opinion on that.
>>>
>>> Given this observation, there's clearly ...
>>>
>>>> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>>>> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>>>>                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>>>>
>>>> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
>>>> +{
>>>> +    EFI_STATUS status;
>>>> +    UINTN info_size = 0, map_key, mdesc_size;
>>>> +    void *memory_map = NULL;
>>>> +    UINT32 ver;
>>>> +    unsigned int i;
>>>> +
>>>> +    for ( ; ; ) {
>>>> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
>>>> +                                      &mdesc_size, &ver);
>>>> +        if ( status == EFI_SUCCESS && memory_map != NULL )
>>>> +            break;
>>>> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
>>>> +        {
>>>> +            info_size += 8 * efi_mdesc_size;
>>>> +            if ( memory_map != NULL )
>>>> +                efi_bs->FreePool(memory_map);
>>>> +            memory_map = NULL;
>>>> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
>>>> +            if ( status == EFI_SUCCESS )
>>>> +                continue;
>>>> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
>>>> +        }
>>>> +        else
>>>> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* Try to obtain the ESRT.  Errors are not fatal. */
>>>> +    for ( i = 0; i < info_size; i += efi_mdesc_size )
>>>> +    {
>>>> +        /*
>>>> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
>>>> +         * so that the memory it is in will not be used for other purposes.
>>>> +         */
>>>> +        void *new_esrt = NULL;
>>>> +        size_t esrt_size = get_esrt_size(efi_memmap + i);
>>>
>>> ... a NULL pointer here.  And the only way that could happen is if
>>> efi_memmap is NULL.
>>
>> Incomplete refactoring - this needs to be memory_map, not efi_memmap.
>> Of course efi_mdesc_size also needs to be mdesc_size. Didn't check
>> for further leftover from the earlier patch version. I'm going to
>> revert the commit.
> 
> Sorry about that.  Want me to submit a v9?

Yes please. And please make sure you have tested this at least on x86.

Jan

> On 6 Jul 2022, at 11:59, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 06.07.2022 12:58, Demi Marie Obenour wrote:
>> On Wed, Jul 06, 2022 at 12:55:50PM +0200, Jan Beulich wrote:
>>> On 06.07.2022 12:44, Andrew Cooper wrote:
>>>> On 06/07/2022 11:32, Luca Fancellu wrote:
>>>>>> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
>>>>>> 
>>>>>> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
>>>>>> index a25e1d29f1..f6f34aa816 100644
>>>>>> --- a/xen/common/efi/boot.c
>>>>>> +++ b/xen/common/efi/boot.c
>>>>>> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
>>>>>> }
>>>>>> #endif
>>>>>> 
>>>>>> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
>>>>>> +
>>>>>> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
>>>>>> +{
>>>>>> + size_t available_len, len;
>>>>>> + const UINTN physical_start = desc->PhysicalStart;
>>>>> Hi,
>>>>> 
>>>>> From my tests on an arm64 machine so far I can tell that desc is NULL here,
>>>>> for this reason we have the problem.
>>>>> 
>>>>> I’ll have a further look on the cause of this, but if you are faster than me you are
>>>>> welcome to give your opinion on that.
>>>> 
>>>> Given this observation, there's clearly ...
>>>> 
>>>>> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>>>>> #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>>>>> (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>>>>> 
>>>>> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
>>>>> +{
>>>>> + EFI_STATUS status;
>>>>> + UINTN info_size = 0, map_key, mdesc_size;
>>>>> + void *memory_map = NULL;
>>>>> + UINT32 ver;
>>>>> + unsigned int i;
>>>>> +
>>>>> + for ( ; ; ) {
>>>>> + status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
>>>>> + &mdesc_size, &ver);
>>>>> + if ( status == EFI_SUCCESS && memory_map != NULL )
>>>>> + break;
>>>>> + if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
>>>>> + {
>>>>> + info_size += 8 * efi_mdesc_size;
>>>>> + if ( memory_map != NULL )
>>>>> + efi_bs->FreePool(memory_map);
>>>>> + memory_map = NULL;
>>>>> + status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
>>>>> + if ( status == EFI_SUCCESS )
>>>>> + continue;
>>>>> + PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
>>>>> + }
>>>>> + else
>>>>> + PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + /* Try to obtain the ESRT. Errors are not fatal. */
>>>>> + for ( i = 0; i < info_size; i += efi_mdesc_size )
>>>>> + {
>>>>> + /*
>>>>> + * ESRT needs to be moved to memory of type EfiRuntimeServicesData
>>>>> + * so that the memory it is in will not be used for other purposes.
>>>>> + */
>>>>> + void *new_esrt = NULL;
>>>>> + size_t esrt_size = get_esrt_size(efi_memmap + i);
>>>> 
>>>> ... a NULL pointer here.  And the only way that could happen is if
>>>> efi_memmap is NULL.
>>> 
>>> Incomplete refactoring - this needs to be memory_map, not efi_memmap.
>>> Of course efi_mdesc_size also needs to be mdesc_size. Didn't check
>>> for further leftover from the earlier patch version. I'm going to
>>> revert the commit.
>> 
>> Sorry about that. Want me to submit a v9?
> 
> Yes please. And please make sure you have tested this at least on x86.

I will test it on arm64, so please ping me if I miss it, I got submerged by ML mails
after my holidays and I didn’t test (on arm) this one like the previous version that
you sent before

Cheers,
Luca

> 
> Jan

On Wed, Jul 06, 2022 at 10:44:46AM +0000, Andrew Cooper wrote:
> On 06/07/2022 11:32, Luca Fancellu wrote:
> >> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@invisiblethingslab.com> wrote:
> >>
> >> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> >> index a25e1d29f1..f6f34aa816 100644
> >> --- a/xen/common/efi/boot.c
> >> +++ b/xen/common/efi/boot.c
> >> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> >> }
> >> #endif
> >>
> >> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
> >> +
> >> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> >> +{
> >> +    size_t available_len, len;
> >> +    const UINTN physical_start = desc->PhysicalStart;
> > Hi,
> >
> > From my tests on an arm64 machine so far I can tell that desc is NULL here,
> > for this reason we have the problem.
> >
> > I’ll have a further look on the cause of this, but if you are faster than me you are
> > welcome to give your opinion on that.
> 
> Given this observation, there's clearly ...
> 
> > @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
> > #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
> >                                  (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
> >
> > +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> > +{
> > +    EFI_STATUS status;
> > +    UINTN info_size = 0, map_key, mdesc_size;
> > +    void *memory_map = NULL;
> > +    UINT32 ver;
> > +    unsigned int i;
> > +
> > +    for ( ; ; ) {
> > +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
> > +                                      &mdesc_size, &ver);
> > +        if ( status == EFI_SUCCESS && memory_map != NULL )
> > +            break;
> > +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
> > +        {
> > +            info_size += 8 * efi_mdesc_size;
> > +            if ( memory_map != NULL )
> > +                efi_bs->FreePool(memory_map);
> > +            memory_map = NULL;
> > +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, &memory_map);
> > +            if ( status == EFI_SUCCESS )
> > +                continue;
> > +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
> > +        }
> > +        else
> > +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
> > +        return;
> > +    }
> > +
> > +    /* Try to obtain the ESRT.  Errors are not fatal. */
> > +    for ( i = 0; i < info_size; i += efi_mdesc_size )
> > +    {
> > +        /*
> > +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
> > +         * so that the memory it is in will not be used for other purposes.
> > +         */
> > +        void *new_esrt = NULL;
> > +        size_t esrt_size = get_esrt_size(efi_memmap + i);
> 
> ... a NULL pointer here.  And the only way that could happen is if
> efi_memmap is NULL.
> 
> Which perhaps isn't surprising because presumably ARM gets memory
> information from the DT, not EFI?
> 
> ~Andrew

Which in turn would explain why the problem is ARM-specific, and would
mean that disabling this on ARM would solve the problem.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

On 24.06.2022 20:17, Demi Marie Obenour wrote:
> The EFI System Resource Table (ESRT) is necessary for fwupd to identify
> firmware updates to install.  According to the UEFI specification §23.4,
> the ESRT shall be stored in memory of type EfiBootServicesData.  However,
> memory of type EfiBootServicesData is considered general-purpose memory
> by Xen, so the ESRT needs to be moved somewhere where Xen will not
> overwrite it.  Copy the ESRT to memory of type EfiRuntimeServicesData,
> which Xen will not reuse.  dom0 can use the ESRT if (and only if) it is
> in memory of type EfiRuntimeServicesData.
> 
> Earlier versions of this patch reserved the memory in which the ESRT was
> located.  This created awkward alignment problems, and required either
> splitting the E820 table or wasting memory.  It also would have required
> a new platform op for dom0 to use to indicate if the ESRT is reserved.
> By copying the ESRT into EfiRuntimeServicesData memory, the E820 table
> does not need to be modified, and dom0 can just check the type of the
> memory region containing the ESRT.  The copy is only done if the ESRT is
> not already in EfiRuntimeServicesData memory, avoiding memory leaks on
> repeated kexec.
> 
> See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
> for details.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one further adjustment:

> @@ -1051,6 +1110,70 @@ static void __init efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>  #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>                                   (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>  
> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> +{
> +    EFI_STATUS status;
> +    UINTN info_size = 0, map_key, mdesc_size;
> +    void *memory_map = NULL;
> +    UINT32 ver;
> +    unsigned int i;
> +
> +    for ( ; ; ) {

In reply to v7 I said:

"Nit: Style:

    for ( ; ; )
    {
"

which you've dealt with just partly. This time I'll take care of this
while committing.

Also for future patches please remember that having brief per-revision
change notes are quite helpful to reviewers.

Jan