AcpiRsdpDescriptor describes revision 2 RSDP table so using sizeof(*rsdp)
for checksum calculation isn't correct since we are adding extra 16 bytes.
But acpi_data_push() zeroes out table, so just by luck we are summing up
exta zeros which still yelds correct checksum.

Fix it up by explicitly stating table size instead of using
pointer arithmetics on stucture.

PS:
Extra 16 bytes are still wasted, but droping them will break migration
for machines older than 2.3 due to size mismatch, for 2.3 and older it's
not an issue since they are using resizable memory regions (a1666142d)
for ACPI blobs. So keep wasting memory to avoid breaking old machines.

Fixes: 72c194f7e (i386: ACPI table generation code from seabios)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
there is no changes to the current RSDP content caused by this patch
---
 hw/i386/acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 236a20e..131c565 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2567,7 +2567,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
 
     /* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
+        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
         (char *)&rsdp->checksum - rsdp_table->data);
 
     return rsdp_table;
-- 
2.7.4

On Thu, Nov 22, 2018 at 11:29:33AM +0100, Igor Mammedov wrote:
> AcpiRsdpDescriptor describes revision 2 RSDP table so using sizeof(*rsdp)
> for checksum calculation isn't correct since we are adding extra 16 bytes.
> But acpi_data_push() zeroes out table, so just by luck we are summing up
> exta zeros which still yelds correct checksum.
> 
> Fix it up by explicitly stating table size instead of using
> pointer arithmetics on stucture.
> 
> PS:
> Extra 16 bytes are still wasted, but droping them will break migration
> for machines older than 2.3 due to size mismatch, for 2.3 and older it's
> not an issue since they are using resizable memory regions (a1666142d)
> for ACPI blobs. So keep wasting memory to avoid breaking old machines.

I'd like this explanation in code comments please.

> Fixes: 72c194f7e (i386: ACPI table generation code from seabios)
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> there is no changes to the current RSDP content caused by this patch
> ---
>  hw/i386/acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 236a20e..131c565 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2567,7 +2567,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
>  
>      /* Checksum to be filled by Guest linker */
>      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> -        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
> +        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
>          (char *)&rsdp->checksum - rsdp_table->data);
>  
>      return rsdp_table;

I dislike hard-coded size math like this.
How about a sub-structure for RSDPv1?


> -- 
> 2.7.4

On Fri, 23 Nov 2018 11:10:01 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Nov 22, 2018 at 11:29:33AM +0100, Igor Mammedov wrote:
> > AcpiRsdpDescriptor describes revision 2 RSDP table so using sizeof(*rsdp)
> > for checksum calculation isn't correct since we are adding extra 16 bytes.
> > But acpi_data_push() zeroes out table, so just by luck we are summing up
> > exta zeros which still yelds correct checksum.
> > 
> > Fix it up by explicitly stating table size instead of using
> > pointer arithmetics on stucture.
> > 
> > PS:
> > Extra 16 bytes are still wasted, but droping them will break migration
> > for machines older than 2.3 due to size mismatch, for 2.3 and older it's
> > not an issue since they are using resizable memory regions (a1666142d)
> > for ACPI blobs. So keep wasting memory to avoid breaking old machines.  
> 
> I'd like this explanation in code comments please.
sure, I'll move it there

> > Fixes: 72c194f7e (i386: ACPI table generation code from seabios)
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > there is no changes to the current RSDP content caused by this patch
> > ---
> >  hw/i386/acpi-build.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 236a20e..131c565 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -2567,7 +2567,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
> >  
> >      /* Checksum to be filled by Guest linker */
> >      bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> > -        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
> > +        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
> >          (char *)&rsdp->checksum - rsdp_table->data);
> >  
> >      return rsdp_table;  
> 
> I dislike hard-coded size math like this.
I thought we had a consensus on this point. i.e.
replace all packed structures with build_append_int_noprefix() API
and use spec defined values directly (with comments), so whenever
we compare spec definition with qemu code it would be 1:1 match.

> How about a sub-structure for RSDPv1?
Structure won't be used for long, this patch fixes immediate error
and Samuel is preparing series that's generalizing build_rsdp() switching
to build_append_int_noprefix() API and dropping no longer used structure
so it couldn't be misused.

> > -- 
> > 2.7.4  

AcpiRsdpDescriptor describes revision 2 RSDP table so using sizeof(*rsdp)
for checksum calculation isn't correct since we are adding extra 16 bytes.
But acpi_data_push() zeroes out table, so just by luck we are summing up
exta zeros which still yelds correct checksum.

Fix it up by explicitly stating table size instead of using
pointer arithmetics on stucture.

PS:
Extra 16 bytes are still wasted, but droping them will break migration
for machines older than 2.3 due to size mismatch, for 2.3 and older it's
not an issue since they are using resizable memory regions (a1666142d)
for ACPI blobs. So keep wasting memory to avoid breaking old machines.

Fixes: 72c194f7e (i386: ACPI table generation code from seabios)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
there is no changes to the current RSDP content caused by this patch

v2:
 - add 16 byte leak comment to the code (Michael)
---
 hw/i386/acpi-build.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 236a20e..5bb3563 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2550,6 +2550,11 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
 static GArray *
 build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
 {
+    /* AcpiRsdpDescriptor describes revision 2 RSDP table and as result we
+     * allocate extra 16 bytes for pc/q35 RSDP rev1 as well. Keep extra 16 bytes
+     * wasted to make sure we won't breake migration for machine types older
+     * than 2.3 due to size mismatch.
+     */
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
     unsigned rsdt_pa_size = sizeof(rsdp->rsdt_physical_address);
     unsigned rsdt_pa_offset =
@@ -2567,7 +2572,7 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
 
     /* Checksum to be filled by Guest linker */
     bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
-        (char *)rsdp - rsdp_table->data, sizeof *rsdp,
+        (char *)rsdp - rsdp_table->data, 20 /* ACPI rev 1.0 RSDP size */,
         (char *)&rsdp->checksum - rsdp_table->data);
 
     return rsdp_table;
-- 
2.7.4