Re: [PATCH 04/12] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests

[Stefano Stabellini]

On Tue, 14 Sep 2021, Stefano Stabellini wrote:
> On Tue, 14 Sep 2021, Jan Beulich wrote:
> > On 13.09.2021 22:31, Stefano Stabellini wrote:
> > > On Mon, 13 Sep 2021, Jan Beulich wrote:
> > >> On 11.09.2021 01:14, Stefano Stabellini wrote:
> > >>> On Tue, 7 Sep 2021, Jan Beulich wrote:
> > >>>> While the hypervisor hasn't been enforcing this, we would still better
> > >>>> avoid issuing requests with GFNs not aligned to the requested order.
> > >>>>
> > >>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > >>>> ---
> > >>>> I wonder how useful it is to include the alignment in the panic()
> > >>>> message.
> > >>>
> > >>> Not very useful given that it is static. I don't mind either way but you
> > >>> can go ahead and remove it if you prefer (and it would make the line
> > >>> shorter.)
> > >>>
> > >>>
> > >>>> I further wonder how useful it is to wrap "bytes" in
> > >>>> PAGE_ALIGN(), when it is a multiple of a segment's size anyway (or at
> > >>>> least was supposed to be, prior to "swiotlb-xen: maintain slab count
> > >>>> properly").
> > >>>
> > >>> This one I would keep, to make sure to print out the same amount passed
> > >>> to memblock_alloc.
> > >>
> > >> Oh - if I was to drop it from the printk(), I would have been meaning to
> > >> also drop it there. If it's useless, then it's useless everywhere.
> > > 
> > > That's fine too
> > 
> > Thanks, I'll see about dropping that then.
> > 
> > Another Arm-related question has occurred to me: Do you actually
> > mind the higher-than-necessary alignment there? If so, a per-arch
> > definition of the needed alignment would need introducing. Maybe
> > that could default to PAGE_SIZE, allowing Arm and alike to get away
> > without explicitly specifying a value ...
> 
> Certainly a patch like that could be good. Given that it is only one
> allocation I was assuming that the higher-than-necessary alignment
> wouldn't be a problem worth addressing (and I cannot completely rule out
> that one day we might have to use XENMEM_exchange on ARM too).

Also this code is currently #ifdef CONFIG_X86