| Message ID | 20200817234033.442511-8-leobras.c@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | DDW indirect mapping | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (97a94d178e5876ad49482c42b13b7296cd6803de) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (9123e3a74ec7b934a4a099e98af6a61c2f80bbf5) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (9123e3a74ec7b934a4a099e98af6a61c2f80bbf5) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (388692e943a58f28aac0fe83e75f5994da9ff8a1) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (0f1fa5848ab32d269a2030caac618bd6a99ab3f3) |
On 18/08/2020 09:40, Leonardo Bras wrote: > enable_ddw() currently returns the address of the DMA window, which is > considered invalid if has the value 0x00. > > Also, it only considers valid an address returned from find_existing_ddw > if it's not 0x00. > > Changing this behavior makes sense, given the users of enable_ddw() only > need to know if direct mapping is possible. It can also allow a DMA window > starting at 0x00 to be used. > > This will be helpful for using a DDW with indirect mapping, as the window > address will be different than 0x00, but it will not map the whole > partition. > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com> > --- > arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++-------------- > 1 file changed, 14 insertions(+), 16 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c > index fcdefcc0f365..4031127c9537 100644 > --- a/arch/powerpc/platforms/pseries/iommu.c > +++ b/arch/powerpc/platforms/pseries/iommu.c > @@ -852,24 +852,25 @@ static void remove_ddw(struct device_node *np, bool remove_prop) > np, ret); > } > > -static u64 find_existing_ddw(struct device_node *pdn) > +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr) > { > struct direct_window *window; > const struct dynamic_dma_window_prop *direct64; > - u64 dma_addr = 0; > + bool found = false; > > spin_lock(&direct_window_list_lock); > /* check if we already created a window and dupe that config if so */ > list_for_each_entry(window, &direct_window_list, list) { > if (window->device == pdn) { > direct64 = window->prop; > - dma_addr = be64_to_cpu(direct64->dma_base); > + *dma_addr = be64_to_cpu(direct64->dma_base); > + found = true; > break; > } > } > spin_unlock(&direct_window_list_lock); > > - return dma_addr; > + return found; > } > > static struct direct_window *ddw_list_add(struct device_node *pdn, > @@ -1131,15 +1132,15 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn) > * pdn: the parent pe node with the ibm,dma_window property > * Future: also check if we can remap the base window for our base page size > * > - * returns the dma offset for use by the direct mapped DMA code. > + * returns true if can map all pages (direct mapping), false otherwise.. > */ > -static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > +static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) > { > int len, ret; > struct ddw_query_response query; > struct ddw_create_response create; > int page_shift; > - u64 dma_addr, max_addr; > + u64 max_addr; > struct device_node *dn; > u32 ddw_avail[DDW_APPLICABLE_SIZE]; > struct direct_window *window; > @@ -1150,8 +1151,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > mutex_lock(&direct_window_init_mutex); > > - dma_addr = find_existing_ddw(pdn); > - if (dma_addr != 0) > + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset)) > goto out_unlock; > > /* > @@ -1292,7 +1292,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > goto out_free_window; > } > > - dma_addr = be64_to_cpu(ddwprop->dma_base); > + dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base); Do not you need the same chunk in the find_existing_ddw() case above as well? Thanks, > goto out_unlock; > > out_free_window: > @@ -1309,6 +1309,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > kfree(win64->name); > kfree(win64->value); > kfree(win64); > + win64 = NULL; > > out_failed: > if (default_win_removed) > @@ -1322,7 +1323,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > out_unlock: > mutex_unlock(&direct_window_init_mutex); > - return dma_addr; > + return win64; > } > > static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) > @@ -1401,11 +1402,8 @@ static bool iommu_bypass_supported_pSeriesLP(struct pci_dev *pdev, u64 dma_mask) > break; > } > > - if (pdn && PCI_DN(pdn)) { > - pdev->dev.archdata.dma_offset = enable_ddw(pdev, pdn); > - if (pdev->dev.archdata.dma_offset) > - return true; > - } > + if (pdn && PCI_DN(pdn)) > + return enable_ddw(pdev, pdn); > > return false; > } >
On Mon, 2020-08-24 at 13:44 +1000, Alexey Kardashevskiy wrote: > > > On 18/08/2020 09:40, Leonardo Bras wrote: > > enable_ddw() currently returns the address of the DMA window, which is > > considered invalid if has the value 0x00. > > > > Also, it only considers valid an address returned from find_existing_ddw > > if it's not 0x00. > > > > Changing this behavior makes sense, given the users of enable_ddw() only > > need to know if direct mapping is possible. It can also allow a DMA window > > starting at 0x00 to be used. > > > > This will be helpful for using a DDW with indirect mapping, as the window > > address will be different than 0x00, but it will not map the whole > > partition. > > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com> > > --- > > arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++-------------- > > 1 file changed, 14 insertions(+), 16 deletions(-) > > > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c > > index fcdefcc0f365..4031127c9537 100644 > > --- a/arch/powerpc/platforms/pseries/iommu.c > > +++ b/arch/powerpc/platforms/pseries/iommu.c > > @@ -852,24 +852,25 @@ static void remove_ddw(struct device_node *np, bool remove_prop) > > np, ret); > > } > > > > > -static u64 find_existing_ddw(struct device_node *pdn) > > +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr) > > { > > struct direct_window *window; > > const struct dynamic_dma_window_prop *direct64; > > - u64 dma_addr = 0; > > + bool found = false; > > > > spin_lock(&direct_window_list_lock); > > /* check if we already created a window and dupe that config if so */ > > list_for_each_entry(window, &direct_window_list, list) { > > if (window->device == pdn) { > > direct64 = window->prop; > > - dma_addr = be64_to_cpu(direct64->dma_base); > > + *dma_addr = be64_to_cpu(direct64->dma_base); > > + found = true; > > break; > > } > > } > > spin_unlock(&direct_window_list_lock); > > > > - return dma_addr; > > + return found; > > } > > > > static struct direct_window *ddw_list_add(struct device_node *pdn, > > @@ -1131,15 +1132,15 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn) > > * pdn: the parent pe node with the ibm,dma_window property > > * Future: also check if we can remap the base window for our base page size > > * > > - * returns the dma offset for use by the direct mapped DMA code. > > + * returns true if can map all pages (direct mapping), false otherwise.. > > */ > > -static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > +static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > { > > int len, ret; > > struct ddw_query_response query; > > struct ddw_create_response create; > > int page_shift; > > - u64 dma_addr, max_addr; > > + u64 max_addr; > > struct device_node *dn; > > u32 ddw_avail[DDW_APPLICABLE_SIZE]; > > struct direct_window *window; > > @@ -1150,8 +1151,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > > > mutex_lock(&direct_window_init_mutex); > > > > - dma_addr = find_existing_ddw(pdn); > > - if (dma_addr != 0) > > + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset)) > > goto out_unlock; > > > > /* > > @@ -1292,7 +1292,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > goto out_free_window; > > } > > > > - dma_addr = be64_to_cpu(ddwprop->dma_base); > > + dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base); > > Do not you need the same chunk in the find_existing_ddw() case above as > well? Thanks, The new signature of find_existing_ddw() is static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr) And on enable_ddw(), we call find_existing_ddw(pdn, &dev->dev.archdata.dma_offset) And inside the function we do: *dma_addr = be64_to_cpu(direct64->dma_base); I think it's the same as the chunk before. Am I missing something? > > > > goto out_unlock; > > > > out_free_window: > > @@ -1309,6 +1309,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > kfree(win64->name); > > kfree(win64->value); > > kfree(win64); > > + win64 = NULL; > > > > out_failed: > > if (default_win_removed) > > @@ -1322,7 +1323,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) > > > > out_unlock: > > mutex_unlock(&direct_window_init_mutex); > > - return dma_addr; > > + return win64; > > } > > > > static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) > > @@ -1401,11 +1402,8 @@ static bool iommu_bypass_supported_pSeriesLP(struct pci_dev *pdev, u64 dma_mask) > > break; > > } > > > > - if (pdn && PCI_DN(pdn)) { > > - pdev->dev.archdata.dma_offset = enable_ddw(pdev, pdn); > > - if (pdev->dev.archdata.dma_offset) > > - return true; > > - } > > + if (pdn && PCI_DN(pdn)) > > + return enable_ddw(pdev, pdn); > > > > return false; > > } > >
On 29/08/2020 00:04, Leonardo Bras wrote: > On Mon, 2020-08-24 at 13:44 +1000, Alexey Kardashevskiy wrote: >> >>> On 18/08/2020 09:40, Leonardo Bras wrote: >>> enable_ddw() currently returns the address of the DMA window, which is >>> considered invalid if has the value 0x00. >>> >>> Also, it only considers valid an address returned from find_existing_ddw >>> if it's not 0x00. >>> >>> Changing this behavior makes sense, given the users of enable_ddw() only >>> need to know if direct mapping is possible. It can also allow a DMA window >>> starting at 0x00 to be used. >>> >>> This will be helpful for using a DDW with indirect mapping, as the window >>> address will be different than 0x00, but it will not map the whole >>> partition. >>> >>> Signed-off-by: Leonardo Bras <leobras.c@gmail.com> >>> --- >>> arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++-------------- >>> 1 file changed, 14 insertions(+), 16 deletions(-) >>> >>> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c >>> index fcdefcc0f365..4031127c9537 100644 >>> --- a/arch/powerpc/platforms/pseries/iommu.c >>> +++ b/arch/powerpc/platforms/pseries/iommu.c >>> @@ -852,24 +852,25 @@ static void remove_ddw(struct device_node *np, bool remove_prop) >>> np, ret); >>> } >>>> >>> -static u64 find_existing_ddw(struct device_node *pdn) >>> +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr) >>> { >>> struct direct_window *window; >>> const struct dynamic_dma_window_prop *direct64; >>> - u64 dma_addr = 0; >>> + bool found = false; >>> >>> spin_lock(&direct_window_list_lock); >>> /* check if we already created a window and dupe that config if so */ >>> list_for_each_entry(window, &direct_window_list, list) { >>> if (window->device == pdn) { >>> direct64 = window->prop; >>> - dma_addr = be64_to_cpu(direct64->dma_base); >>> + *dma_addr = be64_to_cpu(direct64->dma_base); >>> + found = true; >>> break; >>> } >>> } >>> spin_unlock(&direct_window_list_lock); >>> >>> - return dma_addr; >>> + return found; >>> } >>> >>> static struct direct_window *ddw_list_add(struct device_node *pdn, >>> @@ -1131,15 +1132,15 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn) >>> * pdn: the parent pe node with the ibm,dma_window property >>> * Future: also check if we can remap the base window for our base page size >>> * >>> - * returns the dma offset for use by the direct mapped DMA code. >>> + * returns true if can map all pages (direct mapping), false otherwise.. >>> */ >>> -static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) >>> +static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) >>> { >>> int len, ret; >>> struct ddw_query_response query; >>> struct ddw_create_response create; >>> int page_shift; >>> - u64 dma_addr, max_addr; >>> + u64 max_addr; >>> struct device_node *dn; >>> u32 ddw_avail[DDW_APPLICABLE_SIZE]; >>> struct direct_window *window; >>> @@ -1150,8 +1151,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) >>> >>> mutex_lock(&direct_window_init_mutex); >>> >>> - dma_addr = find_existing_ddw(pdn); >>> - if (dma_addr != 0) >>> + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset)) >>> goto out_unlock; >>> >>> /* >>> @@ -1292,7 +1292,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) >>> goto out_free_window; >>> } >>> >>> - dma_addr = be64_to_cpu(ddwprop->dma_base); >>> + dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base); >> >> Do not you need the same chunk in the find_existing_ddw() case above as >> well? Thanks, > > The new signature of find_existing_ddw() is > static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr) > > And on enable_ddw(), we call > find_existing_ddw(pdn, &dev->dev.archdata.dma_offset) > > And inside the function we do: > *dma_addr = be64_to_cpu(direct64->dma_base); > > I think it's the same as the chunk before. > Am I missing something? ah no, sorry, you are not missing anything. Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c index fcdefcc0f365..4031127c9537 100644 --- a/arch/powerpc/platforms/pseries/iommu.c +++ b/arch/powerpc/platforms/pseries/iommu.c @@ -852,24 +852,25 @@ static void remove_ddw(struct device_node *np, bool remove_prop) np, ret); } -static u64 find_existing_ddw(struct device_node *pdn) +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr) { struct direct_window *window; const struct dynamic_dma_window_prop *direct64; - u64 dma_addr = 0; + bool found = false; spin_lock(&direct_window_list_lock); /* check if we already created a window and dupe that config if so */ list_for_each_entry(window, &direct_window_list, list) { if (window->device == pdn) { direct64 = window->prop; - dma_addr = be64_to_cpu(direct64->dma_base); + *dma_addr = be64_to_cpu(direct64->dma_base); + found = true; break; } } spin_unlock(&direct_window_list_lock); - return dma_addr; + return found; } static struct direct_window *ddw_list_add(struct device_node *pdn, @@ -1131,15 +1132,15 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn) * pdn: the parent pe node with the ibm,dma_window property * Future: also check if we can remap the base window for our base page size * - * returns the dma offset for use by the direct mapped DMA code. + * returns true if can map all pages (direct mapping), false otherwise.. */ -static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) +static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn) { int len, ret; struct ddw_query_response query; struct ddw_create_response create; int page_shift; - u64 dma_addr, max_addr; + u64 max_addr; struct device_node *dn; u32 ddw_avail[DDW_APPLICABLE_SIZE]; struct direct_window *window; @@ -1150,8 +1151,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) mutex_lock(&direct_window_init_mutex); - dma_addr = find_existing_ddw(pdn); - if (dma_addr != 0) + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset)) goto out_unlock; /* @@ -1292,7 +1292,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) goto out_free_window; } - dma_addr = be64_to_cpu(ddwprop->dma_base); + dev->dev.archdata.dma_offset = be64_to_cpu(ddwprop->dma_base); goto out_unlock; out_free_window: @@ -1309,6 +1309,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) kfree(win64->name); kfree(win64->value); kfree(win64); + win64 = NULL; out_failed: if (default_win_removed) @@ -1322,7 +1323,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn) out_unlock: mutex_unlock(&direct_window_init_mutex); - return dma_addr; + return win64; } static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev) @@ -1401,11 +1402,8 @@ static bool iommu_bypass_supported_pSeriesLP(struct pci_dev *pdev, u64 dma_mask) break; } - if (pdn && PCI_DN(pdn)) { - pdev->dev.archdata.dma_offset = enable_ddw(pdev, pdn); - if (pdev->dev.archdata.dma_offset) - return true; - } + if (pdn && PCI_DN(pdn)) + return enable_ddw(pdev, pdn); return false; }
enable_ddw() currently returns the address of the DMA window, which is considered invalid if has the value 0x00. Also, it only considers valid an address returned from find_existing_ddw if it's not 0x00. Changing this behavior makes sense, given the users of enable_ddw() only need to know if direct mapping is possible. It can also allow a DMA window starting at 0x00 to be used. This will be helpful for using a DDW with indirect mapping, as the window address will be different than 0x00, but it will not map the whole partition. Signed-off-by: Leonardo Bras <leobras.c@gmail.com> --- arch/powerpc/platforms/pseries/iommu.c | 30 ++++++++++++-------------- 1 file changed, 14 insertions(+), 16 deletions(-)