[Qemu-devel] [PATCH 0/2] acpi: RSDP: fix checksum calculations

[Qemu-devel] [PATCH 0/2] acpi: RSDP: fix checksum calculations

[Igor Mammedov]

 * arm/virt is broken but it looks like linux doesn't care, lets fix it anyways
 * x86, got lucky since we didn't use extended fields,
   fix it so that it will calculate checksum using correct length
   so that it would be easier to unify arm/x86 into one impl.


Igor Mammedov (2):
  pc: acpi: use correct RSDT length for checksum
  arm/virt: acpi: fix incorrect checksums in RSDP

 hw/arm/virt-acpi-build.c | 7 ++++++-
 hw/i386/acpi-build.c     | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] pc: acpi: use correct RSDT length for checksum

[Igor Mammedov]

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

Re: [Qemu-devel] [PATCH 1/2] pc: acpi: use correct RSDT length for checksum

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH 1/2] pc: acpi: use correct RSDT length for checksum

[Igor Mammedov]

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  

[Qemu-devel] [PATCH v2 1/2] pc: acpi: use correct RSDT length for checksum

[Igor Mammedov]

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

[Qemu-devel] [PATCH 2/2] arm/virt: acpi: fix incorrect checksums in RSDP

[Igor Mammedov]

When RSDP table was introduced (d4bec5d87), we calculated only legacy
checksum, and that was incorrect as it
 - specified rev=2 and forgot about extended checksum.
 - legacy checksum calculated on full table instead of the 1st 20 bytes

Fix it by adding extended checksum calculation and using correct
size for legacy checksum.

While at it use explicit constants to specify sub/full tables
sizes instead of relying on AcpiRsdpDescriptor size and fields offsets.
The follow up commits will convert this table to build_append_int_noprefix() API,
will use constants anyway and remove unused AcpiRsdpDescriptor structure.

Based on "[PATCH v5 05/24] hw: acpi: Implement XSDT support for  RSDP"
by Samuel Ortiz, who did it right in his impl.

Fixes: d4bec5d87 (hw/arm/virt-acpi-build: Generate RSDP table)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
CC: Ard Biesheuvel <ard.biesheuvel@linaro.org>
CC: Shannon Zhao <shannon.zhaosl@gmail.com>
CC: Samuel Ortiz <sameo@linux.intel.com>
---
 hw/arm/virt-acpi-build.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 5785fb6..daa093b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -390,9 +390,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_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);
 
+    /* Extended checksum to be filled by Guest linker */
+    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
+        (char *)rsdp - rsdp_table->data, 36 /* ACPI rev 2.0 RSDP size */,
+        (char *)&rsdp->extended_checksum - rsdp_table->data);
+
     return rsdp_table;
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] arm/virt: acpi: fix incorrect checksums in RSDP

[Andrew Jones]

On Thu, Nov 22, 2018 at 11:29:34AM +0100, Igor Mammedov wrote:
> When RSDP table was introduced (d4bec5d87), we calculated only legacy
> checksum, and that was incorrect as it
>  - specified rev=2 and forgot about extended checksum.
>  - legacy checksum calculated on full table instead of the 1st 20 bytes
> 
> Fix it by adding extended checksum calculation and using correct
> size for legacy checksum.
> 
> While at it use explicit constants to specify sub/full tables
> sizes instead of relying on AcpiRsdpDescriptor size and fields offsets.
> The follow up commits will convert this table to build_append_int_noprefix() API,
> will use constants anyway and remove unused AcpiRsdpDescriptor structure.
> 
> Based on "[PATCH v5 05/24] hw: acpi: Implement XSDT support for  RSDP"
> by Samuel Ortiz, who did it right in his impl.
> 
> Fixes: d4bec5d87 (hw/arm/virt-acpi-build: Generate RSDP table)
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> CC: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> CC: Shannon Zhao <shannon.zhaosl@gmail.com>
> CC: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  hw/arm/virt-acpi-build.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 5785fb6..daa093b 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -390,9 +390,14 @@ build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned xsdt_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);
>  
> +    /* Extended checksum to be filled by Guest linker */
> +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_RSDP_FILE,
> +        (char *)rsdp - rsdp_table->data, 36 /* ACPI rev 2.0 RSDP size */,
> +        (char *)&rsdp->extended_checksum - rsdp_table->data);
> +
>      return rsdp_table;
>  }
>  
> -- 
> 2.7.4
> 

