The primary intention really was the last patch, there you go ...
01: swiotlb-xen: avoid double free
02: swiotlb-xen: fix late init retry
03: swiotlb-xen: maintain slab count properly
04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
05: swiotlb-xen: suppress certain init retries
06: swiotlb-xen: limit init retries
07: swiotlb-xen: drop leftover __ref
08: swiotlb-xen: arrange to have buffer info logged
09: swiotlb-xen: drop DEFAULT_NSLABS
10: xen-pcifront: this module is PV-only
11: xen/pci-swiotlb: reduce visibility of symbols
12: swiotlb-xen: this is PV-only on x86
Jan
I consider it unhelpful that address and size of the buffer aren't put
in the log file; it makes diagnosing issues needlessly harder. The
majority of callers of swiotlb_init() also passes 1 for the "verbose"
parameter.
Signed-off-by: Jan Beulich <[email protected]>
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -253,7 +253,7 @@ retry:
panic("%s (rc:%d)", xen_swiotlb_error(XEN_SWIOTLB_EFIXUP), rc);
}
- if (swiotlb_init_with_tbl(start, nslabs, false))
+ if (swiotlb_init_with_tbl(start, nslabs, true))
panic("Cannot allocate SWIOTLB buffer");
swiotlb_set_max_segment(PAGE_SIZE);
}
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 <[email protected]>
---
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.
Generic swiotlb code makes sure to keep the slab count a multiple of the
number of slabs per segment. Yet even without checking whether any such
assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup()
might alter unrelated memory when calling xen_create_contiguous_region()
for the last segment, when that's not a full one - the function acts on
full order-N regions, not individual pages.
Align the slab count suitably when halving it for a retry. Add a build
time check and a runtime one. Replace the no longer useful local
variable "slabs" by an "order" one calculated just once, outside of the
loop. Re-use "order" for calculating "dma_bits", and change the type of
the latter as well as the one of "i" while touching this anyway.
Signed-off-by: Jan Beulich <[email protected]>
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct
static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
{
- int i, rc;
- int dma_bits;
+ int rc;
+ unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
+ unsigned int i, dma_bits = order + PAGE_SHIFT;
dma_addr_t dma_handle;
phys_addr_t p = virt_to_phys(buf);
- dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
+ BUILD_BUG_ON(IO_TLB_SEGSIZE & (IO_TLB_SEGSIZE - 1));
+ BUG_ON(nslabs % IO_TLB_SEGSIZE);
i = 0;
do {
- int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
-
do {
rc = xen_create_contiguous_region(
- p + (i << IO_TLB_SHIFT),
- get_order(slabs << IO_TLB_SHIFT),
+ p + (i << IO_TLB_SHIFT), order,
dma_bits, &dma_handle);
} while (rc && dma_bits++ < MAX_DMA_BITS);
if (rc)
return rc;
- i += slabs;
+ i += IO_TLB_SEGSIZE;
} while (i < nslabs);
return 0;
}
@@ -210,7 +209,7 @@ retry:
error:
if (repeat--) {
/* Min is 2MB */
- nslabs = max(1024UL, (nslabs >> 1));
+ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
bytes = nslabs << IO_TLB_SHIFT;
pr_info("Lowering to %luMB\n", bytes >> 20);
goto retry;
@@ -245,7 +244,7 @@ retry:
memblock_free(__pa(start), PAGE_ALIGN(bytes));
if (repeat--) {
/* Min is 2MB */
- nslabs = max(1024UL, (nslabs >> 1));
+ nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
bytes = nslabs << IO_TLB_SHIFT;
pr_info("Lowering to %luMB\n", bytes >> 20);
goto retry;
Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not
only have added __init to the split off function, but also should have
dropped __ref from the one left.
Signed-off-by: Jan Beulich <[email protected]>
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enu
#define DEFAULT_NSLABS ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
-int __ref xen_swiotlb_init(void)
+int xen_swiotlb_init(void)
{
enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
unsigned long bytes = swiotlb_size_or_default();
Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error"
label it is useful to retry the allocation; the first one did already
iterate through all possible order values.
Signed-off-by: Jan Beulich <[email protected]>
---
I'm not convinced of the (lack of) indentation of the label, but I made
the new one matzch the existing one.
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -184,7 +184,7 @@ retry:
order--;
}
if (!start)
- goto error;
+ goto exit;
if (order != get_order(bytes)) {
pr_warn("Warning: only able to allocate %ld MB for software IO TLB\n",
(PAGE_SIZE << order) >> 20);
@@ -214,6 +214,7 @@ error:
pr_info("Lowering to %luMB\n", bytes >> 20);
goto retry;
}
+exit:
pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
return rc;
}
On Tue, Sep 07, 2021 at 02:05:32PM +0200, 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 <[email protected]>
> ---
> 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);
CAn you avoid the overly long lines here? A good way to make it more
readable would be a variable to hold the byte count.
On Tue, Sep 07, 2021 at 02:07:21PM +0200, Jan Beulich wrote:
> I consider it unhelpful that address and size of the buffer aren't put
> in the log file; it makes diagnosing issues needlessly harder. The
> majority of callers of swiotlb_init() also passes 1 for the "verbose"
> parameter.
>
> Signed-off-by: Jan Beulich <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Sep 07, 2021 at 02:05:12PM +0200, Jan Beulich wrote:
> Generic swiotlb code makes sure to keep the slab count a multiple of the
> number of slabs per segment. Yet even without checking whether any such
> assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup()
> might alter unrelated memory when calling xen_create_contiguous_region()
> for the last segment, when that's not a full one - the function acts on
> full order-N regions, not individual pages.
>
> Align the slab count suitably when halving it for a retry. Add a build
> time check and a runtime one. Replace the no longer useful local
> variable "slabs" by an "order" one calculated just once, outside of the
> loop. Re-use "order" for calculating "dma_bits", and change the type of
> the latter as well as the one of "i" while touching this anyway.
>
> Signed-off-by: Jan Beulich <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Sep 07, 2021 at 02:05:54PM +0200, Jan Beulich wrote:
> Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error"
> label it is useful to retry the allocation; the first one did already
> iterate through all possible order values.
>
> Signed-off-by: Jan Beulich <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Sep 07, 2021 at 02:06:55PM +0200, Jan Beulich wrote:
> Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not
> only have added __init to the split off function, but also should have
> dropped __ref from the one left.
>
> Signed-off-by: Jan Beulich <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On 08.09.2021 08:57, Christoph Hellwig wrote:
> On Tue, Sep 07, 2021 at 02:05:32PM +0200, 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 <[email protected]>
>> ---
>> 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);
>
> CAn you avoid the overly long lines here? A good way to make it more
> readable would be a variable to hold the byte count.
There already is a variable for the byte count - bytes. Did you read
the post-commit-message remark? _That's_ what I'd prefer to shorten
things. Meanwhile I can of course wrap lines; I will admit that I
failed to pay attention to line length here.
Jan
On 07.09.21 14:03, Jan Beulich wrote:
> The primary intention really was the last patch, there you go ...
>
> 01: swiotlb-xen: avoid double free
> 02: swiotlb-xen: fix late init retry
> 03: swiotlb-xen: maintain slab count properly
> 04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
> 05: swiotlb-xen: suppress certain init retries
> 06: swiotlb-xen: limit init retries
> 07: swiotlb-xen: drop leftover __ref
> 08: swiotlb-xen: arrange to have buffer info logged
> 09: swiotlb-xen: drop DEFAULT_NSLABS
> 10: xen-pcifront: this module is PV-only
> 11: xen/pci-swiotlb: reduce visibility of symbols
> 12: swiotlb-xen: this is PV-only on x86
>
> Jan
>
You should have Cc-ed Konrad being the maintainer and
[email protected]. Doing so now.
Juergen
On Tue, 7 Sep 2021, Jan Beulich wrote:
> Generic swiotlb code makes sure to keep the slab count a multiple of the
> number of slabs per segment. Yet even without checking whether any such
> assumption is made elsewhere, it is easy to see that xen_swiotlb_fixup()
> might alter unrelated memory when calling xen_create_contiguous_region()
> for the last segment, when that's not a full one - the function acts on
> full order-N regions, not individual pages.
>
> Align the slab count suitably when halving it for a retry. Add a build
> time check and a runtime one. Replace the no longer useful local
> variable "slabs" by an "order" one calculated just once, outside of the
> loop. Re-use "order" for calculating "dma_bits", and change the type of
> the latter as well as the one of "i" while touching this anyway.
>
> Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -106,27 +106,26 @@ static int is_xen_swiotlb_buffer(struct
>
> static int xen_swiotlb_fixup(void *buf, unsigned long nslabs)
> {
> - int i, rc;
> - int dma_bits;
> + int rc;
> + unsigned int order = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT);
> + unsigned int i, dma_bits = order + PAGE_SHIFT;
> dma_addr_t dma_handle;
> phys_addr_t p = virt_to_phys(buf);
>
> - dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
> + BUILD_BUG_ON(IO_TLB_SEGSIZE & (IO_TLB_SEGSIZE - 1));
> + BUG_ON(nslabs % IO_TLB_SEGSIZE);
>
> i = 0;
> do {
> - int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
> -
> do {
> rc = xen_create_contiguous_region(
> - p + (i << IO_TLB_SHIFT),
> - get_order(slabs << IO_TLB_SHIFT),
> + p + (i << IO_TLB_SHIFT), order,
> dma_bits, &dma_handle);
> } while (rc && dma_bits++ < MAX_DMA_BITS);
> if (rc)
> return rc;
>
> - i += slabs;
> + i += IO_TLB_SEGSIZE;
> } while (i < nslabs);
> return 0;
> }
> @@ -210,7 +209,7 @@ retry:
> error:
> if (repeat--) {
> /* Min is 2MB */
> - nslabs = max(1024UL, (nslabs >> 1));
> + nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
> bytes = nslabs << IO_TLB_SHIFT;
> pr_info("Lowering to %luMB\n", bytes >> 20);
> goto retry;
> @@ -245,7 +244,7 @@ retry:
> memblock_free(__pa(start), PAGE_ALIGN(bytes));
> if (repeat--) {
> /* Min is 2MB */
> - nslabs = max(1024UL, (nslabs >> 1));
> + nslabs = max(1024UL, ALIGN(nslabs >> 1, IO_TLB_SEGSIZE));
> bytes = nslabs << IO_TLB_SHIFT;
> pr_info("Lowering to %luMB\n", bytes >> 20);
> goto retry;
>
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 <[email protected]>
> ---
> 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 Tue, 7 Sep 2021, Jan Beulich wrote:
> Only on the 2nd of the paths leading to xen_swiotlb_init()'s "error"
> label it is useful to retry the allocation; the first one did already
> iterate through all possible order values.
>
> Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
> ---
> I'm not convinced of the (lack of) indentation of the label, but I made
> the new one matzch the existing one.
>
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -184,7 +184,7 @@ retry:
> order--;
> }
> if (!start)
> - goto error;
> + goto exit;
> if (order != get_order(bytes)) {
> pr_warn("Warning: only able to allocate %ld MB for software IO TLB\n",
> (PAGE_SIZE << order) >> 20);
> @@ -214,6 +214,7 @@ error:
> pr_info("Lowering to %luMB\n", bytes >> 20);
> goto retry;
> }
> +exit:
> pr_err("%s (rc:%d)\n", xen_swiotlb_error(m_ret), rc);
> return rc;
> }
>
On Tue, 7 Sep 2021, Jan Beulich wrote:
> Commit a98f565462f0 ("xen-swiotlb: split xen_swiotlb_init") should not
> only have added __init to the split off function, but also should have
> dropped __ref from the one left.
>
> Signed-off-by: Jan Beulich <[email protected]>
Reviewed-by: Stefano Stabellini <[email protected]>
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -154,7 +154,7 @@ static const char *xen_swiotlb_error(enu
>
> #define DEFAULT_NSLABS ALIGN(SZ_64M >> IO_TLB_SHIFT, IO_TLB_SEGSIZE)
>
> -int __ref xen_swiotlb_init(void)
> +int xen_swiotlb_init(void)
> {
> enum xen_swiotlb_err m_ret = XEN_SWIOTLB_UNKNOWN;
> unsigned long bytes = swiotlb_size_or_default();
>
On Tue, 7 Sep 2021, Jan Beulich wrote:
> I consider it unhelpful that address and size of the buffer aren't put
> in the log file; it makes diagnosing issues needlessly harder. The
> majority of callers of swiotlb_init() also passes 1 for the "verbose"
> parameter.
>
> Signed-off-by: Jan Beulich <[email protected]>
Acked-by: Stefano Stabellini <[email protected]>
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -253,7 +253,7 @@ retry:
> panic("%s (rc:%d)", xen_swiotlb_error(XEN_SWIOTLB_EFIXUP), rc);
> }
>
> - if (swiotlb_init_with_tbl(start, nslabs, false))
> + if (swiotlb_init_with_tbl(start, nslabs, true))
> panic("Cannot allocate SWIOTLB buffer");
> swiotlb_set_max_segment(PAGE_SIZE);
> }
>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
> >> ---
> >> 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 07.09.21 14:03, Jan Beulich wrote:
> The primary intention really was the last patch, there you go ...
>
> 01: swiotlb-xen: avoid double free
> 02: swiotlb-xen: fix late init retry
> 03: swiotlb-xen: maintain slab count properly
> 04: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
> 05: swiotlb-xen: suppress certain init retries
> 06: swiotlb-xen: limit init retries
> 07: swiotlb-xen: drop leftover __ref
> 08: swiotlb-xen: arrange to have buffer info logged
> 09: swiotlb-xen: drop DEFAULT_NSLABS
> 10: xen-pcifront: this module is PV-only
> 11: xen/pci-swiotlb: reduce visibility of symbols
> 12: swiotlb-xen: this is PV-only on x86
Pushed patches 1-3 and 5-9 to xen/tip.git for-linus-5.15
Juergen
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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
> >>>> ---
> >>>> 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 <[email protected]>
> > >>>> ---
> > >>>> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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