Re: [PATCH v4 10/13] memory: Change NotifyStateClear() definition to return the result

[Chenyi Qiang <chenyi.qiang@intel.com>]

On 5/9/2025 4:20 PM, David Hildenbrand wrote:
> On 09.05.25 04:38, Chao Gao wrote:
>> On Sun, Apr 27, 2025 at 10:26:52AM +0800, Chenyi Qiang wrote:
>>> Hi David,
>>>
>>> Any thought on patch 10-12, which is to move the change attribute into a
>>> priority listener. A problem is how to handle the error handling of
>>> private_to_shared failure. Previously, we thought it would never be able
>>> to fail, but right now, it is possible in corner cases (e.g. -ENOMEM) in
>>> set_attribute_private(). At present, I simply raise an assert instead of
>>> adding any rollback work (see patch 11).
>>
>> I took a look at patches 10-12, and here are my thoughts:
>>
>> Moving the change attribute into a priority listener seems sensible.
>> It can
>> ensure the correct order between setting memory attributes and VFIO's DMA
>> map/unmap operations, and it can also simplify rollbacks. Since
>> MemoryListener already uses a priority-based list, it should be a good
>> fit
>> for page conversion listeners.
>>
>> Regarding error handling, -ENOMEM won't occur during page conversion
>> because the attribute xarray on the KVM side is populated earlier when
>> QEMU
>> calls kvm_set_phys_mem() -> kvm_set_memory_attributes_private(). 
> 
> I'll note that, with guest_memfd supporting in-place conversion in the
> future, this conversion path will likely change, and we might more
> likely in getting more errors on some conversion paths. (e.g., shared ->
> private could fail).
> 
> But I agree, we should keep complex error handling out of the picture
> for now if not required.

OK, I'll keep the current to_private conversion path simple without
rollback. Just give an assert to kvm_set_memory_attributes_private() in
the listener.

Per the to_shared conversion, As Chao mentioned, since any
failure in page conversion currently leads to QEMU quit (as seen in
kvm_cpu_exec() -> kvm_convert_memory()), the complex rollback seems
meaningless as well. Not sure if we need to keep it with the expectation
that QEMU changes to resume guest with some error status returned in the
future.

>