| Message ID | 20210319040333.183827-1-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Qualify enabling of swiotlb_init() | expand |
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.
On 3/19/2021 6:07 AM, 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. It gets assigned to either 0xffffffff or PHYS_OFFSET + arm_dma_zone_size - 1 which is obtained from the machine descriptor, so I expect it to do the right thing, it works for a Pi 4 in 32-bit mode for instance. This is conditional upon enabling CONFIG_ZONE_DMA for ARM, and will otherwise keep its original value of 0, so this should be safe AFAICT. > >> + 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. Agreed.
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?
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.
On 3/19/21 5:22 PM, Stefano Stabellini wrote: > 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. > Should I toss this in Russell's patch tracker or do you need me to make some changes to the patch?
On Mon, Mar 29, 2021 at 12:30:42PM -0700, Florian Fainelli wrote: > Should I toss this in Russell's patch tracker or do you need me to make > some changes to the patch? Due to all the other changes in this area I don't think anything but the swiotlb tree makes much sense here.
On Tue, Mar 30, 2021 at 07:36:07AM +0200, Christoph Hellwig wrote: > On Mon, Mar 29, 2021 at 12:30:42PM -0700, Florian Fainelli wrote: > > Should I toss this in Russell's patch tracker or do you need me to make > > some changes to the patch? > > Due to all the other changes in this area I don't think anything but > the swiotlb tree makes much sense here. I've put them all on git://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git devel/for-linus-5.13
On 4/1/21 10:33 AM, Konrad Rzeszutek Wilk wrote: > On Tue, Mar 30, 2021 at 07:36:07AM +0200, Christoph Hellwig wrote: >> On Mon, Mar 29, 2021 at 12:30:42PM -0700, Florian Fainelli wrote: >>> Should I toss this in Russell's patch tracker or do you need me to make >>> some changes to the patch? >> >> Due to all the other changes in this area I don't think anything but >> the swiotlb tree makes much sense here. > > I've put them all on > > git://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git > devel/for-linus-5.13 Thanks! Did you also want to queue up this one: https://lore.kernel.org/lkml/20210323015350.399493-1-f.fainelli@gmail.com/
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index 828a2561b229..8356bf1daa28 100644 --- a/arch/arm/mm/init.c +++ b/arch/arm/mm/init.c @@ -301,7 +301,11 @@ static void __init free_highpages(void) void __init mem_init(void) { #ifdef CONFIG_ARM_LPAE - swiotlb_init(1); + if (swiotlb_force == SWIOTLB_FORCE || + max_pfn > arm_dma_pfn_limit) + swiotlb_init(1); + else + swiotlb_force = SWIOTLB_NO_FORCE; #endif set_max_mapnr(pfn_to_page(max_pfn) - mem_map);
We do not need a SWIOTLB unless we have DRAM that is addressable beyond the arm_dma_limit. Compare max_pfn with arm_dma_pfn_limit to determine whether we do need a SWIOTLB to be initialized. Fixes: ad3c7b18c5b3 ("arm: use swiotlb for bounce buffering on LPAE configs") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- arch/arm/mm/init.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)