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: <20240925100441.229790ba@foz.lan>
Date: Wed, 25 Sep 2024 10:04:41 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Peter Xu <peterx@...hat.com>
Cc: Igor Mammedov <imammedo@...hat.com>, Jonathan Cameron
 <Jonathan.Cameron@...wei.com>, Shiju Jose <shiju.jose@...wei.com>, "Michael
 S. Tsirkin" <mst@...hat.com>, Ani Sinha <anisinha@...hat.com>,
 linux-kernel@...r.kernel.org, qemu-devel@...gnu.org, Fabiano Rosas
 <farosas@...e.de>
Subject: Re: [PATCH v10 02/21] acpi/generic_event_device: Update GHES
 migration to cover hest addr

Em Tue, 17 Sep 2024 11:22:31 -0400
Peter Xu <peterx@...hat.com> escreveu:

> On Tue, Sep 17, 2024 at 11:19:21AM +0200, Igor Mammedov wrote:
> > On Sat, 14 Sep 2024 08:13:23 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@...nel.org> wrote:
> >   

> > what I would do:
> >   add ghes_needed_v2(): return  s->ghes_state.hest_addr_le;
> > 
> > and then instead of reusing vmstate_ghes_state would add new
> > vmstate_ghes_v2_state subsection that migrates only 
> >   VMSTATE_UINT64(hest_addr_le, AcpiGhesState)
> > field.
> > 
> > btw: we probably don't need ghes_addr_le for new code that
> > uses HEST to lookup relevant error status block.
> > but we should still keep it for 9.1 and older machine types
> > as they expect/use it. Separate subsections would work with
> > this req just fine.  

Ok, so, if I understood it right, the enclosed patch should do the
job, right?

> Right, if we need bi-directional migration we need above and a compat
> property (or VMSTATE_UINT64_TEST() would work too, iiuc).
> 
> OTOH VMSD versioning only works for forward migration, not backward.

I don't think bi-directional migration would work. See, with
old version, we have:

- Just one Error source block structure, only for ARMv8 using synchronous
  notification (SEA).
- The offsets to access the error block structure and the memory
  position where the OSPM will acknowledge the error assumes a single
  error source;
- such offsets come from ghes_addr_le bios pointer (we will rename it to
  hw_addr_le at the cleanup patch series).

With the new versions, we'll have:

- at least two notification sources on ARMv8 (SEA and GPIO). We may
  end adding more with time;
- other architectures may also have support for bios-first hardware
  errors;
- the number of error block structures is now read from HEST table
  (so it needs that hest_addr_le will be stored at AcpiGedState;
- the offsets to retrieve the addresses are now relative to the offset
  at hest_addr_le.

The new error injection code, which uses either hest_addr_le or
ghes_addr_le (now hw_addr_le) should be able to properly generate
errors from a VM created on 9.1, as it will check if hest_addr_le
is not zero. If it is zero, it will call a backward-compatible
code:

    acpi_ged_state = ACPI_GED(object_resolve_path_type("", TYPE_ACPI_GED,
                                                       NULL));
    if (!acpi_ged_state) {
        error_setg(errp, "Can't find ACPI_GED object");
        return;
    }
    ags = &acpi_ged_state->ghes_state;

    if (!ags->hest_addr_le) {
	/* Assumes just a single source_id */
        get_ghes_offsets(le64_to_cpu(ags->hw_error_le),
                         &cper_addr, &read_ack_register_addr);
    } else {
	/* do a for at the HEST table seeking for an specific source_id */
        get_hest_offsets(source_id, le64_to_cpu(ags->hest_addr_le),
                         &cper_addr, &read_ack_register_addr, errp);
    }

Now, a VM created with 9.2 will have multiple sources. The location of the
read_ack_register_addr depends on the number of sources, which can't be
retrieved without a VIOS pointer to the location of the HEST table
(e. g. ags->hest_addr_le).

So, a 9.1 QEMU with a VM created on 9.2 won't be doing the right thing
with regards to the vaule of the ack offset, thus with RAS errors not
working. I can't see any way to make it work.

> >   
> > >  static const VMStateDescription vmstate_ghes_state = {
> > >      .name = "acpi-ged/ghes",
> > > -    .version_id = 1,
> > > -    .minimum_version_id = 1,
> > > +    .version_id = 2,
> > > +    .minimum_version_id = 2,  
> 
> (and IIUC if we set min ver=2, even forward migration should fail.. better
>  test it with an old binary, migrating back and forth)
> 
> > >      .needed = ghes_needed,
> > >      .fields = (const VMStateField[]) {
> > >          VMSTATE_STRUCT(ghes_state, AcpiGedState, 1,  
> >   
> 
> Thanks,
> 

Thanks,
Mauro

---

[PATCH] acpi/generic_event_device: Update GHES migration to cover hest addr

The GHES migration logic at GED should now support HEST table
location too.

Increase migration version and change needed to check for both
ghes_addr_le and hest_addr_le.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index c5acfb204e5f..bd996d01390c 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -377,6 +377,34 @@ static const VMStateDescription vmstate_ghes_state = {
     }
 };
 
+static const VMStateDescription vmstate_hest = {
+    .name = "acpi-ghes",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64(hest_addr_le, AcpiGhesState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static bool hest_needed(void *opaque)
+{
+    AcpiGedState *s = opaque;
+    return s->ghes_state.hest_addr_le;
+}
+
+static const VMStateDescription vmstate_hest_state = {
+    .name = "acpi-ged/ghes",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = hest_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_STRUCT(ghes_state, AcpiGedState, 1,
+                       vmstate_hest, AcpiGhesState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_acpi_ged = {
     .name = "acpi-ged",
     .version_id = 1,
@@ -388,6 +416,7 @@ static const VMStateDescription vmstate_acpi_ged = {
     .subsections = (const VMStateDescription * const []) {
         &vmstate_memhp_state,
         &vmstate_ghes_state,
+        &vmstate_hest_state,
         NULL
     }
 };


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