| Message ID | 20220805093948.82561-2-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | QMP/HMP: add 'dumpdtb' and 'info fdt' commands | expand |
On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote: > At this moment, arm_load_dtb() can free machine->fdt when > binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be > retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is > the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to > machine->fdt. And, in that case, the existing g_free(fdt) at the end of > arm_load_dtb() will make machine->fdt point to an invalid memory region. > > This is not an issue right now because there's no code that access > machine->fdt after arm_load_dtb(), but we're going to add a couple do > FDT HMP commands that will rely on machine->fdt being valid. > > Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to > machine->fdt. This will allow the FDT of ARM machines that relies on > arm_load_dtb() to be accessed later on. > > Since all ARM machines allocates the FDT only once, we don't need to > worry about leaking the existing FDT during a machine reset (which is > something that other machines have to look after, e.g. the ppc64 pSeries > machine). > > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-arm@nongnu.org > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/arm/boot.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > index ada2717f76..9f5ceb62d2 100644 > --- a/hw/arm/boot.c > +++ b/hw/arm/boot.c > @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo, > */ > rom_add_blob_fixed_as("dtb", fdt, size, addr, as); > > - g_free(fdt); > + /* > + * Update the ms->fdt pointer to enable support for 'dumpdtb' > + * and 'info fdt' commands. Use fdt_pack() to shrink the blob > + * size we're going to store. > + */ > + fdt_pack(fdt); > + ms->fdt = fdt; > > return size; fdt_pack() could change (reduce) the effective size of the dtb blob, so returning a 'size' value from above rather than the new value of fdt_totalsize(fdt) doesn't see right. I believe some of the other patches in the series have similar concerns.
On 8/8/22 00:23, David Gibson wrote: > On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote: >> At this moment, arm_load_dtb() can free machine->fdt when >> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be >> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is >> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to >> machine->fdt. And, in that case, the existing g_free(fdt) at the end of >> arm_load_dtb() will make machine->fdt point to an invalid memory region. >> >> This is not an issue right now because there's no code that access >> machine->fdt after arm_load_dtb(), but we're going to add a couple do >> FDT HMP commands that will rely on machine->fdt being valid. >> >> Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to >> machine->fdt. This will allow the FDT of ARM machines that relies on >> arm_load_dtb() to be accessed later on. >> >> Since all ARM machines allocates the FDT only once, we don't need to >> worry about leaking the existing FDT during a machine reset (which is >> something that other machines have to look after, e.g. the ppc64 pSeries >> machine). >> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Cc: qemu-arm@nongnu.org >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/arm/boot.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index ada2717f76..9f5ceb62d2 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo, >> */ >> rom_add_blob_fixed_as("dtb", fdt, size, addr, as); >> >> - g_free(fdt); >> + /* >> + * Update the ms->fdt pointer to enable support for 'dumpdtb' >> + * and 'info fdt' commands. Use fdt_pack() to shrink the blob >> + * size we're going to store. >> + */ >> + fdt_pack(fdt); >> + ms->fdt = fdt; >> >> return size; > > fdt_pack() could change (reduce) the effective size of the dtb blob, > so returning a 'size' value from above rather than the new value of > fdt_totalsize(fdt) doesn't see right. > > I believe some of the other patches in the series have similar concerns. Ok! I'll revisit those patches and be sure to return the updated value of the fdt. Thanks, Daniel >
David, On 8/8/22 00:23, David Gibson wrote: > On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote: >> At this moment, arm_load_dtb() can free machine->fdt when >> binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be >> retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is >> the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to >> machine->fdt. And, in that case, the existing g_free(fdt) at the end of >> arm_load_dtb() will make machine->fdt point to an invalid memory region. >> >> This is not an issue right now because there's no code that access >> machine->fdt after arm_load_dtb(), but we're going to add a couple do >> FDT HMP commands that will rely on machine->fdt being valid. >> >> Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to >> machine->fdt. This will allow the FDT of ARM machines that relies on >> arm_load_dtb() to be accessed later on. >> >> Since all ARM machines allocates the FDT only once, we don't need to >> worry about leaking the existing FDT during a machine reset (which is >> something that other machines have to look after, e.g. the ppc64 pSeries >> machine). >> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Cc: qemu-arm@nongnu.org >> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> >> --- >> hw/arm/boot.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/arm/boot.c b/hw/arm/boot.c >> index ada2717f76..9f5ceb62d2 100644 >> --- a/hw/arm/boot.c >> +++ b/hw/arm/boot.c >> @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo, >> */ >> rom_add_blob_fixed_as("dtb", fdt, size, addr, as); >> >> - g_free(fdt); >> + /* >> + * Update the ms->fdt pointer to enable support for 'dumpdtb' >> + * and 'info fdt' commands. Use fdt_pack() to shrink the blob >> + * size we're going to store. >> + */ >> + fdt_pack(fdt); >> + ms->fdt = fdt; >> >> return size; > > fdt_pack() could change (reduce) the effective size of the dtb blob, > so returning a 'size' value from above rather than the new value of > fdt_totalsize(fdt) doesn't see right. After some thought I think executing fdt_pack() like I'm doing here is not a good idea. The first problem is that I'm not returning the updated size, as you've said. But I can't just amend a 'return fdt_totalsize(fdt);' either. I'm packing the FDT **after** the machine store it in the guest physical memory. If I return the packed size, but the machine isn't packing the FDT before a cpu_physical_memory_write(), I'll be under-reporting the FDT size written. Machines such as e500 (patch 4) uses this returned value to put more stuff in the guest memory. In that case, returning a smaller size that what was actually written can cause the machine to overwrite the FDT by accident. In fact, only a handful of machines (ppc/pseries, ppc/pvn, riscv, oepenrisc) is doing a fdt_pack() before a cpu_physical_memory_write(). So this change would be potentially harmful to a lot of people. One alternative would be to do a fdt_pack() before the machine writes the FDT in the guest memory, but that is too intrusive to do because I can't say if each of these machines will be OK with that. I have a hunch that it would be OK, but a hunch isn't going to cut it. I'll just drop the fdt_pack() for each of these patches. If the machine code is already packing it, fine. If not, that's fine too. Each maintainer can assess whether packing the FDT is worth it or not. Thanks, Daniel > > I believe some of the other patches in the series have similar concerns. >
On Fri, Aug 12, 2022 at 07:03:26PM -0300, Daniel Henrique Barboza wrote: > David, > > On 8/8/22 00:23, David Gibson wrote: > > On Fri, Aug 05, 2022 at 06:39:29AM -0300, Daniel Henrique Barboza wrote: > > > At this moment, arm_load_dtb() can free machine->fdt when > > > binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be > > > retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is > > > the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to > > > machine->fdt. And, in that case, the existing g_free(fdt) at the end of > > > arm_load_dtb() will make machine->fdt point to an invalid memory region. > > > > > > This is not an issue right now because there's no code that access > > > machine->fdt after arm_load_dtb(), but we're going to add a couple do > > > FDT HMP commands that will rely on machine->fdt being valid. > > > > > > Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to > > > machine->fdt. This will allow the FDT of ARM machines that relies on > > > arm_load_dtb() to be accessed later on. > > > > > > Since all ARM machines allocates the FDT only once, we don't need to > > > worry about leaking the existing FDT during a machine reset (which is > > > something that other machines have to look after, e.g. the ppc64 pSeries > > > machine). > > > > > > Cc: Peter Maydell <peter.maydell@linaro.org> > > > Cc: qemu-arm@nongnu.org > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > > > --- > > > hw/arm/boot.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/arm/boot.c b/hw/arm/boot.c > > > index ada2717f76..9f5ceb62d2 100644 > > > --- a/hw/arm/boot.c > > > +++ b/hw/arm/boot.c > > > @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo, > > > */ > > > rom_add_blob_fixed_as("dtb", fdt, size, addr, as); > > > - g_free(fdt); > > > + /* > > > + * Update the ms->fdt pointer to enable support for 'dumpdtb' > > > + * and 'info fdt' commands. Use fdt_pack() to shrink the blob > > > + * size we're going to store. > > > + */ > > > + fdt_pack(fdt); > > > + ms->fdt = fdt; > > > return size; > > > > fdt_pack() could change (reduce) the effective size of the dtb blob, > > so returning a 'size' value from above rather than the new value of > > fdt_totalsize(fdt) doesn't see right. > > After some thought I think executing fdt_pack() like I'm doing here is not > a good idea. The first problem is that I'm not returning the updated size, > as you've said. > > But I can't just amend a 'return fdt_totalsize(fdt);' either. I'm packing the > FDT **after** the machine store it in the guest physical memory. If I return > the packed size, but the machine isn't packing the FDT before a > cpu_physical_memory_write(), I'll be under-reporting the FDT size written. Ah... good point. > Machines such as e500 (patch 4) uses this returned value to put more stuff in > the guest memory. In that case, returning a smaller size that what was actually > written can cause the machine to overwrite the FDT by accident. In fact, only > a handful of machines (ppc/pseries, ppc/pvn, riscv, oepenrisc) is doing a > fdt_pack() before a cpu_physical_memory_write(). So this change would be > potentially harmful to a lot of people. > > One alternative would be to do a fdt_pack() before the machine writes the > FDT in the guest memory, but that is too intrusive to do because I can't say > if each of these machines will be OK with that. I have a hunch that it would > be OK, but a hunch isn't going to cut it. Hmm.. I'd be fairly confident that would be ok. It certainly should be ok for the fdt content itself, fdt_pack() doesn't change that semantically. If the machine is using that size to put stuff after, I can't really see how that could break, either. Unless the machine were using the fdtsize in one place and a fixed size somewhere else to address the same data, which would be a bug. > I'll just drop the fdt_pack() for each of these patches. If the machine code > is already packing it, fine. If not, that's fine too. Each maintainer can > assess whether packing the FDT is worth it or not. That's probably reasonable for the time being.
diff --git a/hw/arm/boot.c b/hw/arm/boot.c index ada2717f76..9f5ceb62d2 100644 --- a/hw/arm/boot.c +++ b/hw/arm/boot.c @@ -684,7 +684,13 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo, */ rom_add_blob_fixed_as("dtb", fdt, size, addr, as); - g_free(fdt); + /* + * Update the ms->fdt pointer to enable support for 'dumpdtb' + * and 'info fdt' commands. Use fdt_pack() to shrink the blob + * size we're going to store. + */ + fdt_pack(fdt); + ms->fdt = fdt; return size;
At this moment, arm_load_dtb() can free machine->fdt when binfo->dtb_filename is NULL. If there's no 'dtb_filename', 'fdt' will be retrieved by binfo->get_dtb(). If get_dtb() returns machine->fdt, as is the case of machvirt_dtb() from hw/arm/virt.c, fdt now has a pointer to machine->fdt. And, in that case, the existing g_free(fdt) at the end of arm_load_dtb() will make machine->fdt point to an invalid memory region. This is not an issue right now because there's no code that access machine->fdt after arm_load_dtb(), but we're going to add a couple do FDT HMP commands that will rely on machine->fdt being valid. Instead of freeing 'fdt' at the end of arm_load_dtb(), assign it to machine->fdt. This will allow the FDT of ARM machines that relies on arm_load_dtb() to be accessed later on. Since all ARM machines allocates the FDT only once, we don't need to worry about leaking the existing FDT during a machine reset (which is something that other machines have to look after, e.g. the ppc64 pSeries machine). Cc: Peter Maydell <peter.maydell@linaro.org> Cc: qemu-arm@nongnu.org Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/arm/boot.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)