Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230630131753.1549685-1-rick.wertenbroek@gmail.com> <3e49476e-ee18-bfb8-ae1a-3a55a3d13f88@kernel.org>
In-Reply-To: <3e49476e-ee18-bfb8-ae1a-3a55a3d13f88@kernel.org>
From:   Rick Wertenbroek <rick.wertenbroek@gmail.com>
Date:   Mon, 3 Jul 2023 10:54:11 +0200
Message-ID: <CAAEEuhrCHQBi89G2hzJjDZmG3zR=ErbtGQTGuVbiPUOCYh=S5Q@mail.gmail.com>
Subject: Re: [PATCH] PCI: rockchip: Use 64-bit mask on MSI 64-bit PCI address
To:     Damien Le Moal <dlemoal@kernel.org>
Cc:     Dan Carpenter <dan.carpenter@linaro.org>, stable@vger.kernel.org,
        Shawn Lin <shawn.lin@rock-chips.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Heiko Stuebner <heiko@sntech.de>, linux-pci@vger.kernel.org,
        linux-rockchip@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 3, 2023 at 4:05=E2=80=AFAM Damien Le Moal <dlemoal@kernel.org> =
wrote:
>
> On 6/30/23 22:17, Rick Wertenbroek wrote:
> > A 32-bit mask was used on the 64-bit PCI address used for mapping MSIs.
> > This would result in the upper 32 bits being unintentionally zeroed and
> > MSIs getting mapped to incorrect PCI addresses if the address had any
> > of the upper bits set.
> >
> > Replace 32-bit mask by appropriate 64-bit mask and rename 32-bit mask
> > for clarity.
> >
> > Fixes: dc73ed0f1b8b ("PCI: rockchip: Fix window mapping and address tra=
nslation for endpoint")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/linux-pci/8d19e5b7-8fa0-44a4-90e2-9bb06=
f5eb694@moroto.mountain/
> > Signed-off-by: Rick Wertenbroek <rick.wertenbroek@gmail.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/pci/controller/pcie-rockchip-ep.c | 12 ++++++------
> >  drivers/pci/controller/pcie-rockchip.h    |  6 +++---
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/co=
ntroller/pcie-rockchip-ep.c
> > index 0af0e965fb57..313face6a87f 100644
> > --- a/drivers/pci/controller/pcie-rockchip-ep.c
> > +++ b/drivers/pci/controller/pcie-rockchip-ep.c
> > @@ -354,7 +354,7 @@ static int rockchip_pcie_ep_send_msi_irq(struct roc=
kchip_pcie_ep *ep, u8 fn,
> >       struct rockchip_pcie *rockchip =3D &ep->rockchip;
> >       u32 flags, mme, data, data_mask;
> >       u8 msi_count;
> > -     u64 pci_addr;
> > +     u64 pci_addr, pci_addr_mask =3D GENMASK(63, 8);
>
> I think you can simplify all this with only the change to the definition =
of
> PCIE_ADDR_MASK macro. Applying a 64 bits mask for low bits to a 32-bits v=
ariable
> is OK and should not generate any complaints from the compiler.
> Also, that "8" in the GENMASK can be replaced by MIN_AXI_ADDR_BITS_PASSED=
.
> So:
>
> #define PCIE_ADDR_MASK  GENMASK(63, MIN_AXI_ADDR_BITS_PASSED)
>
> Would fix everything I think.
>
> If it does not, I would prefer you define a macro for that GENMASK(63, 8)=
 mask
> instead of using a variable on stack. That would be more efficient code w=
ise,
> removing a memory access.

Indeed this would make the patch much simpler.
I will prepare and send the v2. Thank you for the suggestions

Regards,
Rick

>
> >       u32 r;
> >
> >       /* Check MSI enable bit */
> > @@ -391,18 +391,18 @@ static int rockchip_pcie_ep_send_msi_irq(struct r=
ockchip_pcie_ep *ep, u8 fn,
> >                                      PCI_MSI_ADDRESS_LO);
> >
> >       /* Set the outbound region if needed. */
> > -     if (unlikely(ep->irq_pci_addr !=3D (pci_addr & PCIE_ADDR_MASK) ||
> > +     if (unlikely(ep->irq_pci_addr !=3D (pci_addr & pci_addr_mask) ||
> >                    ep->irq_pci_fn !=3D fn)) {
> >               r =3D rockchip_ob_region(ep->irq_phys_addr);
> >               rockchip_pcie_prog_ep_ob_atu(rockchip, fn, r,
> >                                            ep->irq_phys_addr,
> > -                                          pci_addr & PCIE_ADDR_MASK,
> > -                                          ~PCIE_ADDR_MASK + 1);
> > -             ep->irq_pci_addr =3D (pci_addr & PCIE_ADDR_MASK);
> > +                                          pci_addr & pci_addr_mask,
> > +                                          ~pci_addr_mask + 1);
> > +             ep->irq_pci_addr =3D (pci_addr & pci_addr_mask);
> >               ep->irq_pci_fn =3D fn;
> >       }
> >
> > -     writew(data, ep->irq_cpu_addr + (pci_addr & ~PCIE_ADDR_MASK));
> > +     writew(data, ep->irq_cpu_addr + (pci_addr & ~pci_addr_mask));
> >       return 0;
> >  }
> >
> > diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/contr=
oller/pcie-rockchip.h
> > index fe0333778fd9..2d7b05f07b7e 100644
> > --- a/drivers/pci/controller/pcie-rockchip.h
> > +++ b/drivers/pci/controller/pcie-rockchip.h
> > @@ -158,11 +158,11 @@
> >  #define PCIE_RC_CONFIG_THP_CAP               (PCIE_RC_CONFIG_BASE + 0x=
274)
> >  #define   PCIE_RC_CONFIG_THP_CAP_NEXT_MASK   GENMASK(31, 20)
> >
> > -#define PCIE_ADDR_MASK                       0xffffff00
> > +#define PCIE_LO_ADDR_MASK            GENMASK(31, 8)
> >  #define PCIE_CORE_AXI_CONF_BASE              0xc00000
> >  #define PCIE_CORE_OB_REGION_ADDR0    (PCIE_CORE_AXI_CONF_BASE + 0x0)
> >  #define   PCIE_CORE_OB_REGION_ADDR0_NUM_BITS 0x3f
> > -#define   PCIE_CORE_OB_REGION_ADDR0_LO_ADDR  PCIE_ADDR_MASK
> > +#define   PCIE_CORE_OB_REGION_ADDR0_LO_ADDR  PCIE_LO_ADDR_MASK
> >  #define PCIE_CORE_OB_REGION_ADDR1    (PCIE_CORE_AXI_CONF_BASE + 0x4)
> >  #define PCIE_CORE_OB_REGION_DESC0    (PCIE_CORE_AXI_CONF_BASE + 0x8)
> >  #define PCIE_CORE_OB_REGION_DESC1    (PCIE_CORE_AXI_CONF_BASE + 0xc)
> > @@ -170,7 +170,7 @@
> >  #define PCIE_CORE_AXI_INBOUND_BASE   0xc00800
> >  #define PCIE_RP_IB_ADDR0             (PCIE_CORE_AXI_INBOUND_BASE + 0x0=
)
> >  #define   PCIE_CORE_IB_REGION_ADDR0_NUM_BITS 0x3f
> > -#define   PCIE_CORE_IB_REGION_ADDR0_LO_ADDR  PCIE_ADDR_MASK
> > +#define   PCIE_CORE_IB_REGION_ADDR0_LO_ADDR  PCIE_LO_ADDR_MASK
> >  #define PCIE_RP_IB_ADDR1             (PCIE_CORE_AXI_INBOUND_BASE + 0x4=
)
> >
> >  /* Size of one AXI Region (not Region 0) */
>
> --
> Damien Le Moal
> Western Digital Research
>