Re: [PATCH v5 2/6] PCI: dwc: ep: Add missing checks when dynamically changing a BAR

[Bjorn Helgaas <helgaas@kernel.org>]

[+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@kernel.org>
> ---
>  .../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
>