[+cc NTB list, since NTB seems to be the main user of .set_bar()]
Can we say something specific in the subject line, like "prevent
changing size/flags" or whatever?
On Wed, Nov 27, 2024 at 11:30:18AM +0100, Niklas Cassel wrote:
> In commit 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update
> inbound map address") set_bar() was modified to support dynamically
> changing the physical address of a BAR.
>
> This means that set_bar() can be called twice, without ever calling
> clear_bar(), as calling clear_bar() would clear the BAR's PCI address
> assigned by the host).
Unpaired parenthesis at end.
Apparently calling .set_bar() twice without calling .clear_bar() is a
problem? What problem does it cause? Sorry about my poor
understanding of the endpoint and NTB code; I'm sure this would be
obvious if I understood more.
Maybe a hint about the reason why we need to call .set_bar() twice
would help me understand.
> This can only be done if the new BAR configuration doesn't fundamentally
> differ from the existing BAR configuration. Add these missing checks.
Can you elaborate a bit on what "fundamentally differ" means? Based
on the patch, I guess it has to do with changing the size or type?
And the problem this would cause is ...? I guess it's a problem for
some other entity that knows about the BAR and is doing MMIO or DMA to
it?
> While at it, add comments which clarifies the support for dynamically
> changing the physical address of a BAR. (Which was also missing.)
>
> Fixes: 4284c88fff0e ("PCI: designware-ep: Allow pci_epc_set_bar() update inbound map address")
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
> .../pci/controller/dwc/pcie-designware-ep.c | 22 ++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index bad588ef69a4..01c739aaf61a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -222,8 +222,28 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> if ((flags & PCI_BASE_ADDRESS_MEM_TYPE_64) && (bar & 1))
> return -EINVAL;
>
> - if (ep->epf_bar[bar])
> + /*
> + * Certain EPF drivers dynamically change the physical address of a BAR
> + * (i.e. they call set_bar() twice, without ever calling clear_bar(), as
> + * calling clear_bar() would clear the BAR's PCI address assigned by the
> + * host).
> + */
> + if (ep->epf_bar[bar]) {
> + /*
> + * We can only dynamically change a BAR if the new configuration
> + * doesn't fundamentally differ from the existing configuration.
> + */
> + if (ep->epf_bar[bar]->barno != bar ||
> + ep->epf_bar[bar]->size != size ||
> + ep->epf_bar[bar]->flags != flags)
> + return -EINVAL;
> +
> + /*
> + * When dynamically changing a BAR, skip writing the BAR reg, as
> + * that would clear the BAR's PCI address assigned by the host.
> + */
> goto config_atu;
> + }
>
> reg = PCI_BASE_ADDRESS_0 + (4 * bar);
>
> --
> 2.47.0
>
