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;
> + }
>
>
> 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.
>
> I thought the current implementation properly handles that internally?
> In which scenario is that not the case?
As mentioned above, e.g. if private_to_shared:
1. set_attribute_shared() convert to shared, but bitmap doesn't change
at that stage and is still private.
2. state_change() failed, it undo the operation and bitmap status to
keep unchanged.
3. core mm side status is inconsistent with bitmap.
>
