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

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

[Ani Sinha]

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.

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;
     }
 
     /* new "etc/e820" file -- include ram too */
-- 
2.25.1

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

[Philippe Mathieu-Daudé via]

On 10/2/22 14:28, Ani Sinha 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.
> 
> fixes: 7d67110f2d9a6("pc: add etc/e820 fw_cfg file")

8 years old, so this path is clearly untested (unused...?).

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 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;
>       }
>   
>       /* new "etc/e820" file -- include ram too */

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

[Ani Sinha]

[-- Attachment #1: Type: text/plain, Size: 718 bytes --]



On Thu, 10 Feb 2022, Philippe Mathieu-Daudé wrote:

> On 10/2/22 14:28, Ani Sinha 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.
> >
> > fixes: 7d67110f2d9a6("pc: add etc/e820 fw_cfg file")
>
> 8 years old, so this path is clearly untested (unused...?).
>

untested, yes. unused? nope!

$ git grep e820_add_entry  2>/dev/null | grep E820_RESERVED
hw/i386/pc.c:        e820_add_entry(pcms->sgx_epc.base,
pcms->sgx_epc.size, E820_RESERVED);
target/i386/kvm/kvm.c:    ret = e820_add_entry(identity_base, 0x4000,
E820_RESERVED);

particulatly the kvm code path.

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

[Igor Mammedov]

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.

> 
> 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.
>  
>      /* new "etc/e820" file -- include ram too */

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.

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

[Igor Mammedov]

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

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

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

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

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

[Ani Sinha]

On Thu, Feb 24, 2022 at 6:51 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> 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.
>

This patch is certainly wrong. For ref, please see the following
commit in seabios from Gerd:

ce39bd40318203dd30faa ("Add support for etc/e820 fw_cfg file")

"etc/e820" is supposed to have both E820_RESERVED and E820_RAM entries.