| 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:
<PH0PR12MB81260693EB981E79CB67092295A1A@PH0PR12MB8126.namprd12.prod.outlook.com>
Date: Thu, 11 Dec 2025 11:40:11 +0000
From: "Verma, Devendra" <Devendra.Verma@....com>
To: Bjorn Helgaas <helgaas@...nel.org>, Manivannan Sadhasivam
<mani@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "vkoul@...nel.org"
<vkoul@...nel.org>, "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Simek,
Michal" <michal.simek@....com>, "Verma, Devendra" <Devendra.Verma@....com>
Subject: RE: [PATCH RESEND v6 1/2] dmaengine: dw-edma: Add AMD MDB Endpoint
Support
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Wednesday, December 10, 2025 10:02 PM
> To: Manivannan Sadhasivam <mani@...nel.org>
> Cc: Verma, Devendra <Devendra.Verma@....com>; bhelgaas@...gle.com;
> vkoul@...nel.org; dmaengine@...r.kernel.org; linux-pci@...r.kernel.org;
> linux-kernel@...r.kernel.org; Simek, Michal <michal.simek@....com>
> Subject: Re: [PATCH RESEND v6 1/2] dmaengine: dw-edma: Add AMD MDB
> Endpoint Support
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Dec 10, 2025 at 10:26:38PM +0900, Manivannan Sadhasivam wrote:
> > On Wed, Dec 10, 2025 at 11:40:04AM +0000, Verma, Devendra wrote:
> > > > -----Original Message-----
> > > > From: Manivannan Sadhasivam <mani@...nel.org> On Fri, Nov 21,
> 2025
> > > > at 05:04:54PM +0530, Devendra K Verma wrote:
> > > > > AMD MDB PCIe endpoint support. For AMD specific support added
> > > > > the following
> > > > > - AMD supported PCIe Device IDs and Vendor ID (Xilinx).
> > > > > - AMD MDB specific driver data
> > > > > - AMD MDB specific VSEC capability to retrieve the device DDR
> > > > > base address.
>
> > > > > +/* Synopsys */
> > > > > #define DW_PCIE_VSEC_DMA_ID 0x6
>
> > > > > +/* AMD MDB (Xilinx) specific defines */
> > > > > +#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID 0x6
>
> > > > > static void dw_edma_pcie_get_vsec_dma_data(struct pci_dev *pdev,
>
> > > > > + * Synopsys and AMD (Xilinx) use the same VSEC ID for the
> > > > > + purpose
> > > >
> > > > Same or different?
> > >
> > > It is the same capability for which Synosys and AMD (Xilinx) share
> > > the common value.
> >
> > This is confusing. You are searching for either DW_PCIE_VSEC_DMA_ID or
> > DW_PCIE_XILINX_MDB_VSEC_DMA_ID below, which means that the VSEC
> IDs
> > are different.
>
> This is perennially confusing.
>
> Since this is a "Vendor-Specific" (not a "Designated Vendor-Specific")
> capability, we must search for the per-vendor VSEC ID, i.e.,
> DW_PCIE_VSEC_DMA_ID for PCI_VENDOR_ID_SYNOPSYS devices or
> DW_PCIE_XILINX_MDB_VSEC_DMA_ID for PCI_VENDOR_ID_XILINX devices.
>
> The fact that DW_PCIE_VSEC_DMA_ID ==
> DW_PCIE_XILINX_MDB_VSEC_DMA_ID is a coincidence and is not relevant to
> the code. The comment that "Synopsys and AMD (Xilinx) use the same VSEC
> ID for the purpose"
> should be removed because it adds confusion and the code doesn't rely on
> that.
>
> However, the subsequent code DOES rely on the fact that the Synopsys and
> the Xilinx VSECs have the same *registers* at the same offsets:
>
> pci_read_config_dword(pdev, vsec + 0x8, &val);
> map = FIELD_GET(DW_PCIE_VSEC_DMA_MAP, val);
> pdata->rg.bar = FIELD_GET(DW_PCIE_VSEC_DMA_BAR, val);
>
> pci_read_config_dword(pdev, vsec + 0xc, &val);
> pdata->wr_ch_cnt = min_t(u16, pdata->wr_ch_cnt,
> FIELD_GET(DW_PCIE_VSEC_DMA_WR_CH, val));
> pdata->rd_ch_cnt = min_t(u16, pdata->rd_ch_cnt,
> FIELD_GET(DW_PCIE_VSEC_DMA_RD_CH, val));
>
> pci_read_config_dword(pdev, vsec + 0x14, &val);
> off = val;
>
> pci_read_config_dword(pdev, vsec + 0x10, &val);
>
> The VSEC ID *values* are not relevant, but the fact that the registers in the
> Synopsys and the Xilinx capabilities are identical *is* important and not
> obvious, so if you share the code that reads those registers, there should be a
> comment about that.
>
> The normal way to use VSEC is to look for a capability for a single vendor and
> interpret it using code for that specific vendor. I think I would split this into
> separate dw_edma_pcie_get_synopsys_dma_data()
> and dw_edma_pcie_get_xilinx_dma_data() functions to make it obvious that
> these are indeed controlled by different vendors, e.g.,
>
> dw_edma_pcie_get_synopsys_dma_data()
> {
> vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_SYNOPSYS,
> DW_PCIE_VSEC_DMA_ID);
> if (!vsec)
> return;
>
> pci_read_config_dword(pdev, vsec + 0x8, &val);
> ...
> }
>
> dw_edma_pcie_get_xilinx_dma_data()
> {
> vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
> DW_PCIE_XILINX_MDB_VSEC_DMA_ID);
> if (!vsec)
> return;
>
> pci_read_config_dword(pdev, vsec + 0x8, &val);
> ...
>
> vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
> DW_PCIE_XILINX_MDB_VSEC_ID);
> if (!vsec)
> return;
> ...
> }
>
> It's safe to call both of them for all devices like this:
>
> dw_edma_pcie_probe
> {
> ...
> dw_edma_pcie_get_synopsys_dma_data(pdev, vsec_data);
> dw_edma_pcie_get_xilinx_dma_data(pdev, vsec_data);
> ...
> }
>
> Most of the code in dw_edma_pcie_get_synopsys_dma_data() would be
> duplicated, but that's OK and acknowledges the fact that Synopsys and Xilinx
> can evolve that VSEC independently.
>
> Bjorn
I guess, with some code duplication and separation of functionality for different
vendors shall be OK to reduce the confusion otherwise; we are going in circles with
this particular piece of code.
Thanks for the suggestion, in the next review will update the code.
Regards,
Devendra
Powered by blists - more mailing lists