[PULL 0/2] pc: last minute bugfixes

[PULL 0/2] pc: last minute bugfixes

[Michael S. Tsirkin]

The following changes since commit d83f46d189a26fa32434139954d264326f199a45:

  virtio-pci: compat page aligned ATS (2021-04-06 07:11:36 -0400)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 9106db1038bf3db5e4f8007038b3a1962018fa07:

  x86: acpi: use offset instead of pointer when using build_header() (2021-04-22 18:22:01 -0400)

----------------------------------------------------------------
pc: last minute bugfixes

Two bugfixes - both seem pretty obvious and safe ...

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Igor Mammedov (1):
      x86: acpi: use offset instead of pointer when using build_header()

Jean-Philippe Brucker (1):
      amd_iommu: Fix pte_override_page_mask()

 hw/acpi/aml-build.c  | 15 +++++++++------
 hw/i386/acpi-build.c |  8 ++++++--
 hw/i386/amd_iommu.c  |  4 ++--
 3 files changed, 17 insertions(+), 10 deletions(-)

[PULL 1/2] amd_iommu: Fix pte_override_page_mask()

[Michael S. Tsirkin]

From: Jean-Philippe Brucker <jean-philippe@linaro.org>

AMD IOMMU PTEs have a special mode allowing to specify an arbitrary page
size. Quoting the AMD IOMMU specification: "When the Next Level bits [of
a pte] are 7h, the size of the page is determined by the first zero bit
in the page address, starting from bit 12."

So if the lowest bits of the page address is 0, the page is 8kB. If the
lowest bits are 011, the page is 32kB. Currently pte_override_page_mask()
doesn't compute the right value for this page size and amdvi_translate()
can return the wrong guest-physical address. With a Linux guest, DMA
from SATA devices accesses the wrong memory and causes probe failure:

qemu-system-x86_64 ... -device amd-iommu -drive id=hd1,file=foo.bin,if=none \
		-device ahci,id=ahci -device ide-hd,drive=hd1,bus=ahci.0
[    6.613093] ata1.00: qc timeout (cmd 0xec)
[    6.615062] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)

