From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 55F3F198842 for ; Tue, 28 Jan 2025 10:00:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738058442; cv=none; b=RCv6FRt5wYYEjBzdEJ+2YtQrAcj9pK7BRikVhKc8bBWjTgm++j/glJ4qNWBC4mPwiXnp8qLGhqi3yFUfrGez3xo8bHYrG5WTJCecOT2HldQ7zaBYf3FFHEyTq47oNMnOmjO4AmIoRaFv4wJQwxxdTyQ4f8t9gNlyjA1sdAtBYMk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738058442; c=relaxed/simple; bh=kZ5N8tHCHo1PrNZqE9WlUMEB8RGu5iVv5hQ4/VAaUpc=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=OL1irjx8FST93mma7TYZh01wZb9LnakSxY9bKUDQURBe82245qIrfaVCUR9ifrFmPJjkgpVgZaz2OoDu0JOhSl1FXQNz2UBjDgeyATHJDuC2MN3OKouNaevW1iXg821N+i1/PCrerq6okV2bXoZO1J4MulBVejJeRDzbKAku1so= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=G6vtGOYg; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="G6vtGOYg" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 99BF2C4CEE7; Tue, 28 Jan 2025 10:00:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1738058441; bh=kZ5N8tHCHo1PrNZqE9WlUMEB8RGu5iVv5hQ4/VAaUpc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=G6vtGOYgV8ruY7AF1SWwVYIpaG4+lPlu6Fkv24SvdbRutxSMsJW2GOgelXXeXHj9K XKgnKAamkuOpz34TgqYxcsJpPMfLV5NG62q9KQHMZExlr0iA66S3G35TfiZIWm5Znq JxxmXCl5fBxZsiB3BQXH7iCy4x1Q68HWSU+5tdxum/g3iHnB9J9gWrxU1uhl5yIZ1U +a0gED1Y4Ja2J4wBTqgE6H56TKmSBF7MlKxtsF7o1r/Zo3uRFunaDpxktlHv4oA6fa nEgtP1dxeW1ip/Ey9MYsC8ybPdtbf6f3M+S5/2Vhh3CWhrlzLnv2F6eJdMvFpsbHMw nwG3dIhbGJ8wA== Date: Tue, 28 Jan 2025 11:00:34 +0100 From: Mauro Carvalho Chehab To: Jonathan Cameron Cc: Igor Mammedov , "Michael S . Tsirkin" , Shiju Jose , , , Ani Sinha , Dongjiu Geng , Subject: Re: [PATCH 02/11] acpi/ghes: add a firmware file with HEST address Message-ID: <20250128110034.650445fc@foz.lan> In-Reply-To: <20250123100217.00007373@huawei.com> References: <20250123100217.00007373@huawei.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Em Thu, 23 Jan 2025 10:02:17 +0000 Jonathan Cameron escreveu: > On Wed, 22 Jan 2025 16:46:19 +0100 > Mauro Carvalho Chehab wrote: > > > Store HEST table address at GPA, placing its content at > > hest_addr_le variable. > > > > Signed-off-by: Mauro Carvalho Chehab > > Reviewed-by: Jonathan Cameron > > > A few trivial things inline. > > Jonathan > > > --- > > > > Change from v8: > > - hest_addr_lr is now pointing to the error source size and data. > > > > Signed-off-by: Mauro Carvalho Chehab > Bonus. I guess you really like this patch :) > > --- > > hw/acpi/ghes.c | 17 ++++++++++++++++- > > include/hw/acpi/ghes.h | 1 + > > 2 files changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c > > index 3f519ccab90d..34e3364d3fd8 100644 > > --- a/hw/acpi/ghes.c > > +++ b/hw/acpi/ghes.c > > @@ -30,6 +30,7 @@ > > > > #define ACPI_HW_ERROR_FW_CFG_FILE "etc/hardware_errors" > > #define ACPI_HW_ERROR_ADDR_FW_CFG_FILE "etc/hardware_errors_addr" > > +#define ACPI_HEST_ADDR_FW_CFG_FILE "etc/acpi_table_hest_addr" > > > > /* The max size in bytes for one error block */ > > #define ACPI_GHES_MAX_RAW_DATA_LENGTH (1 * KiB) > > @@ -261,7 +262,7 @@ static void build_ghes_error_table(GArray *hardware_errors, BIOSLinker *linker, > > } > > > > /* > > - * tell firmware to write hardware_errors GPA into > > + * Tell firmware to write hardware_errors GPA into > > Sneaky tidy up. No problem with it in general but adding noise here, so if there > are others in the series maybe gather them up in a cleanup patch. There are no other cleanups pending. Besides, as you noticed, this aligns with the comment below. So, I'm opting to add a note at the patch's description. > > > * hardware_errors_addr fw_cfg, once the former has been initialized. > > */ > > bios_linker_loader_write_pointer(linker, ACPI_HW_ERROR_ADDR_FW_CFG_FILE, 0, > > @@ -355,6 +356,8 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, > > > > acpi_table_begin(&table, table_data); > > > > + int hest_offset = table_data->len; > > Local style looks to be traditional C with definitions at top. Maybe define > hest_offset up a few lines and just set it here? Ok. I'll follow Igor's suggestion of using uint32_t. > > + > > /* Error Source Count */ > > build_append_int_noprefix(table_data, num_sources, 4); > > for (i = 0; i < num_sources; i++) { > > @@ -362,6 +365,15 @@ void acpi_build_hest(GArray *table_data, GArray *hardware_errors, > > } > > > > acpi_table_end(linker, &table); > > + > > + /* > > + * tell firmware to write into GPA the address of HEST via fw_cfg, > > Given the tidy up above, fix this one to have a capital T, or was this > where you meant to change it? OK. > > + * once initialized. > > + */ > > + bios_linker_loader_write_pointer(linker, > > + ACPI_HEST_ADDR_FW_CFG_FILE, 0, > > Could wrap less and stay under 80 chars as both lines above add up to 70 something Why? This follows QEMU coding style and lines aren't longer than 80 columns. Besides, at least for my eyes and some experience doing maintainership on other projects over the years, it is a lot quicker to identify function parameters if they're properly aligned with the parenthesis. Thanks, Mauro