Re: [PATCH v3 08/26] hw/core/loader: Add ROM loader notifier

[Philippe Mathieu-Daudé]

On 25/11/24 20:56, Jean-Philippe Brucker wrote:
> Add a function to register a notifier, that is invoked after a ROM gets
> loaded into guest memory.
>
> It will be used by Arm confidential guest support, in order to register
> all blobs loaded into memory with KVM, so that their content is moved
> into Realm state and measured into the initial VM state.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>   include/hw/loader.h | 15 +++++++++++++++
>   hw/core/loader.c    | 15 +++++++++++++++
>   2 files changed, 30 insertions(+)
>
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 7f6d06b956..0cd9905f97 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -353,6 +353,21 @@ void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t 
> size);
>   ssize_t rom_add_vga(const char *file);
>   ssize_t rom_add_option(const char *file, int32_t bootindex);
>   
> +typedef struct RomLoaderNotify {
> +    /* Parameters passed to rom_add_blob() */
> +    hwaddr addr;

This is the buffer (blob) address in guest physical memory.

> +    size_t len;

This is the buffer length.

> +    size_t max_len;

This is the size of the MemoryRegion ROM containing the buffer.

Do we need to notify it? You don't use it in your next patch.
If so, I'd rather have this API returns a MemoryRegion (Rom->mr),
max_len can be retrieved using memory_region_size(mr); but I
don't think we need this at all (at least for now).

> +} RomLoaderNotify;
> +
> +/**
> + * rom_add_load_notifier - Add a notifier for loaded images
> + *
> + * Add a notifier that will be invoked with a RomLoaderNotify structure for 
> each
> + * blob loaded into guest memory, after the blob is loaded.
> + */
> +void rom_add_load_notifier(Notifier *notifier);
> +
>   /* This is the usual maximum in uboot, so if a uImage overflows this, it would
>    * overflow on real hardware too. */
>   #define UBOOT_MAX_GUNZIP_BYTES (64 << 20)
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 31593a1171..759a62cf58 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -67,6 +67,8 @@
>   #include <zlib.h>
>   
>   static int roms_loaded;
> +static NotifierList rom_loader_notifier =
> +    NOTIFIER_LIST_INITIALIZER(rom_loader_notifier);
>   
>   /* return the size or -1 if error */
>   int64_t get_image_size(const char *filename)
> @@ -1179,6 +1181,11 @@ MemoryRegion *rom_add_blob(const char *name, const void 
> *blob, size_t len,
>       return mr;
>   }
>   
> +void rom_add_load_notifier(Notifier *notifier)
> +{
> +    notifier_list_add(&rom_loader_notifier, notifier);
> +}
> +
>   /* This function is specific for elf program because we don't need to allocate
>    * all the rom. We just allocate the first part and the rest is just zeros. 
> This
>    * is why romsize and datasize are different. Also, this function takes its 
> own
> @@ -1220,6 +1227,7 @@ ssize_t rom_add_option(const char *file, int32_t 
> bootindex)
>   static void rom_reset(void *unused)
>   {
>       Rom *rom;
> +    RomLoaderNotify notify;
>   
>       QTAILQ_FOREACH(rom, &roms, next) {
>           if (rom->fw_file) {
> @@ -1268,6 +1276,13 @@ static void rom_reset(void *unused)
>           cpu_flush_icache_range(rom->addr, rom->datasize);
>   
>           trace_loader_write_rom(rom->name, rom->addr, rom->datasize, rom->isrom);
> +
> +        notify = (RomLoaderNotify) {
> +            .addr = rom->addr,
> +            .len = rom->datasize,
> +            .max_len = rom->romsize,
> +        };
> +        notifier_list_notify(&rom_loader_notifier, &notify);
>       }
>   }
>