linux-kernel - Re: [PATCH 08/15] acpi/ghes: Prepare to support multiple sources on ghes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241001072913.09f82e9f@foz.lan>
Date: Tue, 1 Oct 2024 07:29:13 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Igor Mammedov <imammedo@...hat.com>, Shiju Jose <shiju.jose@...wei.com>,
 "Michael S. Tsirkin" <mst@...hat.com>, Ani Sinha <anisinha@...hat.com>,
 Dongjiu Geng <gengdongjiu1@...il.com>, Peter Maydell
 <peter.maydell@...aro.org>, Shannon Zhao <shannon.zhaosl@...il.com>,
 <linux-kernel@...r.kernel.org>, <qemu-arm@...gnu.org>,
 <qemu-devel@...gnu.org>
Subject: Re: [PATCH 08/15] acpi/ghes: Prepare to support multiple sources on
 ghes

Em Wed, 25 Sep 2024 15:23:33 +0100
Jonathan Cameron <Jonathan.Cameron@...wei.com> escreveu:

> On Wed, 25 Sep 2024 06:04:13 +0200
> Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:
> 
> > The current code is actually dependent on having just one
> > error structure with a single source.
> > 
> > As the number of sources should be arch-dependent, as it
> > will depend on what kind of synchronous/assynchronous
> > notifications will exist, change the logic to dynamically
> > build the table.  
> Not really arch dependent.  Depends on both arch and some
> firmware implementation choices, but I guess that detail
> doesn't matter here.
> 
> > 
> > Yet, for a proper support, we need to get the number of
> > sources by reading the number from the HEST table. However,
> > bios currently doesn't store a pointer to it.
> > 
> > For now just change the logic at table build time, while
> > enforcing that it will behave like before with a single
> > source ID.
> > 
> > A future patch will add a HEST table bios pointer and
> > change the logic at acpi_ghes_record_errors() to
> > dynamically use the new size.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>  
> Trivial comment inline
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> 
> > @@ -335,9 +346,10 @@ static void build_ghes_v2(GArray *table_data,
> >      build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 0x40, 0,
> >                       4 /* QWord access */, 0);
> >      bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
> > -        address_offset + GAS_ADDR_OFFSET,
> > -        sizeof(uint64_t), ACPI_GHES_ERRORS_FW_CFG_FILE,
> > -        (ACPI_GHES_ERROR_SOURCE_COUNT + source_id) * sizeof(uint64_t));
> > +                                   address_offset + GAS_ADDR_OFFSET,  
> 
> I'd prefer if we avoided realigning unless absolutely necessary or
> that it is split into a separate patch.
> Makes things a tiny bit harder to review.

Heh, Igor nacked a patch doing the alignment change on a separate patch,
so let's do it at the patches that are actually changing the code.

At least for me, it is a low easier to review patches that are properly
aligned with parenthesis. So, yeah it may be a little more painful to
review a patch changing alignments, but IMO it pays off on future
revisions, specially if we place one argument per line, like in this
function.

> 
> > +                                   sizeof(uint64_t),
> > +                                   ACPI_GHES_ERRORS_FW_CFG_FILE,
> > +                                   (num_sources + index) * sizeof(uint64_t));
> >    
> 

Thanks,
Mauro