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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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.
> > > 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.
> > We have so far avoided exposing MFNs to HVM/PVH, but I don't see much
> > way to avoid this if there's no option to use IOMMU or NPT page-tables
> > with the PSP and we don't want to intercept PSP accesses in Xen and
> > translate requests on the fly.
>   
> Yeah, I think the same way too.
Regards,
Jason