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@suse.com>
---
I wonder how useful it is to include the alignment in the panic()
message. 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").

--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -231,10 +231,10 @@ retry:
 	/*
 	 * Get IO TLB memory from any location.
 	 */
-	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
+	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
 	if (!start)
-		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
-		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
+		panic("%s: Failed to allocate %lu bytes align=%#x\n",
+		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
 
 	/*
 	 * And replace that memory with pages under 4GB.

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@suse.com>
> ---
> 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.


> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -231,10 +231,10 @@ retry:
>  	/*
>  	 * Get IO TLB memory from any location.
>  	 */
> -	start = memblock_alloc(PAGE_ALIGN(bytes), PAGE_SIZE);
> +	start = memblock_alloc(PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  	if (!start)
> -		panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> -		      __func__, PAGE_ALIGN(bytes), PAGE_SIZE);
> +		panic("%s: Failed to allocate %lu bytes align=%#x\n",
> +		      __func__, PAGE_ALIGN(bytes), IO_TLB_SEGSIZE << IO_TLB_SHIFT);
>  
>  	/*
>  	 * And replace that memory with pages under 4GB.
> 

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@suse.com>
>> ---
>> 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.

Jan

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@suse.com>
> >> ---
> >> 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

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@suse.com>
>>>> ---
>>>> 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 ...

Jan

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@suse.com>
> >>>> ---
> >>>> 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).

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@suse.com>
> > >>>> ---
> > >>>> 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

On 15.09.2021 03:54, Stefano Stabellini wrote:
> 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@suse.com>
>>>>>>> ---
>>>>>>> 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

Oh, good point. When writing the patch I did take this into consideration,
but I had managed to forget that aspect in the meantime. No adjustment to
this effect needed then.

Jan