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

[Igor Mammedov]

On Thu, 24 Feb 2022 18:14:35 +0530
Ani Sinha <ani@anisinha.ca> wrote:

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

see how firmware uses "etc/e820" fwcfg file first, to make up
mind on 'need' part.

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