Re: [PATCH] hw/i386/pc: when adding reserved E820 entries do not allocate dynamic entries

[Ani Sinha]

On Wed, Feb 23, 2022 at 2:34 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 10 Feb 2022 18:58:21 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > When adding E820_RESERVED entries we also accidentally allocate dynamic
> > entries. This is incorrect. We should simply return early with the count of
> > the number of reserved entries added.
>
> can you expand commit message to explain what's wrong and
> how problem manifests ... etc.

The issue has been present for the last 8 years without apparent
visible issues. I think the only issue is that the bug allocates more
memory in the firmware than is actually needed.

>
> >
> > fixes: 7d67110f2d9a6("pc: add etc/e820 fw_cfg file")
> > cc: kraxel@redhat.com
> > Signed-off-by: Ani Sinha <ani@anisinha.ca>
> > ---
> >  hw/i386/e820_memory_layout.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/i386/e820_memory_layout.c b/hw/i386/e820_memory_layout.c
> > index bcf9eaf837..afb08253a4 100644
> > --- a/hw/i386/e820_memory_layout.c
> > +++ b/hw/i386/e820_memory_layout.c
> > @@ -31,6 +31,8 @@ int e820_add_entry(uint64_t address, uint64_t length, 
> > uint32_t type)
> >          entry->type = cpu_to_le32(type);
> >
> >          e820_reserve.count = cpu_to_le32(index);
> > +
> > +        return index;
> >      }
>
> this changes e820_table size/content, which is added by fw_cfg_add_file() to 
> fwcfg,
> as result it breaks ABI in case of migration.

Ugh. So should we keep the bug? or do we add config setting to handle
the ABI breakage.