Re: [PATCH v3 6/7] memory: Attach MemoryAttributeManager to guest_memfd-backed RAMBlocks

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

On 3/14/2025 5:50 PM, David Hildenbrand wrote:
> 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,
>>> &section,
>>> +
>>> kvm_set_memory_attributes_cb,
>>> +                                                       &args);
>>> +        } else {
>>> +            ret = ram_discard_manager_replay_discarded(mr->rdm,
>>> &section,
>>> +
>>> 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.

Yes, "to_private" branch is unnecessary.

> 
>>>
>>> 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.

Good idea. I think listener priorities can make it more elegant with
in-place conversion. And the change_attribute() listener can be given a
highest or lowest priority. Maybe we can add this change in advance
before in-place conversion available.

>