| Message ID | 20220210132822.2969324-1-ani@anisinha.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386/pc: when adding reserved E820 entries do not allocate dynamic entries | expand |
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 */
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.
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 */
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.
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. >
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. > > >
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. > > > > > >
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.
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 */
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(+)