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

[Chao Gao <chao.gao@intel.com>]

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(). Other
errors, such as -EINVAL and -EFAULT, are unlikely to occur unless there is
a bug in QEMU. So, the assertion is appropriate for now. And, since any
failure in page conversion currently leads to QEMU quit (as seen in
kvm_cpu_exec() -> kvm_convert_memory()), implementing a complex rollback in
this case doesn't add value and merely adds code that is difficult to test.

Let's see what others think.