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.
