Re: [PATCH V2] ARM: xen/mm.c: A mundane typo fix
On Wed, 24 Mar 2021, Randy Dunlap wrote: > On 3/24/21 9:35 PM, Bhaskar Chowdhury wrote: > > s/acrros/across/ > > > > Plus some words need prural version...so did it.(page->pages) > > > > Signed-off-by: Bhaskar Chowdhury > > Acked-by: Randy Dunlap Acked-by: Stefano Stabellini > > --- > > Changes from V1: > > Randy's findings incorporated. > > > > arch/arm/xen/mm.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > > index 467fa225c3d0..8596dd32dcd5 100644 > > --- a/arch/arm/xen/mm.c > > +++ b/arch/arm/xen/mm.c > > @@ -105,8 +105,8 @@ bool xen_arch_need_swiotlb(struct device *dev, > > * - The Linux page refers to foreign memory > > * - The device doesn't support coherent DMA request > > * > > -* The Linux page may be spanned acrros multiple Xen page, although > > -* it's not possible to have a mix of local and foreign Xen page. > > +* The Linux page may be spanned across multiple Xen pages, although > > +* it's not possible to have a mix of local and foreign Xen pages. > > * Furthermore, range_straddles_page_boundary is already checking > > * if buffer is physically contiguous in the host RAM. > > * > > -- > > > -- > ~Randy >
Re: [PATCH] ARM: xen/mm.c: A mundane typo fix
On Wed, 24 Mar 2021, Randy Dunlap wrote: > On 3/24/21 11:55 AM, Stefano Stabellini wrote: > > On Wed, 24 Mar 2021, Bhaskar Chowdhury wrote: > >> s/acrros/across/ > >> > >> Signed-off-by: Bhaskar Chowdhury > > > > Reviewed-by: Stefano Stabellini > > > > > > Hi, > It seems to me like some of those "page" should be "pages". Yes, good point actually > >> --- > >> arch/arm/xen/mm.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > >> index 467fa225c3d0..be7c942c74bf 100644 > >> --- a/arch/arm/xen/mm.c > >> +++ b/arch/arm/xen/mm.c > >> @@ -105,7 +105,7 @@ bool xen_arch_need_swiotlb(struct device *dev, > >> * - The Linux page refers to foreign memory > >> * - The device doesn't support coherent DMA request > >> * > >> - * The Linux page may be spanned acrros multiple Xen page, although>> + > >> * The Linux page may be spanned across multiple Xen page, although > >pages, > > >> * it's not possible to have a mix of local and foreign Xen page. > > pages. yes to both > >> * Furthermore, range_straddles_page_boundary is already checking > >> * if buffer is physically contiguous in the host RAM. > >> -- > > > -- > ~Randy >
Re: [PATCH] ARM: xen/mm.c: A mundane typo fix
On Wed, 24 Mar 2021, Bhaskar Chowdhury wrote: > s/acrros/across/ > > Signed-off-by: Bhaskar Chowdhury Reviewed-by: Stefano Stabellini > --- > arch/arm/xen/mm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c > index 467fa225c3d0..be7c942c74bf 100644 > --- a/arch/arm/xen/mm.c > +++ b/arch/arm/xen/mm.c > @@ -105,7 +105,7 @@ bool xen_arch_need_swiotlb(struct device *dev, >* - The Linux page refers to foreign memory >* - The device doesn't support coherent DMA request >* > - * The Linux page may be spanned acrros multiple Xen page, although > + * The Linux page may be spanned across multiple Xen page, although >* it's not possible to have a mix of local and foreign Xen page. >* Furthermore, range_straddles_page_boundary is already checking >* if buffer is physically contiguous in the host RAM. > -- > 2.30.1 >
Re: [PATCH] ARM: Qualify enabling of swiotlb_init()
On Fri, 19 Mar 2021, Konrad Rzeszutek Wilk wrote: > On Fri, Mar 19, 2021 at 02:07:31PM +0100, Christoph Hellwig wrote: > > On Thu, Mar 18, 2021 at 09:03:33PM -0700, Florian Fainelli wrote: > > > #ifdef CONFIG_ARM_LPAE > > > + if (swiotlb_force == SWIOTLB_FORCE || > > > + max_pfn > arm_dma_pfn_limit) > > > > Does arm_dma_pfn_limit do the right thing even with the weirdest > > remapping ranges? Maybe a commen here would be useful. > > > > > + swiotlb_init(1); > > > + else > > > + swiotlb_force = SWIOTLB_NO_FORCE; > > > > Konrad: what do you think of setting swiotlb_force to SWIOTLB_NO_FORCE > > and only switching it to SWIOTLB_NORMAL when swiotlb_init* is called? > > That kind makes more sense than forcing the callers to do it. > > > > While we're at it, I think swiotlb_force should probably be renamed to > > swiotlb_mode or somethng like that. > > swiotlb_mode sounds good. > > Also it got me thinking - ARM on Xen at some point was a bit strange, so not > sure how > the logic works here, Stefano? There is nothing strange in regards to swiotlb_force. swiotlb_force is only used in swiotlb-xen map_page to figure out whether: - we actually have to use the swiotlb bounce buffer (this is the swiotlb_xen == SWIOTLB_FORCE case) - or we can use the provided page directly for dma if other conditions are met (dma_capable, !range_straddles_page_boundary, ...) I don't think that switching to "swiotlb_mode" would cause any issues.
Re: [PATCH RFC v1 5/6] xen-swiotlb: convert variables to arrays
On Fri, 19 Feb 2021, Konrad Rzeszutek Wilk wrote:
> On Sun, Feb 07, 2021 at 04:56:01PM +0100, Christoph Hellwig wrote:
> > On Thu, Feb 04, 2021 at 09:40:23AM +0100, Christoph Hellwig wrote:
> > > So one thing that has been on my mind for a while: I'd really like
> > > to kill the separate dma ops in Xen swiotlb. If we compare xen-swiotlb
> > > to swiotlb the main difference seems to be:
> > >
> > > - additional reasons to bounce I/O vs the plain DMA capable
> > > - the possibility to do a hypercall on arm/arm64
> > > - an extra translation layer before doing the phys_to_dma and vice
> > >versa
> > > - an special memory allocator
> > >
> > > I wonder if inbetween a few jump labels or other no overhead enablement
> > > options and possibly better use of the dma_range_map we could kill
> > > off most of swiotlb-xen instead of maintaining all this code duplication?
> >
> > So I looked at this a bit more.
> >
> > For x86 with XENFEAT_auto_translated_physmap (how common is that?)
>
> Juergen, Boris please correct me if I am wrong, but that
> XENFEAT_auto_translated_physmap
> only works for PVH guests?
ARM is always XENFEAT_auto_translated_physmap
> > pfn_to_gfn is a nop, so plain phys_to_dma/dma_to_phys do work as-is.
> >
> > xen_arch_need_swiotlb always returns true for x86, and
> > range_straddles_page_boundary should never be true for the
> > XENFEAT_auto_translated_physmap case.
>
> Correct. The kernel should have no clue of what the real MFNs are
> for PFNs.
On ARM, Linux knows the MFNs because for local pages MFN == PFN and for
foreign pages it keeps track in arch/arm/xen/p2m.c. More on this below.
xen_arch_need_swiotlb only returns true on ARM in rare situations where
bouncing on swiotlb buffers is required. Today it only happens on old
versions of Xen that don't support the cache flushing hypercall but
there could be more cases in the future.
> >
> > So as far as I can tell the mapping fast path for the
> > XENFEAT_auto_translated_physmap can be trivially reused from swiotlb.
> >
> > That leaves us with the next more complicated case, x86 or fully cache
> > coherent arm{,64} without XENFEAT_auto_translated_physmap. In that case
> > we need to patch in a phys_to_dma/dma_to_phys that performs the MFN
> > lookup, which could be done using alternatives or jump labels.
> > I think if that is done right we should also be able to let that cover
> > the foreign pages in is_xen_swiotlb_buffer/is_swiotlb_buffer, but
> > in that worst case that would need another alternative / jump label.
> >
> > For non-coherent arm{,64} we'd also need to use alternatives or jump
> > labels to for the cache maintainance ops, but that isn't a hard problem
> > either.
With the caveat that ARM is always XENFEAT_auto_translated_physmap, what
you wrote looks correct. I am writing down a brief explanation on how
swiotlb-xen is used on ARM.
pfn: address as seen by the guest, pseudo-physical address in ARM terminology
mfn (or bfn): real address, physical address in ARM terminology
On ARM dom0 is auto_translated (so Xen sets up the stage2 translation
in the MMU) and the translation is 1:1. So pfn == mfn for Dom0.
However, when another domain shares a page with Dom0, that page is not
1:1. Swiotlb-xen is used to retrieve the mfn for the foreign page at
xen_swiotlb_map_page. It does that with xen_phys_to_bus -> pfn_to_bfn.
It is implemented with a rbtree in arch/arm/xen/p2m.c.
In addition, swiotlb-xen is also used to cache-flush the page via
hypercall at xen_swiotlb_unmap_page. That is done because dev_addr is
really the mfn at unmap_page and we don't know the pfn for it. We can do
pfn-to-mfn but we cannot do mfn-to-pfn (there are good reasons for it
unfortunately). The only way to cache-flush by mfn is by issuing a
hypercall. The hypercall is implemented in arch/arm/xen/mm.c.
The pfn != bfn and pfn_valid() checks are used to detect if the page is
local (of dom0) or foreign; they work thanks to the fact that Dom0 is
1:1 mapped.
Getting back to what you wrote, yes if we had a way to do MFN lookups in
phys_to_dma, and a way to call the hypercall at unmap_page if the page
is foreign (e.g. if it fails a pfn_valid check) then I think we would be
good from an ARM perspective. The only exception is when
xen_arch_need_swiotlb returns true, in which case we need to actually
bounce on swiotlb buffers.
Re: [PATCH] arm/xen: Don't probe xenbus as part of an early initcall
On Wed, 10 Feb 2021, Woodhouse, David wrote:
> On Wed, 2021-02-10 at 17:06 +, Julien Grall wrote:
> > From: Julien Grall
> >
> > After Commit 3499ba8198cad ("xen: Fix event channel callback via
> > INTX/GSI"), xenbus_probe() will be called too early on Arm. This will
> > recent to a guest hang during boot.
> >
> > If there hang wasn't there, we would have ended up to call
> > xenbus_probe() twice (the second time is in xenbus_probe_initcall()).
> >
> > We don't need to initialize xenbus_probe() early for Arm guest.
> > Therefore, the call in xen_guest_init() is now removed.
> >
> > After this change, there is no more external caller for xenbus_probe().
> > So the function is turned to a static one. Interestingly there were two
> > prototypes for it.
> >
> > Fixes: 3499ba8198cad ("xen: Fix event channel callback via INTX/GSI")
> > Reported-by: Ian Jackson
> > Signed-off-by: Julien Grall
>
> Reviewed-by: David Woodhouse
> Cc: sta...@vger.kernel.org
Reviewed-by: Stefano Stabellini
Re: [PATCH] fix swiotlb panic on Xen
On Tue, 27 Oct 2020, Konrad Rzeszutek Wilk wrote:
> > As the person who first found this and then confirmed this fixes a bug:
> >
> > Tested-by: Elliott Mitchell
>
> Thank you!!
>
> I changed the title and added the various tags and will put it in
> linux-next later this week.
Looks fine, thank you
> >From a1eb2768bf5954d25aa0f0136b38f0aa5d92d984 Mon Sep 17 00:00:00 2001
> From: Stefano Stabellini
> Date: Mon, 26 Oct 2020 17:02:14 -0700
> Subject: [PATCH] swiotlb: fix "x86: Don't panic if can not alloc buffer for
> swiotlb"
>
> kernel/dma/swiotlb.c:swiotlb_init gets called first and tries to
> allocate a buffer for the swiotlb. It does so by calling
>
> memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
>
> If the allocation must fail, no_iotlb_memory is set.
>
> Later during initialization swiotlb-xen comes in
> (drivers/xen/swiotlb-xen.c:xen_swiotlb_init) and given that io_tlb_start
> is != 0, it thinks the memory is ready to use when actually it is not.
>
> When the swiotlb is actually needed, swiotlb_tbl_map_single gets called
> and since no_iotlb_memory is set the kernel panics.
>
> Instead, if swiotlb-xen.c:xen_swiotlb_init knew the swiotlb hadn't been
> initialized, it would do the initialization itself, which might still
> succeed.
>
> Fix the panic by setting io_tlb_start to 0 on swiotlb initialization
> failure, and also by setting no_iotlb_memory to false on swiotlb
> initialization success.
>
> Fixes: ac2cbab21f31 ("x86: Don't panic if can not alloc buffer for swiotlb")
>
> Reported-by: Elliott Mitchell
> Tested-by: Elliott Mitchell
> Signed-off-by: Stefano Stabellini
> Reviewed-by: Christoph Hellwig
> CC: sta...@vger.kernel.org
> Signed-off-by: Konrad Rzeszutek Wilk
> ---
> kernel/dma/swiotlb.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 465a567678d9..e08cac39c0ba 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -229,6 +229,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long
> nslabs, int verbose)
> io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> }
> io_tlb_index = 0;
> + no_iotlb_memory = false;
>
> if (verbose)
> swiotlb_print_info();
> @@ -260,9 +261,11 @@ swiotlb_init(int verbose)
> if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose))
> return;
>
> - if (io_tlb_start)
> + if (io_tlb_start) {
> memblock_free_early(io_tlb_start,
> PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
> + io_tlb_start = 0;
> + }
> pr_warn("Cannot allocate buffer");
> no_iotlb_memory = true;
> }
> @@ -360,6 +363,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> nslabs)
> io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> }
> io_tlb_index = 0;
> + no_iotlb_memory = false;
>
> swiotlb_print_info();
>
> --
> 2.13.6
>
Re: [PATCH] fix swiotlb panic on Xen
On Tue, 27 Oct 2020, Konrad Rzeszutek Wilk wrote:
> On Mon, Oct 26, 2020 at 05:02:14PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini
> >
> > kernel/dma/swiotlb.c:swiotlb_init gets called first and tries to
> > allocate a buffer for the swiotlb. It does so by calling
> >
> > memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
> >
> > If the allocation must fail, no_iotlb_memory is set.
> >
> >
> > Later during initialization swiotlb-xen comes in
> > (drivers/xen/swiotlb-xen.c:xen_swiotlb_init) and given that io_tlb_start
> > is != 0, it thinks the memory is ready to use when actually it is not.
> >
> > When the swiotlb is actually needed, swiotlb_tbl_map_single gets called
> > and since no_iotlb_memory is set the kernel panics.
> >
> > Instead, if swiotlb-xen.c:xen_swiotlb_init knew the swiotlb hadn't been
> > initialized, it would do the initialization itself, which might still
> > succeed.
> >
> >
> > Fix the panic by setting io_tlb_start to 0 on swiotlb initialization
> > failure, and also by setting no_iotlb_memory to false on swiotlb
> > initialization success.
>
> Should this have a Fixes: flag?
That would be
Fixes: ac2cbab21f31 ("x86: Don't panic if can not alloc buffer for swiotlb")
> > Signed-off-by: Stefano Stabellini
> >
> >
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index c19379fabd20..9924214df60a 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -231,6 +231,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> > long nslabs, int verbose)
> > io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> > }
> > io_tlb_index = 0;
> > + no_iotlb_memory = false;
> >
> > if (verbose)
> > swiotlb_print_info();
> > @@ -262,9 +263,11 @@ swiotlb_init(int verbose)
> > if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose))
> > return;
> >
> > - if (io_tlb_start)
> > + if (io_tlb_start) {
> > memblock_free_early(io_tlb_start,
> > PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
> > + io_tlb_start = 0;
> > + }
> > pr_warn("Cannot allocate buffer");
> > no_iotlb_memory = true;
> > }
> > @@ -362,6 +365,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long
> > nslabs)
> > io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> > }
> > io_tlb_index = 0;
> > + no_iotlb_memory = false;
> >
> > swiotlb_print_info();
> >
>
[PATCH] fix swiotlb panic on Xen
From: Stefano Stabellini
kernel/dma/swiotlb.c:swiotlb_init gets called first and tries to
allocate a buffer for the swiotlb. It does so by calling
memblock_alloc_low(PAGE_ALIGN(bytes), PAGE_SIZE);
If the allocation must fail, no_iotlb_memory is set.
Later during initialization swiotlb-xen comes in
(drivers/xen/swiotlb-xen.c:xen_swiotlb_init) and given that io_tlb_start
is != 0, it thinks the memory is ready to use when actually it is not.
When the swiotlb is actually needed, swiotlb_tbl_map_single gets called
and since no_iotlb_memory is set the kernel panics.
Instead, if swiotlb-xen.c:xen_swiotlb_init knew the swiotlb hadn't been
initialized, it would do the initialization itself, which might still
succeed.
Fix the panic by setting io_tlb_start to 0 on swiotlb initialization
failure, and also by setting no_iotlb_memory to false on swiotlb
initialization success.
Signed-off-by: Stefano Stabellini
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c19379fabd20..9924214df60a 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -231,6 +231,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long
nslabs, int verbose)
io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
}
io_tlb_index = 0;
+ no_iotlb_memory = false;
if (verbose)
swiotlb_print_info();
@@ -262,9 +263,11 @@ swiotlb_init(int verbose)
if (vstart && !swiotlb_init_with_tbl(vstart, io_tlb_nslabs, verbose))
return;
- if (io_tlb_start)
+ if (io_tlb_start) {
memblock_free_early(io_tlb_start,
PAGE_ALIGN(io_tlb_nslabs << IO_TLB_SHIFT));
+ io_tlb_start = 0;
+ }
pr_warn("Cannot allocate buffer");
no_iotlb_memory = true;
}
@@ -362,6 +365,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
}
io_tlb_index = 0;
+ no_iotlb_memory = false;
swiotlb_print_info();
Re: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
On Mon, 5 Oct 2020, Ben Levinsky wrote: > R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this > remotproc driver, we can boot the R5 sub-system in different 2 > configurations - > * split > * lock-step > > The Xilinx R5 Remoteproc Driver boots the R5's via calls to the Xilinx > Platform Management Unit that handles the R5 configuration, memory access > and R5 lifecycle management. The interface to this manager is done in this > driver via zynqmp_pm_* function calls. Mostly minor comments left > Signed-off-by: Wendy Liang > Signed-off-by: Michal Simek > Signed-off-by: Ed Mooring > Signed-off-by: Jason Wu > Signed-off-by: Ben Levinsky > --- > v2: > - remove domain struct as per review from Mathieu > v3: > - add xilinx-related platform mgmt fn's instead of wrapping around >function pointer in xilinx eemi ops struct > v4: > - add default values for enums > - fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1 > check >are still raised as each is due to fixing the warning results in that > particular line going over 80 characters. > v5: > - parse_fw change from use of rproc_of_resm_mem_entry_init to > rproc_mem_entry_init and use of alloc/release > - var's of type zynqmp_r5_pdata all have same local variable name > - use dev_dbg instead of dev_info > v6: > - adding memory carveouts is handled much more similarly. All mem > carveouts are >now described in reserved memory as needed. That is, TCM nodes are not >coupled to remoteproc anymore. This is reflected in the remoteproc R5 > driver >and the device tree binding. > - remove mailbox from device tree binding as it is not necessary for elf >loading > - use lockstep-mode property for configuring RPU > v7: > - remove unused headers > - change u32 *lockstep_mode -> u32 lockstep_mode; > - change device-tree binding "lockstep-mode" to xlnx,cluster-mode > - remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at >remoteproc-probe time > - remove is_r5_mode_set from zynqmp rpu remote processor private data > - do not error out if no mailbox is provided > - remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as > pdata is >handled in zynqmp_r5_remoteproc_remove > v8: > - remove old acks, reviewed-by's in commit message > v9: > - as mboxes are now optional, if pdata->tx_mc_skbs not initialized then > do not call skb_queue_empty > - update usage for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config and > zynqmp_pm_get_rpu_mode > - update 5/5 patch commit message to document supported configurations > and how they are booted by the driver. > - remove copyrights other than SPDX from zynqmp_r5_remoteproc.c > - compilation warnings no longer raised > - remove unused includes from zynqmp_r5_remoteproc.c > - remove unused var autoboot from zynqmp_r5_remoteproc.c > - reorder zynqmp_r5_pdata fpr small mem savings due to alignment > - use of zynqmp_pm_set_tcm_config now does not have > output arg > - in tcm handling, unconditionally use &= 0x000f mask since all nodes > in this fn are for tcm > - update comments for translating dma field in tcm handling to device > address > - update calls to rproc_mem_entry_init in parse_mem_regions so that there > are only 2 cases for types of carveouts instead of 3 > - in parse_mem_regions, check if device tree node is null before using it > - add example device tree nodes used in parse_mem_regions and tcm parsing > - add comment for vring id node length > - add check for string length so that vring id is at least min length > - move tcm nodes from reserved mem to instead own device tree nodes >and only use them if enabled in device tree > - add comment for explaining handling of rproc_elf_load_rsc_table > - remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick > - remove unused field mems in struct zynqmp_r5_pdata > - remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling > is done by zyqmp_r5_pm_request_tcm > - remove obsolete setting of dma_ops and parent device dma_mask > - remove obsolete use of of_dma_configure > - add comment for call to r5_set_mode fn > - make mbox usage optional and gracefully inform user via dev_dbg if not > present > - change var lockstep_mode from u32* to u32 > v11: > - use enums instead of u32 where possible in zynqmp_r5_remoteproc > - update usage of zynqmp_pm_set/get_rpu_mode and zynqmp_pm_set_tcm_config > - update prints to not use carriage return, just newline > - look up tcm banks via property in r5 node instead of string name > - print device tree nodes with %pOF instead of %s with node name field > - update tcm release to unmap VA > - handle r5-1 use case > v12: > - update signed off by so that latest developer name is last > - do not cast enums to u32s for zynqmp_pm* functions > v14: > - change zynqmp_r5_remoteproc::rpus and rpu_mode to static > - fix typo > - zynqmp_r5_remoteproc::r5_set_mode set rpu mode from > property
RE: [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
On Thu, 8 Oct 2020, Ben Levinsky wrote: > > Does it mean that the main CPU see the memory of the > > R5 as "some kind of TCM" and that TCM is physically > > mapped at 0xffe0 (ITCM) and 0xffe2 (DTCM)? > > > > If the first is ITCM and the second DTCM that is pretty > > important to point out, since this reflects the harvard > > architecture properties of these two memory areas. Hi Linus, I don't think Xilinx TCMs are split in ITCM and DTCM in the way you describe here: https://www.kernel.org/doc/Documentation/arm/tcm.txt. Either TCM could be used for anything. See https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf at page 82.
Re: [PATCH] arm/arm64: xen: Fix to convert percpu address to gfn correctly
On Thu, 8 Oct 2020, Masami Hiramatsu wrote: > On Tue, 6 Oct 2020 10:56:52 -0700 (PDT) > Stefano Stabellini wrote: > > > On Tue, 6 Oct 2020, Masami Hiramatsu wrote: > > > On Mon, 5 Oct 2020 18:13:22 -0700 (PDT) > > > Stefano Stabellini wrote: > > > > > > > On Mon, 5 Oct 2020, Julien Grall wrote: > > > > > Hi Masami, > > > > > > > > > > On 05/10/2020 14:39, Masami Hiramatsu wrote: > > > > > > Use per_cpu_ptr_to_phys() instead of virt_to_phys() for per-cpu > > > > > > address conversion. > > > > > > > > > > > > In xen_starting_cpu(), per-cpu xen_vcpu_info address is converted > > > > > > to gfn by virt_to_gfn() macro. However, since the virt_to_gfn(v) > > > > > > assumes the given virtual address is in contiguous kernel memory > > > > > > area, it can not convert the per-cpu memory if it is allocated on > > > > > > vmalloc area (depends on CONFIG_SMP). > > > > > > > > > > Are you sure about this? I have a .config with CONFIG_SMP=y where the > > > > > per-cpu > > > > > region for CPU0 is allocated outside of vmalloc area. > > > > > > > > > > However, I was able to trigger the bug as soon as > > > > > CONFIG_NUMA_BALANCING was > > > > > enabled. > > > > > > > > I cannot reproduce the issue with defconfig, but I can with Masami's > > > > kconfig. > > > > > > > > If I disable just CONFIG_NUMA_BALANCING from Masami's kconfig, the > > > > problem still appears. > > > > > > > > If I disable CONFIG_NUMA from Masami's kconfig, it works, which is > > > > strange because CONFIG_NUMA is enabled in defconfig, and defconfig > > > > works. > > > > > > Hmm, strange, because when I disabled CONFIG_NUMA_BALANCING, the issue > > > disappeared. > > > > > > --- config-5.9.0-rc4+ 2020-10-06 11:36:20.620107129 +0900 > > > +++ config-5.9.0-rc4+.buggy 2020-10-05 21:04:40.369936461 +0900 > > > @@ -131,7 +131,8 @@ > > > CONFIG_ARCH_SUPPORTS_NUMA_BALANCING=y > > > CONFIG_CC_HAS_INT128=y > > > CONFIG_ARCH_SUPPORTS_INT128=y > > > -# CONFIG_NUMA_BALANCING is not set > > > +CONFIG_NUMA_BALANCING=y > > > +CONFIG_NUMA_BALANCING_DEFAULT_ENABLED=y > > > CONFIG_CGROUPS=y > > > CONFIG_PAGE_COUNTER=y > > > CONFIG_MEMCG=y > > > > > > So buggy config just enabled NUMA_BALANCING (and default enabled) > > > > Yeah but both NUMA and NUMA_BALANCING are enabled in defconfig which > > works fine... > > Hmm, I found that the xen_vcpu_info was allocated on km if the Dom0 has > enough memory. On my environment, if Xen passed 2GB of RAM to Dom0, it > was allocated on kernel linear mapped address, but with 1GB of RAM, it > was on vmalloc area. > As far as I can see, it seems that the percpu allocates memory from > 2nd chunk if the default memory size is small. > > I've built a kernel with patch [1] and boot the same kernel up with > different dom0_mem option with "trace_event=percpu:*" kernel cmdline. > Then got following logs. > > Boot with 4GB: > -0 [000] 0.543208: percpu_create_chunk: > base_addr=5d5ad71c > [...] > systemd-1 [000] 0.568931: percpu_alloc_percpu: > reserved=0 is_atomic=0 size=48 align=8 base_addr=fa92a086 off=32672 > ptr=8da0b73d > systemd-1 [000] 0.568938: xen_guest_init: Xen: alloc > xen_vcpu_info 800011003fa0 id=8da0b73d > systemd-1 [000] 0.586635: xen_starting_cpu: Xen: > xen_vcpu_info 800011003fa0, vcpup 00092f4ebfa0 per_cpu_offset[0] > 80091e4e8000 > > (NOTE: base_addr and ptr are encoded to the ids, not actual address > because of "%p" printk format) > > In this log, we can see the xen_vcpu_info is allocated NOT on the > new chunk (this is the 2nd chunk). As you can see, the vcpup is in > the kernel linear address in this case, because it came from the > 1st kernel embedded chunk. > > > Boot with 1GB > -0 [000] 0.516221: percpu_create_chunk: > base_addr=8456b989 > [...] > systemd-1 [000] 0.541982: percpu_alloc_percpu: > reserved=0 is_atomic=0 size=48 align=8 base_addr=8456b989 off=17920 > ptr=c247612d > systemd-1 [000] 0.541989: xen_guest_init: Xen: alloc > xen_vcpu_
Re: [PATCH v3] arm/arm64: xen: Fix to convert percpu address to gfn correctly
deadbeef
> [0.771916] x1 : deadbeef x0 : ffea
> [0.777304] Call trace:
> [0.779819] xen_starting_cpu+0x178/0x180
> [0.783898] cpuhp_invoke_callback+0xac/0x640
> [0.788325] cpuhp_issue_call+0xf4/0x150
> [0.792317] __cpuhp_setup_state_cpuslocked+0x128/0x2c8
> [0.797619] __cpuhp_setup_state+0x84/0xf8
> [0.801779] xen_guest_init+0x324/0x364
> [0.805683] do_one_initcall+0x54/0x250
> [0.809590] kernel_init_freeable+0x12c/0x2c8
> [0.814016] kernel_init+0x1c/0x128
> [0.817583] ret_from_fork+0x10/0x18
> [0.821226] Code: d0006980 f9427c00 cb000300 17ea (d421)
> [0.827415] ---[ end trace d95b5309a33f8b28 ]---
> [0.832076] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x000b
> [0.839815] ---[ end Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x000b ]---
>
> Theoletically, this issue has been introduced by commit 9a9ab3cc00dc
> ("xen/arm: SMP support") because it uses __pa() on percpu address.
>
> Signed-off-by: Masami Hiramatsu
> Reviewed-by: Stefano Stabellini
> ---
> Changes in v3:
>Update patch description to explain the mechanism of the problem.
> ---
> arch/arm/xen/enlighten.c |2 +-
> include/xen/arm/page.h |3 +++
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index e93145d72c26..a6ab3689b2f4 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -150,7 +150,7 @@ static int xen_starting_cpu(unsigned int cpu)
> pr_info("Xen: initializing cpu%d\n", cpu);
> vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
>
> - info.mfn = virt_to_gfn(vcpup);
> + info.mfn = percpu_to_gfn(vcpup);
> info.offset = xen_offset_in_page(vcpup);
>
> err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info, xen_vcpu_nr(cpu),
> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> index 39df751d0dc4..ac1b65470563 100644
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -83,6 +83,9 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
> })
> #define gfn_to_virt(m) (__va(gfn_to_pfn(m) << XEN_PAGE_SHIFT))
>
> +#define percpu_to_gfn(v) \
> + (pfn_to_gfn(per_cpu_ptr_to_phys(v) >> XEN_PAGE_SHIFT))
> +
> /* Only used in PV code. But ARM guests are always HVM. */
> static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr)
> {
>
Re: [PATCH] arm/arm64: xen: Fix to convert percpu address to gfn correctly
On Tue, 6 Oct 2020, Masami Hiramatsu wrote:
> On Mon, 5 Oct 2020 18:13:22 -0700 (PDT)
> Stefano Stabellini wrote:
>
> > On Mon, 5 Oct 2020, Julien Grall wrote:
> > > Hi Masami,
> > >
> > > On 05/10/2020 14:39, Masami Hiramatsu wrote:
> > > > Use per_cpu_ptr_to_phys() instead of virt_to_phys() for per-cpu
> > > > address conversion.
> > > >
> > > > In xen_starting_cpu(), per-cpu xen_vcpu_info address is converted
> > > > to gfn by virt_to_gfn() macro. However, since the virt_to_gfn(v)
> > > > assumes the given virtual address is in contiguous kernel memory
> > > > area, it can not convert the per-cpu memory if it is allocated on
> > > > vmalloc area (depends on CONFIG_SMP).
> > >
> > > Are you sure about this? I have a .config with CONFIG_SMP=y where the
> > > per-cpu
> > > region for CPU0 is allocated outside of vmalloc area.
> > >
> > > However, I was able to trigger the bug as soon as CONFIG_NUMA_BALANCING
> > > was
> > > enabled.
> >
> > I cannot reproduce the issue with defconfig, but I can with Masami's
> > kconfig.
> >
> > If I disable just CONFIG_NUMA_BALANCING from Masami's kconfig, the
> > problem still appears.
> >
> > If I disable CONFIG_NUMA from Masami's kconfig, it works, which is
> > strange because CONFIG_NUMA is enabled in defconfig, and defconfig
> > works.
>
> Hmm, strange, because when I disabled CONFIG_NUMA_BALANCING, the issue
> disappeared.
>
> --- config-5.9.0-rc4+ 2020-10-06 11:36:20.620107129 +0900
> +++ config-5.9.0-rc4+.buggy 2020-10-05 21:04:40.369936461 +0900
> @@ -131,7 +131,8 @@
> CONFIG_ARCH_SUPPORTS_NUMA_BALANCING=y
> CONFIG_CC_HAS_INT128=y
> CONFIG_ARCH_SUPPORTS_INT128=y
> -# CONFIG_NUMA_BALANCING is not set
> +CONFIG_NUMA_BALANCING=y
> +CONFIG_NUMA_BALANCING_DEFAULT_ENABLED=y
> CONFIG_CGROUPS=y
> CONFIG_PAGE_COUNTER=y
> CONFIG_MEMCG=y
>
> So buggy config just enabled NUMA_BALANCING (and default enabled)
Yeah but both NUMA and NUMA_BALANCING are enabled in defconfig which
works fine...
[...]
> > The fix is fine for me. I tested it and it works. We need to remove the
> > "Fixes:" line from the commit message. Ideally, replacing it with a
> > reference to what is the source of the problem.
>
> OK, as I said, it seems commit 9a9ab3cc00dc ("xen/arm: SMP support") has
> introduced the per-cpu code. So note it instead of Fixes tag.
...and commit 9a9ab3cc00dc was already present in 5.8 which also works
fine with your kconfig. Something else changed in 5.9 causing this
breakage as a side effect. Commit 9a9ab3cc00dc is there since 2013, I
think it is OK -- this patch is fixing something else.
Re: [PATCH] arm/arm64: xen: Fix to convert percpu address to gfn correctly
On Mon, 5 Oct 2020, Julien Grall wrote:
> Hi Masami,
>
> On 05/10/2020 14:39, Masami Hiramatsu wrote:
> > Use per_cpu_ptr_to_phys() instead of virt_to_phys() for per-cpu
> > address conversion.
> >
> > In xen_starting_cpu(), per-cpu xen_vcpu_info address is converted
> > to gfn by virt_to_gfn() macro. However, since the virt_to_gfn(v)
> > assumes the given virtual address is in contiguous kernel memory
> > area, it can not convert the per-cpu memory if it is allocated on
> > vmalloc area (depends on CONFIG_SMP).
>
> Are you sure about this? I have a .config with CONFIG_SMP=y where the per-cpu
> region for CPU0 is allocated outside of vmalloc area.
>
> However, I was able to trigger the bug as soon as CONFIG_NUMA_BALANCING was
> enabled.
I cannot reproduce the issue with defconfig, but I can with Masami's
kconfig.
If I disable just CONFIG_NUMA_BALANCING from Masami's kconfig, the
problem still appears.
If I disable CONFIG_NUMA from Masami's kconfig, it works, which is
strange because CONFIG_NUMA is enabled in defconfig, and defconfig
works.
> [...]
>
> > Fixes: 250c9af3d831 ("arm/xen: Add support for 64KB page granularity")
>
> FWIW, I think the bug was already present before 250c9af3d831.
Yeah, I bet 250c9af3d831 is not what introduced the issue. Whatever
caused virt_to_phys to stop working on vmalloc'ed addresses is the cause
of the problem. It is something that went in 5.9 (5.8 works) but I don't
know what for sure.
> > Signed-off-by: Masami Hiramatsu
> > ---
> > arch/arm/xen/enlighten.c |2 +-
> > include/xen/arm/page.h |3 +++
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> > index e93145d72c26..a6ab3689b2f4 100644
> > --- a/arch/arm/xen/enlighten.c
> > +++ b/arch/arm/xen/enlighten.c
> > @@ -150,7 +150,7 @@ static int xen_starting_cpu(unsigned int cpu)
> > pr_info("Xen: initializing cpu%d\n", cpu);
> > vcpup = per_cpu_ptr(xen_vcpu_info, cpu);
> > - info.mfn = virt_to_gfn(vcpup);
> > + info.mfn = percpu_to_gfn(vcpup);
> > info.offset = xen_offset_in_page(vcpup);
> > err = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_info,
> > xen_vcpu_nr(cpu),
> > diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> > index 39df751d0dc4..ac1b65470563 100644
> > --- a/include/xen/arm/page.h
> > +++ b/include/xen/arm/page.h
> > @@ -83,6 +83,9 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
> > })
> > #define gfn_to_virt(m)(__va(gfn_to_pfn(m) <<
> > XEN_PAGE_SHIFT))
> > +#define percpu_to_gfn(v) \
> > + (pfn_to_gfn(per_cpu_ptr_to_phys(v) >> XEN_PAGE_SHIFT))
> > +
> > /* Only used in PV code. But ARM guests are always HVM. */
> > static inline xmaddr_t arbitrary_virt_to_machine(void *vaddr)
> > {
The fix is fine for me. I tested it and it works. We need to remove the
"Fixes:" line from the commit message. Ideally, replacing it with a
reference to what is the source of the problem.
Aside from that:
Reviewed-by: Stefano Stabellini
Re: [PATCH v16 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
On Tue, 29 Sep 2020, Rob Herring wrote:
> > index ..ce02e425692e
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> > @@ -0,0 +1,120 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> > "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml#;
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#;
> > +
> > +title: Xilinx R5 remote processor controller bindings
> > +
> > +description:
> > + This document defines the binding for the remoteproc component that
> > loads and
> > + boots firmwares on the Xilinx Zynqmp and Versal family chipset.
> > +
> > + Note that the Linux has global addressing view of the R5-related memory
> > (TCM)
> > + so the absolute address ranges are provided in TCM reg's.
>
> blank line needed.
>
> TCMs specifically I'm concerned about how they are represented in system
> DT and here...
So far I have been keeping the TCMs in system DT as regular nodes under
/amba. E.g.:
tcm: tcm@ffe0 {
compatible = "mmio-sram";
reg = <0x0 0xffe0 0x0 0x1>;
};
(I am not sure if "mmio-sram" is the right compatible.)
[PATCH] xen/arm: do not setup the runstate info page if kpti is enabled
From: Stefano Stabellini
The VCPUOP_register_runstate_memory_area hypercall takes a virtual
address of a buffer as a parameter. The semantics of the hypercall are
such that the virtual address should always be valid.
When KPTI is enabled and we are running userspace code, the virtual
address is not valid, thus, Linux is violating the semantics of
VCPUOP_register_runstate_memory_area.
Do not call VCPUOP_register_runstate_memory_area when KPTI is enabled.
Signed-off-by: Stefano Stabellini
CC: Bertrand Marquis
CC: boris.ostrov...@oracle.com
CC: jgr...@suse.com
---
arch/arm/include/asm/xen/page.h | 5 +
arch/arm/xen/enlighten.c | 6 --
arch/arm64/include/asm/xen/page.h | 6 ++
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
index 31bbc803cecb..dc7f6e91aafa 100644
--- a/arch/arm/include/asm/xen/page.h
+++ b/arch/arm/include/asm/xen/page.h
@@ -1 +1,6 @@
#include
+
+static inline bool xen_kernel_unmapped_at_usr(void)
+{
+ return false;
+}
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index e93145d72c26..ea76562af1e9 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -158,7 +158,8 @@ static int xen_starting_cpu(unsigned int cpu)
BUG_ON(err);
per_cpu(xen_vcpu, cpu) = vcpup;
- xen_setup_runstate_info(cpu);
+ if (!xen_kernel_unmapped_at_usr())
+ xen_setup_runstate_info(cpu);
after_register_vcpu_info:
enable_percpu_irq(xen_events_irq, 0);
@@ -387,7 +388,8 @@ static int __init xen_guest_init(void)
return -EINVAL;
}
- xen_time_setup_guest();
+ if (!xen_kernel_unmapped_at_usr())
+ xen_time_setup_guest();
if (xen_initial_domain())
pvclock_gtod_register_notifier(_pvclock_gtod_notifier);
diff --git a/arch/arm64/include/asm/xen/page.h
b/arch/arm64/include/asm/xen/page.h
index 31bbc803cecb..dffdc773221b 100644
--- a/arch/arm64/include/asm/xen/page.h
+++ b/arch/arm64/include/asm/xen/page.h
@@ -1 +1,7 @@
#include
+#include
+
+static inline bool xen_kernel_unmapped_at_usr(void)
+{
+ return arm64_kernel_unmapped_at_el0();
+}
--
2.17.1
Re: [PATCH v13 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
On Fri, 4 Sep 2020, Ben Levinsky wrote:
> Add binding for ZynqMP R5 OpenAMP.
>
> Represent the RPU domain resources in one device node. Each RPU
> processor is a subnode of the top RPU domain node.
>
> Signed-off-by: Jason Wu
> Signed-off-by: Wendy Liang
> Signed-off-by: Michal Simek
>
> Signed-off-by: Ben Levinsky
> ---
> v3:
> - update zynqmp_r5 yaml parsing to not raise warnings for extra
> information in children of R5 node. The warning "node has a unit
> name, but no reg or ranges property" will still be raised though
> as this particular node is needed to describe the
> '#address-cells' and '#size-cells' information.
> v4::
> - remove warning '/example-0/rpu@ff9a/r5@0:
> node has a unit name, but no reg or ranges property'
> by adding reg to r5 node.
> v5:
> - update device tree sample and yaml parsing to not raise any warnings
> - description for memory-region in yaml parsing
> - compatible string in yaml parsing for TCM
> v6:
> - remove coupling TCM nodes with remoteproc
> - remove mailbox as it is optional not needed
> v7:
> - change lockstep-mode to xlnx,cluster-mode
> v9:
> - show example IPC nodes and tcm bank nodes
> v11:
> - add property meta-memory-regions to illustrate link
> between r5 and TCM banks
> - update so no warnings from 'make dt_binding_check'
> ---
> .../xilinx,zynqmp-r5-remoteproc.yaml | 123 ++
> 1 file changed, 123 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
>
> b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> new file mode 100644
> index ..148374c61908
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id:
> "http://devicetree.org/schemas/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml#;
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#;
> +
> +title: Xilinx R5 remote processor controller bindings
> +
> +description:
> + This document defines the binding for the remoteproc component that loads
> and
> + boots firmwares on the Xilinx Zynqmp and Versal family chipset.
> +
> + Note that the Linux has global addressing view of the R5-related memory
> (TCM)
> + so the absolute address ranges are provided in TCM reg's.
> +maintainers:
> + - Ed Mooring
> + - Ben Levinsky
> +
> +properties:
> + compatible:
> +const: "xlnx,zynqmp-r5-remoteproc-1.0"
> +
> + lockstep-mode:
> +description:
> + R5 core configuration (split is 0 or lock-step and 1)
> +maxItems: 1
> +
> + interrupts:
> +description:
> + Interrupt mapping for remoteproc IPI. It is required if the
> + user uses the remoteproc driver with the RPMsg kernel driver.
> +maxItems: 6
> +
> + memory-region:
> +description:
> + collection of memory carveouts used for elf-loading and inter-processor
> + communication.
> +maxItems: 4
> +minItems: 4
> + meta-memory-regions:
> +description:
> + collection of memories that are not present in the top level memory
> + nodes' mapping. For example, R5s' TCM banks. These banks are needed
> + for R5 firmware meta data such as the R5 firmware's heap and stack
> + pnode-id:
> +maxItems: 1
> + mboxes:
> +maxItems: 2
> + mbox-names:
> +maxItems: 2
> +
> +examples:
> + - |
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + elf_load: rproc@3ed00 {
> + no-map;
> + reg = <0x3ed0 0x4>;
> + };
> + };
> + rpu {
> + compatible = "xlnx,zynqmp-r5-remoteproc-1.0";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + lockstep-mode = <0>;
> + r5_0 {
> + ranges;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + memory-region = <_load>;
> + meta-memory-regions = <0x40 0x41>;
> + pnode-id = <0x7>;
I can see you are expecting a link from this node to the tcm nodes in
the driver, but I can't find such a link in this example.
> + };
> + };
> +
> + /*
> + * Below nodes are required if using TCM to load R5 firmware
> + * if not, then either do not provide nodes are label as disabled in
> + * status property
> + */
> + tcm_0a@ffe0 {
> + reg = <0xffe0 0x1>;
> + pnode-id = <0xf>;
> + no-map;
> + status = "okay";
> + phandle = <0x40>;
> + compatible = "xlnx,tcm";
> + };
> + tcm_1a@ffe2 {
> + reg = <0xffe2 0x1>;
> + pnode-id = <0x10>;
> + no-map;
> + status =
RE: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
On Mon, 24 Aug 2020, Ben Levinsky wrote:
> > -Original Message-
> > From: linux-remoteproc-ow...@vger.kernel.org > ow...@vger.kernel.org> On Behalf Of Ben Levinsky
> > Sent: Tuesday, August 18, 2020 7:24 AM
> > To: Stefano Stabellini
> > Cc: Michal Simek ; devicet...@vger.kernel.org;
> > mathieu.poir...@linaro.org; Ed T. Mooring ; linux-
> > remotep...@vger.kernel.org; linux-kernel@vger.kernel.org;
> > robh...@kernel.org; Jiaying Liang ; linux-arm-
> > ker...@lists.infradead.org
> > Subject: RE: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
> >
> >
> >
> > > -Original Message-
> > > From: Stefano Stabellini
> > > Sent: Thursday, August 13, 2020 1:36 PM
> > > To: Ben Levinsky
> > > Cc: Stefano Stabellini ; Michal Simek
> > > ; devicet...@vger.kernel.org;
> > > mathieu.poir...@linaro.org; Ed T. Mooring ; linux-
> > > remotep...@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > robh...@kernel.org; Jiaying Liang ; linux-arm-
> > > ker...@lists.infradead.org
> > > Subject: Re: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
> > >
> > > On Mon, 10 Aug 2020, Ben Levinsky wrote:
> > > > Add shutdown/wakeup a resource eemi operations to shutdown
> > > > or bringup a resource.
> > > >
> > > > Signed-off-by: Ben Levinsky
> > > > ---
> > > > v3:
> > > > - add xilinx-related platform mgmt fn's instead of wrapping around
> > > > function pointer in xilinx eemi ops struct
> > > > - fix formatting
> > > > v4:
> > > > - add default values for enumv3:
> > > > - add xilinx-related platform mgmt fn's instead of wrapping around
> > > > function pointer in xilinx eemi ops struct
> > > > - fix formatting
> > > > v4:
> > > > - add default values for enum
> > > > ---
> > > > drivers/firmware/xilinx/zynqmp.c | 35
> >
> > > > include/linux/firmware/xlnx-zynqmp.h | 22 +
> > > > 2 files changed, 57 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > > b/drivers/firmware/xilinx/zynqmp.c
> > > > index 8d1ff2454e2e..f1a0bd35deeb 100644
> > > > --- a/drivers/firmware/xilinx/zynqmp.c
> > > > +++ b/drivers/firmware/xilinx/zynqmp.c
> > > > @@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node)
> > > > }
> > > > EXPORT_SYMBOL_GPL(zynqmp_pm_release_node);
> > > >
> > > > +/**
> > > > + * zynqmp_pm_force_powerdown - PM call to request for another PU or
> > > subsystem to
> > > > + * be powered down forcefully
> > > > + * @target:Node ID of the targeted PU or subsystem
> > > > + * @ack: Flag to specify whether acknowledge is requested
> > > > + *
> > > > + * Return: status, either success or error+reason
> > > > + */
> > > > +int zynqmp_pm_force_powerdown(const u32 target,
> > >
> > > If the target is really the node id, why not calling it "node", the same
> > > way as below?
> > [Ben Levinsky] good point. Will change to "target" in v9
> > >
> > >
> > > > + const enum zynqmp_pm_request_ack ack)
> > > > +{
> > > > + return zynqmp_pm_invoke_fn(PM_FORCE_POWERDOWN, target, ack,
> > > 0, 0, NULL);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(zynqmp_pm_force_powerdown);
> > > > +
> > > > +/**
> > > > + * zynqmp_pm_request_wakeup - PM call to wake up selected master or
> > > subsystem
> > > > + * @node: Node ID of the master or subsystem
> > > > + * @set_addr: Specifies whether the address argument is relevant
> > > > + * @address: Address from which to resume when woken up
> > > > + * @ack: Flag to specify whether acknowledge requested
> > > > + *
> > > > + * Return: status, either success or error+reason
> > > > + */
> > > > +int zynqmp_pm_request_wakeup(const u32 node,
> > > > +const bool set_addr,
> > > > +const u64 address,
> > > > +const enum zynqmp_pm_request_ack ack)
> > > > +{
> > > > + /* set_addr flag is encoded into 1st bit of address */
> >
RE: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
On Tue, 18 Aug 2020, Ben Levinsky wrote:
> > > +/**
> > > + * struct zynqmp_r5_mem - zynqmp rpu memory data
> > > + * @pnode_id: TCM power domain ids
> > > + * @res: memory resource
> > > + * @node: list node
> > > + */
> > > +struct zynqmp_r5_mem {
> > > + u32 pnode_id[MAX_MEM_PNODES];
> > > + struct resource res;
> > > + struct list_head node;
> > > +};
> > > +
> > > +/**
> > > + * struct zynqmp_r5_pdata - zynqmp rpu remote processor private data
> > > + * @dev: device of RPU instance
> > > + * @rproc: rproc handle
> > > + * @pnode_id: RPU CPU power domain id
> > > + * @mems: memory resources
> > > + * @is_r5_mode_set: indicate if r5 operation mode is set
> > > + * @tx_mc: tx mailbox client
> > > + * @rx_mc: rx mailbox client
> > > + * @tx_chan: tx mailbox channel
> > > + * @rx_chan: rx mailbox channel
> > > + * @mbox_work: mbox_work for the RPU remoteproc
> > > + * @tx_mc_skbs: socket buffers for tx mailbox client
> > > + * @rx_mc_buf: rx mailbox client buffer to save the rx message
> > > + */
> > > +struct zynqmp_r5_pdata {
> > > + struct device dev;
> > > + struct rproc *rproc;
> > > + u32 pnode_id;
> > > + struct list_head mems;
> > > + bool is_r5_mode_set;
> > > + struct mbox_client tx_mc;
> > > + struct mbox_client rx_mc;
> > > + struct mbox_chan *tx_chan;
> > > + struct mbox_chan *rx_chan;
> > > + struct work_struct mbox_work;
> > > + struct sk_buff_head tx_mc_skbs;
> > > + unsigned char rx_mc_buf[RX_MBOX_CLIENT_BUF_MAX];
> >
> > A simple small reordering of the struct fields would lead to small
> > memory savings due to alignment.
> >
> >
> [Ben Levinsky] will do. Do you mean ordering in either ascending or
> descending order?
Each field has a different alignment in the struct, so for example after
pnode_id there are probably 4 unused bytes because mems is 64bit
aligned.
> > > +/*
> > > + * TCM needs mapping to R5 relative address and xilinx platform mgmt call
> > > + */
> > > +struct rproc_mem_entry *handle_tcm_parsing(struct device *dev,
> > > + struct reserved_mem *rmem,
> > > + struct device_node *node,
> > > + int lookup_idx)
> > > +{
> > > + void *va;
> > > + dma_addr_t dma;
> > > + resource_size_t size;
> > > + int ret;
> > > + u32 pnode_id;
> > > + struct resource rsc;
> > > + struct rproc_mem_entry *mem;
> > > +
> > > + pnode_id = tcm_addr_to_pnode[lookup_idx][1];
> > > + ret = zynqmp_pm_request_node(pnode_id,
> > > + ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> > > + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to request power node: %u\n",
> > pnode_id);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = of_address_to_resource(node, 0, );
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to get resource of memory %s",
> > > + of_node_full_name(node));
> > > + return -EINVAL;
> > > + }
> > > + size = resource_size();
> > > + va = devm_ioremap_wc(dev, rsc.start, size);
> > > + if (!va)
> > > + return -ENOMEM;
> > > +
> > > + /* zero out tcm base address */
> > > + if (rsc.start & 0xffe0) {
> > > + /* R5 can't see anything past 0xf so wipe it */
> > > + rsc.start &= 0x000f;
> >
> > If that is the case why not do:
> >
> > rsc.start &= 0x000f;
> >
> > unconditionally? if (rsc.start & 0xffe0) is superfluous.
> >
> > But I thought that actually the R5s could see TCM at both the low
> > address (< 0x000f) and also at the high address (i.e. 0xffe0).
> >
> >
> [Ben Levinsky] Here yes can make rsc.start &= 0x000f undconditional. Will
> update in v9 as such
> Also, this logic is because this is only for the Xilinx R5
> mappings of TCM banks that are at (from the TCM point of view) 0x0 and 0x2000
What I meant is that as far as I understand from the TRM the RPU should
also be able to access the same banks at the same address of the APU,
which I imagine is in the 0xffe0 range. But I could be wrong on
this.
If we could use the same addresses for RPU and APU, it would simplify
this driver.
> > > + /*
> > > + * handle tcm banks 1 a and b (0xffe9000 and
> > > + * 0xffeb)
> > > + */
> > > + if (rsc.start & 0x8)
> > > + rsc.start -= 0x9;
> >
> > It is very unclear to me why we have to do this
> >
> >
> [Ben Levinsky] This is for TCM bank 0B and bank 1B to map them to 0x0002
> so that the TCM relative addressing lines up. For example (0xffe9 &
> 0x000f) - 0x9 = 0x2
Could you please explain the mapping in an in-code comment?
The comment currently mentions 0xffe9000 and 0xffeb but:
- 0xffe9000 & 0x000f = 0xe9000
0xe9000 - 0x9 = 0x59000
- 0xffeb & 0x000f = 0xeb000
0xeb000 - 0x9 = 0xeb000
Either way we don't get 0x2. What am I missing?
>
Re: [PATCH v8 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
On Mon, 10 Aug 2020, Ben Levinsky wrote: > R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this > remotproc driver, we can boot the R5 sub-system in different > configurations. Which different configurations? How can you boot them? > Signed-off-by: Ben Levinsky > Signed-off-by: Wendy Liang > Signed-off-by: Michal Simek > Signed-off-by: Ed Mooring > Signed-off-by: Jason Wu > --- > v2: > - remove domain struct as per review from Mathieu > v3: > - add xilinx-related platform mgmt fn's instead of wrapping around >function pointer in xilinx eemi ops struct > v4: > - add default values for enums > - fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1 > check >are still raised as each is due to fixing the warning results in that > particular line going over 80 characters. > v5: > - parse_fw change from use of rproc_of_resm_mem_entry_init to > rproc_mem_entry_init and use of alloc/release > - var's of type zynqmp_r5_pdata all have same local variable name > - use dev_dbg instead of dev_info > v6: > - adding memory carveouts is handled much more similarly. All mem > carveouts are >now described in reserved memory as needed. That is, TCM nodes are not >coupled to remoteproc anymore. This is reflected in the remoteproc R5 > driver >and the device tree binding. > - remove mailbox from device tree binding as it is not necessary for elf >loading > - use lockstep-mode property for configuring RPU > v7: > - remove unused headers > - change u32 *lockstep_mode -> u32 lockstep_mode; > - change device-tree binding "lockstep-mode" to xlnx,cluster-mode > - remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at >remoteproc-probe time > - remove is_r5_mode_set from zynqmp rpu remote processor private data > - do not error out if no mailbox is provided > - remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as > pdata is >handled in zynqmp_r5_remoteproc_remove > v8: > - remove old acks, reviewed-by's in commit message > --- > drivers/remoteproc/Kconfig| 10 + > drivers/remoteproc/Makefile | 1 + > drivers/remoteproc/zynqmp_r5_remoteproc.c | 911 ++ > 3 files changed, 922 insertions(+) > create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c > > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig > index c4d1731295eb..342a7e668636 100644 > --- a/drivers/remoteproc/Kconfig > +++ b/drivers/remoteproc/Kconfig > @@ -249,6 +249,16 @@ config STM32_RPROC > > This can be either built-in or a loadable module. > > +config ZYNQMP_R5_REMOTEPROC > + tristate "ZynqMP_R5 remoteproc support" > + depends on ARM64 && PM && ARCH_ZYNQMP > + select RPMSG_VIRTIO > + select MAILBOX > + select ZYNQMP_IPI_MBOX > + help > + Say y here to support ZynqMP R5 remote processors via the remote > + processor framework. > + > endif # REMOTEPROC > > endmenu > diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile > index e8b886e511f0..04d1c95d06d7 100644 > --- a/drivers/remoteproc/Makefile > +++ b/drivers/remoteproc/Makefile > @@ -28,5 +28,6 @@ obj-$(CONFIG_QCOM_WCNSS_PIL)+= > qcom_wcnss_pil.o > qcom_wcnss_pil-y += qcom_wcnss.o > qcom_wcnss_pil-y += qcom_wcnss_iris.o > obj-$(CONFIG_ST_REMOTEPROC) += st_remoteproc.o > +obj-$(CONFIG_ZYNQMP_R5_REMOTEPROC) += zynqmp_r5_remoteproc.o > obj-$(CONFIG_ST_SLIM_REMOTEPROC) += st_slim_rproc.o > obj-$(CONFIG_STM32_RPROC)+= stm32_rproc.o > diff --git a/drivers/remoteproc/zynqmp_r5_remoteproc.c > b/drivers/remoteproc/zynqmp_r5_remoteproc.c > new file mode 100644 > index ..b600759e257e > --- /dev/null > +++ b/drivers/remoteproc/zynqmp_r5_remoteproc.c I tried to build this but I get 4 warnings: drivers/remoteproc/zynqmp_r5_remoteproc.c: In function 'handle_tcm_parsing': drivers/remoteproc/zynqmp_r5_remoteproc.c:286:10: warning: return makes pointer from integer without a cast [-Wint-conversion] return -EINVAL; ^ drivers/remoteproc/zynqmp_r5_remoteproc.c:293:10: warning: return makes pointer from integer without a cast [-Wint-conversion] return -EINVAL; ^ drivers/remoteproc/zynqmp_r5_remoteproc.c:298:10: warning: return makes pointer from integer without a cast [-Wint-conversion] return -ENOMEM; ^ drivers/remoteproc/zynqmp_r5_remoteproc.c:317:10: warning: return makes pointer from integer without a cast [-Wint-conversion] return -ENOMEM; Please fix all warnings before submitting to the list. > @@ -0,0 +1,911 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Zynq R5 Remote Processor driver > + * > + * Copyright (C) 2019, 2020 Xilinx Inc. Ben Levinsky > > + * Copyright (C) 2015 - 2018 Xilinx Inc. > + * Copyright (C) 2015 Jason Wu > + * > + * Based on origin OMAP and Zynq Remote Processor driver > + * > + *
Re: [PATCH v8 2/5] firmware: xilinx: Add shutdown/wakeup APIs
On Mon, 10 Aug 2020, Ben Levinsky wrote:
> Add shutdown/wakeup a resource eemi operations to shutdown
> or bringup a resource.
>
> Signed-off-by: Ben Levinsky
> ---
> v3:
> - add xilinx-related platform mgmt fn's instead of wrapping around
> function pointer in xilinx eemi ops struct
> - fix formatting
> v4:
> - add default values for enumv3:
> - add xilinx-related platform mgmt fn's instead of wrapping around
> function pointer in xilinx eemi ops struct
> - fix formatting
> v4:
> - add default values for enum
> ---
> drivers/firmware/xilinx/zynqmp.c | 35
> include/linux/firmware/xlnx-zynqmp.h | 22 +
> 2 files changed, 57 insertions(+)
>
> diff --git a/drivers/firmware/xilinx/zynqmp.c
> b/drivers/firmware/xilinx/zynqmp.c
> index 8d1ff2454e2e..f1a0bd35deeb 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -846,6 +846,41 @@ int zynqmp_pm_release_node(const u32 node)
> }
> EXPORT_SYMBOL_GPL(zynqmp_pm_release_node);
>
> +/**
> + * zynqmp_pm_force_powerdown - PM call to request for another PU or
> subsystem to
> + * be powered down forcefully
> + * @target:Node ID of the targeted PU or subsystem
> + * @ack: Flag to specify whether acknowledge is requested
> + *
> + * Return: status, either success or error+reason
> + */
> +int zynqmp_pm_force_powerdown(const u32 target,
If the target is really the node id, why not calling it "node", the same
way as below?
> + const enum zynqmp_pm_request_ack ack)
> +{
> + return zynqmp_pm_invoke_fn(PM_FORCE_POWERDOWN, target, ack, 0, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_force_powerdown);
> +
> +/**
> + * zynqmp_pm_request_wakeup - PM call to wake up selected master or subsystem
> + * @node: Node ID of the master or subsystem
> + * @set_addr: Specifies whether the address argument is relevant
> + * @address: Address from which to resume when woken up
> + * @ack: Flag to specify whether acknowledge requested
> + *
> + * Return: status, either success or error+reason
> + */
> +int zynqmp_pm_request_wakeup(const u32 node,
> + const bool set_addr,
> + const u64 address,
> + const enum zynqmp_pm_request_ack ack)
> +{
> + /* set_addr flag is encoded into 1st bit of address */
> + return zynqmp_pm_invoke_fn(PM_REQUEST_WAKEUP, node, address | set_addr,
> +address >> 32, ack, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_request_wakeup);
> +
> /**
> * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves
> * @node:Node ID of the slave
> diff --git a/include/linux/firmware/xlnx-zynqmp.h
> b/include/linux/firmware/xlnx-zynqmp.h
> index bb347dfe4ba4..4d83a7d69c4c 100644
> --- a/include/linux/firmware/xlnx-zynqmp.h
> +++ b/include/linux/firmware/xlnx-zynqmp.h
> @@ -64,6 +64,8 @@
>
> enum pm_api_id {
> PM_GET_API_VERSION = 1,
> + PM_FORCE_POWERDOWN = 8,
> + PM_REQUEST_WAKEUP = 10,
> PM_SYSTEM_SHUTDOWN = 12,
> PM_REQUEST_NODE = 13,
> PM_RELEASE_NODE,
> @@ -376,6 +378,12 @@ int zynqmp_pm_write_pggs(u32 index, u32 value);
> int zynqmp_pm_read_pggs(u32 index, u32 *value);
> int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype);
> int zynqmp_pm_set_boot_health_status(u32 value);
> +int zynqmp_pm_force_powerdown(const u32 target,
> + const enum zynqmp_pm_request_ack ack);
> +int zynqmp_pm_request_wakeup(const u32 node,
> + const bool set_addr,
> + const u64 address,
> + const enum zynqmp_pm_request_ack ack);
> #else
> static inline struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void)
> {
> @@ -526,6 +534,20 @@ static inline int zynqmp_pm_set_boot_health_status(u32
> value)
> {
> return -ENODEV;
> }
> +
> +static inline int zynqmp_pm_force_powerdown(const u32 target,
> + const enum zynqmp_pm_request_ack ack)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int zynqmp_pm_request_wakeup(const u32 node,
> +const bool set_addr,
> +const u64 address,
> +const enum zynqmp_pm_request_ack ack)
code style
> +{
> + return -ENODEV;
> +}
> #endif
>
> #endif /* __FIRMWARE_ZYNQMP_H__ */
> --
> 2.17.1
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Re: [PATCH v8 3/5] firmware: xilinx: Add RPU configuration APIs
On Mon, 10 Aug 2020, Ben Levinsky wrote:
> This patch adds APIs to provide access and a configuration interface
> to the current power state of a sub-system on Zynqmp sub-system.
This doesn't read correctly
> Signed-off-by: Ben Levinsky
> ---
> v3:
> - add xilinx-related platform mgmt fn's instead of wrapping around
> function pointer in xilinx eemi ops struct
> v4:
> - add default values for enums
> ---
> drivers/firmware/xilinx/zynqmp.c | 99
> include/linux/firmware/xlnx-zynqmp.h | 34 ++
> 2 files changed, 133 insertions(+)
>
> diff --git a/drivers/firmware/xilinx/zynqmp.c
> b/drivers/firmware/xilinx/zynqmp.c
> index f1a0bd35deeb..fdd61d258e55 100644
> --- a/drivers/firmware/xilinx/zynqmp.c
> +++ b/drivers/firmware/xilinx/zynqmp.c
> @@ -846,6 +846,61 @@ int zynqmp_pm_release_node(const u32 node)
> }
> EXPORT_SYMBOL_GPL(zynqmp_pm_release_node);
>
> +/**
> + * zynqmp_pm_get_rpu_mode() - Get RPU mode
> + * @node_id: Node ID of the device
> + * @arg1:Argument 1 to requested IOCTL call
> + * @arg2:Argument 2 to requested IOCTL call
> + * @out: Returned output value
> + *
> + * Return: RPU mode
What kind of output are we expecting? An enum? Which one?
> + */
> +int zynqmp_pm_get_rpu_mode(u32 node_id,
> +u32 arg1, u32 arg2, u32 *out)
> +{
> + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
> +IOCTL_GET_RPU_OPER_MODE, 0, 0, out);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_get_rpu_mode);
> +
> +/**
> + * zynqmp_pm_set_rpu_mode() - Set RPU mode
> + * @node_id: Node ID of the device
> + * @ioctl_id:ID of the requested IOCTL
> + * @arg2:Argument 2 to requested IOCTL call
> + * @out: Returned output value
> + *
> + * This function is used to set RPU mode.
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +int zynqmp_pm_set_rpu_mode(u32 node_id,
> +u32 arg1, u32 arg2, u32 *out)
> +{
> + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
> +IOCTL_SET_RPU_OPER_MODE, 0, 0, out);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_set_rpu_mode);
> +
> +/**
> + * zynqmp_pm_tcm_comb_config - configure TCM
> + * @node_id: Node ID of the device
> + * @arg1:Argument 1 to requested IOCTL call
> + * @arg2:Argument 2 to requested IOCTL call
> + * @out: Returned output value
Same question on the type of the output value
> + * This function is used to set RPU mode.
> + *
> + * Return: Returns status, either success or error+reason
> + */
> +int zynqmp_pm_set_tcm_config(u32 node_id,
> + u32 arg1, u32 arg2, u32 *out)
> +{
> + return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
> +IOCTL_TCM_COMB_CONFIG, 0, 0, out);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_set_tcm_config);
> +
> /**
> * zynqmp_pm_force_powerdown - PM call to request for another PU or
> subsystem to
> * be powered down forcefully
> @@ -881,6 +936,50 @@ int zynqmp_pm_request_wakeup(const u32 node,
> }
> EXPORT_SYMBOL_GPL(zynqmp_pm_request_wakeup);
>
> +/**
> + * zynqmp_pm_get_node_status - PM call to request a node's current power
> state
> + * @node: ID of the component or sub-system in question
> + * @status:Current operating state of the requested node
> + * @requirements: Current requirements asserted on the node,
> + * used for slave nodes only.
> + * @usage: Usage information, used for slave nodes only:
> + * PM_USAGE_NO_MASTER - No master is currently using
> + * the node
> + * PM_USAGE_CURRENT_MASTER - Only requesting master is
> + * currently using the node
> + * PM_USAGE_OTHER_MASTER - Only other masters are
> + * currently using the node
> + * PM_USAGE_BOTH_MASTERS - Both the current and at least
> + * one other master is currently
> + * using the node
> + *
> + * Return: status, either success or error+reason
Same question on status
> + */
> +int zynqmp_pm_get_node_status(const u32 node,
> + u32 *status, u32 *requirements,
> + u32 *usage)
> +
> +{
> + u32 ret_payload[PAYLOAD_ARG_CNT];
> + int ret;
> +
> + if (!status)
> + return -EINVAL;
> +
> + ret = zynqmp_pm_invoke_fn(PM_GET_NODE_STATUS, node, 0, 0, 0,
> + ret_payload);
> + if (ret_payload[0] == XST_PM_SUCCESS) {
> + *status = ret_payload[1];
> + if (requirements)
> + *requirements = ret_payload[2];
> + if (usage)
> + *usage = ret_payload[3];
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_get_node_status);
> +
> /**
> * zynqmp_pm_set_requirement() - PM call to set requirement for PM slaves
> * @node:Node ID of the slave
>
Re: [PATCH v3 00/11] fix swiotlb-xen for RPi4
On Tue, 4 Aug 2020, Jürgen Groß wrote: > On 11.07.20 00:34, Stefano Stabellini wrote: > > Hi all, > > > > This series is a collection of fixes to get Linux running on the RPi4 as > > dom0. Conceptually there are only two significant changes: > > > > - make sure not to call virt_to_page on vmalloc virt addresses (patch > >#1) > > - use phys_to_dma and dma_to_phys to translate phys to/from dma > >addresses (all other patches) > > > > > > I addressed all comments by Christoph to v2 of the series except from > > the one about merging the precursor "add struct device *" patches. I can > > always merge them together at any time as needed. > > > > > > Boris gave his Reviewed-by to the whole series v2. I added his > > Reviewed-by to all patches, including the ones with small cosmetic > > fixes, except for patch #8 #9 #10 because they are either new or changed > > significantly in this version of the series. > > > > I retained Roman and Corey's Tested-by. > > Series pushed to: xen/tip.git for-linus-5.9 Fantastic, thank you!
Re: [PATCH v6 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
On Tue, 28 Jul 2020, Mathieu Poirier wrote:
> On Wed, Jul 15, 2020 at 08:33:17AM -0700, Ben Levinsky wrote:
> > R5 is included in Xilinx Zynq UltraScale MPSoC so by adding this
> > remotproc driver, we can boot the R5 sub-system in different
> > configurations.
> >
> > Acked-by: Stefano Stabellini
> > Acked-by: Ben Levinsky
> > Reviewed-by: Radhey Shyam Pandey
> > Signed-off-by: Ben Levinsky
> > Signed-off-by: Wendy Liang
> > Signed-off-by: Michal Simek
> > Signed-off-by: Ed Mooring
> > Signed-off-by: Jason Wu
> > Tested-by: Ben Levinsky
[...]
> > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
> > +{
> > + int ret, i = 0;
> > + u32 *lockstep_mode;
> > + struct device *dev = >dev;
> > + struct device_node *nc;
> > + struct zynqmp_r5_pdata *pdata;
> > +
> > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > + lockstep_mode = devm_kzalloc(dev, sizeof(u32 *), GFP_KERNEL);
> > + if (!pdata || !lockstep_mode)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, pdata);
>
> As far as I can tell the above, along with allocating memory for @pdata, is
> not
> needed since zynqmp_r5_remoteproc_remove() uses rpus[].
>
> I have only reviewed the _probe() function and already encountered a fair
> amount
> of fundemantal errors. As such I will stop my review here. I will need to
> see a
> reviewed-by tag (on the mailing list) by Stephano or Michal before reviewing
> the
> next set.
Let me take this opportunity to say that my Acked-by on this version of
the series was an unintentional miscommunication: I didn't give my
Acked-by as I haven't even read the patches yet.
I'll circle back with Michal and we'll make sure for either of us to do
a round of public reviews on the next version.
Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
On Thu, 23 Jul 2020, Anchal Agarwal wrote:
> On Wed, Jul 22, 2020 at 04:49:16PM -0700, Stefano Stabellini wrote:
> > CAUTION: This email originated from outside of the organization. Do not
> > click links or open attachments unless you can confirm the sender and know
> > the content is safe.
> >
> >
> >
> > On Wed, 22 Jul 2020, Anchal Agarwal wrote:
> > > On Tue, Jul 21, 2020 at 05:18:34PM -0700, Stefano Stabellini wrote:
> > > > On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> > > > > >>>>>> +static int xen_setup_pm_notifier(void)
> > > > > >>>>>> +{
> > > > > >>>>>> + if (!xen_hvm_domain())
> > > > > >>>>>> + return -ENODEV;
> > > > > >>>>>>
> > > > > >>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > > > > >>>>> It would be great to support that however, its out of
> > > > > >>>>> scope for this patch set.
> > > > > >>>>> I’ll be happy to discuss it separately.
> > > > > >>>>
> > > > > >>>> I wasn't implying that this *should* work on ARM but rather
> > > > > >>>> whether this
> > > > > >>>> will break ARM somehow (because xen_hvm_domain() is true there).
> > > > > >>>>
> > > > > >>>>
> > > > > >>> Ok makes sense. TBH, I haven't tested this part of code on ARM
> > > > > >>> and the series
> > > > > >>> was only support x86 guests hibernation.
> > > > > >>> Moreover, this notifier is there to distinguish between 2 PM
> > > > > >>> events PM SUSPEND and PM hibernation. Now since we only care
> > > > > >>> about PM
> > > > > >>> HIBERNATION I may just remove this code and rely on
> > > > > >>> "SHUTDOWN_SUSPEND" state.
> > > > > >>> However, I may have to fix other patches in the series where this
> > > > > >>> check may
> > > > > >>> appear and cater it only for x86 right?
> > > > > >>
> > > > > >>
> > > > > >> I don't know what would happen if ARM guest tries to handle
> > > > > >> hibernation
> > > > > >> callbacks. The only ones that you are introducing are in block and
> > > > > >> net
> > > > > >> fronts and that's arch-independent.
> > > > > >>
> > > > > >>
> > > > > >> You do add a bunch of x86-specific code though (syscore ops), would
> > > > > >> something similar be needed for ARM?
> > > > > >>
> > > > > >>
> > > > > > I don't expect this to work out of the box on ARM. To start with
> > > > > > something
> > > > > > similar will be needed for ARM too.
> > > > > > We may still want to keep the driver code as-is.
> > > > > >
> > > > > > I understand the concern here wrt ARM, however, currently the
> > > > > > support is only
> > > > > > proposed for x86 guests here and similar work could be carried out
> > > > > > for ARM.
> > > > > > Also, if regular hibernation works correctly on arm, then all is
> > > > > > needed is to
> > > > > > fix Xen side of things.
> > > > > >
> > > > > > I am not sure what could be done to achieve any assurances on arm
> > > > > > side as far as
> > > > > > this series is concerned.
> > > >
> > > > Just to clarify: new features don't need to work on ARM or cause any
> > > > addition efforts to you to make them work on ARM. The patch series only
> > > > needs not to break existing code paths (on ARM and any other platforms).
> > > > It should also not make it overly difficult to implement the ARM side of
> > > > things (if there is one) at some point in the future.
> > > >
> > > > FYI drivers/xen/manage.c is compiled and working on ARM today, however
> > > > Xen suspend/resume is not supported. I don't know for sure if
> > > > guest-initiated hibernation works because I have
Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
On Wed, 22 Jul 2020, Anchal Agarwal wrote:
> On Tue, Jul 21, 2020 at 05:18:34PM -0700, Stefano Stabellini wrote:
> > On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> > > >>>>>> +static int xen_setup_pm_notifier(void)
> > > >>>>>> +{
> > > >>>>>> + if (!xen_hvm_domain())
> > > >>>>>> + return -ENODEV;
> > > >>>>>>
> > > >>>>>> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > > >>>>> It would be great to support that however, its out of
> > > >>>>> scope for this patch set.
> > > >>>>> I’ll be happy to discuss it separately.
> > > >>>>
> > > >>>> I wasn't implying that this *should* work on ARM but rather whether
> > > >>>> this
> > > >>>> will break ARM somehow (because xen_hvm_domain() is true there).
> > > >>>>
> > > >>>>
> > > >>> Ok makes sense. TBH, I haven't tested this part of code on ARM and
> > > >>> the series
> > > >>> was only support x86 guests hibernation.
> > > >>> Moreover, this notifier is there to distinguish between 2 PM
> > > >>> events PM SUSPEND and PM hibernation. Now since we only care about PM
> > > >>> HIBERNATION I may just remove this code and rely on
> > > >>> "SHUTDOWN_SUSPEND" state.
> > > >>> However, I may have to fix other patches in the series where this
> > > >>> check may
> > > >>> appear and cater it only for x86 right?
> > > >>
> > > >>
> > > >> I don't know what would happen if ARM guest tries to handle hibernation
> > > >> callbacks. The only ones that you are introducing are in block and net
> > > >> fronts and that's arch-independent.
> > > >>
> > > >>
> > > >> You do add a bunch of x86-specific code though (syscore ops), would
> > > >> something similar be needed for ARM?
> > > >>
> > > >>
> > > > I don't expect this to work out of the box on ARM. To start with
> > > > something
> > > > similar will be needed for ARM too.
> > > > We may still want to keep the driver code as-is.
> > > >
> > > > I understand the concern here wrt ARM, however, currently the support
> > > > is only
> > > > proposed for x86 guests here and similar work could be carried out for
> > > > ARM.
> > > > Also, if regular hibernation works correctly on arm, then all is needed
> > > > is to
> > > > fix Xen side of things.
> > > >
> > > > I am not sure what could be done to achieve any assurances on arm side
> > > > as far as
> > > > this series is concerned.
> >
> > Just to clarify: new features don't need to work on ARM or cause any
> > addition efforts to you to make them work on ARM. The patch series only
> > needs not to break existing code paths (on ARM and any other platforms).
> > It should also not make it overly difficult to implement the ARM side of
> > things (if there is one) at some point in the future.
> >
> > FYI drivers/xen/manage.c is compiled and working on ARM today, however
> > Xen suspend/resume is not supported. I don't know for sure if
> > guest-initiated hibernation works because I have not tested it.
> >
> >
> >
> > > If you are not sure what the effects are (or sure that it won't work) on
> > > ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
> > >
> > >
> > > if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
> > > return -ENODEV;
> >
> > That is a good principle to have and thanks for suggesting it. However,
> > in this specific case there is nothing in this patch that doesn't work
> > on ARM. From an ARM perspective I think we should enable it and
> > _pm_notifier_block should be registered.
> >
> This question is for Boris, I think you we decided to get rid of the notifier
> in V3 as all we need to check is SHUTDOWN_SUSPEND state which sounds
> plausible
> to me. So this check may go away. It may still be needed for sycore_ops
> callbacks registration.
> > Given that all guests are HVM guests on ARM, it should work fine as is.
> >
> >
> > I gave a quick look at the rest of the series and everything looks fin
Re: [PATCH v2 01/11] xen/manage: keep track of the on-going suspend mode
On Tue, 21 Jul 2020, Boris Ostrovsky wrote:
> >> +static int xen_setup_pm_notifier(void)
> >> +{
> >> + if (!xen_hvm_domain())
> >> + return -ENODEV;
> >>
> >> I forgot --- what did we decide about non-x86 (i.e. ARM)?
> > It would be great to support that however, its out of
> > scope for this patch set.
> > I’ll be happy to discuss it separately.
>
> I wasn't implying that this *should* work on ARM but rather whether this
> will break ARM somehow (because xen_hvm_domain() is true there).
>
>
> >>> Ok makes sense. TBH, I haven't tested this part of code on ARM and the
> >>> series
> >>> was only support x86 guests hibernation.
> >>> Moreover, this notifier is there to distinguish between 2 PM
> >>> events PM SUSPEND and PM hibernation. Now since we only care about PM
> >>> HIBERNATION I may just remove this code and rely on "SHUTDOWN_SUSPEND"
> >>> state.
> >>> However, I may have to fix other patches in the series where this check
> >>> may
> >>> appear and cater it only for x86 right?
> >>
> >>
> >> I don't know what would happen if ARM guest tries to handle hibernation
> >> callbacks. The only ones that you are introducing are in block and net
> >> fronts and that's arch-independent.
> >>
> >>
> >> You do add a bunch of x86-specific code though (syscore ops), would
> >> something similar be needed for ARM?
> >>
> >>
> > I don't expect this to work out of the box on ARM. To start with something
> > similar will be needed for ARM too.
> > We may still want to keep the driver code as-is.
> >
> > I understand the concern here wrt ARM, however, currently the support is
> > only
> > proposed for x86 guests here and similar work could be carried out for ARM.
> > Also, if regular hibernation works correctly on arm, then all is needed is
> > to
> > fix Xen side of things.
> >
> > I am not sure what could be done to achieve any assurances on arm side as
> > far as
> > this series is concerned.
Just to clarify: new features don't need to work on ARM or cause any
addition efforts to you to make them work on ARM. The patch series only
needs not to break existing code paths (on ARM and any other platforms).
It should also not make it overly difficult to implement the ARM side of
things (if there is one) at some point in the future.
FYI drivers/xen/manage.c is compiled and working on ARM today, however
Xen suspend/resume is not supported. I don't know for sure if
guest-initiated hibernation works because I have not tested it.
> If you are not sure what the effects are (or sure that it won't work) on
> ARM then I'd add IS_ENABLED(CONFIG_X86) check, i.e.
>
>
> if (!IS_ENABLED(CONFIG_X86) || !xen_hvm_domain())
> return -ENODEV;
That is a good principle to have and thanks for suggesting it. However,
in this specific case there is nothing in this patch that doesn't work
on ARM. From an ARM perspective I think we should enable it and
_pm_notifier_block should be registered.
Given that all guests are HVM guests on ARM, it should work fine as is.
I gave a quick look at the rest of the series and everything looks fine
to me from an ARM perspective. I cannot imaging that the new freeze,
thaw, and restore callbacks for net and block are going to cause any
trouble on ARM. The two main x86-specific functions are
xen_syscore_suspend/resume and they look trivial to implement on ARM (in
the sense that they are likely going to look exactly the same.)
One question for Anchal: what's going to happen if you trigger a
hibernation, you have the new callbacks, but you are missing
xen_syscore_suspend/resume?
Is it any worse than not having the new freeze, thaw and restore
callbacks at all and try to do a hibernation?
Re: [PATCH v2 1/4] dt-bindings: remoteproc: Add bindings for R5F subsystem on TI K3 SoCs
On Thu, 16 Jul 2020, Mathieu Poirier wrote: > Hi Rob, > > On Tue, Jul 14, 2020 at 11:15:53AM -0600, Rob Herring wrote: > > On Mon, Jun 29, 2020 at 09:49:19PM -0500, Suman Anna wrote: > > > The Texas Instruments K3 family of SoCs have one or more dual-core > > > Arm Cortex R5F processor subsystems/clusters (R5FSS). The clusters > > > can be split between multiple voltage domains as well. Add the device > > > tree bindings document for these R5F subsystem devices. These R5F > > > processors do not have an MMU, and so require fixed memory carveout > > > regions matching the firmware image addresses. The nodes require more > > > than one memory region, with the first memory region used for DMA > > > allocations at runtime. The remaining memory regions are reserved > > > and are used for the loading and running of the R5F remote processors. > > > The R5F processors can also optionally use any internal on-chip SRAM > > > memories either for executing code or using it as fast-access data. > > > > > > The added example illustrates the DT nodes for the single R5FSS device > > > present on K3 AM65x family of SoCs. > > > > > > Signed-off-by: Suman Anna > > > --- > > > v2: > > > - Renamed "lockstep-mode" property to "ti,cluster-mode" > > > > I don't think that's a move in the right direction given this is at > > least partially a standard feature. > > > > As I said before, I'm very hesistant to accept anything here given I > > know the desires and activity to define 'system Devicetrees' of which > > TI is participating. While maybe an rproc node is sufficient for a > > DSP, it seems multiple vendors have R cores and want to define them in > > system DT. > > > > Though the system DT effort has not yet given any thought to what is the > > view of one processor or instance to another instance (which is what > > this binding is). We'll still need something defined for that, but I'd > > expect that to be dependent on what is defined for system DT. > > Efforts related to the definition of the system DT are under way, something I > expect to keep going on for some time to come. I agree with the need to use > the > system DT to define remote processors and I look forward to the time we can do > so. I'll take this opportunity to add that I should be able to publicly present a System Device Tree proposal for this during the next call (the next one after the call early next week that has already a full agenda.) > That being said we need to find a concensus on how to move forward with > patches > that are ready to be merged. What is your opinion on that? In my opinion we don't have to necessarily wait for System Device Tree to make progress with those if they look OK.
Re: [PATCH] xen: introduce xen_vring_use_dma
On Sat, 11 Jul 2020, Michael S. Tsirkin wrote: > On Fri, Jul 10, 2020 at 10:23:22AM -0700, Stefano Stabellini wrote: > > Sorry for the late reply -- a couple of conferences kept me busy. > > > > > > On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > > > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > > > Would you be in favor of a more flexible check along the lines of the > > > > one proposed in the patch that started this thread: > > > > > > > > if (xen_vring_use_dma()) > > > > return true; > > > > > > > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > > > xen_swiotlb is required and false otherwise. > > > > > > Just to stress - with a patch like this virtio can *still* use DMA API > > > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > > > as you seem to be saying, you guys should fix it before doing something > > > like this.. > > > > Yes, DMA API is broken with some interfaces (specifically: rpmesg and > > trusty), but for them PLATFORM_ACCESS is never set. That is why the > > errors weren't reported before. Xen special case aside, there is no > > problem under normal circumstances. > > So why not fix DMA API? Then this patch is not needed. It looks like the conversation is going in circles :-) I tried to explain the reason why, even if we fixed the DMA API to work with rpmesg and trusty, we still need this patch with the following email: https://marc.info/?l=linux-kernel=159347446709625=2 At that time it looked like you agreed that we needed to improve this check? (https://marc.info/?l=linux-kernel=159363662418250=2)
[PATCH v3 03/11] swiotlb-xen: add struct device * parameter to xen_phys_to_bus
From: Stefano Stabellini
No functional changes. The parameter is unused in this patch but will be
used by next patches.
Signed-off-by: Stefano Stabellini
Reviewed-by: Boris Ostrovsky
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v3:
- add whitespace in title
- improve commit message
---
drivers/xen/swiotlb-xen.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 89a775948a02..dbe710a59bf2 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -57,7 +57,7 @@ static unsigned long xen_io_tlb_nslabs;
* can be 32bit when dma_addr_t is 64bit leading to a loss in
* information if the shift is done before casting to 64bit.
*/
-static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
+static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
@@ -78,9 +78,9 @@ static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
return paddr;
}
-static inline dma_addr_t xen_virt_to_bus(void *address)
+static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(virt_to_phys(address));
+ return xen_phys_to_bus(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -309,7 +309,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
phys = *dma_handle;
- dev_addr = xen_phys_to_bus(phys);
+ dev_addr = xen_phys_to_bus(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -370,7 +370,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
- dma_addr_t dev_addr = xen_phys_to_bus(phys);
+ dma_addr_t dev_addr = xen_phys_to_bus(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
@@ -395,7 +395,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
return DMA_MAPPING_ERROR;
phys = map;
- dev_addr = xen_phys_to_bus(map);
+ dev_addr = xen_phys_to_bus(dev, map);
/*
* Ensure that the address returned is DMA'ble
@@ -539,7 +539,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct
scatterlist *sgl,
static int
xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
- return xen_virt_to_bus(xen_io_tlb_end - 1) <= mask;
+ return xen_virt_to_bus(hwdev, xen_io_tlb_end - 1) <= mask;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
--
2.17.1
[PATCH v3 01/11] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses
From: Boris Ostrovsky xen_alloc_coherent_pages might return pages for which virt_to_phys and virt_to_page don't work, e.g. ioremap'ed pages. So in xen_swiotlb_free_coherent we can't assume that virt_to_page works. Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc virt addresses. This patch fixes the following crash at boot on RPi4 (the underlying issue is not RPi4 specific): https://marc.info/?l=xen-devel=158862573216800 Signed-off-by: Boris Ostrovsky Signed-off-by: Stefano Stabellini Reviewed-by: Boris Ostrovsky Tested-by: Corey Minyard Tested-by: Roman Shaposhnik --- Changes in v3: - style improvements Changes in v2: - update commit message --- drivers/xen/swiotlb-xen.c | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index b6d27762c6f8..5fbadd07819b 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, int order = get_order(size); phys_addr_t phys; u64 dma_mask = DMA_BIT_MASK(32); + struct page *page; if (hwdev && hwdev->coherent_dma_mask) dma_mask = hwdev->coherent_dma_mask; @@ -346,9 +347,14 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); + if (is_vmalloc_addr(vaddr)) + page = vmalloc_to_page(vaddr); + else + page = virt_to_page(vaddr); + if (!WARN_ON((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) && - TestClearPageXenRemapped(virt_to_page(vaddr))) + TestClearPageXenRemapped(page)) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); -- 2.17.1
[PATCH v3 11/11] xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint
From: Stefano Stabellini
dma_cache_maint is getting called passing a dma address which could be
different from a physical address.
Add a struct device* parameter to dma_cache_maint.
Translate the dma_addr_t parameter of dma_cache_maint by calling
dma_to_phys. Do it for the first page and all the following pages, in
case of multipage handling.
Signed-off-by: Stefano Stabellini
Reviewed-by: Boris Ostrovsky
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- improve commit message
---
arch/arm/xen/mm.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index a8251a70f442..396797ffe2b1 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -43,15 +43,18 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)
static bool hypercall_cflush = false;
/* buffers in highmem or foreign pages cannot cross page boundaries */
-static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
+static void dma_cache_maint(struct device *dev, dma_addr_t handle,
+ size_t size, u32 op)
{
struct gnttab_cache_flush cflush;
- cflush.a.dev_bus_addr = handle & XEN_PAGE_MASK;
cflush.offset = xen_offset_in_page(handle);
cflush.op = op;
+ handle &= XEN_PAGE_MASK;
do {
+ cflush.a.dev_bus_addr = dma_to_phys(dev, handle);
+
if (size + cflush.offset > XEN_PAGE_SIZE)
cflush.length = XEN_PAGE_SIZE - cflush.offset;
else
@@ -60,7 +63,7 @@ static void dma_cache_maint(dma_addr_t handle, size_t size,
u32 op)
HYPERVISOR_grant_table_op(GNTTABOP_cache_flush, , 1);
cflush.offset = 0;
- cflush.a.dev_bus_addr += cflush.length;
+ handle += cflush.length;
size -= cflush.length;
} while (size);
}
@@ -76,16 +79,16 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
size_t size, enum dma_data_direction dir)
{
if (dir != DMA_TO_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
}
void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
size_t size, enum dma_data_direction dir)
{
if (dir == DMA_FROM_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
else
- dma_cache_maint(handle, size, GNTTAB_CACHE_CLEAN);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_CLEAN);
}
bool xen_arch_need_swiotlb(struct device *dev,
--
2.17.1
[PATCH v3 10/11] xen/arm: introduce phys/dma translations in xen_dma_sync_for_*
From: Stefano Stabellini
xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
getting called passing dma addresses. On some platforms dma addresses
could be different from physical addresses. Before doing any operations
on these addresses we need to convert them back to physical addresses
using dma_to_phys.
Move the arch_sync_dma_for_cpu and arch_sync_dma_for_device calls from
xen_dma_sync_for_cpu/device to swiotlb-xen.c, and add a call dma_to_phys
to do address translations there.
dma_cache_maint is fixed by the next patch.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- improve commit message
- don't use pfn_valid
Changes in v3:
- move arch_sync_dma_for_cpu/device calls to swiotlb-xen.c
---
arch/arm/xen/mm.c | 17 ++---
drivers/xen/swiotlb-xen.c | 32
include/xen/swiotlb-xen.h | 6 ++
3 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index f2414ea40a79..a8251a70f442 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
#include
+#include
#include
#include
#include
@@ -72,22 +73,16 @@ static void dma_cache_maint(dma_addr_t handle, size_t size,
u32 op)
* dma-direct functions, otherwise we call the Xen specific version.
*/
void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
- phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+ size_t size, enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
- arch_sync_dma_for_cpu(paddr, size, dir);
- else if (dir != DMA_TO_DEVICE)
+ if (dir != DMA_TO_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
}
void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
-phys_addr_t paddr, size_t size,
-enum dma_data_direction dir)
+size_t size, enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
- arch_sync_dma_for_device(paddr, size, dir);
- else if (dir == DMA_FROM_DEVICE)
+ if (dir == DMA_FROM_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
else
dma_cache_maint(handle, size, GNTTAB_CACHE_CLEAN);
@@ -98,7 +93,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
dma_addr_t dev_addr)
{
unsigned int xen_pfn = XEN_PFN_DOWN(phys);
- unsigned int bfn = XEN_PFN_DOWN(dev_addr);
+ unsigned int bfn = XEN_PFN_DOWN(dma_to_phys(dev, dev_addr));
/*
* The swiotlb buffer should be used if
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a6a95358a8cb..39a0f2e0847c 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -413,8 +413,12 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
}
done:
- if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_device(dev, dev_addr, phys, size, dir);
+ if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dev_addr
+ arch_sync_dma_for_device(phys, size, dir);
+ else
+ xen_dma_sync_for_device(dev, dev_addr, size, dir);
+ }
return dev_addr;
}
@@ -433,8 +437,12 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
dma_addr_t dev_addr,
BUG_ON(dir == DMA_NONE);
- if (!dev_is_dma_coherent(hwdev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
+ if (!dev_is_dma_coherent(hwdev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
+ if (pfn_valid(PFN_DOWN(dma_to_phys(hwdev, dev_addr
+ arch_sync_dma_for_cpu(paddr, size, dir);
+ else
+ xen_dma_sync_for_cpu(hwdev, dev_addr, size, dir);
+ }
/* NOTE: We use dev_addr here, not paddr! */
if (is_xen_swiotlb_buffer(hwdev, dev_addr))
@@ -447,8 +455,12 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev,
dma_addr_t dma_addr,
{
phys_addr_t paddr = xen_dma_to_phys(dev, dma_addr);
- if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
+ if (!dev_is_dma_coherent(dev)) {
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, dma_addr
+ arch_sync_dma_for_cpu(paddr, size, dir);
+ else
+ xen_dma_sync_for_cpu(dev, dma_addr, size, dir);
+ }
if (is_xen_swiotlb_buffer(dev, dma_addr))
[PATCH v3 09/11] swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
From: Stefano Stabellini
With some devices physical addresses are different than dma addresses.
To be able to deal with these cases, we need to call phys_to_dma on
physical addresses (including machine addresses in Xen terminology)
before returning them from xen_swiotlb_alloc_coherent and
xen_swiotlb_map_page.
We also need to convert dma addresses back to physical addresses using
dma_to_phys in xen_swiotlb_free_coherent and xen_swiotlb_unmap_page if
we want to do any operations on them.
Call dma_to_phys in is_xen_swiotlb_buffer.
Introduce xen_phys_to_dma and call phys_to_dma in its implementation.
Introduce xen_dma_to_phys and call dma_to_phys in its implementation.
Call xen_phys_to_dma/xen_dma_to_phys instead of
xen_phys_to_bus/xen_bus_to_phys through swiotlb-xen.c.
Everything is taken care of by these changes except for
xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a
few explicit phys_to_dma/dma_to_phys calls.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- improve commit message
Changes in v3:
- add xen_phys_to_dma and xen_dma_to_phys in this patch
---
drivers/xen/swiotlb-xen.c | 53 +++
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 03d118b6c141..a6a95358a8cb 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -52,30 +52,39 @@ static unsigned long xen_io_tlb_nslabs;
* Quick lookup value of the bus address of the IOTLB.
*/
-static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
+static inline phys_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t
paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
- dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
+ phys_addr_t baddr = (phys_addr_t)bfn << XEN_PAGE_SHIFT;
- dma |= paddr & ~XEN_PAGE_MASK;
+ baddr |= paddr & ~XEN_PAGE_MASK;
+ return baddr;
+}
- return dma;
+static inline dma_addr_t xen_phys_to_dma(struct device *dev, phys_addr_t paddr)
+{
+ return phys_to_dma(dev, xen_phys_to_bus(dev, paddr));
}
-static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr)
+static inline phys_addr_t xen_bus_to_phys(struct device *dev,
+ phys_addr_t baddr)
{
unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
- dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
- phys_addr_t paddr = dma;
-
- paddr |= baddr & ~XEN_PAGE_MASK;
+ phys_addr_t paddr = (xen_pfn << XEN_PAGE_SHIFT) |
+ (baddr & ~XEN_PAGE_MASK);
return paddr;
}
+static inline phys_addr_t xen_dma_to_phys(struct device *dev,
+ dma_addr_t dma_addr)
+{
+ return xen_bus_to_phys(dev, dma_to_phys(dev, dma_addr));
+}
+
static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(dev, virt_to_phys(address));
+ return xen_phys_to_dma(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -94,7 +103,7 @@ static inline int range_straddles_page_boundary(phys_addr_t
p, size_t size)
static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
{
- unsigned long bfn = XEN_PFN_DOWN(dma_addr);
+ unsigned long bfn = XEN_PFN_DOWN(dma_to_phys(dev, dma_addr));
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
phys_addr_t paddr = (phys_addr_t)xen_pfn << XEN_PAGE_SHIFT;
@@ -299,12 +308,12 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
if (hwdev && hwdev->coherent_dma_mask)
dma_mask = hwdev->coherent_dma_mask;
- /* At this point dma_handle is the physical address, next we are
+ /* At this point dma_handle is the dma address, next we are
* going to set it to the machine address.
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
- phys = *dma_handle;
- dev_addr = xen_phys_to_bus(hwdev, phys);
+ phys = dma_to_phys(hwdev, *dma_handle);
+ dev_addr = xen_phys_to_dma(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -314,6 +323,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
xen_free_coherent_pages(hwdev, size, ret,
(dma_addr_t)phys, attrs);
return NULL;
}
+ *dma_handle = phys_to_dma(hwdev, *dma_handle);
SetPageXenRemapped(virt_to_page(ret));
}
memset(ret, 0, size);
@@ -334,7 +344,7 @@ xen_swiotlb_free_coherent(s
[PATCH v3 08/11] swiotlb-xen: remove XEN_PFN_PHYS
From: Stefano Stabellini
XEN_PFN_PHYS is only used in one place in swiotlb-xen making things more
complex than need to be.
Remove the definition of XEN_PFN_PHYS and open code the cast in the one
place where it is needed.
Signed-off-by: Stefano Stabellini
---
drivers/xen/swiotlb-xen.c | 7 +--
include/xen/page.h| 1 -
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index e2c35f45f91e..03d118b6c141 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -52,11 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
* Quick lookup value of the bus address of the IOTLB.
*/
-/*
- * Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
- * can be 32bit when dma_addr_t is 64bit leading to a loss in
- * information if the shift is done before casting to 64bit.
- */
static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
@@ -101,7 +96,7 @@ static int is_xen_swiotlb_buffer(struct device *dev,
dma_addr_t dma_addr)
{
unsigned long bfn = XEN_PFN_DOWN(dma_addr);
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
- phys_addr_t paddr = XEN_PFN_PHYS(xen_pfn);
+ phys_addr_t paddr = (phys_addr_t)xen_pfn << XEN_PAGE_SHIFT;
/* If the address is outside our domain, it CAN
* have the same virtual address as another address
diff --git a/include/xen/page.h b/include/xen/page.h
index df6d6b6ec66e..285677b42943 100644
--- a/include/xen/page.h
+++ b/include/xen/page.h
@@ -24,7 +24,6 @@
#define XEN_PFN_DOWN(x)((x) >> XEN_PAGE_SHIFT)
#define XEN_PFN_UP(x) (((x) + XEN_PAGE_SIZE-1) >> XEN_PAGE_SHIFT)
-#define XEN_PFN_PHYS(x)((phys_addr_t)(x) << XEN_PAGE_SHIFT)
#include
--
2.17.1
[PATCH v3 05/11] swiotlb-xen: add struct device * parameter to xen_dma_sync_for_cpu
From: Stefano Stabellini
No functional changes. The parameter is unused in this patch but will be
used by next patches.
Signed-off-by: Stefano Stabellini
Reviewed-by: Boris Ostrovsky
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v3:
- add whitespace in title
- improve commit message
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index d40e9e5fc52b..1a00e8003c64 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -71,8 +71,9 @@ static void dma_cache_maint(dma_addr_t handle, size_t size,
u32 op)
* pfn_valid returns true the pages is local and we can use the native
* dma-direct functions, otherwise we call the Xen specific version.
*/
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_cpu(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a8e447137faf..d04b7a15124f 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -428,7 +428,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
dma_addr_t dev_addr,
BUG_ON(dir == DMA_NONE);
if (!dev_is_dma_coherent(hwdev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_cpu(dev_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
if (is_xen_swiotlb_buffer(dev_addr))
@@ -442,7 +442,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev,
dma_addr_t dma_addr,
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index ffc0d3902b71..f62d1854780b 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -4,8 +4,9 @@
#include
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir);
void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
--
2.17.1
[PATCH v3 06/11] swiotlb-xen: add struct device * parameter to xen_dma_sync_for_device
From: Stefano Stabellini
No functional changes. The parameter is unused in this patch but will be
used by next patches.
Signed-off-by: Stefano Stabellini
Reviewed-by: Boris Ostrovsky
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v3:
- add whitespace in title
- improve commit message
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 1a00e8003c64..f2414ea40a79 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -81,8 +81,9 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
}
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+phys_addr_t paddr, size_t size,
+enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_device(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index d04b7a15124f..8a3a7bcc5ec0 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -408,7 +408,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
done:
if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_device(dev_addr, phys, size, dir);
+ xen_dma_sync_for_device(dev, dev_addr, phys, size, dir);
return dev_addr;
}
@@ -458,7 +458,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev,
dma_addr_t dma_addr,
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_device(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_device(dev, dma_addr, paddr, size, dir);
}
/*
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index f62d1854780b..6d235fe2b92d 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -7,8 +7,9 @@
void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+phys_addr_t paddr, size_t size,
+enum dma_data_direction dir);
extern int xen_swiotlb_init(int verbose, bool early);
extern const struct dma_map_ops xen_swiotlb_dma_ops;
--
2.17.1
[PATCH v3 02/11] swiotlb-xen: remove start_dma_addr
From: Stefano Stabellini
It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
instead. It will be useful not to have a start_dma_addr around with the
next patches.
Note that virt_to_phys is not the same as xen_virt_to_bus but actually
it is used to compared again __pa(xen_io_tlb_start) as passed to
swiotlb_init_with_tbl, so virt_to_phys is actually what we want.
Signed-off-by: Stefano Stabellini
Reviewed-by: Boris Ostrovsky
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- update commit message
---
drivers/xen/swiotlb-xen.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 5fbadd07819b..89a775948a02 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
* Quick lookup value of the bus address of the IOTLB.
*/
-static u64 start_dma_addr;
-
/*
* Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
* can be 32bit when dma_addr_t is 64bit leading to a loss in
@@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
m_ret = XEN_SWIOTLB_EFIXUP;
goto error;
}
- start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
if (early) {
if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
verbose))
@@ -392,8 +389,8 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
*/
trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
- map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
-size, size, dir, attrs);
+ map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start),
+phys, size, size, dir, attrs);
if (map == (phys_addr_t)DMA_MAPPING_ERROR)
return DMA_MAPPING_ERROR;
--
2.17.1
[PATCH v3 04/11] swiotlb-xen: add struct device * parameter to xen_bus_to_phys
From: Stefano Stabellini
No functional changes. The parameter is unused in this patch but will be
used by next patches.
Signed-off-by: Stefano Stabellini
Reviewed-by: Boris Ostrovsky
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v3:
- add whitespace in title
- improve commit message
---
drivers/xen/swiotlb-xen.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index dbe710a59bf2..a8e447137faf 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -67,7 +67,7 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev,
phys_addr_t paddr)
return dma;
}
-static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
+static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr)
{
unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
@@ -339,7 +339,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
size, void *vaddr,
/* do not use virt_to_phys because on ARM it doesn't return you the
* physical address */
- phys = xen_bus_to_phys(dev_addr);
+ phys = xen_bus_to_phys(hwdev, dev_addr);
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
@@ -423,7 +423,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- phys_addr_t paddr = xen_bus_to_phys(dev_addr);
+ phys_addr_t paddr = xen_bus_to_phys(hwdev, dev_addr);
BUG_ON(dir == DMA_NONE);
@@ -439,7 +439,7 @@ static void
xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
@@ -452,7 +452,7 @@ static void
xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
--
2.17.1
[PATCH v3 07/11] swiotlb-xen: add struct device * parameter to is_xen_swiotlb_buffer
From: Stefano Stabellini
No functional changes. The parameter is unused in this patch but will be
used by next patches.
Signed-off-by: Stefano Stabellini
Reviewed-by: Boris Ostrovsky
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v3:
- add whitespace in title
- improve commit message
---
drivers/xen/swiotlb-xen.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 8a3a7bcc5ec0..e2c35f45f91e 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -97,7 +97,7 @@ static inline int range_straddles_page_boundary(phys_addr_t
p, size_t size)
return 0;
}
-static int is_xen_swiotlb_buffer(dma_addr_t dma_addr)
+static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
{
unsigned long bfn = XEN_PFN_DOWN(dma_addr);
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
@@ -431,7 +431,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
dma_addr_t dev_addr,
xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
- if (is_xen_swiotlb_buffer(dev_addr))
+ if (is_xen_swiotlb_buffer(hwdev, dev_addr))
swiotlb_tbl_unmap_single(hwdev, paddr, size, size, dir, attrs);
}
@@ -444,7 +444,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev,
dma_addr_t dma_addr,
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
}
@@ -454,7 +454,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev,
dma_addr_t dma_addr,
{
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
--
2.17.1
[PATCH v3 00/11] fix swiotlb-xen for RPi4
Hi all, This series is a collection of fixes to get Linux running on the RPi4 as dom0. Conceptually there are only two significant changes: - make sure not to call virt_to_page on vmalloc virt addresses (patch #1) - use phys_to_dma and dma_to_phys to translate phys to/from dma addresses (all other patches) I addressed all comments by Christoph to v2 of the series except from the one about merging the precursor "add struct device *" patches. I can always merge them together at any time as needed. Boris gave his Reviewed-by to the whole series v2. I added his Reviewed-by to all patches, including the ones with small cosmetic fixes, except for patch #8 #9 #10 because they are either new or changed significantly in this version of the series. I retained Roman and Corey's Tested-by. Cheers, Stefano git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git fix-rpi4-v3 Boris Ostrovsky (1): swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses Stefano Stabellini (10): swiotlb-xen: remove start_dma_addr swiotlb-xen: add struct device * parameter to xen_phys_to_bus swiotlb-xen: add struct device * parameter to xen_bus_to_phys swiotlb-xen: add struct device * parameter to xen_dma_sync_for_cpu swiotlb-xen: add struct device * parameter to xen_dma_sync_for_device swiotlb-xen: add struct device * parameter to is_xen_swiotlb_buffer swiotlb-xen: remove XEN_PFN_PHYS swiotlb-xen: introduce phys_to_dma/dma_to_phys translations xen/arm: introduce phys/dma translations in xen_dma_sync_for_* xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint arch/arm/xen/mm.c | 34 +++ drivers/xen/swiotlb-xen.c | 119 ++--- include/xen/page.h| 1 - include/xen/swiotlb-xen.h | 8 4 files changed, 93 insertions(+), 69 deletions(-)
Re: [PATCH v4 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
Sorry for the late reply, a couple of conferences kept me busy. On Mon, 29 Jun 2020, Bjorn Andersson wrote: > > However, given the fragmentation of the remoteproc bindings across > > multiple vendors (they are all different), I think it is a good idea for > > Linux, for System Device Tree, and in general to come up with simpler > > remoteproc bindings, more aligned between the vendors. If nothing else, > > it is going to make Lopper's development easier. > > > > In my view the big reason for the fragmentation between bindings is > because they all describe different hardware. There has been common > properties of remoteprocs discussed, but apart from the firmware-name > property I don't think we have agreed on any. Yeah, it is as you wrote. I meant to say that there might be room for improvement if the vendors come together and agree on a few more common properties. However, I don't have any concrete suggestions on this yet. Also, as mentioned, we can work with today's bindings just fine from a system device tree perspective. > Can you give some examples of how you will be able to describe the > hardware involved in powering/clocking resources surrounding your > remoteproc and the necessary resources in a "simpler and vendor neutral" > way that then can be further lopped(?) into something that Linux can use > to control any remoteproc? The description at the system device tree level looks a bit different, which might make the problem a bit easier, or at least different. Let me give you some context. Lopper (https://github.com/devicetree-org/lopper) is a tool that takes a system device tree as input and generates one or more traditional device trees as output (i.e. today's device tree for Linux.) System device tree comes with the description of multiple "execution domains" (https://connect.linaro.org/resources/ltd20/ltd20-205/) and the ability to assign resources to each of them. That part is vendor-neutral. We also have the ability to define a vendor-specific flag when assigning resources. All together it enables us to describe an openamp/remoteproc system with only very few vendor-specific info. I am working on a full example of an input system device tree with openamp information and the resulting traditional Linux devicetree. I'll make sure to reach out when I have it ready. > > So I think it is a good idea to take this opportunity to simplify the > > Xilinx remoteproc bindings as you suggested. The idea of to removing the > > TCM nodes is a good one. In addition I asked Ben to have a look at > > whether the mboxes and mbox-names properties can be removed too. > > > > If your remoteproc uses a mailbox for signaling, then this should be > described in devicetree. This will allow you to reuse components in > other designs where either part is replaced or reused. OK
Re: [PATCH] xen: introduce xen_vring_use_dma
Sorry for the late reply -- a couple of conferences kept me busy. On Wed, 1 Jul 2020, Michael S. Tsirkin wrote: > On Wed, Jul 01, 2020 at 10:34:53AM -0700, Stefano Stabellini wrote: > > Would you be in favor of a more flexible check along the lines of the > > one proposed in the patch that started this thread: > > > > if (xen_vring_use_dma()) > > return true; > > > > > > xen_vring_use_dma would be implemented so that it returns true when > > xen_swiotlb is required and false otherwise. > > Just to stress - with a patch like this virtio can *still* use DMA API > if PLATFORM_ACCESS is set. So if DMA API is broken on some platforms > as you seem to be saying, you guys should fix it before doing something > like this.. Yes, DMA API is broken with some interfaces (specifically: rpmesg and trusty), but for them PLATFORM_ACCESS is never set. That is why the errors weren't reported before. Xen special case aside, there is no problem under normal circumstances. If you are OK with this patch (after a little bit of clean-up), Peng, are you OK with sending an update or do you want me to?
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 1 Jul 2020, Christoph Hellwig wrote: > On Mon, Jun 29, 2020 at 04:46:09PM -0700, Stefano Stabellini wrote: > > > I could imagine some future Xen hosts setting a flag somewhere in the > > > platform capability saying "no xen specific flag, rely on > > > "VIRTIO_F_ACCESS_PLATFORM". Then you set that accordingly in QEMU. > > > How about that? > > > > Yes, that would be fine and there is no problem implementing something > > like that when we get virtio support in Xen. Today there are still no > > virtio interfaces provided by Xen to ARM guests (no virtio-block/net, > > etc.) > > > > In fact, in both cases we are discussing virtio is *not* provided by > > Xen; it is a firmware interface to something entirely different: > > > > 1) virtio is used to talk to a remote AMP processor (RPMesg) > > 2) virtio is used to talk to a secure-world firmware/OS (Trusty) > > > > VIRTIO_F_ACCESS_PLATFORM is not set by Xen in these cases but by RPMesg > > and by Trusty respectively. I don't know if Trusty should or should not > > set VIRTIO_F_ACCESS_PLATFORM, but I think Linux should still work > > without issues. > > > > Any virtio implementation that is not in control of the memory map > (aka not the hypervisor) absolutely must set VIRTIO_F_ACCESS_PLATFORM, > else it is completely broken. Lots of broken virtio implementations out there it would seem :-( > > The xen_domain() check in Linux makes it so that vring_use_dma_api > > returns the opposite value on native Linux compared to Linux as Xen/ARM > > DomU by "accident". By "accident" because there is no architectural > > reason why Linux Xen/ARM DomU should behave differently compared to > > native Linux in this regard. > > > > I hope that now it is clearer why I think the if (xen_domain()) check > > needs to be improved anyway, even if we fix generic dma_ops with virtio > > interfaces missing VIRTIO_F_ACCESS_PLATFORM. > > IMHO that Xen quirk should never have been added in this form.. Would you be in favor of a more flexible check along the lines of the one proposed in the patch that started this thread: if (xen_vring_use_dma()) return true; xen_vring_use_dma would be implemented so that it returns true when xen_swiotlb is required and false otherwise.
Re: [PATCH v4 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings
On Wed, 10 Jun 2020, Rob Herring wrote: > On Tue, May 26, 2020 at 11:40 AM Ben Levinsky wrote: > > > > Hi Rob, > > > > The Xilinx R5 Remoteproc driver has been around for a long time -- > > admittedly we should have upstreamed it long ago. The driver in the current > > form is using an "classic" remoteproc device tree node as described here. > > I would rather not have 2 possible bindings to maintain. If there's > been no rush to upstream this til now, then it can wait longer. > > > > > I am working with Stefano to come up with an appropriate System Device Tree > > representation but it is not going to be ready right away. Our preference > > would be to upstream the remoteproc node and driver in their current forms > > while system device tree is maturing. > > There's obviously going to still need to be some sort of description > of the interface between cores, but this has parts that obviously > conflict with what's getting defined for system DT. The TCMs are the > most obvious. If you can remove (or hardcode in the driver) what > conflicts, then perhaps this can be upstreamed now. Hi Rob, Sorry it took a while to answer back but we wanted to do some research to make sure the reply is correct. The System Device Tree version of the OpenAMP remoteproc bindings aims at being simpler and vendor-neutral. As anything else System Device Tree, Lopper will read it and generate a "traditional" device tree with the existing remoteproc bindings. In that sense, it might not affect Linux directly. However, given the fragmentation of the remoteproc bindings across multiple vendors (they are all different), I think it is a good idea for Linux, for System Device Tree, and in general to come up with simpler remoteproc bindings, more aligned between the vendors. If nothing else, it is going to make Lopper's development easier. So I think it is a good idea to take this opportunity to simplify the Xilinx remoteproc bindings as you suggested. The idea of to removing the TCM nodes is a good one. In addition I asked Ben to have a look at whether the mboxes and mbox-names properties can be removed too. Ben will reply with a simplified bindings proposal.
RE: [PATCH] xen: introduce xen_vring_use_dma
On Mon, 29 Jun 2020, Peng Fan wrote: > > > If that is the case, how is it possible that virtio breaks on ARM > > > using the default dma_ops? The breakage is not Xen related (except > > > that Xen turns dma_ops on). The original message from Peng was: > > > > > > vring_map_one_sg -> vring_use_dma_api > > >-> dma_map_page > > > -> __swiotlb_map_page > > > ->swiotlb_map_page > > > > > > ->__dma_map_area(phys_to_virt(dma_to_phys(dev, > > dev_addr)), size, dir); > > > However we are using per device dma area for rpmsg, phys_to_virt > > > could not return a correct virtual address for virtual address in > > > vmalloc area. Then kernel panic. > > > > > > I must be missing something. Maybe it is because it has to do with RPMesg? > > > > I think it's an RPMesg bug, yes > > rpmsg bug is another issue, it should not use dma_alloc_coherent for reserved > area, > and use vmalloc_to_page. > > Anyway here using dma api will also trigger issue. Is the stack trace above for the RPMesg issue or for the Trusty issue? If it is the stack trace for RPMesg, can you also post the Trusty stack trace? Or are they indentical?
Re: [PATCH] xen: introduce xen_vring_use_dma
On Fri, 26 Jun 2020, Michael S. Tsirkin wrote: > On Thu, Jun 25, 2020 at 10:31:27AM -0700, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote: > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to > > > > > > > > map the > > > > > > > > ring: when xen_swiotlb is enabled the dma API is required. When > > > > > > > > it is > > > > > > > > disabled, it is not required. > > > > > > > > > > > > > > > > Signed-off-by: Peng Fan > > > > > > > > > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > > > > > Xen was there first, but everyone else is using that now. > > > > > > > > > > > > Unfortunately it is complicated and it is not related to > > > > > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > > > > > > > > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > > > > > foreign mappings (memory coming from other VMs) to physical > > > > > > addresses. > > > > > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > > > > > address into a real physical address (this is unneeded on ARM.) > > > > > > > > > > > > > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on > > > > > > Xen/x86 > > > > > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > > > > > mappings.) That is why we have the if (xen_domain) return true; in > > > > > > vring_use_dma_api. > > > > > > > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > > > > > > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > > > > > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > > > > > > > > > Unfortunately as a result Xen never got around to > > > > > properly setting VIRTIO_F_IOMMU_PLATFORM. > > > > > > > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because > > > > the usage of swiotlb_xen is not a property of virtio, > > > > > > > > > Basically any device without VIRTIO_F_ACCESS_PLATFORM > > > (that is it's name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is > > > what linux calls it) is declared as "special, don't follow normal rules > > > for access". > > > > > > So yes swiotlb_xen is not a property of virtio, but what *is* a property > > > of virtio is that it's not special, just a regular device from DMA POV. > > > > I am trying to understand what you meant but I think I am missing > > something. > > > > Are you saying that modern virtio should always have > > VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other devices? > > I am saying it's a safe default. Clear VIRTIO_F_ACCESS_PLATFORM if you > have some special needs e.g. you are very sure it's ok to bypass DMA > ops, or you need to support a legacy guest (produced in the window > between virtio 1 support in 2014 and support for > VIRTIO_F_ACCESS_PLATFORM in 2016). Ok thanks. I understand and it makes sense to me. > > > > > > You might have noticed that I missed one possible case above: > > > > > > Xen/ARM > > > > > > DomU :-) > > > > > > > > > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. > > > > > > So if > > > > > > (xen_domain) return true; would give the wrong answer in that case. > > > > > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, > > > > > > and > > > > > > the "normal" dma_ops fail. > > > > > > >
Re: linux-next: Signed-off-by missing for commit in the xen-tip tree
On Tue, 30 Jun 2020, Stephen Rothwell wrote:
> Hi all,
>
> Commit
>
> 724f239c3401 ("arm/xen: remove the unused macro GRANT_TABLE_PHYSADDR")
>
> is missing a Signed-off-by from its committer.
Fixed. Thank you so much, I love linux-next :-)
Re: [PATCH] arm/xen: remove the unused macro GRANT_TABLE_PHYSADDR
On Sun, 28 Jun 2020, Xiaofei Tan wrote:
> Fix the following sparse warning:
>
> arch/arm64/xen/../../arm/xen/enlighten.c:244: warning: macro
> "GRANT_TABLE_PHYSADDR" is not used [-Wunused-macros]
>
> It is an isolated macro, and should be removed when its last user
> was deleted in the following commit 3cf4095d7446 ("arm/xen: Use
> xen_xlate_map_ballooned_pages to setup grant table")
>
> Signed-off-by: Xiaofei Tan
Reviewed-by: Stefano Stabellini
> ---
> arch/arm/xen/enlighten.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
> index fd4e1ce1..e93145d 100644
> --- a/arch/arm/xen/enlighten.c
> +++ b/arch/arm/xen/enlighten.c
> @@ -241,7 +241,6 @@ static int __init fdt_find_hyper_node(unsigned long node,
> const char *uname,
> * see Documentation/devicetree/bindings/arm/xen.txt for the
> * documentation of the Xen Device Tree format.
> */
> -#define GRANT_TABLE_PHYSADDR 0
> void __init xen_early_init(void)
> {
> of_scan_flat_dt(fdt_find_hyper_node, NULL);
> --
> 2.7.4
>
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to map > > > > > > the > > > > > > ring: when xen_swiotlb is enabled the dma API is required. When it > > > > > > is > > > > > > disabled, it is not required. > > > > > > > > > > > > Signed-off-by: Peng Fan > > > > > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > > > Xen was there first, but everyone else is using that now. > > > > > > > > Unfortunately it is complicated and it is not related to > > > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > > > foreign mappings (memory coming from other VMs) to physical addresses. > > > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > > > address into a real physical address (this is unneeded on ARM.) > > > > > > > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on > > > > Xen/x86 > > > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > > > mappings.) That is why we have the if (xen_domain) return true; in > > > > vring_use_dma_api. > > > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > > > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > > > > > Unfortunately as a result Xen never got around to > > > properly setting VIRTIO_F_IOMMU_PLATFORM. > > > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because > > the usage of swiotlb_xen is not a property of virtio, > > > Basically any device without VIRTIO_F_ACCESS_PLATFORM > (that is it's name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is > what linux calls it) is declared as "special, don't follow normal rules > for access". > > So yes swiotlb_xen is not a property of virtio, but what *is* a property > of virtio is that it's not special, just a regular device from DMA POV. I am trying to understand what you meant but I think I am missing something. Are you saying that modern virtio should always have VIRTIO_F_ACCESS_PLATFORM, hence use normal dma_ops as any other devices? If that is the case, how is it possible that virtio breaks on ARM using the default dma_ops? The breakage is not Xen related (except that Xen turns dma_ops on). The original message from Peng was: vring_map_one_sg -> vring_use_dma_api -> dma_map_page -> __swiotlb_map_page ->swiotlb_map_page ->__dma_map_area(phys_to_virt(dma_to_phys(dev, dev_addr)), size, dir); However we are using per device dma area for rpmsg, phys_to_virt could not return a correct virtual address for virtual address in vmalloc area. Then kernel panic. I must be missing something. Maybe it is because it has to do with RPMesg? > > > > You might have noticed that I missed one possible case above: Xen/ARM > > > > DomU :-) > > > > > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if > > > > (xen_domain) return true; would give the wrong answer in that case. > > > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and > > > > the "normal" dma_ops fail. > > > > > > > > > > > > The solution I suggested was to make the check in vring_use_dma_api more > > > > flexible by returning true if the swiotlb_xen is supposed to be used, > > > > not in general for all Xen domains, because that is what the check was > > > > really meant to do. > > > > > > Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is wrong > > > with that? > > > > swiotlb-xen is not used on Xen/ARM DomU, the default dma_ops are the > >
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > > > disabled, it is not required. > > > > > > > > Signed-off-by: Peng Fan > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > Xen was there first, but everyone else is using that now. > > > > Unfortunately it is complicated and it is not related to > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > foreign mappings (memory coming from other VMs) to physical addresses. > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > address into a real physical address (this is unneeded on ARM.) > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on Xen/x86 > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > mappings.) That is why we have the if (xen_domain) return true; in > > vring_use_dma_api. > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > Unfortunately as a result Xen never got around to > properly setting VIRTIO_F_IOMMU_PLATFORM. I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because the usage of swiotlb_xen is not a property of virtio, it is a detail of the way Linux does Xen address translations. swiotlb-xen is used to do these translations and it is hooked into the dma_ops framework. It would be possible to have a device in hardware that is virtio-compatible and doesn't set VIRTIO_F_IOMMU_PLATFORM. The device could be directly assigned (passthrough) to a DomU. We would still have to use swiotlb_xen if Xen is running. You should think of swiotlb-xen as only internal to Linux and not related to whether the (virtual or non-virtual) hardware comes with an IOMMU or not. > > You might have noticed that I missed one possible case above: Xen/ARM > > DomU :-) > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if > > (xen_domain) return true; would give the wrong answer in that case. > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and > > the "normal" dma_ops fail. > > > > > > The solution I suggested was to make the check in vring_use_dma_api more > > flexible by returning true if the swiotlb_xen is supposed to be used, > > not in general for all Xen domains, because that is what the check was > > really meant to do. > > Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is wrong with > that? swiotlb-xen is not used on Xen/ARM DomU, the default dma_ops are the ones that are used. So you are saying, why don't we fix the default dma_ops to work with virtio? It is bad that the default dma_ops crash with virtio, so yes I think it would be good to fix that. However, even if we fixed that, the if (xen_domain()) check in vring_use_dma_api is still a problem. Alternatively we could try to work-around it from swiotlb-xen. We could enable swiotlb-xen for Xen/ARM DomUs with a different implementation so that we could leave the vring_use_dma_api check unmodified. It would be ugly because we would have to figure out from the new swiotlb-xen functions if the device is a normal device, so we have to call the regular dma_ops functions, or if the device is a virtio device, in which case there is nothing to do. I think it is undesirable but could probably be made to work.
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > Export xen_swiotlb for all platforms using xen swiotlb > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > disabled, it is not required. > > > > Signed-off-by: Peng Fan > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > Xen was there first, but everyone else is using that now. Unfortunately it is complicated and it is not related to VIRTIO_F_IOMMU_PLATFORM :-( The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate foreign mappings (memory coming from other VMs) to physical addresses. On x86, it also uses dma_ops to translate Linux's idea of a physical address into a real physical address (this is unneeded on ARM.) So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on Xen/x86 always and on Xen/ARM if Linux is Dom0 (because it has foreign mappings.) That is why we have the if (xen_domain) return true; in vring_use_dma_api. You might have noticed that I missed one possible case above: Xen/ARM DomU :-) Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if (xen_domain) return true; would give the wrong answer in that case. Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and the "normal" dma_ops fail. The solution I suggested was to make the check in vring_use_dma_api more flexible by returning true if the swiotlb_xen is supposed to be used, not in general for all Xen domains, because that is what the check was really meant to do. In this regards I have two more comments: - the comment on top of the check in vring_use_dma_api is still valid - the patch looks broken on x86: it should always return true, but it returns false instead > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index a2de775801af..768afd79f67a 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device > > *vdev) > > * the DMA API if we're a Xen guest, which at least allows > > * all of the sensible Xen configurations to work correctly. > > */ > > - if (xen_domain()) > > + if (xen_vring_use_dma()) > > return true; > > > > return false; > > > The comment above this should probably be fixed. >
Re: [PATCH v2 10/11] xen/arm: introduce phys/dma translations in xen_dma_sync_for_*
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 10:38:02PM -0700, Christoph Hellwig wrote:
> > On Mon, Jun 08, 2020 at 05:38:28PM -0700, Stefano Stabellini wrote:
> > > Yeah, the pfn_valid check is a bit weird by definition because we are
> > > using it to understand whether the address belong to us or to another
> > > VM. To do the pfn_valid check we need to translate the dma address into
> > > something the CPU understands, hence, the dma_to_phys call.
> > >
> > > Why can't we use the already-provided paddr? Because paddr has been
> > > translated twice:
> > > 1) from dma to maybe-foreign phys address (could be ours, or another VM)
> > > 2) from maybe-foreign address to local (using our local mapping of the
> > > foreign page)
> > >
> > > In fact, it would be clearer if we had all three addresses as parameters
> > > of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
> > > address (we tend to call it xenbus address, baddr), the local physical
> > > address. Something like:
> >
> > I think instead we should move the arch_sync_dma_for_{device,cpu}
> > calls from xen_dma_sync_for_{device,cpu} into the callers, as they
> > are provided by the generic dma-noncoherent.h and optimized out for
> > coherent architectures like x86. Then the swiotlb-xen.c code only
> > need to call dma_cache_maint as the interface (which would have to
> > grow a better name), which should then only need a single kind of
> > address.
>
> ... actually I'd keep the xen_dma_sync_for_{device,cpu} names for the
> low-level interface, just move the arch_sync_dma_for_{device,cpu}
> calls up.
I can do that.
Re: [PATCH v2 08/11] swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Mon, Jun 08, 2020 at 04:06:57PM -0700, Stefano Stabellini wrote:
> > I understand what you are suggesting about having something like:
> >
> > xen_phys_to_dma(...)
> > {
> > phys_addr_t phys = xen_phys_to_bus(dev, paddr)
> > return phys_to_dma(phys);
> > }
> >
> > I thought about it myself. I'll do it.
>
> "something", yes. Except that I think the bus is a little confusing,
> isn't it? What is the Xen term for these addresses?
Xen reasons in terms of frame numbers. Xen calls them "dfn" for device
frame number. They were supposed to be called "bfn" but eventually they
settled for a different name when the series was committed.
I could s/bfn/dfn/g to match the terminology, if that helps.
> Also we probably don't need the extra local variable.
Sure
> > But I don't think I understood the comment about XEN_PFN_PHYS.
>
> There is a comment above xen_phys_to_bus that it avoids using
> XEN_PFN_PHYS because XEN_PFN_PHYS of the phys_addr_t vs dma_addr_t
> mismatch. But XEN_PFN_PHYS could just use a u64 instead of the
> phys_addr_t and then we could use it. Especially as XEN_PFN_PHYS
> isn't actually used anywhere except in swiotlb-xen.c. Or we could
> remove XEN_PFN_PHYS enirely, as it isn't all that useful to start
> with.
I'll remove it.
Re: [PATCH v2 08/11] swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
On Mon, 8 Jun 2020, Stefano Stabellini wrote:
> On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> > On Wed, Jun 03, 2020 at 03:22:44PM -0700, Stefano Stabellini wrote:
> > > From: Stefano Stabellini
> > >
> > > With some devices physical addresses are different than dma addresses.
> > > To be able to deal with these cases, we need to call phys_to_dma on
> > > physical addresses (including machine addresses in Xen terminology)
> > > before returning them from xen_swiotlb_alloc_coherent and
> > > xen_swiotlb_map_page.
> > >
> > > We also need to convert dma addresses back to physical addresses using
> > > dma_to_phys in xen_swiotlb_free_coherent and xen_swiotlb_unmap_page if
> > > we want to do any operations on them.
> > >
> > > Call dma_to_phys in is_xen_swiotlb_buffer.
> > > Call phys_to_dma in xen_phys_to_bus.
> > > Call dma_to_phys in xen_bus_to_phys.
> > >
> > > Everything is taken care of by these changes except for
> > > xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a
> > > few explicit phys_to_dma/dma_to_phys calls.
> > >
> > > Signed-off-by: Stefano Stabellini
> > > Tested-by: Corey Minyard
> > > Tested-by: Roman Shaposhnik
> > > ---
> > > Changes in v2:
> > > - improve commit message
> > > ---
> > > drivers/xen/swiotlb-xen.c | 22 --
> > > 1 file changed, 12 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > index 0a6cb67f0fc4..60ef07440905 100644
> > > --- a/drivers/xen/swiotlb-xen.c
> > > +++ b/drivers/xen/swiotlb-xen.c
> > > @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct
> > > device *dev, phys_addr_t paddr)
> > >
> > > dma |= paddr & ~XEN_PAGE_MASK;
> > >
> > > - return dma;
> > > + return phys_to_dma(dev, dma);
> >
> > So looking at this function:
> >
> > The dma name for something passed to phys_to_dma is really
> > weird.
>
> Yeah, that is true, I am not sure why I chose that confusing name. I'll
> rename it.
>
>
> > The fact that the comments says don't use XEN_PFN_PHYS
> > beause of the type mismatch while nothing but swiotlb-xen is the only
> > user of XEN_PFN_PHYS is also weird. I think XEN_PFN_PHYS needs to move
> > to swiotlb-xen first, then use a hardcoded u64 for the size, and the
> > split the function into a phys_to_xen_phys (or so) function where
> > the result gets passed to phys_to_dma.
>
> I understand what you are suggesting about having something like:
>
> xen_phys_to_dma(...)
> {
> phys_addr_t phys = xen_phys_to_bus(dev, paddr)
> return phys_to_dma(phys);
> }
>
> I thought about it myself. I'll do it.
>
> But I don't think I understood the comment about XEN_PFN_PHYS.
You meant to move the #define from the header to swiotlb-xen.c, didn't
you, and to use a cast to u64 instead of phys_addr_t?
Re: [PATCH v2 10/11] xen/arm: introduce phys/dma translations in xen_dma_sync_for_*
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 03:22:46PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini
> >
> > xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
> > getting called passing dma addresses. On some platforms dma addresses
> > could be different from physical addresses. Before doing any operations
> > on these addresses we need to convert them back to physical addresses
> > using dma_to_phys.
> >
> > Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
> > and xen_arch_need_swiotlb.
> >
> > dma_cache_maint is fixed by the next patch.
>
> The calling conventions because really weird now because
> xen_dma_sync_for_{device,cpu} already get both a phys_addr_t and
> a dma_addr_t.
>
> >
> > - if (pfn_valid(PFN_DOWN(handle)))
> > + if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
>
> But here we translate the dma address to a phys addr
>
> > arch_sync_dma_for_cpu(paddr, size, dir);
>
> While this still uses the passed in paddr. I think the uses of
> addresses in this code really needs a major rethink.
Yeah, the pfn_valid check is a bit weird by definition because we are
using it to understand whether the address belong to us or to another
VM. To do the pfn_valid check we need to translate the dma address into
something the CPU understands, hence, the dma_to_phys call.
Why can't we use the already-provided paddr? Because paddr has been
translated twice:
1) from dma to maybe-foreign phys address (could be ours, or another VM)
2) from maybe-foreign address to local (using our local mapping of the foreign
page)
In fact, it would be clearer if we had all three addresses as parameters
of xen_dma_sync_for_cpu: the dma address, the maybe-foreign physical
address (we tend to call it xenbus address, baddr), the local physical
address. Something like:
void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
phys_addr_t baddr, phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
if (pfn_valid(baddr))
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
}
Re: [PATCH v2 08/11] swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
On Mon, 8 Jun 2020, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 03:22:44PM -0700, Stefano Stabellini wrote:
> > From: Stefano Stabellini
> >
> > With some devices physical addresses are different than dma addresses.
> > To be able to deal with these cases, we need to call phys_to_dma on
> > physical addresses (including machine addresses in Xen terminology)
> > before returning them from xen_swiotlb_alloc_coherent and
> > xen_swiotlb_map_page.
> >
> > We also need to convert dma addresses back to physical addresses using
> > dma_to_phys in xen_swiotlb_free_coherent and xen_swiotlb_unmap_page if
> > we want to do any operations on them.
> >
> > Call dma_to_phys in is_xen_swiotlb_buffer.
> > Call phys_to_dma in xen_phys_to_bus.
> > Call dma_to_phys in xen_bus_to_phys.
> >
> > Everything is taken care of by these changes except for
> > xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a
> > few explicit phys_to_dma/dma_to_phys calls.
> >
> > Signed-off-by: Stefano Stabellini
> > Tested-by: Corey Minyard
> > Tested-by: Roman Shaposhnik
> > ---
> > Changes in v2:
> > - improve commit message
> > ---
> > drivers/xen/swiotlb-xen.c | 22 --
> > 1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 0a6cb67f0fc4..60ef07440905 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device
> > *dev, phys_addr_t paddr)
> >
> > dma |= paddr & ~XEN_PAGE_MASK;
> >
> > - return dma;
> > + return phys_to_dma(dev, dma);
>
> So looking at this function:
>
> The dma name for something passed to phys_to_dma is really
> weird.
Yeah, that is true, I am not sure why I chose that confusing name. I'll
rename it.
> The fact that the comments says don't use XEN_PFN_PHYS
> beause of the type mismatch while nothing but swiotlb-xen is the only
> user of XEN_PFN_PHYS is also weird. I think XEN_PFN_PHYS needs to move
> to swiotlb-xen first, then use a hardcoded u64 for the size, and the
> split the function into a phys_to_xen_phys (or so) function where
> the result gets passed to phys_to_dma.
I understand what you are suggesting about having something like:
xen_phys_to_dma(...)
{
phys_addr_t phys = xen_phys_to_bus(dev, paddr)
return phys_to_dma(phys);
}
I thought about it myself. I'll do it.
But I don't think I understood the comment about XEN_PFN_PHYS.
> Similar for the reverse direction.
OK
Re: [PATCH v2 03/11] swiotlb-xen: add struct device* parameter to xen_phys_to_bus
On Mon, 8 Jun 2020, Christoph Hellwig wrote: > On Wed, Jun 03, 2020 at 03:22:39PM -0700, Stefano Stabellini wrote: > > From: Stefano Stabellini > > > > The parameter is unused in this patch. > > No functional changes. > > This looks weird. I'm pretty sure you are going to use it later, but > why not just add the argument when it actually is used? It is just a matter of taste. Xen reviewers tend to ask for splitting patches into small chunks, especially large verbose non-functional changes like renaming or adding parameters. It is supposed to make it easier to review, to make it easier not to get distracted by renaming/non-functional changes while looking at the important changes. As a contributor, I am happy either way.
Re: [PATCH v2 01/11] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses
Hi Christoph, Thanks you for the review. On Mon, 8 Jun 2020, Christoph Hellwig wrote: > Well, this isn't just RPi4, but basically any ARM or ARM64 system > with non-coherent DMA (which is most of them). Well... yes :-) > > + struct page *pg; > > Please spell out page. OK > > > > if (hwdev && hwdev->coherent_dma_mask) > > dma_mask = hwdev->coherent_dma_mask; > > @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t > > size, void *vaddr, > > /* Convert the size to actually allocated. */ > > size = 1UL << (order + XEN_PAGE_SHIFT); > > > > + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) : > > + virt_to_page(vaddr); > > Please use plain old if/else to make this more readable. Sure
[PATCH v2 02/11] swiotlb-xen: remove start_dma_addr
From: Stefano Stabellini
It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
instead. It will be useful not to have a start_dma_addr around with the
next patches.
Note that virt_to_phys is not the same as xen_virt_to_bus but actually
it is used to compared again __pa(xen_io_tlb_start) as passed to
swiotlb_init_with_tbl, so virt_to_phys is actually what we want.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- update commit message
---
---
drivers/xen/swiotlb-xen.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a42129cba36e..ed09f8ac34c5 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
* Quick lookup value of the bus address of the IOTLB.
*/
-static u64 start_dma_addr;
-
/*
* Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
* can be 32bit when dma_addr_t is 64bit leading to a loss in
@@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
m_ret = XEN_SWIOTLB_EFIXUP;
goto error;
}
- start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
if (early) {
if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
verbose))
@@ -389,8 +386,8 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
*/
trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
- map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
-size, size, dir, attrs);
+ map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start),
+phys, size, size, dir, attrs);
if (map == (phys_addr_t)DMA_MAPPING_ERROR)
return DMA_MAPPING_ERROR;
--
2.17.1
[PATCH v2 00/11] fix swiotlb-xen for RPi4
Hi all, This series is a collection of fixes to get Linux running on the RPi4 as dom0. Conceptually there are only two significant changes: - make sure not to call virt_to_page on vmalloc virt addresses (patch #1) - use phys_to_dma and dma_to_phys to translate phys to/from dma addresses (all other patches) In particular in regards to the second part, the RPi4 is the first board where Xen can run that has the property that dma addresses are different from physical addresses, and swiotlb-xen was written with the assumption that phys addr == dma addr. This series adds the phys_to_dma and dma_to_phys calls to make it work. Cheers, Stefano The following changes since commit b85051e755b0e9d6dd8f17ef1da083851b83287d: Merge tag 'fixes-for-5.7-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux (2020-05-20 13:23:55 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git fix-rpi4-v2 for you to fetch changes up to 49783ba67f75da3490d2c01ed9b445d8a89bbb0d: xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint (2020-06-03 15:05:53 -0700) Boris Ostrovsky (1): swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses Stefano Stabellini (10): swiotlb-xen: remove start_dma_addr swiotlb-xen: add struct device* parameter to xen_phys_to_bus swiotlb-xen: add struct device* parameter to xen_bus_to_phys swiotlb-xen: add struct device* parameter to xen_dma_sync_for_cpu swiotlb-xen: add struct device* parameter to xen_dma_sync_for_device swiotlb-xen: add struct device* parameter to is_xen_swiotlb_buffer swiotlb-xen: introduce phys_to_dma/dma_to_phys translations swiotlb-xen: rename xen_phys_to_bus to xen_phys_to_dma and xen_bus_to_phys to xen_dma_to_phys xen/arm: introduce phys/dma translations in xen_dma_sync_for_* xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint arch/arm/xen/mm.c | 32 +++- drivers/xen/swiotlb-xen.c | 72 +--- include/xen/swiotlb-xen.h | 10 ++ 3 files changed, 62 insertions(+), 52 deletions(-)
[PATCH v2 11/11] xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint
From: Stefano Stabellini
dma_cache_maint is getting called passing a dma address which could be
different from a physical address.
Add a struct device* parameter to dma_cache_maint.
Translate the dma_addr_t parameter of dma_cache_maint by calling
dma_to_phys. Do it for the first page and all the following pages, in
case of multipage handling.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- improve commit message
---
arch/arm/xen/mm.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index bbad712a890d..1dc20f4bdc33 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -43,15 +43,18 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)
static bool hypercall_cflush = false;
/* buffers in highmem or foreign pages cannot cross page boundaries */
-static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
+static void dma_cache_maint(struct device *dev, dma_addr_t handle,
+ size_t size, u32 op)
{
struct gnttab_cache_flush cflush;
- cflush.a.dev_bus_addr = handle & XEN_PAGE_MASK;
cflush.offset = xen_offset_in_page(handle);
cflush.op = op;
+ handle &= XEN_PAGE_MASK;
do {
+ cflush.a.dev_bus_addr = dma_to_phys(dev, handle);
+
if (size + cflush.offset > XEN_PAGE_SIZE)
cflush.length = XEN_PAGE_SIZE - cflush.offset;
else
@@ -60,7 +63,7 @@ static void dma_cache_maint(dma_addr_t handle, size_t size,
u32 op)
HYPERVISOR_grant_table_op(GNTTABOP_cache_flush, , 1);
cflush.offset = 0;
- cflush.a.dev_bus_addr += cflush.length;
+ handle += cflush.length;
size -= cflush.length;
} while (size);
}
@@ -79,7 +82,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
}
void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
@@ -89,9 +92,9 @@ void xen_dma_sync_for_device(struct device *dev, dma_addr_t
handle,
if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_device(paddr, size, dir);
else if (dir == DMA_FROM_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
else
- dma_cache_maint(handle, size, GNTTAB_CACHE_CLEAN);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_CLEAN);
}
bool xen_arch_need_swiotlb(struct device *dev,
--
2.17.1
[PATCH v2 05/11] swiotlb-xen: add struct device* parameter to xen_dma_sync_for_cpu
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index d40e9e5fc52b..1a00e8003c64 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -71,8 +71,9 @@ static void dma_cache_maint(dma_addr_t handle, size_t size,
u32 op)
* pfn_valid returns true the pages is local and we can use the native
* dma-direct functions, otherwise we call the Xen specific version.
*/
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_cpu(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index e38a1cce4100..d9b3d9f2a7d1 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -425,7 +425,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
dma_addr_t dev_addr,
BUG_ON(dir == DMA_NONE);
if (!dev_is_dma_coherent(hwdev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_cpu(dev_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
if (is_xen_swiotlb_buffer(dev_addr))
@@ -439,7 +439,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev,
dma_addr_t dma_addr,
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index ffc0d3902b71..f62d1854780b 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -4,8 +4,9 @@
#include
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir);
void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
--
2.17.1
[PATCH v2 07/11] swiotlb-xen: add struct device* parameter to is_xen_swiotlb_buffer
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
drivers/xen/swiotlb-xen.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 9b846c143c90..0a6cb67f0fc4 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -97,7 +97,7 @@ static inline int range_straddles_page_boundary(phys_addr_t
p, size_t size)
return 0;
}
-static int is_xen_swiotlb_buffer(dma_addr_t dma_addr)
+static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
{
unsigned long bfn = XEN_PFN_DOWN(dma_addr);
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
@@ -428,7 +428,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
dma_addr_t dev_addr,
xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
- if (is_xen_swiotlb_buffer(dev_addr))
+ if (is_xen_swiotlb_buffer(hwdev, dev_addr))
swiotlb_tbl_unmap_single(hwdev, paddr, size, size, dir, attrs);
}
@@ -441,7 +441,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev,
dma_addr_t dma_addr,
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
}
@@ -451,7 +451,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev,
dma_addr_t dma_addr,
{
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
--
2.17.1
[PATCH v2 01/11] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses
From: Boris Ostrovsky xen_alloc_coherent_pages might return pages for which virt_to_phys and virt_to_page don't work, e.g. ioremap'ed pages. So in xen_swiotlb_free_coherent we can't assume that virt_to_page works. Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc virt addresses. This patch fixes the following crash at boot on RPi4: https://marc.info/?l=xen-devel=158862573216800 Signed-off-by: Boris Ostrovsky Signed-off-by: Stefano Stabellini Tested-by: Corey Minyard Tested-by: Roman Shaposhnik --- Changes in v2: - update commit message --- drivers/xen/swiotlb-xen.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index b6d27762c6f8..a42129cba36e 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, int order = get_order(size); phys_addr_t phys; u64 dma_mask = DMA_BIT_MASK(32); + struct page *pg; if (hwdev && hwdev->coherent_dma_mask) dma_mask = hwdev->coherent_dma_mask; @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) : + virt_to_page(vaddr); if (!WARN_ON((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) && - TestClearPageXenRemapped(virt_to_page(vaddr))) + TestClearPageXenRemapped(pg)) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); -- 2.17.1
[PATCH v2 03/11] swiotlb-xen: add struct device* parameter to xen_phys_to_bus
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
drivers/xen/swiotlb-xen.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index ed09f8ac34c5..1c44dbc69b70 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -57,7 +57,7 @@ static unsigned long xen_io_tlb_nslabs;
* can be 32bit when dma_addr_t is 64bit leading to a loss in
* information if the shift is done before casting to 64bit.
*/
-static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
+static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
@@ -78,9 +78,9 @@ static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
return paddr;
}
-static inline dma_addr_t xen_virt_to_bus(void *address)
+static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(virt_to_phys(address));
+ return xen_phys_to_bus(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -309,7 +309,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
phys = *dma_handle;
- dev_addr = xen_phys_to_bus(phys);
+ dev_addr = xen_phys_to_bus(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -367,7 +367,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
- dma_addr_t dev_addr = xen_phys_to_bus(phys);
+ dma_addr_t dev_addr = xen_phys_to_bus(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
@@ -392,7 +392,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
return DMA_MAPPING_ERROR;
phys = map;
- dev_addr = xen_phys_to_bus(map);
+ dev_addr = xen_phys_to_bus(dev, map);
/*
* Ensure that the address returned is DMA'ble
@@ -536,7 +536,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct
scatterlist *sgl,
static int
xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
- return xen_virt_to_bus(xen_io_tlb_end - 1) <= mask;
+ return xen_virt_to_bus(hwdev, xen_io_tlb_end - 1) <= mask;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
--
2.17.1
[PATCH v2 09/11] swiotlb-xen: rename xen_phys_to_bus to xen_phys_to_dma and xen_bus_to_phys to xen_dma_to_phys
so that their names can better describe their behavior.
No functional changes.
Signed-off-by: Stefano Stabellini
---
drivers/xen/swiotlb-xen.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 60ef07440905..41129c02d59a 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -57,7 +57,7 @@ static unsigned long xen_io_tlb_nslabs;
* can be 32bit when dma_addr_t is 64bit leading to a loss in
* information if the shift is done before casting to 64bit.
*/
-static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
+static inline dma_addr_t xen_phys_to_dma(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
@@ -67,7 +67,7 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev,
phys_addr_t paddr)
return phys_to_dma(dev, dma);
}
-static inline phys_addr_t xen_bus_to_phys(struct device *dev,
+static inline phys_addr_t xen_dma_to_phys(struct device *dev,
dma_addr_t dma_addr)
{
phys_addr_t baddr = dma_to_phys(dev, dma_addr);
@@ -80,7 +80,7 @@ static inline phys_addr_t xen_bus_to_phys(struct device *dev,
static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(dev, virt_to_phys(address));
+ return xen_phys_to_dma(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -309,7 +309,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
phys = dma_to_phys(hwdev, *dma_handle);
- dev_addr = xen_phys_to_bus(hwdev, phys);
+ dev_addr = xen_phys_to_dma(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -340,7 +340,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
size, void *vaddr,
/* do not use virt_to_phys because on ARM it doesn't return you the
* physical address */
- phys = xen_bus_to_phys(hwdev, dev_addr);
+ phys = xen_dma_to_phys(hwdev, dev_addr);
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
@@ -369,7 +369,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
- dma_addr_t dev_addr = xen_phys_to_bus(dev, phys);
+ dma_addr_t dev_addr = xen_phys_to_dma(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
@@ -394,7 +394,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
return DMA_MAPPING_ERROR;
phys = map;
- dev_addr = xen_phys_to_bus(dev, map);
+ dev_addr = xen_phys_to_dma(dev, map);
/*
* Ensure that the address returned is DMA'ble
@@ -422,7 +422,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- phys_addr_t paddr = xen_bus_to_phys(hwdev, dev_addr);
+ phys_addr_t paddr = xen_dma_to_phys(hwdev, dev_addr);
BUG_ON(dir == DMA_NONE);
@@ -438,7 +438,7 @@ static void
xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
+ phys_addr_t paddr = xen_dma_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
@@ -451,7 +451,7 @@ static void
xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
+ phys_addr_t paddr = xen_dma_to_phys(dev, dma_addr);
if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
--
2.17.1
[PATCH v2 06/11] swiotlb-xen: add struct device* parameter to xen_dma_sync_for_device
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 1a00e8003c64..f2414ea40a79 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -81,8 +81,9 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
}
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+phys_addr_t paddr, size_t size,
+enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_device(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index d9b3d9f2a7d1..9b846c143c90 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -405,7 +405,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
done:
if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_device(dev_addr, phys, size, dir);
+ xen_dma_sync_for_device(dev, dev_addr, phys, size, dir);
return dev_addr;
}
@@ -455,7 +455,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev,
dma_addr_t dma_addr,
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_device(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_device(dev, dma_addr, paddr, size, dir);
}
/*
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index f62d1854780b..6d235fe2b92d 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -7,8 +7,9 @@
void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+phys_addr_t paddr, size_t size,
+enum dma_data_direction dir);
extern int xen_swiotlb_init(int verbose, bool early);
extern const struct dma_map_ops xen_swiotlb_dma_ops;
--
2.17.1
[PATCH v2 04/11] swiotlb-xen: add struct device* parameter to xen_bus_to_phys
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
drivers/xen/swiotlb-xen.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 1c44dbc69b70..e38a1cce4100 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -67,7 +67,7 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev,
phys_addr_t paddr)
return dma;
}
-static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
+static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr)
{
unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
@@ -339,7 +339,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
size, void *vaddr,
/* do not use virt_to_phys because on ARM it doesn't return you the
* physical address */
- phys = xen_bus_to_phys(dev_addr);
+ phys = xen_bus_to_phys(hwdev, dev_addr);
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
@@ -420,7 +420,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- phys_addr_t paddr = xen_bus_to_phys(dev_addr);
+ phys_addr_t paddr = xen_bus_to_phys(hwdev, dev_addr);
BUG_ON(dir == DMA_NONE);
@@ -436,7 +436,7 @@ static void
xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
@@ -449,7 +449,7 @@ static void
xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
--
2.17.1
[PATCH v2 10/11] xen/arm: introduce phys/dma translations in xen_dma_sync_for_*
From: Stefano Stabellini
xen_dma_sync_for_cpu, xen_dma_sync_for_device, xen_arch_need_swiotlb are
getting called passing dma addresses. On some platforms dma addresses
could be different from physical addresses. Before doing any operations
on these addresses we need to convert them back to physical addresses
using dma_to_phys.
Add dma_to_phys calls to xen_dma_sync_for_cpu, xen_dma_sync_for_device,
and xen_arch_need_swiotlb.
dma_cache_maint is fixed by the next patch.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- improve commit message
- don't use pfn_valid
---
arch/arm/xen/mm.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index f2414ea40a79..bbad712a890d 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
#include
+#include
#include
#include
#include
@@ -75,7 +76,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -85,7 +86,7 @@ void xen_dma_sync_for_device(struct device *dev, dma_addr_t
handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_device(paddr, size, dir);
else if (dir == DMA_FROM_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -98,7 +99,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
dma_addr_t dev_addr)
{
unsigned int xen_pfn = XEN_PFN_DOWN(phys);
- unsigned int bfn = XEN_PFN_DOWN(dev_addr);
+ unsigned int bfn = XEN_PFN_DOWN(dma_to_phys(dev, dev_addr));
/*
* The swiotlb buffer should be used if
--
2.17.1
[PATCH v2 08/11] swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
From: Stefano Stabellini
With some devices physical addresses are different than dma addresses.
To be able to deal with these cases, we need to call phys_to_dma on
physical addresses (including machine addresses in Xen terminology)
before returning them from xen_swiotlb_alloc_coherent and
xen_swiotlb_map_page.
We also need to convert dma addresses back to physical addresses using
dma_to_phys in xen_swiotlb_free_coherent and xen_swiotlb_unmap_page if
we want to do any operations on them.
Call dma_to_phys in is_xen_swiotlb_buffer.
Call phys_to_dma in xen_phys_to_bus.
Call dma_to_phys in xen_bus_to_phys.
Everything is taken care of by these changes except for
xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a
few explicit phys_to_dma/dma_to_phys calls.
Signed-off-by: Stefano Stabellini
Tested-by: Corey Minyard
Tested-by: Roman Shaposhnik
---
Changes in v2:
- improve commit message
---
drivers/xen/swiotlb-xen.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 0a6cb67f0fc4..60ef07440905 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -64,16 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device
*dev, phys_addr_t paddr)
dma |= paddr & ~XEN_PAGE_MASK;
- return dma;
+ return phys_to_dma(dev, dma);
}
-static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr)
+static inline phys_addr_t xen_bus_to_phys(struct device *dev,
+ dma_addr_t dma_addr)
{
+ phys_addr_t baddr = dma_to_phys(dev, dma_addr);
unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
- dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
- phys_addr_t paddr = dma;
-
- paddr |= baddr & ~XEN_PAGE_MASK;
+ phys_addr_t paddr = (xen_pfn << XEN_PAGE_SHIFT) |
+ (baddr & ~XEN_PAGE_MASK);
return paddr;
}
@@ -99,7 +99,7 @@ static inline int range_straddles_page_boundary(phys_addr_t
p, size_t size)
static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
{
- unsigned long bfn = XEN_PFN_DOWN(dma_addr);
+ unsigned long bfn = XEN_PFN_DOWN(dma_to_phys(dev, dma_addr));
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
phys_addr_t paddr = XEN_PFN_PHYS(xen_pfn);
@@ -304,11 +304,11 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
if (hwdev && hwdev->coherent_dma_mask)
dma_mask = hwdev->coherent_dma_mask;
- /* At this point dma_handle is the physical address, next we are
+ /* At this point dma_handle is the dma address, next we are
* going to set it to the machine address.
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
- phys = *dma_handle;
+ phys = dma_to_phys(hwdev, *dma_handle);
dev_addr = xen_phys_to_bus(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
@@ -319,6 +319,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
xen_free_coherent_pages(hwdev, size, ret,
(dma_addr_t)phys, attrs);
return NULL;
}
+ *dma_handle = phys_to_dma(hwdev, *dma_handle);
SetPageXenRemapped(virt_to_page(ret));
}
memset(ret, 0, size);
@@ -351,7 +352,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
size, void *vaddr,
TestClearPageXenRemapped(pg))
xen_destroy_contiguous_region(phys, order);
- xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
+ xen_free_coherent_pages(hwdev, size, vaddr, phys_to_dma(hwdev, phys),
+ attrs);
}
/*
--
2.17.1
Re: [PATCH 00/10] fix swiotlb-xen for RPi4
I would like to ask the maintainers, Juergen, Boris, Konrad, whether you have any more feedback before I send v2 of the series. Cheers, Stefano On Wed, 20 May 2020, Stefano Stabellini wrote: > Hi all, > > This series is a collection of fixes to get Linux running on the RPi4 as > dom0. > > Conceptually there are only two significant changes: > > - make sure not to call virt_to_page on vmalloc virt addresses (patch > #1) > - use phys_to_dma and dma_to_phys to translate phys to/from dma > addresses (all other patches) > > In particular in regards to the second part, the RPi4 is the first > board where Xen can run that has the property that dma addresses are > different from physical addresses, and swiotlb-xen was written with the > assumption that phys addr == dma addr. > > This series adds the phys_to_dma and dma_to_phys calls to make it work. > > > Cheers, > > Stefano >
Re: [PATCH v2] 9p/xen: increase XEN_9PFS_RING_ORDER
On Tue, 2 Jun 2020, Dominique Martinet wrote: > Stefano Stabellini wrote on Mon, Jun 01, 2020: > > > LGTM, I'll try to find some time to test this by the end of next week or > > > will trust you if I can't make it -- ping me around June 1st if I don't > > > reply again until then... > > > > Ping :-) > > I actually did think about this patch this weekend! . . .but didn't > quite make it :/ > Anyway, as promised pushed to linux-next, I'll submit this for 5.8 Thank you!
Re: [PATCH v2] 9p/xen: increase XEN_9PFS_RING_ORDER
n Fri, 22 May 2020, Dominique Martinet wrote: > Stefano Stabellini wrote on Thu, May 21, 2020: > > From: Stefano Stabellini > > > > Increase XEN_9PFS_RING_ORDER to 9 for performance reason. Order 9 is the > > max allowed by the protocol. > > > > We can't assume that all backends will support order 9. The xenstore > > property max-ring-page-order specifies the max order supported by the > > backend. We'll use max-ring-page-order for the size of the ring. > > > > This means that the size of the ring is not static > > (XEN_FLEX_RING_SIZE(9)) anymore. Change XEN_9PFS_RING_SIZE to take an > > argument and base the calculation on the order chosen at setup time. > > > > Finally, modify p9_xen_trans.maxsize to be divided by 4 compared to the > > original value. We need to divide it by 2 because we have two rings > > coming off the same order allocation: the in and out rings. This was a > > mistake in the original code. Also divide it further by 2 because we > > don't want a single request/reply to fill up the entire ring. There can > > be multiple requests/replies outstanding at any given time and if we use > > the full ring with one, we risk forcing the backend to wait for the > > client to read back more replies before continuing, which is not > > performant. > > > > Signed-off-by: Stefano Stabellini > > LGTM, I'll try to find some time to test this by the end of next week or > will trust you if I can't make it -- ping me around June 1st if I don't > reply again until then... Ping :-)
Re: [PATCH 02/10] swiotlb-xen: remove start_dma_addr
On Fri, 22 May 2020, Julien Grall wrote:
> On 22/05/2020 04:55, Stefano Stabellini wrote:
> > On Thu, 21 May 2020, Julien Grall wrote:
> > > Hi,
> > >
> > > On 21/05/2020 00:45, Stefano Stabellini wrote:
> > > > From: Stefano Stabellini
> > > >
> > > > It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
> > > > instead. It will be useful not to have a start_dma_addr around with the
> > > > next patches.
> > > >
> > > > Signed-off-by: Stefano Stabellini
> > > > ---
> > > >drivers/xen/swiotlb-xen.c | 5 +
> > > >1 file changed, 1 insertion(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > > > index a42129cba36e..b5e0492b07b9 100644
> > > > --- a/drivers/xen/swiotlb-xen.c
> > > > +++ b/drivers/xen/swiotlb-xen.c
> > > > @@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
> > > > * Quick lookup value of the bus address of the IOTLB.
> > > > */
> > > >-static u64 start_dma_addr;
> > > > -
> > > >/*
> > > > * Both of these functions should avoid XEN_PFN_PHYS because
> > > > phys_addr_t
> > > > * can be 32bit when dma_addr_t is 64bit leading to a loss in
> > > > @@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> > > > m_ret = XEN_SWIOTLB_EFIXUP;
> > > > goto error;
> > > > }
> > > > - start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
> > > > if (early) {
> > > > if (swiotlb_init_with_tbl(xen_io_tlb_start,
> > > > xen_io_tlb_nslabs,
> > > > verbose))
> > > > @@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> > > > *dev, struct page *page,
> > > > */
> > > > trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
> > > >-map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
> > > > + map = swiotlb_tbl_map_single(dev,
> > > > virt_to_phys(xen_io_tlb_start),
> > > > phys,
> > >
> > > xen_virt_to_bus() is implemented as xen_phys_to_bus(virt_to_phys()). Can
> > > you
> > > explain how the two are equivalent?
> >
> > They are not equivalent. Looking at what swiotlb_tbl_map_single expects,
> > and also the implementation of swiotlb_init_with_tbl, I think
> > virt_to_phys is actually the one we want.
> >
> > swiotlb_tbl_map_single compares the argument with __pa(tlb) which is
> > __pa(xen_io_tlb_start) which is virt_to_phys(xen_io_tlb_start).
>
> I can't find such check in master. What is your baseline? Could you point to
> the exact line of code?
My base is b85051e755b0e9d6dd8f17ef1da083851b83287d, which is master
from a couple of days back.
xen_swiotlb_init calls swiotlb_init_with_tbl which takes a virt address
as a parameter (xen_io_tlb_start), it gets converted to phys and stored
in io_tlb_start as a physical address.
Later, xen_swiotlb_map_page calls swiotlb_tbl_map_single passing a
dma addr as parameter (tbl_dma_addr). tbl_dma_addr is used to calculate
the right slot in the swiotlb buffer to use. (Strangely tbl_dma_addr is
a dma_addr_t and it is not converted to phys_addr_t before doing
operations... I think tbl_dma_addr should be a phys addr.)
The comparison with io_tlb_start is done here:
do {
while (iommu_is_span_boundary(index, nslots, offset_slots,
max_slots)) {
index += stride;
if (index >= io_tlb_nslabs)
index = 0;
if (index == wrap)
goto not_found;
}
index is io_tlb_start and offset_slots is derived by tbl_dma_addr.
Re: [PATCH 01/10] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses
On Fri, 22 May 2020, Julien Grall wrote: > Hi Stefano, > > On 22/05/2020 04:54, Stefano Stabellini wrote: > > On Thu, 21 May 2020, Julien Grall wrote: > > > Hi, > > > > > > On 21/05/2020 00:45, Stefano Stabellini wrote: > > > > From: Boris Ostrovsky > > > > > > > > Don't just assume that virt_to_page works on all virtual addresses. > > > > Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc > > > > virt addresses. > > > > > > Can you provide an example where swiotlb is used with vmalloc()? > > > > The issue was reported here happening on the Rasperry Pi 4: > > https://marc.info/?l=xen-devel=158862573216800 > > Thanks, it would be good if the commit message contains a bit more details. > > > > > If you are asking where in the Linux codebase the vmalloc is happening > > specifically, I don't know for sure, my information is limited to the > > stack trace that you see in the link (I don't have a Rasperry Pi 4 yet > > but I shall have one soon.) > > Looking at the code there is a comment in xen_swiotlb_alloc_coherent() > suggesting that xen_alloc_coherent_pages() may return an ioremap'ped region on > Arm. So it feels to me that commit b877ac9815a8fe7e5f6d7fdde3dc34652408840a > "xen/swiotlb: remember having called xen_create_contiguous_region()" has > always been broken on Arm. Yes, I think you are right > As an aside, your commit message also suggests this is an issue for every > virtual address used in swiotlb. But only the virt_to_page() call in > xen_swiotlb_free_coherent() is modified. Is it intended? If yes, I think you > want to update your commit message. I see, yes I can explain better
Re: [PATCH 08/10] swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
On Thu, 21 May 2020, Boris Ostrovsky wrote:
> On 5/20/20 7:45 PM, Stefano Stabellini wrote:
> > From: Stefano Stabellini
> >
> > Call dma_to_phys in is_xen_swiotlb_buffer.
> > Call phys_to_dma in xen_phys_to_bus.
> > Call dma_to_phys in xen_bus_to_phys.
> >
> > Everything is taken care of by these changes except for
> > xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a
> > few explicit phys_to_dma/dma_to_phys calls.
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > drivers/xen/swiotlb-xen.c | 20
> > 1 file changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index c50448fd9b75..d011c4c7aa72 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -64,14 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device
> > *dev, phys_addr_t paddr)
> >
> > dma |= paddr & ~XEN_PAGE_MASK;
> >
> > - return dma;
> > + return phys_to_dma(dev, dma);
> > }
> >
> > -static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t
> > baddr)
> > +static inline phys_addr_t xen_bus_to_phys(struct device *dev,
> > + dma_addr_t dma_addr)
>
>
> Since now dma address != bus address this is no longer
> xen_bus_to_phys(). (And I guess the same is rue for xen_phys_to_bus()).
Should I rename them to xen_dma_to_phys and xen_phys_to_dma?
> > {
> > + phys_addr_t baddr = dma_to_phys(dev, dma_addr);
> > unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
> > - dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
> > - phys_addr_t paddr = dma;
> > + phys_addr_t paddr = (xen_pfn << XEN_PAGE_SHIFT) |
> > + (baddr & ~XEN_PAGE_MASK);
> >
> > paddr |= baddr & ~XEN_PAGE_MASK;
>
>
> This line needs to go, no?
Yes, good point
Re: [PATCH 01/10] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses
On Thu, 21 May 2020, Julien Grall wrote: > Hi, > > On 21/05/2020 00:45, Stefano Stabellini wrote: > > From: Boris Ostrovsky > > > > Don't just assume that virt_to_page works on all virtual addresses. > > Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc > > virt addresses. > > Can you provide an example where swiotlb is used with vmalloc()? The issue was reported here happening on the Rasperry Pi 4: https://marc.info/?l=xen-devel=158862573216800 If you are asking where in the Linux codebase the vmalloc is happening specifically, I don't know for sure, my information is limited to the stack trace that you see in the link (I don't have a Rasperry Pi 4 yet but I shall have one soon.) > > Signed-off-by: Boris Ostrovsky > > Signed-off-by: Stefano Stabellini > > --- > > drivers/xen/swiotlb-xen.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index b6d27762c6f8..a42129cba36e 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t > > size, void *vaddr, > > int order = get_order(size); > > phys_addr_t phys; > > u64 dma_mask = DMA_BIT_MASK(32); > > + struct page *pg; > > if (hwdev && hwdev->coherent_dma_mask) > > dma_mask = hwdev->coherent_dma_mask; > > @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t > > size, void *vaddr, > > /* Convert the size to actually allocated. */ > > size = 1UL << (order + XEN_PAGE_SHIFT); > > + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) : > > + virt_to_page(vaddr); > > Common DMA code seems to protect this check with CONFIG_DMA_REMAP. Is it > something we want to do it here as well? Or is there any other condition where > vmalloc can happen? I can see it in dma_direct_free_pages: if (IS_ENABLED(CONFIG_DMA_REMAP) && is_vmalloc_addr(cpu_addr)) vunmap(cpu_addr); I wonder why the common DMA code does that. is_vmalloc_addr should work regardless of CONFIG_DMA_REMAP. Maybe just for efficiency?
Re: [PATCH 02/10] swiotlb-xen: remove start_dma_addr
On Thu, 21 May 2020, Julien Grall wrote:
> Hi,
>
> On 21/05/2020 00:45, Stefano Stabellini wrote:
> > From: Stefano Stabellini
> >
> > It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
> > instead. It will be useful not to have a start_dma_addr around with the
> > next patches.
> >
> > Signed-off-by: Stefano Stabellini
> > ---
> > drivers/xen/swiotlb-xen.c | 5 +
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index a42129cba36e..b5e0492b07b9 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
> >* Quick lookup value of the bus address of the IOTLB.
> >*/
> > -static u64 start_dma_addr;
> > -
> > /*
> >* Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
> >* can be 32bit when dma_addr_t is 64bit leading to a loss in
> > @@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
> > m_ret = XEN_SWIOTLB_EFIXUP;
> > goto error;
> > }
> > - start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
> > if (early) {
> > if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
> > verbose))
> > @@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device
> > *dev, struct page *page,
> > */
> > trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
> > - map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
> > + map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start),
> > phys,
>
> xen_virt_to_bus() is implemented as xen_phys_to_bus(virt_to_phys()). Can you
> explain how the two are equivalent?
They are not equivalent. Looking at what swiotlb_tbl_map_single expects,
and also the implementation of swiotlb_init_with_tbl, I think
virt_to_phys is actually the one we want.
swiotlb_tbl_map_single compares the argument with __pa(tlb) which is
__pa(xen_io_tlb_start) which is virt_to_phys(xen_io_tlb_start).
Re: [PATCH 09/10] xen/arm: introduce phys/dma translations in xen_dma_sync_for_*
On Thu, 21 May 2020, Julien Grall wrote:
> > @@ -97,8 +98,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
> >phys_addr_t phys,
> >dma_addr_t dev_addr)
> > {
> > - unsigned int xen_pfn = XEN_PFN_DOWN(phys);
> > - unsigned int bfn = XEN_PFN_DOWN(dev_addr);
> > + unsigned int bfn = XEN_PFN_DOWN(dma_to_phys(dev, dev_addr));
> > /*
> > * The swiotlb buffer should be used if
> > @@ -115,7 +115,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
> > * require a bounce buffer because the device doesn't support coherent
> > * memory and we are not able to flush the cache.
> > */
> > - return (!hypercall_cflush && (xen_pfn != bfn) &&
> > + return (!hypercall_cflush && !pfn_valid(bfn) &&
>
> I believe this change is incorrect. The bfn is a frame based on Xen page
> granularity (always 4K) while pfn_valid() is expecting a frame based on the
> Kernel page granularity.
Given that kernel granularity >= xen granularity it looks like it would
be safe to use PFN_DOWN instead of XEN_PFN_DOWN:
unsigned int bfn = PFN_DOWN(dma_to_phys(dev, dev_addr));
return (!hypercall_cflush && !pfn_valid(bfn) &&
[PATCH v2] 9p/xen: increase XEN_9PFS_RING_ORDER
From: Stefano Stabellini
Increase XEN_9PFS_RING_ORDER to 9 for performance reason. Order 9 is the
max allowed by the protocol.
We can't assume that all backends will support order 9. The xenstore
property max-ring-page-order specifies the max order supported by the
backend. We'll use max-ring-page-order for the size of the ring.
This means that the size of the ring is not static
(XEN_FLEX_RING_SIZE(9)) anymore. Change XEN_9PFS_RING_SIZE to take an
argument and base the calculation on the order chosen at setup time.
Finally, modify p9_xen_trans.maxsize to be divided by 4 compared to the
original value. We need to divide it by 2 because we have two rings
coming off the same order allocation: the in and out rings. This was a
mistake in the original code. Also divide it further by 2 because we
don't want a single request/reply to fill up the entire ring. There can
be multiple requests/replies outstanding at any given time and if we use
the full ring with one, we risk forcing the backend to wait for the
client to read back more replies before continuing, which is not
performant.
Signed-off-by: Stefano Stabellini
---
Changes in v2:
- Fix setting of p9_xen_trans.maxsize in xen_9pfs_front_probe to match
the initial setting for the reasons explained in the commit message
---
net/9p/trans_xen.c | 61 ++
1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 3963eb11c3fb..3debad93be1a 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -43,8 +43,8 @@
#include
#define XEN_9PFS_NUM_RINGS 2
-#define XEN_9PFS_RING_ORDER 6
-#define XEN_9PFS_RING_SIZE XEN_FLEX_RING_SIZE(XEN_9PFS_RING_ORDER)
+#define XEN_9PFS_RING_ORDER 9
+#define XEN_9PFS_RING_SIZE(ring) XEN_FLEX_RING_SIZE(ring->intf->ring_order)
struct xen_9pfs_header {
uint32_t size;
@@ -132,8 +132,8 @@ static bool p9_xen_write_todo(struct xen_9pfs_dataring
*ring, RING_IDX size)
prod = ring->intf->out_prod;
virt_mb();
- return XEN_9PFS_RING_SIZE -
- xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size;
+ return XEN_9PFS_RING_SIZE(ring) -
+ xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE(ring)) >= size;
}
static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
@@ -167,17 +167,18 @@ static int p9_xen_request(struct p9_client *client,
struct p9_req_t *p9_req)
prod = ring->intf->out_prod;
virt_mb();
- if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons,
-XEN_9PFS_RING_SIZE) < size) {
+ if (XEN_9PFS_RING_SIZE(ring) -
+ xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE(ring)) < size) {
spin_unlock_irqrestore(>lock, flags);
goto again;
}
- masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
- masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+ masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE(ring));
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE(ring));
xen_9pfs_write_packet(ring->data.out, p9_req->tc.sdata, size,
- _prod, masked_cons, XEN_9PFS_RING_SIZE);
+ _prod, masked_cons,
+ XEN_9PFS_RING_SIZE(ring));
p9_req->status = REQ_STATUS_SENT;
virt_wmb(); /* write ring before updating pointer */
@@ -207,19 +208,19 @@ static void p9_xen_response(struct work_struct *work)
prod = ring->intf->in_prod;
virt_rmb();
- if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) <
+ if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE(ring)) <
sizeof(h)) {
notify_remote_via_irq(ring->irq);
return;
}
- masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
- masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+ masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE(ring));
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE(ring));
/* First, read just the header */
xen_9pfs_read_packet(, ring->data.in, sizeof(h),
masked_prod, _cons,
-XEN_9PFS_RING_SIZE);
+XEN_9PFS_RING_SIZE(ring));
req = p9_tag_lookup(priv->client, h.tag);
if (!req || req->status != REQ_STATUS_SENT) {
@@ -233,11 +234,11 @@ static void p9_xen_response(struct work_struct *work)
memcpy(>rc, , sizeof(h));
req->rc.offset = 0;
- masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+ masked_cons = xen_9pfs_mask(
Re: [PATCH 00/10] fix swiotlb-xen for RPi4
On Wed, 20 May 2020, Roman Shaposhnik wrote: > On Wed, May 20, 2020 at 4:45 PM Stefano Stabellini > wrote: > > > > Hi all, > > > > This series is a collection of fixes to get Linux running on the RPi4 as > > dom0. > > > > Conceptually there are only two significant changes: > > > > - make sure not to call virt_to_page on vmalloc virt addresses (patch > > #1) > > - use phys_to_dma and dma_to_phys to translate phys to/from dma > > addresses (all other patches) > > > > In particular in regards to the second part, the RPi4 is the first > > board where Xen can run that has the property that dma addresses are > > different from physical addresses, and swiotlb-xen was written with the > > assumption that phys addr == dma addr. > > > > This series adds the phys_to_dma and dma_to_phys calls to make it work. > > Great to see this! Stefano, any chance you can put it in a branch some place > so I can test the final version? Here it is, but keep in mind that it is based on Linux master (because it is meant to go upstream): git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git fix-rip4-v1
[PATCH 07/10] swiotlb-xen: add struct device* parameter to is_xen_swiotlb_buffer
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
---
drivers/xen/swiotlb-xen.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index ef58f05ae445..c50448fd9b75 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -97,7 +97,7 @@ static inline int range_straddles_page_boundary(phys_addr_t
p, size_t size)
return 0;
}
-static int is_xen_swiotlb_buffer(dma_addr_t dma_addr)
+static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
{
unsigned long bfn = XEN_PFN_DOWN(dma_addr);
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
@@ -428,7 +428,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
dma_addr_t dev_addr,
xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
- if (is_xen_swiotlb_buffer(dev_addr))
+ if (is_xen_swiotlb_buffer(hwdev, dev_addr))
swiotlb_tbl_unmap_single(hwdev, paddr, size, size, dir, attrs);
}
@@ -441,7 +441,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev,
dma_addr_t dma_addr,
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
}
@@ -451,7 +451,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev,
dma_addr_t dma_addr,
{
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
- if (is_xen_swiotlb_buffer(dma_addr))
+ if (is_xen_swiotlb_buffer(dev, dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
--
2.17.1
[PATCH 09/10] xen/arm: introduce phys/dma translations in xen_dma_sync_for_*
From: Stefano Stabellini
Add phys_to_dma/dma_to_phys calls to
xen_dma_sync_for_cpu, xen_dma_sync_for_device, and
xen_arch_need_swiotlb.
In xen_arch_need_swiotlb, take the opportunity to switch to the simpler
pfn_valid check we use everywhere else.
dma_cache_maint is fixed by the next patch.
Signed-off-by: Stefano Stabellini
---
arch/arm/xen/mm.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index f2414ea40a79..7639251bcc79 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
#include
+#include
#include
#include
#include
@@ -75,7 +76,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -85,7 +86,7 @@ void xen_dma_sync_for_device(struct device *dev, dma_addr_t
handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir)
{
- if (pfn_valid(PFN_DOWN(handle)))
+ if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_device(paddr, size, dir);
else if (dir == DMA_FROM_DEVICE)
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
@@ -97,8 +98,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
phys_addr_t phys,
dma_addr_t dev_addr)
{
- unsigned int xen_pfn = XEN_PFN_DOWN(phys);
- unsigned int bfn = XEN_PFN_DOWN(dev_addr);
+ unsigned int bfn = XEN_PFN_DOWN(dma_to_phys(dev, dev_addr));
/*
* The swiotlb buffer should be used if
@@ -115,7 +115,7 @@ bool xen_arch_need_swiotlb(struct device *dev,
* require a bounce buffer because the device doesn't support coherent
* memory and we are not able to flush the cache.
*/
- return (!hypercall_cflush && (xen_pfn != bfn) &&
+ return (!hypercall_cflush && !pfn_valid(bfn) &&
!dev_is_dma_coherent(dev));
}
--
2.17.1
[PATCH 05/10] swiotlb-xen: add struct device* parameter to xen_dma_sync_for_cpu
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index d40e9e5fc52b..1a00e8003c64 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -71,8 +71,9 @@ static void dma_cache_maint(dma_addr_t handle, size_t size,
u32 op)
* pfn_valid returns true the pages is local and we can use the native
* dma-direct functions, otherwise we call the Xen specific version.
*/
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_cpu(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 9b4306a56feb..f9aa932973dd 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -425,7 +425,7 @@ static void xen_swiotlb_unmap_page(struct device *hwdev,
dma_addr_t dev_addr,
BUG_ON(dir == DMA_NONE);
if (!dev_is_dma_coherent(hwdev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_cpu(dev_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(hwdev, dev_addr, paddr, size, dir);
/* NOTE: We use dev_addr here, not paddr! */
if (is_xen_swiotlb_buffer(dev_addr))
@@ -439,7 +439,7 @@ xen_swiotlb_sync_single_for_cpu(struct device *dev,
dma_addr_t dma_addr,
phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_cpu(dev, dma_addr, paddr, size, dir);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_CPU);
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index ffc0d3902b71..f62d1854780b 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -4,8 +4,9 @@
#include
-void xen_dma_sync_for_cpu(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
+ phys_addr_t paddr, size_t size,
+ enum dma_data_direction dir);
void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
--
2.17.1
[PATCH 02/10] swiotlb-xen: remove start_dma_addr
From: Stefano Stabellini
It is not strictly needed. Call virt_to_phys on xen_io_tlb_start
instead. It will be useful not to have a start_dma_addr around with the
next patches.
Signed-off-by: Stefano Stabellini
---
drivers/xen/swiotlb-xen.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index a42129cba36e..b5e0492b07b9 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -52,8 +52,6 @@ static unsigned long xen_io_tlb_nslabs;
* Quick lookup value of the bus address of the IOTLB.
*/
-static u64 start_dma_addr;
-
/*
* Both of these functions should avoid XEN_PFN_PHYS because phys_addr_t
* can be 32bit when dma_addr_t is 64bit leading to a loss in
@@ -241,7 +239,6 @@ int __ref xen_swiotlb_init(int verbose, bool early)
m_ret = XEN_SWIOTLB_EFIXUP;
goto error;
}
- start_dma_addr = xen_virt_to_bus(xen_io_tlb_start);
if (early) {
if (swiotlb_init_with_tbl(xen_io_tlb_start, xen_io_tlb_nslabs,
verbose))
@@ -389,7 +386,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
*/
trace_swiotlb_bounced(dev, dev_addr, size, swiotlb_force);
- map = swiotlb_tbl_map_single(dev, start_dma_addr, phys,
+ map = swiotlb_tbl_map_single(dev, virt_to_phys(xen_io_tlb_start), phys,
size, size, dir, attrs);
if (map == (phys_addr_t)DMA_MAPPING_ERROR)
return DMA_MAPPING_ERROR;
--
2.17.1
[PATCH 10/10] xen/arm: call dma_to_phys on the dma_addr_t parameter of dma_cache_maint
From: Stefano Stabellini
Add a struct device* parameter to dma_cache_maint.
Translate the dma_addr_t parameter of dma_cache_maint by calling
dma_to_phys. Do it for the first page and all the following pages, in
case of multipage handling.
Signed-off-by: Stefano Stabellini
---
arch/arm/xen/mm.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 7639251bcc79..6ddf3b3c1ab5 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -43,15 +43,18 @@ unsigned long xen_get_swiotlb_free_pages(unsigned int order)
static bool hypercall_cflush = false;
/* buffers in highmem or foreign pages cannot cross page boundaries */
-static void dma_cache_maint(dma_addr_t handle, size_t size, u32 op)
+static void dma_cache_maint(struct device *dev, dma_addr_t handle,
+ size_t size, u32 op)
{
struct gnttab_cache_flush cflush;
- cflush.a.dev_bus_addr = handle & XEN_PAGE_MASK;
cflush.offset = xen_offset_in_page(handle);
cflush.op = op;
+ handle &= XEN_PAGE_MASK;
do {
+ cflush.a.dev_bus_addr = dma_to_phys(dev, handle);
+
if (size + cflush.offset > XEN_PAGE_SIZE)
cflush.length = XEN_PAGE_SIZE - cflush.offset;
else
@@ -60,7 +63,7 @@ static void dma_cache_maint(dma_addr_t handle, size_t size,
u32 op)
HYPERVISOR_grant_table_op(GNTTABOP_cache_flush, , 1);
cflush.offset = 0;
- cflush.a.dev_bus_addr += cflush.length;
+ handle += cflush.length;
size -= cflush.length;
} while (size);
}
@@ -79,7 +82,7 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_cpu(paddr, size, dir);
else if (dir != DMA_TO_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
}
void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
@@ -89,9 +92,9 @@ void xen_dma_sync_for_device(struct device *dev, dma_addr_t
handle,
if (pfn_valid(PFN_DOWN(dma_to_phys(dev, handle
arch_sync_dma_for_device(paddr, size, dir);
else if (dir == DMA_FROM_DEVICE)
- dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_INVAL);
else
- dma_cache_maint(handle, size, GNTTAB_CACHE_CLEAN);
+ dma_cache_maint(dev, handle, size, GNTTAB_CACHE_CLEAN);
}
bool xen_arch_need_swiotlb(struct device *dev,
--
2.17.1
[PATCH 06/10] swiotlb-xen: add struct device* parameter to xen_dma_sync_for_device
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
---
arch/arm/xen/mm.c | 5 +++--
drivers/xen/swiotlb-xen.c | 4 ++--
include/xen/swiotlb-xen.h | 5 +++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index 1a00e8003c64..f2414ea40a79 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -81,8 +81,9 @@ void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t
handle,
dma_cache_maint(handle, size, GNTTAB_CACHE_INVAL);
}
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir)
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+phys_addr_t paddr, size_t size,
+enum dma_data_direction dir)
{
if (pfn_valid(PFN_DOWN(handle)))
arch_sync_dma_for_device(paddr, size, dir);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index f9aa932973dd..ef58f05ae445 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -405,7 +405,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
done:
if (!dev_is_dma_coherent(dev) && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
- xen_dma_sync_for_device(dev_addr, phys, size, dir);
+ xen_dma_sync_for_device(dev, dev_addr, phys, size, dir);
return dev_addr;
}
@@ -455,7 +455,7 @@ xen_swiotlb_sync_single_for_device(struct device *dev,
dma_addr_t dma_addr,
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
if (!dev_is_dma_coherent(dev))
- xen_dma_sync_for_device(dma_addr, paddr, size, dir);
+ xen_dma_sync_for_device(dev, dma_addr, paddr, size, dir);
}
/*
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index f62d1854780b..6d235fe2b92d 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -7,8 +7,9 @@
void xen_dma_sync_for_cpu(struct device *dev, dma_addr_t handle,
phys_addr_t paddr, size_t size,
enum dma_data_direction dir);
-void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t paddr, size_t size,
- enum dma_data_direction dir);
+void xen_dma_sync_for_device(struct device *dev, dma_addr_t handle,
+phys_addr_t paddr, size_t size,
+enum dma_data_direction dir);
extern int xen_swiotlb_init(int verbose, bool early);
extern const struct dma_map_ops xen_swiotlb_dma_ops;
--
2.17.1
[PATCH 08/10] swiotlb-xen: introduce phys_to_dma/dma_to_phys translations
From: Stefano Stabellini
Call dma_to_phys in is_xen_swiotlb_buffer.
Call phys_to_dma in xen_phys_to_bus.
Call dma_to_phys in xen_bus_to_phys.
Everything is taken care of by these changes except for
xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent, which need a
few explicit phys_to_dma/dma_to_phys calls.
Signed-off-by: Stefano Stabellini
---
drivers/xen/swiotlb-xen.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index c50448fd9b75..d011c4c7aa72 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -64,14 +64,16 @@ static inline dma_addr_t xen_phys_to_bus(struct device
*dev, phys_addr_t paddr)
dma |= paddr & ~XEN_PAGE_MASK;
- return dma;
+ return phys_to_dma(dev, dma);
}
-static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr)
+static inline phys_addr_t xen_bus_to_phys(struct device *dev,
+ dma_addr_t dma_addr)
{
+ phys_addr_t baddr = dma_to_phys(dev, dma_addr);
unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
- dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
- phys_addr_t paddr = dma;
+ phys_addr_t paddr = (xen_pfn << XEN_PAGE_SHIFT) |
+ (baddr & ~XEN_PAGE_MASK);
paddr |= baddr & ~XEN_PAGE_MASK;
@@ -99,7 +101,7 @@ static inline int range_straddles_page_boundary(phys_addr_t
p, size_t size)
static int is_xen_swiotlb_buffer(struct device *dev, dma_addr_t dma_addr)
{
- unsigned long bfn = XEN_PFN_DOWN(dma_addr);
+ unsigned long bfn = XEN_PFN_DOWN(dma_to_phys(dev, dma_addr));
unsigned long xen_pfn = bfn_to_local_pfn(bfn);
phys_addr_t paddr = XEN_PFN_PHYS(xen_pfn);
@@ -304,11 +306,11 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
if (hwdev && hwdev->coherent_dma_mask)
dma_mask = hwdev->coherent_dma_mask;
- /* At this point dma_handle is the physical address, next we are
+ /* At this point dma_handle is the dma address, next we are
* going to set it to the machine address.
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
- phys = *dma_handle;
+ phys = dma_to_phys(hwdev, *dma_handle);
dev_addr = xen_phys_to_bus(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
@@ -319,6 +321,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
xen_free_coherent_pages(hwdev, size, ret,
(dma_addr_t)phys, attrs);
return NULL;
}
+ *dma_handle = phys_to_dma(hwdev, *dma_handle);
SetPageXenRemapped(virt_to_page(ret));
}
memset(ret, 0, size);
@@ -351,7 +354,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
size, void *vaddr,
TestClearPageXenRemapped(pg))
xen_destroy_contiguous_region(phys, order);
- xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs);
+ xen_free_coherent_pages(hwdev, size, vaddr, phys_to_dma(hwdev, phys),
+ attrs);
}
/*
--
2.17.1
[PATCH 04/10] swiotlb-xen: add struct device* parameter to xen_bus_to_phys
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
---
drivers/xen/swiotlb-xen.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 958ee5517e0b..9b4306a56feb 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -67,7 +67,7 @@ static inline dma_addr_t xen_phys_to_bus(struct device *dev,
phys_addr_t paddr)
return dma;
}
-static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
+static inline phys_addr_t xen_bus_to_phys(struct device *dev, dma_addr_t baddr)
{
unsigned long xen_pfn = bfn_to_pfn(XEN_PFN_DOWN(baddr));
dma_addr_t dma = (dma_addr_t)xen_pfn << XEN_PAGE_SHIFT;
@@ -339,7 +339,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t
size, void *vaddr,
/* do not use virt_to_phys because on ARM it doesn't return you the
* physical address */
- phys = xen_bus_to_phys(dev_addr);
+ phys = xen_bus_to_phys(hwdev, dev_addr);
/* Convert the size to actually allocated. */
size = 1UL << (order + XEN_PAGE_SHIFT);
@@ -420,7 +420,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
static void xen_swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
size_t size, enum dma_data_direction dir, unsigned long attrs)
{
- phys_addr_t paddr = xen_bus_to_phys(dev_addr);
+ phys_addr_t paddr = xen_bus_to_phys(hwdev, dev_addr);
BUG_ON(dir == DMA_NONE);
@@ -436,7 +436,7 @@ static void
xen_swiotlb_sync_single_for_cpu(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (!dev_is_dma_coherent(dev))
xen_dma_sync_for_cpu(dma_addr, paddr, size, dir);
@@ -449,7 +449,7 @@ static void
xen_swiotlb_sync_single_for_device(struct device *dev, dma_addr_t dma_addr,
size_t size, enum dma_data_direction dir)
{
- phys_addr_t paddr = xen_bus_to_phys(dma_addr);
+ phys_addr_t paddr = xen_bus_to_phys(dev, dma_addr);
if (is_xen_swiotlb_buffer(dma_addr))
swiotlb_tbl_sync_single(dev, paddr, size, dir, SYNC_FOR_DEVICE);
--
2.17.1
[PATCH 01/10] swiotlb-xen: use vmalloc_to_page on vmalloc virt addresses
From: Boris Ostrovsky Don't just assume that virt_to_page works on all virtual addresses. Instead add a is_vmalloc_addr check and use vmalloc_to_page on vmalloc virt addresses. Signed-off-by: Boris Ostrovsky Signed-off-by: Stefano Stabellini --- drivers/xen/swiotlb-xen.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index b6d27762c6f8..a42129cba36e 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -335,6 +335,7 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, int order = get_order(size); phys_addr_t phys; u64 dma_mask = DMA_BIT_MASK(32); + struct page *pg; if (hwdev && hwdev->coherent_dma_mask) dma_mask = hwdev->coherent_dma_mask; @@ -346,9 +347,11 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr, /* Convert the size to actually allocated. */ size = 1UL << (order + XEN_PAGE_SHIFT); + pg = is_vmalloc_addr(vaddr) ? vmalloc_to_page(vaddr) : + virt_to_page(vaddr); if (!WARN_ON((dev_addr + size - 1 > dma_mask) || range_straddles_page_boundary(phys, size)) && - TestClearPageXenRemapped(virt_to_page(vaddr))) + TestClearPageXenRemapped(pg)) xen_destroy_contiguous_region(phys, order); xen_free_coherent_pages(hwdev, size, vaddr, (dma_addr_t)phys, attrs); -- 2.17.1
[PATCH 03/10] swiotlb-xen: add struct device* parameter to xen_phys_to_bus
From: Stefano Stabellini
The parameter is unused in this patch.
No functional changes.
Signed-off-by: Stefano Stabellini
---
drivers/xen/swiotlb-xen.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index b5e0492b07b9..958ee5517e0b 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -57,7 +57,7 @@ static unsigned long xen_io_tlb_nslabs;
* can be 32bit when dma_addr_t is 64bit leading to a loss in
* information if the shift is done before casting to 64bit.
*/
-static inline dma_addr_t xen_phys_to_bus(phys_addr_t paddr)
+static inline dma_addr_t xen_phys_to_bus(struct device *dev, phys_addr_t paddr)
{
unsigned long bfn = pfn_to_bfn(XEN_PFN_DOWN(paddr));
dma_addr_t dma = (dma_addr_t)bfn << XEN_PAGE_SHIFT;
@@ -78,9 +78,9 @@ static inline phys_addr_t xen_bus_to_phys(dma_addr_t baddr)
return paddr;
}
-static inline dma_addr_t xen_virt_to_bus(void *address)
+static inline dma_addr_t xen_virt_to_bus(struct device *dev, void *address)
{
- return xen_phys_to_bus(virt_to_phys(address));
+ return xen_phys_to_bus(dev, virt_to_phys(address));
}
static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
@@ -309,7 +309,7 @@ xen_swiotlb_alloc_coherent(struct device *hwdev, size_t
size,
* Do not use virt_to_phys(ret) because on ARM it doesn't correspond
* to *dma_handle. */
phys = *dma_handle;
- dev_addr = xen_phys_to_bus(phys);
+ dev_addr = xen_phys_to_bus(hwdev, phys);
if (((dev_addr + size - 1 <= dma_mask)) &&
!range_straddles_page_boundary(phys, size))
*dma_handle = dev_addr;
@@ -367,7 +367,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
unsigned long attrs)
{
phys_addr_t map, phys = page_to_phys(page) + offset;
- dma_addr_t dev_addr = xen_phys_to_bus(phys);
+ dma_addr_t dev_addr = xen_phys_to_bus(dev, phys);
BUG_ON(dir == DMA_NONE);
/*
@@ -392,7 +392,7 @@ static dma_addr_t xen_swiotlb_map_page(struct device *dev,
struct page *page,
return DMA_MAPPING_ERROR;
phys = map;
- dev_addr = xen_phys_to_bus(map);
+ dev_addr = xen_phys_to_bus(dev, map);
/*
* Ensure that the address returned is DMA'ble
@@ -536,7 +536,7 @@ xen_swiotlb_sync_sg_for_device(struct device *dev, struct
scatterlist *sgl,
static int
xen_swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
- return xen_virt_to_bus(xen_io_tlb_end - 1) <= mask;
+ return xen_virt_to_bus(hwdev, xen_io_tlb_end - 1) <= mask;
}
const struct dma_map_ops xen_swiotlb_dma_ops = {
--
2.17.1
[PATCH 00/10] fix swiotlb-xen for RPi4
Hi all, This series is a collection of fixes to get Linux running on the RPi4 as dom0. Conceptually there are only two significant changes: - make sure not to call virt_to_page on vmalloc virt addresses (patch #1) - use phys_to_dma and dma_to_phys to translate phys to/from dma addresses (all other patches) In particular in regards to the second part, the RPi4 is the first board where Xen can run that has the property that dma addresses are different from physical addresses, and swiotlb-xen was written with the assumption that phys addr == dma addr. This series adds the phys_to_dma and dma_to_phys calls to make it work. Cheers, Stefano
Re: [V9fs-developer] [PATCH] 9p/xen: increase XEN_9PFS_RING_ORDER
On Wed, 20 May 2020, Dominique Martinet wrote:
> Stefano Stabellini wrote on Wed, May 20, 2020:
> > From: Stefano Stabellini
> >
> > Increase XEN_9PFS_RING_ORDER to 9 for performance reason. Order 9 is the
> > max allowed by the protocol.
> >
> > We can't assume that all backends will support order 9. The xenstore
> > property max-ring-page-order specifies the max order supported by the
> > backend. We'll use max-ring-page-order for the size of the ring.
> >
> > This means that the size of the ring is not static
> > (XEN_FLEX_RING_SIZE(9)) anymore. Change XEN_9PFS_RING_SIZE to take an
> > argument and base the calculation on the order chosen at setup time.
> >
> >
> > Finally, modify p9_xen_trans.maxsize to be divided by 4 compared to the
> > original value. We need to divide it by 2 because we have two rings
> > coming off the same order allocation: the in and out rings. This was a
> > mistake in the original code. Also divide it further by 2 because we
> > don't want a single request/reply to fill up the entire ring. There can
> > be multiple requests/replies outstanding at any given time and if we use
> > the full ring with one, we risk forcing the backend to wait for the
> > client to read back more replies before continuing, which is not
> > performant.
>
> Sounds good to me overall. A couple of comments inline.
> Also worth noting I need to rebuild myself a test setup so might take a
> bit of time to actually run tests, but I might just trust you on this
> one for now if it builds with no new warning... Looks like it would
> probably work :p
>
> > [...]
> > @@ -264,7 +265,7 @@ static irqreturn_t xen_9pfs_front_event_handler(int
> > irq, void *r)
> >
> > static struct p9_trans_module p9_xen_trans = {
> > .name = "xen",
> > - .maxsize = 1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT),
> > + .maxsize = 1 << (XEN_9PFS_RING_ORDER + XEN_PAGE_SHIFT - 2),
> > .def = 1,
> > .create = p9_xen_create,
> > .close = p9_xen_close,
> > [...]
> > @@ -401,8 +405,10 @@ static int xen_9pfs_front_probe(struct xenbus_device
> > *dev,
> > return -EINVAL;
> > max_ring_order = xenbus_read_unsigned(dev->otherend,
> > "max-ring-page-order", 0);
> > - if (max_ring_order < XEN_9PFS_RING_ORDER)
> > - return -EINVAL;
> > + if (max_ring_order > XEN_9PFS_RING_ORDER)
> > + max_ring_order = XEN_9PFS_RING_ORDER;
>
> (If there are backends with very small max_ring_orders, we no longer
> error out when we encounter one, it might make sense to add a min
> define? Although to be honest 9p works with pretty small maxsizes so I
> don't see much reason to error out, and even order 0 will be one page
> worth.. I hope there is no xenbus that small though :))
Your point is valid but the size calculation (XEN_FLEX_RING_SIZE) should
work correctly even with order 0:
(1UL << ((0) + XEN_PAGE_SHIFT - 1)) = 1 << (12 - 1) = 2048
So I am thinking that the protocol should still work correctly, although
the performance might be undesirable.
FYI The smallest backend I know of has order 6.
> > + if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order))
> > + p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order);
>
> So base maxsize initial value is 1 << (order + page_shift - 2) ; but
> this is 1 << (order + page_shift - 1) -- I agree with the logic you gave
> in commit message so would think this needs to be shifted down one more
> like the base value as well.
> What do you think?
Yes, you are right, thanks for noticing this! I meant to do that here
too but somehow forgot. This should be:
p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2;
[PATCH] 9p/xen: increase XEN_9PFS_RING_ORDER
From: Stefano Stabellini
Increase XEN_9PFS_RING_ORDER to 9 for performance reason. Order 9 is the
max allowed by the protocol.
We can't assume that all backends will support order 9. The xenstore
property max-ring-page-order specifies the max order supported by the
backend. We'll use max-ring-page-order for the size of the ring.
This means that the size of the ring is not static
(XEN_FLEX_RING_SIZE(9)) anymore. Change XEN_9PFS_RING_SIZE to take an
argument and base the calculation on the order chosen at setup time.
Finally, modify p9_xen_trans.maxsize to be divided by 4 compared to the
original value. We need to divide it by 2 because we have two rings
coming off the same order allocation: the in and out rings. This was a
mistake in the original code. Also divide it further by 2 because we
don't want a single request/reply to fill up the entire ring. There can
be multiple requests/replies outstanding at any given time and if we use
the full ring with one, we risk forcing the backend to wait for the
client to read back more replies before continuing, which is not
performant.
Signed-off-by: Stefano Stabellini
---
net/9p/trans_xen.c | 61 ++
1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 086a4abdfa7c..cf5ea74be7cc 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -44,8 +44,8 @@
#include
#define XEN_9PFS_NUM_RINGS 2
-#define XEN_9PFS_RING_ORDER 6
-#define XEN_9PFS_RING_SIZE XEN_FLEX_RING_SIZE(XEN_9PFS_RING_ORDER)
+#define XEN_9PFS_RING_ORDER 9
+#define XEN_9PFS_RING_SIZE(ring) XEN_FLEX_RING_SIZE(ring->intf->ring_order)
struct xen_9pfs_header {
uint32_t size;
@@ -130,8 +130,8 @@ static bool p9_xen_write_todo(struct xen_9pfs_dataring
*ring, RING_IDX size)
prod = ring->intf->out_prod;
virt_mb();
- return XEN_9PFS_RING_SIZE -
- xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) >= size;
+ return XEN_9PFS_RING_SIZE(ring) -
+ xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE(ring)) >= size;
}
static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
@@ -165,17 +165,18 @@ static int p9_xen_request(struct p9_client *client,
struct p9_req_t *p9_req)
prod = ring->intf->out_prod;
virt_mb();
- if (XEN_9PFS_RING_SIZE - xen_9pfs_queued(prod, cons,
-XEN_9PFS_RING_SIZE) < size) {
+ if (XEN_9PFS_RING_SIZE(ring) -
+ xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE(ring)) < size) {
spin_unlock_irqrestore(>lock, flags);
goto again;
}
- masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
- masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+ masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE(ring));
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE(ring));
xen_9pfs_write_packet(ring->data.out, p9_req->tc->sdata, size,
- _prod, masked_cons, XEN_9PFS_RING_SIZE);
+ _prod, masked_cons,
+ XEN_9PFS_RING_SIZE(ring));
p9_req->status = REQ_STATUS_SENT;
virt_wmb(); /* write ring before updating pointer */
@@ -204,19 +205,19 @@ static void p9_xen_response(struct work_struct *work)
prod = ring->intf->in_prod;
virt_rmb();
- if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE) <
+ if (xen_9pfs_queued(prod, cons, XEN_9PFS_RING_SIZE(ring)) <
sizeof(h)) {
notify_remote_via_irq(ring->irq);
return;
}
- masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE);
- masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+ masked_prod = xen_9pfs_mask(prod, XEN_9PFS_RING_SIZE(ring));
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE(ring));
/* First, read just the header */
xen_9pfs_read_packet(, ring->data.in, sizeof(h),
masked_prod, _cons,
-XEN_9PFS_RING_SIZE);
+XEN_9PFS_RING_SIZE(ring));
req = p9_tag_lookup(priv->client, h.tag);
if (!req || req->status != REQ_STATUS_SENT) {
@@ -230,11 +231,11 @@ static void p9_xen_response(struct work_struct *work)
memcpy(req->rc, , sizeof(h));
req->rc->offset = 0;
- masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE);
+ masked_cons = xen_9pfs_mask(cons, XEN_9PFS_RING_SIZE(ring));
/* Then, read the whole packet (including the header)
Re: [PATCH] xen/pvcalls-back: test for errors when calling backend_connect()
On Mon, 11 May 2020, Juergen Gross wrote:
> backend_connect() can fail, so switch the device to connected only if
> no error occurred.
>
> Fixes: 0a9c75c2c7258f2 ("xen/pvcalls: xenbus state handling")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Juergen Gross
Reviewed-by: Stefano Stabellini
> ---
> drivers/xen/pvcalls-back.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index cf4ce3e9358d..41a18ece029a 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -1088,7 +1088,8 @@ static void set_backend_state(struct xenbus_device *dev,
> case XenbusStateInitialised:
> switch (state) {
> case XenbusStateConnected:
> - backend_connect(dev);
> + if (backend_connect(dev))
> + return;
Do you think it would make sense to WARN?
> xenbus_switch_state(dev, XenbusStateConnected);
> break;
> case XenbusStateClosing:
> --
> 2.26.1
>
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, 28 Apr 2020, Michael S. Tsirkin wrote: > On Tue, Apr 28, 2020 at 11:19:52PM +0530, Srivatsa Vaddagiri wrote: > > * Michael S. Tsirkin [2020-04-28 12:17:57]: > > > > > Okay, but how is all this virtio specific? For example, why not allow > > > separate swiotlbs for any type of device? > > > For example, this might make sense if a given device is from a > > > different, less trusted vendor. > > > > Is swiotlb commonly used for multiple devices that may be on different trust > > boundaries (and not behind a hardware iommu)? The trust boundary is not a good way of describing the scenario and I think it leads to miscommunication. A better way to describe the scenario would be that the device can only DMA to/from a small reserved-memory region advertised on device tree. Do we have other instances of devices that can only DMA to/from very specific and non-configurable address ranges? If so, this series could follow their example. > Even a hardware iommu does not imply a 100% security from malicious > hardware. First lots of people use iommu=pt for performance reasons. > Second even without pt, unmaps are often batched, and sub-page buffers > might be used for DMA, so we are not 100% protected at all times.
Re: [PATCH 5/5] virtio: Add bounce DMA ops
On Tue, 28 Apr 2020, Srivatsa Vaddagiri wrote:
> For better security, its desirable that a guest VM's memory is
> not accessible to any entity that executes outside the context of
> guest VM. In case of virtio, backend drivers execute outside the
> context of guest VM and in general will need access to complete
> guest VM memory. One option to restrict the access provided to
> backend driver is to make use of a bounce buffer. The bounce
> buffer is accessible to both backend and frontend drivers. All IO
> buffers that are in private space of guest VM are bounced to be
> accessible to backend.
[...]
> +static int __init virtio_bounce_setup(struct reserved_mem *rmem)
> +{
> + unsigned long node = rmem->fdt_node;
> +
> + if (!of_get_flat_dt_prop(node, "no-map", NULL))
> + return -EINVAL;
> +
> + return virtio_register_bounce_buffer(rmem->base, rmem->size);
> +}
> +
> +RESERVEDMEM_OF_DECLARE(virtio, "virtio_bounce_pool", virtio_bounce_setup);
Is this special reserved-memory region documented somewhere?
Re: [PATCH] xen/swiotlb: correct the check for xen_destroy_contiguous_region
On Tue, 28 Apr 2020, Jürgen Groß wrote: > On 28.04.20 09:33, peng@nxp.com wrote: > > From: Peng Fan > > > > When booting xen on i.MX8QM, met: > > " > > [3.602128] Unable to handle kernel paging request at virtual address > > 00272d40 > > [3.610804] Mem abort info: > > [3.613905] ESR = 0x9604 > > [3.617332] EC = 0x25: DABT (current EL), IL = 32 bits > > [3.623211] SET = 0, FnV = 0 > > [3.626628] EA = 0, S1PTW = 0 > > [3.630128] Data abort info: > > [3.633362] ISV = 0, ISS = 0x0004 > > [3.637630] CM = 0, WnR = 0 > > [3.640955] [00272d40] user address but active_mm is swapper > > [3.647983] Internal error: Oops: 9604 [#1] PREEMPT SMP > > [3.654137] Modules linked in: > > [3.677285] Hardware name: Freescale i.MX8QM MEK (DT) > > [3.677302] Workqueue: events deferred_probe_work_func > > [3.684253] imx6q-pcie 5f00.pcie: PCI host bridge to bus :00 > > [3.688297] pstate: 6005 (nZCv daif -PAN -UAO) > > [3.688310] pc : xen_swiotlb_free_coherent+0x180/0x1c0 > > [3.693993] pci_bus :00: root bus resource [bus 00-ff] > > [3.701002] lr : xen_swiotlb_free_coherent+0x44/0x1c0 > > " > > > > In xen_swiotlb_alloc_coherent, if !(dev_addr + size - 1 <= dma_mask) or > > range_straddles_page_boundary(phys, size) are true, it will > > create contiguous region. So when free, we need to free contiguous > > region use upper check condition. > > No, this will break PV guests on x86. > > I think there is something wrong with your setup in combination with > the ARM xen_create_contiguous_region() implementation. > > Stefano? Let me start by asking Peng a couple of questions: Peng, could you please add a printk to check which one of the two conditions is True for you? Is it (dev_addr + size - 1 > dma_mask) or range_straddles_page_boundary(phys, size)? Is hwdev->coherent_dma_mask set for your DMA capable device? Finally, is this patch supposed to fix the crash you are seeing? If not, can you tell where is the crash exactly? Juergen, keep in mind that xen_create_contiguous_region does nothing on ARM because in dom0 guest_phys == phys. xen_create_contiguous_region simply sets dma_handle to phys. Whatever condition caused the code to take the xen_create_contiguous_region branch in xen_swiotlb_alloc_coherent, it will also cause it to WARN in xen_swiotlb_free_coherent. range_straddles_page_boundary should never return True because guest_phys == phys. That leaves us with the dma_mask check: dev_addr + size - 1 <= dma_mask dev_addr is the dma_handle allocated by xen_alloc_coherent_pages. dma_mask is either DMA_BIT_MASK(32) or hwdev->coherent_dma_mask. The implementation of xen_alloc_coherent_pages has been recently changed to use dma_direct_alloc. Christoff, does dma_direct_alloc respect hwdev->coherent_dma_mask if present? Also, can it return highmem pages? > Juergen > > > > > Signed-off-by: Peng Fan > > --- > > drivers/xen/swiotlb-xen.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > > index b6d27762c6f8..ab96e468584f 100644 > > --- a/drivers/xen/swiotlb-xen.c > > +++ b/drivers/xen/swiotlb-xen.c > > @@ -346,8 +346,8 @@ xen_swiotlb_free_coherent(struct device *hwdev, size_t > > size, void *vaddr, > > /* Convert the size to actually allocated. */ > > size = 1UL << (order + XEN_PAGE_SHIFT); > > - if (!WARN_ON((dev_addr + size - 1 > dma_mask) || > > -range_straddles_page_boundary(phys, size)) && > > + if (((dev_addr + size - 1 > dma_mask) || > > + range_straddles_page_boundary(phys, size)) && > > TestClearPageXenRemapped(virt_to_page(vaddr))) > > xen_destroy_contiguous_region(phys, order); > > >
