From nobody Wed May 8 03:46:42 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org Return-Path: Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) by mx.zohomail.com with SMTPS id 164989665462111.358496776192624; Wed, 13 Apr 2022 17:37:34 -0700 (PDT) Received: from localhost ([::1]:55928 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nenUL-00053P-BY for importer@patchew.org; Wed, 13 Apr 2022 20:37:33 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:45878) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nekr1-00086I-0A for qemu-devel@nongnu.org; Wed, 13 Apr 2022 17:48:47 -0400 Received: from cae.in-ulm.de ([217.10.14.231]:42878) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nekqy-0002JE-QV for qemu-devel@nongnu.org; Wed, 13 Apr 2022 17:48:46 -0400 Received: by cae.in-ulm.de (Postfix, from userid 1000) id 7D1BA140113; Wed, 13 Apr 2022 23:48:33 +0200 (CEST) Date: Wed, 13 Apr 2022 23:48:33 +0200 From: "Christian A. Ehrhardt" To: qemu-devel@nongnu.org Subject: fwcfg: Wrong callback behaviour after fw_cfg_modify_bytes_read Message-ID: MIME-Version: 1.0 Content-Disposition: inline Received-SPF: pass (zohomail.com: domain of gnu.org designates 209.51.188.17 as permitted sender) client-ip=209.51.188.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Received-SPF: none client-ip=217.10.14.231; envelope-from=lk@c--e.de; helo=cae.in-ulm.de X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_NONE=0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-Mailman-Approved-At: Wed, 13 Apr 2022 20:35:12 -0400 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZM-MESSAGEID: 1649896656588100001 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" Hi, there's a long story behind this (see below). However, I'll start with the result: fw_cfg_modify_bytes_read() sets the callback data of an existing fw_cfg file to NULL but leaves the actual callbacks in place. Additioanlly, this function sets ->allow_write to false for no good reason AFAICS. For most callbacks, the callback will just crash on the NULL pointer in ->callback_opaque if this path is ever hit.=20 I think the following patch is required (I can properly format it if you agree). I'm not 100% sure about the "allow_write" part, tough: diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index e5f3c98184..b8b6d8fe10 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -742,8 +742,6 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, ui= nt16_t key, ptr =3D s->entries[arch][key].data; s->entries[arch][key].data =3D data; s->entries[arch][key].len =3D len; - s->entries[arch][key].callback_opaque =3D NULL; - s->entries[arch][key].allow_write =3D false; =20 return ptr; } Oppinions? For those interesed here's the somewhat longer story and the reason why the diff actually matters: We are running Windows in a Q35 based machine in UEFI mode with OVMF. In some situations we saw that the Windows guest would hang in the Windows boot loader after a guest initiated reboot of the virtual machine. A hard "system_reset" would trigger the same bug. The guest was hanging in a loop trying to read from unassigned I/O port 0xb008. This is the default port used for the ACPI PM timer on PIIX based machines (but remember that we use Q35 where the PM timer lives at 0x608 instead). It turned out that after the reboot OVMF would try to read the ACPI tables from FWCFG but commands in the table-loader file could not be executed correctly and OVMF falls back to some hard coded PIIX based default. ACPI tables and the table-loader data is initially generated during setup but this data is re-generated via an FWCFG callback (acpi_update_build) when the first of these files is accessed. The tables generated at this later time differ slightly from those generated during initial setup. In our case these differences required a resize of the table-loader romfile. This resize calls fw_cfg_modify_file() via the resize hook of the memory region that contains the FWCFG file. As described above this clears the ->callback_opaque data that points to the build_state. After a reboot rom_reset will restore the original contents of the linker-loader file. In theory, this is only temporary. However, due to the missing callback_opaque data the first call to acpi_update_build() will do nothing. As a result the OVMF guest reads an outdated version of the table-loader file. The actual tables are properly re-generated on the next access to a different FWCFG file that did not go through a resize. But at this point the guest has already read the outdated table-loader data and trying to apply this to the re-generated ACPI tables results in errors. This results in broken ACPI tables as discussed above. regards Christian