On 3/14/2025 6:23 PM, Chenyi Qiang wrote:
>
>
> 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,
>>>> §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.
>
> 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.
Hi David,
To add the change_attribute() listener priorities changes, I can think
of several steps:
1) change the *NotifyRamDiscard() to return the result, because change
attribute to private could return failure.
2) Add a list in confidential_guest_support structure (or some other
place) to save the wrapped the listener like VFIORamDiscardListener. And
add related listener_register/unregister() in kvm_region_add/del()
3) Add priority in listener and related handling to follow the listener
expected sequence.
Regarding the step 1), with change_attribute() listener, the to_private
path could also fail. The tricky part is still the error handling. If we
simply assume it couldn't fail, maybe add a g_assert() to avoid expected
case. Or we also need to add the undo operation for to_private case.
>
>>
>
