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

[Jean-Philippe Brucker]

On Thu, Dec 05, 2024 at 10:59:52PM +0100, Philippe Mathieu-Daudé wrote:
> 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).

No I don't think we need it either, what matters is the size of the data
copied into guest memory. I'll remove this

Thanks,
Jean

> 
> > +} 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);
> >       }
> >   }
>