Re: [PATCH v5 10/10] ram-block-attribute: Add more error handling during state changes

[David Hildenbrand <david@redhat.com>]

On 26.05.25 12:19, Chenyi Qiang wrote:
> 
> 
> On 5/26/2025 5:17 PM, David Hildenbrand wrote:
>> On 20.05.25 12:28, Chenyi Qiang wrote:
>>> The current error handling is simple with the following assumption:
>>> - QEMU will quit instead of resuming the guest if kvm_convert_memory()
>>>     fails, thus no need to do rollback.
>>> - The convert range is required to be in the desired state. It is not
>>>     allowed to handle the mixture case.
>>> - The conversion from shared to private is a non-failure operation.
>>>
>>> This is sufficient for now as complext error handling is not required.
>>> For future extension, add some potential error handling.
>>> - For private to shared conversion, do the rollback operation if
>>>     ram_block_attribute_notify_to_populated() fails.
>>> - For shared to private conversion, still assert it as a non-failure
>>>     operation for now. It could be an easy fail path with in-place
>>>     conversion, which will likely have to retry the conversion until it
>>>     works in the future.
>>> - For mixture case, process individual blocks for ease of rollback.
>>>
>>> Signed-off-by: Chenyi Qiang <chenyi.qiang@intel.com>
>>> ---
>>>    system/ram-block-attribute.c | 116 +++++++++++++++++++++++++++--------
>>>    1 file changed, 90 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/system/ram-block-attribute.c b/system/ram-block-attribute.c
>>> index 387501b569..0af3396aa4 100644
>>> --- a/system/ram-block-attribute.c
>>> +++ b/system/ram-block-attribute.c
>>> @@ -289,7 +289,12 @@ static int
>>> ram_block_attribute_notify_to_discard(RamBlockAttribute *attr,
>>>            }
>>>            ret = rdl->notify_discard(rdl, &tmp);
>>>            if (ret) {
>>> -            break;
>>> +            /*
>>> +             * The current to_private listeners (VFIO dma_unmap and
>>> +             * KVM set_attribute_private) are non-failing operations.
>>> +             * TODO: add rollback operations if it is allowed to fail.
>>> +             */
>>> +            g_assert(ret);
>>>            }
>>>        }
>>>    
>>
>> If it's not allowed to fail for now, then patch #8 does not make sense
>> and should be dropped :)
> 
> It was intended for future extension as in-place conversion to_private
> allows it to fail. So I add the patch #8.
> 
> But as you mentioned, since the conversion path is changing, and maybe
> it is easier to handle from KVM code directly. Let me drop patch #8 and
> wait for the in-place conversion to mature.

Makes sense. I'm afraid it might all be a bit complicated to handle: 
vfio can fail private -> shared conversion and KVM the shared -> private 
conversion.

So recovering ... will not be straight forward once multiple pages are 
converted.

> 
>>
>> The implementations (vfio) should likely exit() instead on unexpected
>> errors when discarding.
> 
> After drop patch #8, maybe keep vfio discard handling as it was. Adding
> an additional exit() is also OK to me since it's non-fail case.
> 
>>
>>
>>
>> Why not squash all the below into the corresponding patch? Looks mostly
>> like handling partial conversions correctly (as discussed previously)?
> 
> I extract these two handling 1) mixture conversion; 2) operation
> rollback into this individual patch because they are not the practical
> cases and are untested.
> 
> For 1), I still don't see any real case which will convert a range with
> mixture attributes.

Okay. I thought we were not sure if the guest could trigger that?

I think it would be better to just include the "mixture" handling in the 
original patch.

> 
> For 2), current failure of memory conversion (as seen in kvm_cpu_exec()
> ->kvm_convert_memory()) will cause the QEMU to quit instead of resuming
> guest. Doing the rollback seems useless at present.

Makes sense.

-- 
Cheers,

David / dhildenb