Re: [PATCH v6 3/6] accel/kvm: Report the loss of a large memory page

[William Roche <william.roche@oracle.com>]

On 1/30/25 18:02, David Hildenbrand wrote:
> On 27.01.25 22:31, “William Roche wrote:
>> From: William Roche <william.roche@oracle.com>
>>
>> In case of a large page impacted by a memory error, provide an
>> information about the impacted large page before the memory
>> error injection message.
>>
>> This message would also appear on ras enabled ARM platforms, with
>> the introduction of an x86 similar error injection message.
>>
>> In the case of a large page impacted, we now report:
>> Memory Error on large page from <backend>:<address>+<fd_offset> 
>> +<page_size>
>>
>> Signed-off-by: William Roche <william.roche@oracle.com>
>> ---
>>   accel/kvm/kvm-all.c       | 11 +++++++++++
>>   include/exec/cpu-common.h |  9 +++++++++
>>   system/physmem.c          | 21 +++++++++++++++++++++
>>   target/arm/kvm.c          |  3 +++
>>   4 files changed, 44 insertions(+)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index f89568bfa3..08e14f8960 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -1296,6 +1296,17 @@ static void kvm_unpoison_all(void *param)
>>   void kvm_hwpoison_page_add(ram_addr_t ram_addr)
>>   {
>>       HWPoisonPage *page;
>> +    struct RAMBlockInfo rb_info;
>> +
>> +    if (qemu_ram_block_location_info_from_addr(ram_addr, &rb_info)) {
>> +        size_t ps = rb_info.page_size;
>> +        if (ps > TARGET_PAGE_SIZE) {
>> +            uint64_t offset = ram_addr - rb_info.offset;
>> +            error_report("Memory Error on large page from %s:%" PRIx64
>> +                         "+%" PRIx64 " +%zx", rb_info.idstr,
>> +                         QEMU_ALIGN_DOWN(offset, ps), 
>> rb_info.fd_offset, ps);
>> +        }
>> +    }
> 
> Some smaller nits:
> 
> 1) I'd call it qemu_ram_block_info_from_addr() --  drop the "_location"
> 
> 2) Printing the fd_offset only makes sense if there is an fd, right? 
> You'd have to communicate that information as well.
> 
> 
> 
> Apart from that, this series LGTM, thanks!


Thank you David for your feedback.

I'll change the 2 nits above, and add the 2 empty lines missing (in 
patch 3/6).
I also removed the fd_offset information in the message from the 
qemu_ram_remap() function when we don't have an associated fd (patch 2/6).

You also asked me:

> Don't you have to align the fd_offset also down?
> 

fd_offset doesn't need to be aligned as it is used with the value given, 
which should already be adapted to the backend needs (when given by the 
administrator for example)


> I suggest doing the alignment already when calculating "uint64_t offset"
> 

But yes for the offset value itself, it's much better to already align 
it when giving it an initial value. Thanks, I've also made the change.

I'm sending a v7 now including all these changes.

I'll also send an update about the kdump behavior on ARM later next week.

Thanks again,
William.