[PATCH v2 4/4] swiotlb-xen: this is PV-only on x86
The code is unreachable for HVM or PVH, and it also makes little sense
in auto-translated environments. On Arm, with
xen_{create,destroy}_contiguous_region() both being stubs, I have a hard
time seeing what good the Xen specific variant does - the generic one
ought to be fine for all purposes there. Still Arm code explicitly
references symbols here, so the code will continue to be included there.
Instead of making PCI_XEN's "select" conditional, simply drop it -
SWIOTLB_XEN will be available unconditionally in the PV case anyway, and
is - as explained above - dead code in non-PV environments.
This in turn allows dropping the stubs for
xen_{create,destroy}_contiguous_region(), the former of which was broken
anyway - it failed to set the DMA handle output.
Signed-off-by: Jan Beulich
Reviewed-by: Christoph Hellwig
Reviewed-by: Stefano Stabellini
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2605,7 +2605,6 @@ config PCI_OLPC
config PCI_XEN
def_bool y
depends on PCI && XEN
- select SWIOTLB_XEN
config MMCONF_FAM10H
def_bool y
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -177,6 +177,7 @@ config XEN_GRANT_DMA_ALLOC
config SWIOTLB_XEN
def_bool y
+ depends on XEN_PV || ARM || ARM64
select DMA_OPS
select SWIOTLB
--- a/include/xen/xen-ops.h
+++ b/include/xen/xen-ops.h
@@ -46,19 +46,7 @@ extern unsigned long *xen_contiguous_bit
int xen_create_contiguous_region(phys_addr_t pstart, unsigned int order,
unsigned int address_bits,
dma_addr_t *dma_handle);
-
void xen_destroy_contiguous_region(phys_addr_t pstart, unsigned int order);
-#else
-static inline int xen_create_contiguous_region(phys_addr_t pstart,
- unsigned int order,
- unsigned int address_bits,
- dma_addr_t *dma_handle)
-{
- return 0;
-}
-
-static inline void xen_destroy_contiguous_region(phys_addr_t pstart,
-unsigned int order) { }
#endif
#if defined(CONFIG_XEN_PV)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 3/4] xen/pci-swiotlb: reduce visibility of symbols
xen_swiotlb and pci_xen_swiotlb_init() are only used within the file
defining them, so make them static and remove the stubs. Otoh
pci_xen_swiotlb_detect() has a use (as function pointer) from the main
pci-swiotlb.c file - convert its stub to a #define to NULL.
Signed-off-by: Jan Beulich
Reviewed-by: Christoph Hellwig
--- a/arch/x86/include/asm/xen/swiotlb-xen.h
+++ b/arch/x86/include/asm/xen/swiotlb-xen.h
@@ -3,14 +3,10 @@
#define _ASM_X86_SWIOTLB_XEN_H
#ifdef CONFIG_SWIOTLB_XEN
-extern int xen_swiotlb;
extern int __init pci_xen_swiotlb_detect(void);
-extern void __init pci_xen_swiotlb_init(void);
extern int pci_xen_swiotlb_init_late(void);
#else
-#define xen_swiotlb (0)
-static inline int __init pci_xen_swiotlb_detect(void) { return 0; }
-static inline void __init pci_xen_swiotlb_init(void) { }
+#define pci_xen_swiotlb_detect NULL
static inline int pci_xen_swiotlb_init_late(void) { return -ENXIO; }
#endif
--- a/arch/x86/xen/pci-swiotlb-xen.c
+++ b/arch/x86/xen/pci-swiotlb-xen.c
@@ -18,7 +18,7 @@
#endif
#include
-int xen_swiotlb __read_mostly;
+static int xen_swiotlb __read_mostly;
/*
* pci_xen_swiotlb_detect - set xen_swiotlb to 1 if necessary
@@ -56,7 +56,7 @@ int __init pci_xen_swiotlb_detect(void)
return xen_swiotlb;
}
-void __init pci_xen_swiotlb_init(void)
+static void __init pci_xen_swiotlb_init(void)
{
if (xen_swiotlb) {
xen_swiotlb_init_early();
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 2/4] PCI: only build xen-pcifront in PV-enabled environments
The driver's module init function, pcifront_init(), invokes xen_pv_domain() first thing. That construct produces constant "false" when !CONFIG_XEN_PV. Hence there's no point building the driver in non-PV configurations. Drop the (now implicit and generally wrong) X86 dependency: At present, XEN_PV con only be set when X86 is also enabled. In general an architecture supporting Xen PV (and PCI) would want to have this driver built. Signed-off-by: Jan Beulich Reviewed-by: Stefano Stabellini --- v2: Title and description redone. --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -110,7 +110,7 @@ config PCI_PF_STUB config XEN_PCIDEV_FRONTEND tristate "Xen PCI Frontend" - depends on X86 && XEN + depends on XEN_PV select PCI_XEN select XEN_XENBUS_FRONTEND default y ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 1/4] swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests
While the hypervisor hasn't been enforcing this, we would still better
avoid issuing requests with GFNs not aligned to the requested order.
Instead of altering the value also in the call to panic(), drop it
there for being static and hence easy to determine without being part
of the panic message.
Signed-off-by: Jan Beulich
---
I question 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"). But that's
perhaps yet another separate patch.
---
v2: Drop logging of alignment. Wrap lines.
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -230,10 +230,11 @@ 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\n",
+ __func__, PAGE_ALIGN(bytes));
/*
* And replace that memory with pages under 4GB.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v2 0/4] swiotlb-xen: remaining fixes and adjustments
The primary intention really was the last patch, there you go (on top of what is already in xen/tip.git for-linus-5.15) ... 1: swiotlb-xen: ensure to issue well-formed XENMEM_exchange requests 2: PCI: only build xen-pcifront in PV-enabled environments 3: xen/pci-swiotlb: reduce visibility of symbols 4: swiotlb-xen: this is PV-only on x86 Jan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On 17.09.2021 11:36, Roman Skakun wrote: > I use Xen PV display. In my case, PV display backend(Dom0) allocates > contiguous buffer via DMA-API to > to implement zero-copy between Dom0 and DomU. Why does the buffer need to be allocated by Dom0? If it was allocated by DomU, it could use grants to give the backend direct (zero-copy) access. Jan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On 15.09.2021 15:37, Roman Skakun wrote: >>> From: Roman Skakun >>> >>> It is possible when default IO TLB size is not >>> enough to fit a long buffers as described here [1]. >>> >>> This patch makes a way to set this parameter >>> using cmdline instead of recompiling a kernel. >>> >>> [1] https://www.xilinx.com/support/answers/72694.html >> >> I'm not convinced the swiotlb use describe there falls under "intended >> use" - mapping a 1280x720 framebuffer in a single chunk? > > I had the same issue while mapping DMA chuck ~4MB for gem fb when > using xen vdispl. > I got the next log: > [ 142.030421] rcar-fcp fea2f000.fcp: swiotlb buffer is full (sz: > 3686400 bytes), total 32768 (slots), used 32 (slots) > > It happened when I tried to map bounce buffer, which has a large size. > The default size if 128(IO_TLB_SEGSIZE) * 2048(IO_TLB_SHIFT) = 262144 > bytes, but we requested 3686400 bytes. > When I change IO_TLB_SEGSIZE to 2048. (2048(IO_TLB_SEGSIZE) * > 2048(IO_TLB_SHIFT) = 4194304bytes). > It makes possible to retrieve a bounce buffer for requested size. > After changing this value, the problem is gone. But the question remains: Why does the framebuffer need to be mapped in a single giant chunk? >> In order to be sure to catch all uses like this one (including ones >> which make it upstream in parallel to yours), I think you will want >> to rename the original IO_TLB_SEGSIZE to e.g. IO_TLB_DEFAULT_SEGSIZE. > > I don't understand your point. Can you clarify this? There's a concrete present example: I have a patch pending adding another use of IO_TLB_SEGSIZE. This use would need to be replaced like you do here in several places. The need for the additional replacement would be quite obvious (from a build failure) if you renamed the manifest constant. Without renaming, it'll take someone running into an issue on a live system, which I consider far worse. This is because a simple re-basing of one of the patches on top of the other will not point out the need for the extra replacement, nor would a test build (with both patches in place). Jan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] swiotlb: set IO TLB segment size via cmdline
On 14.09.2021 17:10, Roman Skakun wrote:
> From: Roman Skakun
>
> It is possible when default IO TLB size is not
> enough to fit a long buffers as described here [1].
>
> This patch makes a way to set this parameter
> using cmdline instead of recompiling a kernel.
>
> [1] https://www.xilinx.com/support/answers/72694.html
I'm not convinced the swiotlb use describe there falls under "intended
use" - mapping a 1280x720 framebuffer in a single chunk? (As an aside,
the bottom of this page is also confusing, as following "Then we can
confirm the modified swiotlb size in the boot log:" there is a log
fragment showing the same original size of 64Mb.
> --- a/arch/mips/cavium-octeon/dma-octeon.c
> +++ b/arch/mips/cavium-octeon/dma-octeon.c
> @@ -237,7 +237,7 @@ void __init plat_swiotlb_setup(void)
> swiotlbsize = 64 * (1<<20);
> #endif
> swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
> - swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
> + swiotlb_nslabs = ALIGN(swiotlb_nslabs, swiotlb_io_seg_size());
In order to be sure to catch all uses like this one (including ones
which make it upstream in parallel to yours), I think you will want
to rename the original IO_TLB_SEGSIZE to e.g. IO_TLB_DEFAULT_SEGSIZE.
> @@ -81,15 +86,30 @@ static unsigned int max_segment;
> static unsigned long default_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT;
>
> static int __init
> -setup_io_tlb_npages(char *str)
> +setup_io_tlb_params(char *str)
> {
> + unsigned long tmp;
> +
> if (isdigit(*str)) {
> - /* avoid tail segment of size < IO_TLB_SEGSIZE */
> - default_nslabs =
> - ALIGN(simple_strtoul(str, , 0), IO_TLB_SEGSIZE);
> + default_nslabs = simple_strtoul(str, , 0);
> }
> if (*str == ',')
> ++str;
> +
> + /* get max IO TLB segment size */
> + if (isdigit(*str)) {
> + tmp = simple_strtoul(str, , 0);
> + if (tmp)
> + io_tlb_seg_size = ALIGN(tmp, IO_TLB_SEGSIZE);
>From all I can tell io_tlb_seg_size wants to be a power of 2. Merely
aligning to a multiple of IO_TLB_SEGSIZE isn't going to be enough.
Jan
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
