Re: [PATCH] xen/x86: allow Dom0 PVH to call XENMEM_exchange

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2025 18:49, Stefano Stabellini wrote:
> On Fri, 2 May 2025, Jan Beulich wrote:
>> On 01.05.2025 15:44, Jason Andryuk wrote:
>>> On 2025-04-30 20:19, Stefano Stabellini wrote:
>>>> On Wed, 30 Apr 2025, Roger Pau Monné wrote:
>>>>> On Wed, Apr 30, 2025 at 08:27:55AM +0200, Jan Beulich wrote:
>>>>>> On 29.04.2025 23:52, Stefano Stabellini wrote:
>>>>>>> On Tue, 29 Apr 2025, Jan Beulich wrote:
>>>>>>>> On 28.04.2025 22:00, Stefano Stabellini wrote:
>>>>>>>>> On Mon, 28 Apr 2025, Jan Beulich wrote:
>>>>>>>>>> On 25.04.2025 22:19, Stefano Stabellini wrote:
>>>
>>>>>>>>>>> --- a/xen/common/memory.c
>>>>>>>>>>> +++ b/xen/common/memory.c
>>>>>>>>>>> @@ -794,7 +794,7 @@ static long 
>>>>>>>>>>> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>>>>>>>>>>               rc = guest_physmap_add_page(d, _gfn(gpfn), mfn,
>>>>>>>>>>>                                           exch.out.extent_order) ?: 
>>>>>>>>>>> rc;
>>>>>>>>>>>   
>>>>>>>>>>> -            if ( !paging_mode_translate(d) &&
>>>>>>>>>>> +            if ( (!paging_mode_translate(d) || 
>>>>>>>>>>> is_hardware_domain(d)) &&
>>>>>>>>>>>                    __copy_mfn_to_guest_offset(exch.out.extent_start,
>>>>>>>>>>>                                               (i << 
>>>>>>>>>>> out_chunk_order) + j,
>>>>>>>>>>>                                               mfn) )
>>>>>>>>>>
>>>>>>>>>> Wait, no: A PVH domain (Dom0 or not) can't very well make use of 
>>>>>>>>>> MFNs, can
>>>>>>>>>> it?
>>>>>>>>>
>>>>>>>>> One way or another Dom0 PVH needs to know the MFN to pass it to the
>>>>>>>>> co-processor.
>>>>>>>>
>>>>>>>> I see. That's pretty odd, though. I'm then further concerned of the 
>>>>>>>> order of
>>>>>>>> the chunks. At present we're rather lax, in permitting PVH and PV Dom0 
>>>>>>>> the
>>>>>>>> same upper bound. With both CPU and I/O side translation there is, in
>>>>>>>> principle, no reason to permit any kind of contiguity. Of course 
>>>>>>>> there's a
>>>>>>>> performance aspect, but that hardly matters in the specific case here. 
>>>>>>>> Yet at
>>>>>>>> the same time, once we expose MFNs, contiguity will start mattering as 
>>>>>>>> soon
>>>>>>>> as any piece of memory needs to be larger than PAGE_SIZE. Which means 
>>>>>>>> it will
>>>>>>>> make tightening of the presently lax handling prone to regressions in 
>>>>>>>> this
>>>>>>>> new behavior you're introducing. What chunk size does the PSP driver 
>>>>>>>> require?
>>>>>>>
>>>>>>> I don't know. The memory returned by XENMEM_exchange is contiguous,
>>>>>>> right? Are you worried that Xen cannot allocate the requested amount of
>>>>>>> memory contiguously?
>>>
>>> In the case I looked at, it is 8 pages.  The driver defines a ring of 32 
>>> * 1k entries.  I'm not sure if there are other paths or device versions 
>>> where it might differ.
>>
>> As per this ...
>>
>>>>>> That would be Dom0's problem then. But really for a translated guest the
>>>>>> exchanged chunks being contiguous shouldn't matter, correctness-wise. 
>>>>>> That is,
>>>>>> within Xen, rather than failing a request, we could choose to retry using
>>>>>> discontiguous chunks (contiguous only in GFN space). Such an (afaict)
>>>>>> otherwise correct change would break your use case, as it would 
>>>>>> invalidate the
>>>>>> MFN information passed back. (This fallback approach would similarly 
>>>>>> apply to
>>>>>> other related mem-ops. It's just that during domain creation the tool 
>>>>>> stack
>>>>>> has its own fallback, so it may not be of much use right now.)
>>>>>
>>>>> I think the description in the public header needs to be expanded to
>>>>> specify what the XENMEM_exchange does for translated guests, and
>>>>> clearly write down that the underlying MFNs for the exchanged region
>>>>> will be contiguous.  Possibly a new XENMEMF_ flag needs to be added to
>>>>> request contiguous physical memory for the new range.
>>>>>
>>>>> Sadly this also has the side effect of quite likely shattering
>>>>> superpages for dom0 EPT/NPT, which will result in decreased dom0
>>>>> performance.
>>>
>>> Yes, this appears to happen as memory_exchange seems to always replace 
>>> the pages.  I tested returning the existing MFNs if they are already 
>>> contiguous since that was sufficient for this driver.  It worked, but it 
>>> was messy.  A big loop to copy in the GFN array and check contiguity 
>>> which duplicated much of the real loop.
>>
>> ... there may not be a need for the output range to be contiguous? In which
>> case - wouldn't a simple "give me the MFN for this GFN" hypercall do? I seem
>> to vaguely recall that we even had one, long ago; it was purged because of
>> it violating the "no MFNs exposed" principle (and because it not having had
>> any use [anymore]). XENMEM_translate_gpfn_list looks like is what I mean;
>> see commit 2d2f7977a052.
> 
> Unfortunately I don't think that would work because I am pretty sure
> that the PSP needs a consecutive range. In other words, I think the
> output needs to be contiguous.

Hmm, higher up (context) you said "ring of 32 * 1k entries". That reads like
independent 1k areas, and hence no need for contiguity.

Jan