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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan