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

[Ani Sinha]

On Thu, Feb 24, 2022 at 2:33 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 23 Feb 2022 17:30:34 +0530
> Ani Sinha <ani@anisinha.ca> wrote:
>
> > 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.
>
> let me repeat: Why do you think it's an issue or why it's wrong

Allocating more memory than what we need unnecessarily bloats up the
rom. We should not be allocating memory that we do not use.

>
> >
> > >
> > > >
> > > > 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.
> >
>