Re: [PATCH 2/2] xen/swiotlb: don't destroy contiguous region in all cases

[Jürgen Groß <jgross@xxxxxxxx>]

On 12.02.25 08:38, Jan Beulich wrote:
> On 11.02.2025 13:04, Juergen Gross wrote:
> > In case xen_swiotlb_alloc_coherent() needed to create a contiguous
> > region only for other reason than the memory not being compliant with
> > the device's DMA mask, there is no reason why this contiguous region
> > should be destroyed by xen_swiotlb_free_coherent() later. Destroying
> > this region should be done only, if the memory of the region was
> > allocated with more stringent placement requirements than the memory
> > it did replace.
> I'm not convinced of this: Even the mere property of being contiguous
> may already be enough to warrant freeing when possible. The hypervisor
> may not have that many contiguous areas available. The bigger the
> chunk, the more important to give it back once no longer needed in
> this shape.
Really? When creating a domain Xen tries to use GB pages and 2MB pages as
much as possible. Why would this special case here have more restrictions?

> Plus also take into account how Xen behaves here: It specifically tries
> to hold back, during boot, lower addressed memory to later satisfy such
> requests. Hence even if you don't ask for address restricted memory,
> you may get back such. You'd need to compare input and output addresses,
> not input addresses and requested restriction to alleviate this.
Fair enough.

> > --- a/arch/x86/xen/mmu_pv.c
> > +++ b/arch/x86/xen/mmu_pv.c
> > @@ -2208,19 +2208,22 @@ void __init xen_init_mmu_ops(void)
> >   static unsigned long discontig_frames[1<<MAX_CONTIG_ORDER];
> >   
> >   #define VOID_PTE (mfn_pte(0, __pgprot(0)))
> > -static void xen_zap_pfn_range(unsigned long vaddr, unsigned int order,
> > -                               unsigned long *in_frames,
> > -                               unsigned long *out_frames)
> > +static int xen_zap_pfn_range(unsigned long vaddr, unsigned int order,
> > +                            unsigned long *in_frames,
> > +                            unsigned long *out_frames)
> >   {
> >         int i;
> > +       u64 address_bits = 0;
> First I was inclined to suggest to use paddr_t here, but ...
>
> >         struct multicall_space mcs;
> >   
> >   	xen_mc_batch();
> >         for (i = 0; i < (1UL<<order); i++, vaddr += PAGE_SIZE) {
> >                 mcs = __xen_mc_entry(0);
> >   
> > -		if (in_frames)
> > +               if (in_frames) {
> >                         in_frames[i] = virt_to_mfn((void *)vaddr);
> > +                       address_bits |= in_frames[i] << PAGE_SHIFT;
> ... why do a shift on every loop iteration when you can ...
>
> > +               }
> >   
> >   		MULTI_update_va_mapping(mcs.mc, vaddr, VOID_PTE, 0);
> >                 __set_phys_to_machine(virt_to_pfn((void *)vaddr), 
> > INVALID_P2M_ENTRY);
> > @@ -2229,6 +2232,8 @@ static void xen_zap_pfn_range(unsigned long vaddr, 
> > unsigned int order,
> >                         out_frames[i] = virt_to_pfn((void *)vaddr);
> >         }
> >         xen_mc_issue(0);
> > +
> > +       return fls64(address_bits);
> ... simply add in PAGE_SHIFT here, once?
True.

> > @@ -2321,7 +2326,8 @@ static int xen_exchange_memory(unsigned long extents_in, 
> > unsigned int order_in,
> >   
> >   int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
> >                                  unsigned int address_bits,
> > -                                dma_addr_t *dma_handle)
> > +                                dma_addr_t *dma_handle,
> > +                                unsigned int *address_bits_in)
> >   {
> >         unsigned long *in_frames = discontig_frames, out_frame;
> >         unsigned long  flags;
> > @@ -2336,7 +2342,7 @@ int xen_create_contiguous_region(phys_addr_t pstart, 
> > unsigned int order,
> >         spin_lock_irqsave(&xen_reservation_lock, flags);
> >   
> >   	/* 1. Zap current PTEs, remembering MFNs. */
> > -       xen_zap_pfn_range(vstart, order, in_frames, NULL);
> > +       *address_bits_in = xen_zap_pfn_range(vstart, order, in_frames, NULL);
> Nit: Converting plain int to unsigned int, when there's no real reason
> to do any conversion. Since xen_zap_pfn_range() can't return a negative
> value for the caller caring about the return value (yet more obviously
> so with the suggested adjustment, and then true for both callers), the
> function could easily return unsigned int.
Will change that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature