On 14.03.25 10:30, Chenyi Qiang wrote:
On 3/14/2025 5:00 PM, David Hildenbrand wrote:
On 14.03.25 09:21, Chenyi Qiang wrote:
Hi David & Alexey,
Hi,
To keep the bitmap aligned, I add the undo operation for
set_memory_attributes() and use the bitmap + replay callback to do
set_memory_attributes(). Does this change make sense?
I assume you mean this hunk:
+ ret =
memory_attribute_manager_state_change(MEMORY_ATTRIBUTE_MANAGER(mr->rdm),
+ offset, size, to_private);
+ if (ret) {
+ warn_report("Failed to notify the listener the state change of "
+ "(0x%"HWADDR_PRIx" + 0x%"HWADDR_PRIx") to %s",
+ start, size, to_private ? "private" : "shared");
+ args.to_private = !to_private;
+ if (to_private) {
+ ret = ram_discard_manager_replay_populated(mr->rdm, §ion,
+
kvm_set_memory_attributes_cb,
+ &args);
+ } else {
+ ret = ram_discard_manager_replay_discarded(mr->rdm, §ion,
+
kvm_set_memory_attributes_cb,
+ &args);
+ }
+ if (ret) {
+ goto out_unref;
+ }
We should probably document that memory_attribute_state_change() cannot
fail with "to_private", so you can simplify it to only handle the "to
shared" case.
Why is that undo necessary? The bitmap + listeners should be held in
sync inside of
memory_attribute_manager_state_change(). Handling this in the caller
looks wrong.
state_change() handles the listener, i.e. VFIO/IOMMU. And the caller
handles the core mm side (guest_memfd set_attribute()) undo if
state_change() failed. Just want to keep the attribute consistent with
the bitmap on both side. Do we need this? If not, the bitmap can only
represent the status of listeners.
Ah, so you meant that you effectively want to undo the attribute change,
because the state effectively cannot change, and we want to revert the
attribute change.
That makes sense when we are converting private->shared.
BTW, I'm thinking if the orders should be the following (with in-place
conversion in mind where we would mmap guest_memfd for the shared memory
parts).
On private -> shared conversion:
(1) change_attribute()
(2) state_change(): IOMMU pins shared memory
(3) restore_attribute() if it failed
On shared -> private conversion
(1) state_change(): IOMMU unpins shared memory
(2) change_attribute(): can convert in-place because there are not pins
I'm wondering if the whole attribute change could actually be a
listener, invoked by the memory_attribute_manager_state_change() call
itself in the right order.
We'd probably need listener priorities, and invoke them in reverse order
on shared -> private conversion. Just an idea to get rid of the manual
ram_discard_manager_replay_discarded() call in your code.
--
Cheers,
David / dhildenbh
