Re: [PATCH v4 01/14] acpi/ghes: prepare to change the way HEST offsets are calculated

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 27 Feb 2025 12:45:38 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Em Wed, 26 Feb 2025 15:37:14 +0100
> Igor Mammedov <imammedo@redhat.com> escreveu:
> 
> > On Fri, 21 Feb 2025 15:35:10 +0100
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >   
> 
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 3ac8f8e17861..8ab8d11b6536 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -946,9 +946,18 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> > >      build_dbg2(tables_blob, tables->linker, vms);
> > >  
> > >      if (vms->ras) {
> > > -        acpi_add_table(table_offsets, tables_blob);
> > > -        acpi_build_hest(tables_blob, tables->hardware_errors, tables->linker,
> > > -                        vms->oem_id, vms->oem_table_id);
> > > +        AcpiGedState *acpi_ged_state;
> > > +        AcpiGhesState *ags;
> > > +
> > > +        acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,    
> >                             ^^^ will explode if object_resolve_path_type() returns NULL  
> > > +                                                       NULL));    
> > 
> > it's also expensive load-wise.
> > You have access to vms with ged pointer here, use that
> > (search for 'acpi_ged_state = ACPI_GED' example)  
> 
> Ok, but the state binding on ghes were designed to use ACPI_GED. I moved
> the code that it is using ACPI_GED() to the beginning of v5 series,
> just after the HEST table test addition.
> 
> With that, ACPI_GED() is now used only on two places inside ghes:
> 
> - at virt_acpi_build(), during VM initialization;

ACPI_GED() is not expensive, what I'm referring to is
  object_resolve_path_type()

given it's a new code and virt_acpi_build() has direct access
to ged pointer, there is no excuse to use object_resolve_path_type().

all you have to do here is:

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index e6328af5d2..040d875d4e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -949,8 +949,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
         AcpiGedState *acpi_ged_state;
         AcpiGhesState *ags;
 
-        acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
-                                                       NULL));
+        acpi_ged_state = ACPI_GED(vms->acpi_dev);
         ags = &acpi_ged_state->ghes_state;
         if (ags) {
             acpi_add_table(table_offsets, tables_blob);


> - at acpi_ghes_get_state().

this one is different, it doesn't have access to ged so it
has to look up for it.

> 
> If you want to replace it by some other solution, IMO we should do
> it on some separate series, as this is not related to neither error
> injection nor with offset calculation to get read ack address. 
> 
> > > +        if (acpi_ged_state) {    
> > 
> >                 hence, this check is not really needed,
> >                 we have to have GED at this point or abort
> > 
> >                 earlier code that instantiates GED should take care of
> >                 cleanly exiting if it failed to create GED so we would never get
> >                 to missing GED here  
> 
> I dropped this check on v5.
> 
> Thanks,
> Mauro
>