Indeed this slipped by because Linux doesn't seem to care. Linux doesn't
even check the checksums by default. It can be forced to check them by
adding the acpi_force_table_verification kernel command line parameter,
but even then it didn't care that we got it wrong for this table, at it
apparently doesn't bother checking it the RSDP, but rather just the XSDT
it points to. In any case, a guest that was depending on that broken
behavior is broken, so we can fix this without concern for compatibility,
and with this change we now match the spec.

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] acpi: RSDP: fix checksum calculations

[Michael S. Tsirkin]

On Thu, Nov 22, 2018 at 11:29:32AM +0100, Igor Mammedov wrote:
>  * arm/virt is broken but it looks like linux doesn't care, lets fix it anyways
>  * x86, got lucky since we didn't use extended fields,
>    fix it so that it will calculate checksum using correct length
>    so that it would be easier to unify arm/x86 into one impl.
> 

I'd say patch 2 should go into this release.
What about patch 1? It's cosmetic and it isn't the best we can do.

Let me know whether you agree.

> Igor Mammedov (2):
>   pc: acpi: use correct RSDT length for checksum
>   arm/virt: acpi: fix incorrect checksums in RSDP
> 
>  hw/arm/virt-acpi-build.c | 7 ++++++-
>  hw/i386/acpi-build.c     | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 0/2] acpi: RSDP: fix checksum calculations

[Igor Mammedov]

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

> On Thu, Nov 22, 2018 at 11:29:32AM +0100, Igor Mammedov wrote:
> >  * arm/virt is broken but it looks like linux doesn't care, lets fix it anyways
> >  * x86, got lucky since we didn't use extended fields,
> >    fix it so that it will calculate checksum using correct length
> >    so that it would be easier to unify arm/x86 into one impl.
> >   
> 
> I'd say patch 2 should go into this release.
> What about patch 1? It's cosmetic and it isn't the best we can do.
> 
> Let me know whether you agree.
I'd say they are both 'cosmetic' as they don't cause guest
failure nor even warning (linux doesn't care (so far) and
we don't have windows for ARM to verify extended checksum). 
So we don't have to delay release for both of them.

I assumed that Samuel, would include patches in his RSDP
refactoring series and we would merge them as whole later on.


> 
> > Igor Mammedov (2):
> >   pc: acpi: use correct RSDT length for checksum
> >   arm/virt: acpi: fix incorrect checksums in RSDP
> > 
> >  hw/arm/virt-acpi-build.c | 7 ++++++-
> >  hw/i386/acpi-build.c     | 2 +-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > -- 
> > 2.7.4  

Re: [Qemu-devel] [PATCH 0/2] acpi: RSDP: fix checksum calculations

[Samuel Ortiz]

On Mon, Nov 26, 2018 at 11:31:25AM +0100, Igor Mammedov wrote:
> On Fri, 23 Nov 2018 11:11:50 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Nov 22, 2018 at 11:29:32AM +0100, Igor Mammedov wrote:
> > >  * arm/virt is broken but it looks like linux doesn't care, lets fix it anyways
> > >  * x86, got lucky since we didn't use extended fields,
> > >    fix it so that it will calculate checksum using correct length
> > >    so that it would be easier to unify arm/x86 into one impl.
> > >   
> > 
> > I'd say patch 2 should go into this release.
> > What about patch 1? It's cosmetic and it isn't the best we can do.
> > 
> > Let me know whether you agree.
> I'd say they are both 'cosmetic' as they don't cause guest
> failure nor even warning (linux doesn't care (so far) and
> we don't have windows for ARM to verify extended checksum). 
> So we don't have to delay release for both of them.
Agreed.


> I assumed that Samuel, would include patches in his RSDP
> refactoring series and we would merge them as whole later on.
That's correct, yes.

Cheers,
Samuel.