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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 02, 2025 at 09:49:30AM -0700, 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.

The plan for AMD-SEV support was to place a PSP driver in Xen, and not
let dom0 interact with the PSP directly (or at least that was my
impression from the talks we had about implementing SEV support).

Is what you use from the PSP fully isolated from the functionality
needed by SEV, so that we could still expose the interface you require
to dom0 while letting Xen drive the SEV parts?

Otherwise I think we need an agreement about how to integrate your
usage of the PSP with the SEV work, as having both Xen and dom0
driving the PSP in parallel might not be a wise idea.

Thanks, Roger.