| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <79B7CF70-0A66-43DA-91ED-3C8638FED141@kernel.crashing.org>
Date: Wed, 15 Jul 2009 18:59:03 -0500
From: Becky Bruce <beckyb@...nel.crashing.org>
To: FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
Cc: linuxppc-dev@...ts.ozlabs.org, benh@...nel.crashing.org,
galak@...nel.crashing.org, linux-kernel@...r.kernel.org
Subject: Re: removing addr_needs_map in struct dma_mapping_ops
On Jul 13, 2009, at 7:49 PM, FUJITA Tomonori wrote:
> On Mon, 13 Jul 2009 16:50:43 -0500
> Becky Bruce <beckyb@...nel.crashing.org> wrote:
>
>>> talked about defining something like struct dma_data. Then we could
>>>
>>> struct dev_archdata {
>>> ...
>>>
>>> struct dma_data *ddata;
>>> };
>>>
>>> or
>>>
>>> struct dev_archdata {
>>> ...
>>>
>>> struct dma_data ddata;
>>> };
>>>
>>>
>>> struct dma_data needs dma_direct_offset, iommu_table, dma_base, and
>>> dma_window_size, anything else?
>>
>> IIRC, what we had talked about was simpler - we talked about changing
>> the current dev_archdata from this:
>>
>> struct dev_archdata {
>> struct device_node *of_node;
>> struct dma_mapping_ops *dma_ops;
>> void *dma_data;
>> };
>>
>> to this:
>>
>> struct dev_archdata {
>> struct device_node *of_node;
>> struct dma_mapping_ops *dma_ops;
>> unsigned long long dma_data;
>> #ifdef CONFIG_SWIOTLB
>> dma_addr_t max_direct_dma_addr;
>> #endif
>> };
>>
>> Where max_direct_dma_addr is the address beyond which a specific
>> device must use swiotlb, and dma_data is the offset like it is now
>> (but wider on 32-bit systems than void *). I believe Ben had
>> mentioned
>> wanting to make the max_direct_dma_addr part conditional so we don't
>> bloat archdata on platforms that don't ever bounce.
>
> Only maximum address is enough? The minimum (dma_window_base_cur in
> swiotlb_pci_addr_needs_map) is not necessary?
>
>
>> The change to the type of dma_data is actually in preparation for an
>> optimization I have planned for 64-bit PCI devices (and which
>> probably
>> requires more discussion), so that doesn't need to happen now - just
>> leave it as a void *, and I can post a followup patch.
>>
>> Let me know if I can help or do any testing - I've been meaning to
>> look into switching to dma_map_ops for a while now but it hasn't
>> managed to pop off my todo stack.
>
> Ok, how about this? I'm not familiar with POWERPC so I might
> misunderstand something.
This is close, but it misses the setup for non-pci devices. We have a
bus notifier that we use to set up archdata for those devices -
ppc_swiotlb_bus_notify() in arch/powerpc/kernel/dma-swiotlb.c. It
won't cause breakage to not have this set up, because those will fall
through to the dma_capable(), but I think we should initialize it
anyway (who knows what it will end up used for later....).
>
>
>
> diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/
> include/asm/device.h
> index 7d2277c..0086f8d 100644
> --- a/arch/powerpc/include/asm/device.h
> +++ b/arch/powerpc/include/asm/device.h
> @@ -16,6 +16,9 @@ struct dev_archdata {
> /* DMA operations on that device */
> struct dma_mapping_ops *dma_ops;
> void *dma_data;
> +#ifdef CONFIG_SWIOTLB
> + dma_addr_t max_direct_dma_addr;
> +#endif
> };
>
> static inline void dev_archdata_set_node(struct dev_archdata *ad,
> diff --git a/arch/powerpc/include/asm/swiotlb.h b/arch/powerpc/
> include/asm/swiotlb.h
> index 30891d6..b23a4f1 100644
> --- a/arch/powerpc/include/asm/swiotlb.h
> +++ b/arch/powerpc/include/asm/swiotlb.h
> @@ -24,4 +24,6 @@ static inline void dma_mark_clean(void *addr,
> size_t size) {}
> extern unsigned int ppc_swiotlb_enable;
> int __init swiotlb_setup_bus_notifier(void);
>
> +extern void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev);
> +
> #endif /* __ASM_SWIOTLB_H */
> diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/
> dma-swiotlb.c
> index 68ccf11..e21359e 100644
> --- a/arch/powerpc/kernel/dma-swiotlb.c
> +++ b/arch/powerpc/kernel/dma-swiotlb.c
> @@ -56,39 +56,16 @@ swiotlb_arch_address_needs_mapping(struct device
> *hwdev, dma_addr_t addr,
> size_t size)
> {
> struct dma_mapping_ops *dma_ops = get_dma_ops(hwdev);
> + struct dev_archdata *sd = &hwdev->archdata;
>
> BUG_ON(!dma_ops);
> - return dma_ops->addr_needs_map(hwdev, addr, size);
> -}
You can get rid of the dma_ops stuff here.... it's no longer needed.
>
>
> -/*
> - * Determine if an address is reachable by a pci device, or if we
> must bounce.
> - */
> -static int
> -swiotlb_pci_addr_needs_map(struct device *hwdev, dma_addr_t addr,
> size_t size)
> -{
> - u64 mask = dma_get_mask(hwdev);
> - dma_addr_t max;
> - struct pci_controller *hose;
> - struct pci_dev *pdev = to_pci_dev(hwdev);
> -
> - hose = pci_bus_to_host(pdev->bus);
> - max = hose->dma_window_base_cur + hose->dma_window_size;
> -
> - /* check that we're within mapped pci window space */
> - if ((addr + size > max) | (addr < hose->dma_window_base_cur))
> + if (sd->max_direct_dma_addr && addr + size > sd-
> >max_direct_dma_addr)
> return 1;
>
> - return !is_buffer_dma_capable(mask, addr, size);
> -}
> -
> -static int
> -swiotlb_addr_needs_map(struct device *hwdev, dma_addr_t addr,
> size_t size)
> -{
> return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
> }
>
> -
> /*
> * At the moment, all platforms that use this code only require
> * swiotlb to be used if we're operating on HIGHMEM. Since
> @@ -104,7 +81,6 @@ struct dma_mapping_ops swiotlb_dma_ops = {
> .dma_supported = swiotlb_dma_supported,
> .map_page = swiotlb_map_page,
> .unmap_page = swiotlb_unmap_page,
> - .addr_needs_map = swiotlb_addr_needs_map,
> .sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
> .sync_single_range_for_device = swiotlb_sync_single_range_for_device,
> .sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
> @@ -119,13 +95,23 @@ struct dma_mapping_ops swiotlb_pci_dma_ops = {
> .dma_supported = swiotlb_dma_supported,
> .map_page = swiotlb_map_page,
> .unmap_page = swiotlb_unmap_page,
> - .addr_needs_map = swiotlb_pci_addr_needs_map,
> .sync_single_range_for_cpu = swiotlb_sync_single_range_for_cpu,
> .sync_single_range_for_device = swiotlb_sync_single_range_for_device,
> .sync_sg_for_cpu = swiotlb_sync_sg_for_cpu,
> .sync_sg_for_device = swiotlb_sync_sg_for_device
> };
>
> +void pci_dma_dev_setup_swiotlb(struct pci_dev *pdev)
> +{
> + struct pci_controller *hose;
> + struct dev_archdata *sd;
> +
> + hose = pci_bus_to_host(pdev->bus);
> + sd = &pdev->dev.archdata;
> + sd->max_direct_dma_addr =
> + hose->dma_window_base_cur + hose->dma_window_size;
> +}
> +
> static int ppc_swiotlb_bus_notify(struct notifier_block *nb,
> unsigned long action, void *data)
> {
> diff --git a/arch/powerpc/platforms/85xx/mpc8536_ds.c b/arch/powerpc/
> platforms/85xx/mpc8536_ds.c
> index 055ff41..401751b 100644
> --- a/arch/powerpc/platforms/85xx/mpc8536_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc8536_ds.c
> @@ -136,6 +136,7 @@ define_machine(mpc8536_ds) {
> .init_IRQ = mpc8536_ds_pic_init,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> + .pci_dma_dev_setup = pci_dma_dev_setup_swiotlb,
> #endif
> .get_irq = mpic_get_irq,
> .restart = fsl_rstcr_restart,
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_ds.c b/arch/powerpc/
> platforms/85xx/mpc85xx_ds.c
> index 849c0ac..1ba8e38 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_ds.c
> @@ -277,6 +277,7 @@ define_machine(mpc8544_ds) {
> .init_IRQ = mpc85xx_ds_pic_init,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> + .pci_dma_dev_setup = pci_dma_dev_setup_swiotlb,
> #endif
> .get_irq = mpic_get_irq,
> .restart = fsl_rstcr_restart,
> @@ -291,6 +292,7 @@ define_machine(mpc8572_ds) {
> .init_IRQ = mpc85xx_ds_pic_init,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> + .pci_dma_dev_setup = pci_dma_dev_setup_swiotlb,
> #endif
> .get_irq = mpic_get_irq,
> .restart = fsl_rstcr_restart,
> @@ -305,6 +307,7 @@ define_machine(p2020_ds) {
> .init_IRQ = mpc85xx_ds_pic_init,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> + .pci_dma_dev_setup = pci_dma_dev_setup_swiotlb,
> #endif
> .get_irq = mpic_get_irq,
> .restart = fsl_rstcr_restart,
> diff --git a/arch/powerpc/platforms/85xx/mpc85xx_mds.c b/arch/
> powerpc/platforms/85xx/mpc85xx_mds.c
> index 60ed9c0..165a2de 100644
> --- a/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> +++ b/arch/powerpc/platforms/85xx/mpc85xx_mds.c
> @@ -356,6 +356,7 @@ define_machine(mpc8568_mds) {
> .progress = udbg_progress,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> + .pci_dma_dev_setup = pci_dma_dev_setup_swiotlb,
> #endif
> };
>
> @@ -377,5 +378,6 @@ define_machine(mpc8569_mds) {
> .progress = udbg_progress,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> + .pci_dma_dev_setup = pci_dma_dev_setup_swiotlb,
> #endif
> };
> diff --git a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c b/arch/
> powerpc/platforms/86xx/mpc86xx_hpcn.c
> index 6632702..d1878f3 100644
> --- a/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
> +++ b/arch/powerpc/platforms/86xx/mpc86xx_hpcn.c
> @@ -187,5 +187,6 @@ define_machine(mpc86xx_hpcn) {
> .progress = udbg_progress,
> #ifdef CONFIG_PCI
> .pcibios_fixup_bus = fsl_pcibios_fixup_bus,
> + .pci_dma_dev_setup = pci_dma_dev_setup_swiotlb,
> #endif
> };
Instead of initializing this here (which has problems if !
CONFIG_SWIOTLB), place this in the xxxxx_xxxx_setup_arch function in
the same files, which already have an #ifdef CONFIG_SWIOTLB in which
this can be embedded.
I'm about to be off-list for a few days but will be happy to help when
I'm back next week.
Thanks!
Becky
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists