Re: [PATCH RESEND v2 1/3] acpi/ghes: Extend acpi_ghes_memory_errors() to support multiple CPERs

[Gavin Shan <gshan@redhat.com>]

Hi Jonathan,

On 10/31/25 8:08 PM, Jonathan Cameron wrote:
> On Fri, 31 Oct 2025 09:58:50 +0000
> Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> 
>> On Tue,  7 Oct 2025 16:08:08 +1000
>> Gavin Shan <gshan@redhat.com> wrote:
>>
>>> In the situation where host and guest has 64KB and 4KB page sizes, one
>>> error on the host's page affects 16 guest's pages. we need to send 16
>>> consective errors in this specific case.
>>
>> Hi Gavin,
>>
>> Sorry this one has been on my to review list far too long.
>>

No worries. Thanks for your review and comments. Igor suggested to have
separate error source for each vCPU. In that way, there will have multiple
read_ack_reg and the specific issue addressed by this series can be resolved.

Answering your question below.

>>>
>>> Extend acpi_ghes_memory_errors() to support multiple CPERs after the
>>> hunk of code to generate the GHES error status is pulled out from
>>> ghes_gen_err_data_uncorrectable_recoverable().
>>
>> I think this description needs to be more detailed wrt to how those
>> multiple CPERs are handled.  Specifically that they are in a single
>> error status block (so should only represent related errors.)
>>
>> This is to make it clear this isn't queuing events, but instead just
>> presenting them as one block.
>>
>>>
>>> No functional changes intended.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>>   hw/acpi/ghes-stub.c    |  2 +-
>>>   hw/acpi/ghes.c         | 27 ++++++++++++++-------------
>>>   include/hw/acpi/ghes.h |  2 +-
>>>   target/arm/kvm.c       |  7 ++++++-
>>>   4 files changed, 22 insertions(+), 16 deletions(-)
>>
>>> diff --git a/hw/acpi/ghes.c b/hw/acpi/ghes.c
>>> index 06555905ce..045b77715f 100644
>>> --- a/hw/acpi/ghes.c
>>> +++ b/hw/acpi/ghes.c
>>> @@ -214,18 +214,13 @@ static void acpi_ghes_build_append_mem_cper(GArray *table,
>>>   
>>>   static void
>>>   ghes_gen_err_data_uncorrectable_recoverable(GArray *block,
>>> -                                            const uint8_t *section_type,
>>> -                                            int data_length)
>>> +                                            const uint8_t *section_type)
>>>   {
>>>       /* invalid fru id: ACPI 4.0: 17.3.2.6.1 Generic Error Data,
>>>        * Table 17-13 Generic Error Data Entry
>>>        */
>>>       QemuUUID fru_id = {};
>>>   
>>> -    /* Build the new generic error status block header */
>>> -    acpi_ghes_generic_error_status(block, ACPI_GEBS_UNCORRECTABLE,
>>> -        0, 0, data_length, ACPI_CPER_SEV_RECOVERABLE);
>>> -]
>>
>> With this bit gone, is it worth having the helper?  Perhaps just move
>> the remains to where it is called.
>>

Ack.

>>>       /* Build this new generic error data entry header */
>>>       acpi_ghes_generic_error_data(block, section_type,
>>>           ACPI_CPER_SEV_RECOVERABLE, 0, 0,
>>
>>> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
>>> index 4f769d69b3..9a47ac9e3a 100644
>>> --- a/target/arm/kvm.c
>>> +++ b/target/arm/kvm.c
>>> @@ -2434,6 +2434,7 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>>>       ram_addr_t ram_addr;
>>>       hwaddr paddr;
>>>       AcpiGhesState *ags;
>>> +    GArray *addresses;
>>>   
>>>       assert(code == BUS_MCEERR_AR || code == BUS_MCEERR_AO);
>>>   
>>> @@ -2442,6 +2443,7 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>>>           ram_addr = qemu_ram_addr_from_host(addr);
>>>           if (ram_addr != RAM_ADDR_INVALID &&
>>>               kvm_physical_memory_addr_from_host(c->kvm_state, addr, &paddr)) {
>>> +            addresses = g_array_new(false, false, sizeof(paddr));
>>
>> Given you are going to free in all paths, maybe a g_autofree?
> 
> Oddly there is no use of g_auto() with g_array in qemu. Ah well, next suggestion is
> my preference anyway.
> 

Something like below can be used, and GArray is interchangeable to a regular array.
I guess GArray is preferred in QEMU.

     g_autoptr(GArray) sources = g_array_new(false, true,
                                             sizeof(AcpiNotificationSourceId));

>>
>> Also, we know this only grows to a fixed max size (16 after patch 3), so maybe just
>> provide a hwaddr paddrs[16]; and pass forwards the count?
>>
>>>               kvm_hwpoison_page_add(ram_addr);
>>>               /*
>>>                * If this is a BUS_MCEERR_AR, we know we have been called
>>> @@ -2454,16 +2456,19 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
>>>                * later from the main thread, so doing the injection of
>>>                * the error would be more complicated.
>>>                */
>>> +            g_array_append_vals(addresses, &paddr, 1);
>>>               if (code == BUS_MCEERR_AR) {
>>>                   kvm_cpu_synchronize_state(c);
>>>                   if (!acpi_ghes_memory_errors(ags, ACPI_HEST_SRC_ID_SYNC,
>>> -                                             paddr)) {
>>> +                                             addresses)) {
>>>                       kvm_inject_arm_sea(c);
>>>                   } else {
>>>                       error_report("failed to record the error");
>>>                       abort();
>>>                   }
>>>               }
>>> +
>>> +            g_array_free(addresses, true);
>>>               return;
>>>           }
>>>           if (code == BUS_MCEERR_AO) {
>>

Thanks,
Gavin