Re: [PATCH RESEND v2 2/3] kvm/arm/kvm: Introduce helper push_ghes_memory_errors()

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 3 Nov 2025 09:39:50 +1000
Gavin Shan <gshan@redhat.com> wrote:

> On 10/31/25 8:09 PM, Jonathan Cameron wrote:
> > On Tue,  7 Oct 2025 16:08:09 +1000
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> >> Introduce helper push_ghes_memory_errors(), which sends ACPI GHES memory
> >> errors, injects SEA exception or aborts on errors. This function will
> >> be extended to support multiple ACPI GHES memory errors in the next
> >> path.
> >>
> >> No functional changes intended.
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >>   target/arm/kvm.c | 36 ++++++++++++++++++++++++------------
> >>   1 file changed, 24 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> >> index 9a47ac9e3a..c5d5b3b16e 100644
> >> --- a/target/arm/kvm.c
> >> +++ b/target/arm/kvm.c
> >> @@ -2429,12 +2429,34 @@ int kvm_arch_get_registers(CPUState *cs, Error **errp)
> >>       return ret;
> >>   }
> >>   
> >> +static void push_ghes_memory_errors(CPUState *c, AcpiGhesState *ags,
> >> +                                    uint64_t paddr)  
> > 
> > Why not hwaddr paddr?
> >   
> 
> Because acpi_ghes_memory_errors() accepts it as uint64_t.

ack to that, it's uint64_t in spec so I'd stick to that.

> >> +{
> >> +    GArray *addresses = g_array_new(false, false, sizeof(paddr));  
> > 
> > As in previous I'd just have
> > 	hwaddr paddrs[16];
> > 
> > rather than bothering with a g_array.
> >   
> 
> Ok.
> 
> >> +    int ret;
> >> +
> >> +    kvm_cpu_synchronize_state(c);
> >> +    g_array_append_vals(addresses, &paddr, 1);
> >> +    ret = acpi_ghes_memory_errors(ags, ACPI_HEST_SRC_ID_SYNC, addresses);
> >> +    if (ret) {
> >> +        goto error;
> >> +    }
> >> +
> >> +    kvm_inject_arm_sea(c);
> >> +
> >> +    g_array_free(addresses, true);
> >> +
> >> +    return;
> >> +error:
> >> +    error_report("failed to record the error");  
> > 
> > I'd just do this inline at the error case. In the next
> > patch you add a more specific report of why to another path
> > that would then be followed by this.
> >   
> 
> Ok.
> 
> >> +    abort();  
> > 
> > If you do the above with the message, just duplicate this in the
> > two error paths (by end of next patch).
> >   
> 
> Will correct this in next revision if we still take current design. Igor suggested
> to have individual error source per vCPU in another thread.
> 
> >> +}  
> >   
> 
> Thanks,
> Gavin
>