Fix the page mask.

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Message-Id: <20210421084007.1190546-1-jean-philippe@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/i386/amd_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index 74a93a5d93..43b6e9bf51 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -860,8 +860,8 @@ static inline uint8_t get_pte_translation_mode(uint64_t pte)
 
 static inline uint64_t pte_override_page_mask(uint64_t pte)
 {
-    uint8_t page_mask = 12;
-    uint64_t addr = (pte & AMDVI_DEV_PT_ROOT_MASK) ^ AMDVI_DEV_PT_ROOT_MASK;
+    uint8_t page_mask = 13;
+    uint64_t addr = (pte & AMDVI_DEV_PT_ROOT_MASK) >> 12;
     /* find the first zero bit */
     while (addr & 1) {
         page_mask++;
-- 
MST

[PULL 2/2] x86: acpi: use offset instead of pointer when using build_header()

[Michael S. Tsirkin]

From: Igor Mammedov <imammedo@redhat.com>

Do the same as in commit
 (4d027afeb3a97 Virt: ACPI: fix qemu assert due to re-assigned table data address)
for remaining tables that happen to use saved at
the beginning pointer to build header to avoid assert
when table_data is relocated due to implicit re-size.

In this case user is trying to start Windows 10 and getting assert at
 hw/acpi/bios-linker-loader.c:239:
  bios_linker_loader_add_checksum: Assertion `start_offset < file->blob->len' failed.

Fixes: https://bugs.launchpad.net/bugs/1923497
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20210414084356.3792113-1-imammedo@redhat.com>
Cc: mst@redhat.com, qemu-stable@nongnu.org
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi/aml-build.c  | 15 +++++++++------
 hw/i386/acpi-build.c |  8 ++++++--
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d33ce8954a..f0035d2b4a 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1830,6 +1830,7 @@ build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
     int i;
     unsigned rsdt_entries_offset;
     AcpiRsdtDescriptorRev1 *rsdt;
+    int rsdt_start = table_data->len;
     const unsigned table_data_len = (sizeof(uint32_t) * table_offsets->len);
     const unsigned rsdt_entry_size = sizeof(rsdt->table_offset_entry[0]);
     const size_t rsdt_len = sizeof(*rsdt) + table_data_len;
@@ -1846,7 +1847,8 @@ build_rsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
             ACPI_BUILD_TABLE_FILE, ref_tbl_offset);
     }
     build_header(linker, table_data,
-                 (void *)rsdt, "RSDT", rsdt_len, 1, oem_id, oem_table_id);
+                 (void *)(table_data->data + rsdt_start),
+                 "RSDT", rsdt_len, 1, oem_id, oem_table_id);
 }
 
 /* Build xsdt table */
@@ -1857,6 +1859,7 @@ build_xsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
     int i;
     unsigned xsdt_entries_offset;
     AcpiXsdtDescriptorRev2 *xsdt;
+    int xsdt_start = table_data->len;
     const unsigned table_data_len = (sizeof(uint64_t) * table_offsets->len);
     const unsigned xsdt_entry_size = sizeof(xsdt->table_offset_entry[0]);
     const size_t xsdt_len = sizeof(*xsdt) + table_data_len;
@@ -1873,7 +1876,8 @@ build_xsdt(GArray *table_data, BIOSLinker *linker, GArray *table_offsets,
             ACPI_BUILD_TABLE_FILE, ref_tbl_offset);
     }
     build_header(linker, table_data,
-                 (void *)xsdt, "XSDT", xsdt_len, 1, oem_id, oem_table_id);
+                 (void *)(table_data->data + xsdt_start),
+                 "XSDT", xsdt_len, 1, oem_id, oem_table_id);
 }
 
 void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
@@ -2053,10 +2057,9 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
     uint64_t control_area_start_address;
     TPMIf *tpmif = tpm_find();
     uint32_t start_method;
-    void *tpm2_ptr;
 
     tpm2_start = table_data->len;
-    tpm2_ptr = acpi_data_push(table_data, sizeof(AcpiTableHeader));
+    acpi_data_push(table_data, sizeof(AcpiTableHeader));
 
     /* Platform Class */
     build_append_int_noprefix(table_data, TPM2_ACPI_CLASS_CLIENT, 2);
@@ -2095,8 +2098,8 @@ void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
                                    log_addr_offset, 8,
                                    ACPI_BUILD_TPMLOG_FILE, 0);
     build_header(linker, table_data,
-                 tpm2_ptr, "TPM2", table_data->len - tpm2_start, 4, oem_id,
-                 oem_table_id);
+                 (void *)(table_data->data + tpm2_start),
+                 "TPM2", table_data->len - tpm2_start, 4, oem_id, oem_table_id);
 }
 
 Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set, uint32_t io_offset,
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index de98750aef..daaf8f473e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1816,6 +1816,7 @@ build_hpet(GArray *table_data, BIOSLinker *linker, const char *oem_id,
            const char *oem_table_id)
 {
     Acpi20Hpet *hpet;
+    int hpet_start = table_data->len;
 
     hpet = acpi_data_push(table_data, sizeof(*hpet));
     /* Note timer_block_id value must be kept in sync with value advertised by
@@ -1824,13 +1825,15 @@ build_hpet(GArray *table_data, BIOSLinker *linker, const char *oem_id,
     hpet->timer_block_id = cpu_to_le32(0x8086a201);
     hpet->addr.address = cpu_to_le64(HPET_BASE);
     build_header(linker, table_data,
-                 (void *)hpet, "HPET", sizeof(*hpet), 1, oem_id, oem_table_id);
+                 (void *)(table_data->data + hpet_start),
+                 "HPET", sizeof(*hpet), 1, oem_id, oem_table_id);
 }
 
 static void
 build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
                const char *oem_id, const char *oem_table_id)
 {
+    int tcpa_start = table_data->len;
     Acpi20Tcpa *tcpa = acpi_data_push(table_data, sizeof *tcpa);
     unsigned log_addr_size = sizeof(tcpa->log_area_start_address);
     unsigned log_addr_offset =
@@ -1849,7 +1852,8 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
         ACPI_BUILD_TPMLOG_FILE, 0);
 
     build_header(linker, table_data,
-                 (void *)tcpa, "TCPA", sizeof(*tcpa), 2, oem_id, oem_table_id);
+                 (void *)(table_data->data + tcpa_start),
+                 "TCPA", sizeof(*tcpa), 2, oem_id, oem_table_id);
 }
 
 #define HOLE_640K_START  (640 * KiB)
-- 
MST

Re: [PULL 0/2] pc: last minute bugfixes

[Peter Maydell]

On Thu, 22 Apr 2021 at 23:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> The following changes since commit d83f46d189a26fa32434139954d264326f199a45:
>
>   virtio-pci: compat page aligned ATS (2021-04-06 07:11:36 -0400)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 9106db1038bf3db5e4f8007038b3a1962018fa07:
>
>   x86: acpi: use offset instead of pointer when using build_header() (2021-04-22 18:22:01 -0400)
>
> ----------------------------------------------------------------
> pc: last minute bugfixes
>
> Two bugfixes - both seem pretty obvious and safe ...
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----------------------------------------------------------------

Come on, you should know the rules by now. Any pullrequest at this
point needs to have a detailed justification in the cover letter of
why it is such a release critical bug that we need to roll a new
release candidate and delay the release for it.

thanks
-- PMM

Re: [PULL 0/2] pc: last minute bugfixes

[Daniel P. Berrangé]

On Thu, Apr 22, 2021 at 06:24:48PM -0400, Michael S. Tsirkin wrote:
> The following changes since commit d83f46d189a26fa32434139954d264326f199a45:
> 
>   virtio-pci: compat page aligned ATS (2021-04-06 07:11:36 -0400)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> 
> for you to fetch changes up to 9106db1038bf3db5e4f8007038b3a1962018fa07:
> 
>   x86: acpi: use offset instead of pointer when using build_header() (2021-04-22 18:22:01 -0400)
> 
> ----------------------------------------------------------------
> pc: last minute bugfixes
> 
> Two bugfixes - both seem pretty obvious and safe ...

Are they fixing regressions from the previous release, and if so
what's the severity of them ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PULL 0/2] pc: last minute bugfixes

[Igor Mammedov]

On Fri, 23 Apr 2021 10:01:49 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Thu, Apr 22, 2021 at 06:24:48PM -0400, Michael S. Tsirkin wrote:
> > The following changes since commit d83f46d189a26fa32434139954d264326f199a45:
> > 
> >   virtio-pci: compat page aligned ATS (2021-04-06 07:11:36 -0400)
> > 
> > are available in the Git repository at:
> > 
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> > 
> > for you to fetch changes up to 9106db1038bf3db5e4f8007038b3a1962018fa07:
> > 
> >   x86: acpi: use offset instead of pointer when using build_header() (2021-04-22 18:22:01 -0400)
> > 
> > ----------------------------------------------------------------
> > pc: last minute bugfixes
> > 
> > Two bugfixes - both seem pretty obvious and safe ...  
> 
> Are they fixing regressions from the previous release, and if so
> what's the severity of them ?

 - [PULL 2/2] x86: acpi: use offset instead of pointer when using  build_header()
    * regression: no (reported on 5.1)
    * severity: qemu assert on guest boot
        could happen when configuration pushes ACPI tables blob over current limit (virt/arm had similar fix)
         (frankly speaking, why it's happening in reporter's case is still not clear)
      I was hoping it would get into rc4, but it missed that train,
      it probably would be fine to postpone patch to 6.1 at this point

> Regards,
> Daniel

Re: [PULL 1/2] amd_iommu: Fix pte_override_page_mask()

[Peter Maydell]

On Thu, 22 Apr 2021 at 23:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> AMD IOMMU PTEs have a special mode allowing to specify an arbitrary page
> size. Quoting the AMD IOMMU specification: "When the Next Level bits [of
> a pte] are 7h, the size of the page is determined by the first zero bit
> in the page address, starting from bit 12."
>
> So if the lowest bits of the page address is 0, the page is 8kB. If the
> lowest bits are 011, the page is 32kB. Currently pte_override_page_mask()
> doesn't compute the right value for this page size and amdvi_translate()
> can return the wrong guest-physical address. With a Linux guest, DMA
> from SATA devices accesses the wrong memory and causes probe failure:
>
> qemu-system-x86_64 ... -device amd-iommu -drive id=hd1,file=foo.bin,if=none \
>                 -device ahci,id=ahci -device ide-hd,drive=hd1,bus=ahci.0
> [    6.613093] ata1.00: qc timeout (cmd 0xec)
> [    6.615062] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>
> Fix the page mask.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Message-Id: <20210421084007.1190546-1-jean-philippe@linaro.org>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Jean-Philippe, do you know if this is a regression since 5.2?
I'm guessing not given that the function in question has been that
way since the amd_iommu was introduced in 2016.

thanks
-- PMM

Re: [PULL 1/2] amd_iommu: Fix pte_override_page_mask()

[Jean-Philippe Brucker]

On Fri, Apr 23, 2021 at 02:01:19PM +0100, Peter Maydell wrote:
> On Thu, 22 Apr 2021 at 23:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> >
> > AMD IOMMU PTEs have a special mode allowing to specify an arbitrary page
> > size. Quoting the AMD IOMMU specification: "When the Next Level bits [of
> > a pte] are 7h, the size of the page is determined by the first zero bit
> > in the page address, starting from bit 12."
> >
> > So if the lowest bits of the page address is 0, the page is 8kB. If the
> > lowest bits are 011, the page is 32kB. Currently pte_override_page_mask()
> > doesn't compute the right value for this page size and amdvi_translate()
> > can return the wrong guest-physical address. With a Linux guest, DMA
> > from SATA devices accesses the wrong memory and causes probe failure:
> >
> > qemu-system-x86_64 ... -device amd-iommu -drive id=hd1,file=foo.bin,if=none \
> >                 -device ahci,id=ahci -device ide-hd,drive=hd1,bus=ahci.0
> > [    6.613093] ata1.00: qc timeout (cmd 0xec)
> > [    6.615062] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
> >
> > Fix the page mask.
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > Message-Id: <20210421084007.1190546-1-jean-philippe@linaro.org>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Jean-Philippe, do you know if this is a regression since 5.2?

I don't think so, I can reproduce it with v5.2.0.

> I'm guessing not given that the function in question has been that
> way since the amd_iommu was introduced in 2016.

There has been a lot of work on the AMD IOMMU driver in Linux recently.
Maybe that exacerbated the problem but I can't find a relevant change.
It's also possible that this path hasn't been exercised before - I just
happened to run a SATA device under AMD IOMMU this week to debug an
unrelated Linux issue. The other devices in the VM don't seem to have a
problem doing DMA.

Thanks,
Jean

Re: [PULL 1/2] amd_iommu: Fix pte_override_page_mask()

[Peter Maydell]

On Fri, 23 Apr 2021 at 14:35, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
>
> On Fri, Apr 23, 2021 at 02:01:19PM +0100, Peter Maydell wrote:
> > On Thu, 22 Apr 2021 at 23:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > >
> > > AMD IOMMU PTEs have a special mode allowing to specify an arbitrary page
> > > size. Quoting the AMD IOMMU specification: "When the Next Level bits [of
> > > a pte] are 7h, the size of the page is determined by the first zero bit
> > > in the page address, starting from bit 12."
> > >
> > > So if the lowest bits of the page address is 0, the page is 8kB. If the
> > > lowest bits are 011, the page is 32kB. Currently pte_override_page_mask()
> > > doesn't compute the right value for this page size and amdvi_translate()
> > > can return the wrong guest-physical address. With a Linux guest, DMA
> > > from SATA devices accesses the wrong memory and causes probe failure:
> > >
> > > qemu-system-x86_64 ... -device amd-iommu -drive id=hd1,file=foo.bin,if=none \
> > >                 -device ahci,id=ahci -device ide-hd,drive=hd1,bus=ahci.0
> > > [    6.613093] ata1.00: qc timeout (cmd 0xec)
> > > [    6.615062] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
> > >
> > > Fix the page mask.
> > >
> > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> > > Message-Id: <20210421084007.1190546-1-jean-philippe@linaro.org>
> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Jean-Philippe, do you know if this is a regression since 5.2?
>
> I don't think so, I can reproduce it with v5.2.0.

OK, thanks; I think I favour not putting this into rc5, then.

-- PMM

Re: [PULL 0/2] pc: last minute bugfixes

[Michael S. Tsirkin]

On Fri, Apr 23, 2021 at 09:53:01AM +0100, Peter Maydell wrote:
> On Thu, 22 Apr 2021 at 23:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > The following changes since commit d83f46d189a26fa32434139954d264326f199a45:
> >
> >   virtio-pci: compat page aligned ATS (2021-04-06 07:11:36 -0400)
> >
> > are available in the Git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to 9106db1038bf3db5e4f8007038b3a1962018fa07:
> >
> >   x86: acpi: use offset instead of pointer when using build_header() (2021-04-22 18:22:01 -0400)
> >
> > ----------------------------------------------------------------
> > pc: last minute bugfixes
> >
> > Two bugfixes - both seem pretty obvious and safe ...
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ----------------------------------------------------------------
> 
> Come on, you should know the rules by now. Any pullrequest at this
> point needs to have a detailed justification in the cover letter of
> why it is such a release critical bug that we need to roll a new
> release candidate and delay the release for it.
> 
> thanks
> -- PMM

Hmm sorry, I was a bit rushed.
acpi one fixes a regression but not one in a recent release.
amd one is not.

Bottom line let's just defer this till after the release.

-- 
MST

Re: [PULL 1/2] amd_iommu: Fix pte_override_page_mask()

[Jean-Philippe Brucker]

On Fri, Apr 23, 2021 at 05:11:33PM +0100, Peter Maydell wrote:
> > > Jean-Philippe, do you know if this is a regression since 5.2?
> >
> > I don't think so, I can reproduce it with v5.2.0.
> 
> OK, thanks; I think I favour not putting this into rc5, then.

No problem, please let me know if I should resend after the next release

Thanks,
Jean