Re: [PATCH v3 07/70] physmem: Relax the alignment check of host_startaddr in ram_block_discard_range()

David Hildenbrand
2023-11-20T09:56:28Z

On 16.11.23 03:56, Xiaoyao Li wrote:
> On 11/16/2023 2:20 AM, David Hildenbrand wrote:
>> On 15.11.23 08:14, Xiaoyao Li wrote:
>>> Commit d3a5038c461 ("exec: ram_block_discard_range") introduced
>>> ram_block_discard_range() which grabs some code from
>>> ram_discard_range(). However, during code movement, it changed alignment
>>> check of host_startaddr from qemu_host_page_size to rb->page_size.
>>>
>>> When ramblock is back'ed by hugepage, it requires the startaddr to be
>>> huge page size aligned, which is a overkill. e.g., TDX's private-shared
>>> page conversion is done at 4KB granularity. Shared page is discarded
>>> when it gets converts to private and when shared page back'ed by
>>> hugepage it is going to fail on this check.
>>>
>>> So change to alignment check back to qemu_host_page_size.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> ---
>>> Changes in v3:
>>>   - Newly added in v3;
>>> ---
>>>   system/physmem.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/system/physmem.c b/system/physmem.c
>>> index c56b17e44df6..8a4e42c7cf60 100644
>>> --- a/system/physmem.c
>>> +++ b/system/physmem.c
>>> @@ -3532,7 +3532,7 @@ int ram_block_discard_range(RAMBlock *rb,
>>> uint64_t start, size_t length)
>>>       uint8_t *host_startaddr = rb->host + start;
>>> -    if (!QEMU_PTR_IS_ALIGNED(host_startaddr, rb->page_size)) {
>>> +    if (!QEMU_PTR_IS_ALIGNED(host_startaddr, qemu_host_page_size)) {
>>
>> For your use cases, rb->page_size should always match qemu_host_page_size.
>>
>> IIRC, we only set rb->page_size to different values for hugetlb. And
>> guest_memfd does not support hugetlb.
>>
>> Even if QEMU is using THP, rb->page_size should 4k.
>>
>> Please elaborate how you can actually trigger that. From what I recall,
>> guest_memfd is not compatible with hugetlb.
>
> It's the shared memory that can be back'ed by hugetlb.

Serious question: does that configuration make any sense to support at
this point? I claim: no.

>
> Later patch 9 introduces ram_block_convert_page(), which will discard
> shared memory when it gets converted to private. TD guest can request
> convert a 4K to private while the page is previously back'ed by hugetlb
> as 2M shared page.

So you can call ram_block_discard_guest_memfd_range() on subpage basis,
but not ram_block_discard_range().

ram_block_convert_range() would have to thought that that (questionable)
combination of hugetlb for shmem and ordinary pages for guest_memfd
cannot discard shared memory.

And it probably shouldn't either way. There are other problems when not
using hugetlb along with preallocation.

The check in ram_block_discard_range() is correct, whoever ends up
calling it has to stop calling it.

--
Cheers,

David / dhildenb