| 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:
<SA1PR12MB81204C807C2F7C72730EC82295ADA@SA1PR12MB8120.namprd12.prod.outlook.com>
Date: Mon, 15 Dec 2025 11:39:48 +0000
From: "Verma, Devendra" <Devendra.Verma@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "mani@...nel.org"
<mani@...nel.org>, "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 v7 1/2] dmaengine: dw-edma: Add AMD MDB Endpoint Support
[AMD Official Use Only - AMD Internal Distribution Only]
Hi Bjorn
Thank you for your illustrative inputs and suggestions.
Please find my responses inline.
Regards,
Devendra
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Friday, December 12, 2025 11:38 PM
> To: Verma, Devendra <Devendra.Verma@....com>
> Cc: bhelgaas@...gle.com; mani@...nel.org; 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 v7 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 Fri, Dec 12, 2025 at 05:50:55PM +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.
> > ...
>
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
>
> > +/* Synopsys */
> > #define DW_PCIE_VSEC_DMA_ID 0x6
> > #define DW_PCIE_VSEC_DMA_BAR GENMASK(10, 8)
> > #define DW_PCIE_VSEC_DMA_MAP GENMASK(2, 0)
> > #define DW_PCIE_VSEC_DMA_WR_CH GENMASK(9, 0)
> > #define DW_PCIE_VSEC_DMA_RD_CH GENMASK(25, 16)
>
> These should include "SYNOPSYS" since they are specific to
> PCI_VENDOR_ID_SYNOPSYS. Add corresponding XILINX #defines below for
> the XILINX VSEC. They'll be the same masks.
>
> > +/* AMD MDB (Xilinx) specific defines */
> > +#define DW_PCIE_XILINX_MDB_VSEC_DMA_ID 0x6
> > +#define DW_PCIE_XILINX_MDB_VSEC_ID 0x20
> > +#define PCI_DEVICE_ID_AMD_MDB_B054 0xb054
>
> Looks odd to me that PCI_DEVICE_ID_AMD_MDB_B054 goes with
> PCI_VENDOR_ID_XILINX. To me this would make more sense as
> PCI_DEVICE_ID_XILINX_B054. Move it so it's not in the middle of the VSEC-
> related things.
>
> > +#define DW_PCIE_AMD_MDB_INVALID_ADDR (~0ULL)
>
> It looks like this is related to the value from
> DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG and is only used for Xilinx, so
> should be named similarly, e.g., DW_PCIE_XILINX_MDB_INVALID_ADDR, and
> moved to be next to it.
>
Agreed on renaming the variables to match the name with the product vendor rather
than the product owner to maintain consistency. The local defines will use the name
DW_PCIE_XILINX_MDB_<VAR-NAME>. The
> > +#define DW_PCIE_XILINX_LL_OFF_GAP 0x200000
> > +#define DW_PCIE_XILINX_LL_SIZE 0x800
> > +#define DW_PCIE_XILINX_DT_OFF_GAP 0x100000
> > +#define DW_PCIE_XILINX_DT_SIZE 0x800
>
> These LL/DT gap and size #defines don't look like they're directly related to
> the VSEC, so they should at least be moved after the
> DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG #defines, since those *are* part
> of the VSEC.
>
> > +#define DW_PCIE_XILINX_MDB_VSEC_HDR_ID 0x20
>
> DW_PCIE_XILINX_MDB_VSEC_HDR_ID is pointless and should be removed.
> See below.
>
Agreed, this will be removed in next revision.
> > +#define DW_PCIE_XILINX_MDB_VSEC_REV 0x1
> > +#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_HIGH 0xc
> > +#define DW_PCIE_XILINX_MDB_DEVMEM_OFF_REG_LOW 0x8
>
> > +static const struct dw_edma_pcie_data amd_mdb_data = {
>
> This is a confusing mix of "xilinx" and "amd_mdb". The DEVICE_ID #define
> uses "AMD_MDB". The other #defines mostly use XILINX. This data structure
> uses "amd_mdb". The function uses "xilinx".
>
> Since this patch only applies to PCI_VENDOR_ID_XILINX, I would make this
> "xilinx_mdb_data". If new devices come with a different vendor ID, e.g.,
> AMD, you can add a corresponding block for that.
>
Agreed for this, the variable names should have relation with the product vendor
rather than the product owner.
> > +static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
> > + struct dw_edma_pcie_data
> > +*pdata) {
> > + u32 val, map;
> > + u16 vsec;
> > + u64 off;
> > +
> > + 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 + PCI_VNDR_HEADER, &val);
> > + if (PCI_VNDR_HEADER_REV(val) != 0x00 ||
> > + PCI_VNDR_HEADER_LEN(val) != 0x18)
> > + return;
> > +
> > + pci_dbg(pdev, "Detected PCIe Vendor-Specific Extended Capability
> > + DMA\n");
>
> Perhaps reword this to "Detected Xilinx Vendor-Specific Extended Capability
> DMA", and the one in dw_edma_pcie_get_synopsys_dma_data()
> to similarly mention "Synopsys" to reinforce the fact that these are Xilinx-
> specific and Synopsys-specific.
>
> I think the REV and LEN checks are unnecessary; see below.
>
> > + pci_read_config_dword(pdev, vsec + 0x8, &val);
> > + map = FIELD_GET(DW_PCIE_VSEC_DMA_MAP, val);
>
> Should use XILINX #defines. Another reason for adding "SYNOPSYS" to the
> #defines for the Synopsys VSEC.
>
Agreed, for the reason mentioned above.
> > + vsec = pci_find_vsec_capability(pdev, PCI_VENDOR_ID_XILINX,
> > + DW_PCIE_XILINX_MDB_VSEC_ID);
> > + if (!vsec)
> > + return;
> > +
> > + pci_read_config_dword(pdev, vsec + PCI_VNDR_HEADER, &val);
> > + if (PCI_VNDR_HEADER_ID(val) != DW_PCIE_XILINX_MDB_VSEC_HDR_ID
> ||
>
> pci_find_vsec_capability() already checks that PCI_VNDR_HEADER_ID() ==
> DW_PCIE_XILINX_MDB_VSEC_ID, so there's no need to check this again.
>
> > + PCI_VNDR_HEADER_REV(val) != DW_PCIE_XILINX_MDB_VSEC_REV)
>
> I know this is copied from dw_edma_pcie_get_vsec_dma_data(), but I think
> it's a bad idea to check for the exact revision because it forces a change to
> existing, working code if there's ever a device with a new revision of this VSEC
> ID.
>
> If there are new revisions of this VSEC, they should preserve the semantics of
> previous revisions. If there was a rev 0 of this VSEC, I think we should check
> for PCI_VNDR_HEADER_REV() >= 1. If rev 1 was the first revision, you could
> skip the check altogether.
>
> If rev 2 *adds* new registers or functionality, we would have to add new code
> to support that, and *that* code should check for
> PCI_VNDR_HEADER_REV() >= 2.
>
> I think the REV and LEN checking in dw_edma_pcie_get_vsec_dma_data() is
> also too aggressive.
>
Agreed. Revision and header check will be removed.
> > static int dw_edma_pcie_probe(struct pci_dev *pdev,
> > const struct pci_device_id *pid) { @@
> > -179,12 +318,34 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> > }
> >
> > memcpy(vsec_data, pdata, sizeof(struct dw_edma_pcie_data));
> > + vsec_data->devmem_phys_off = DW_PCIE_AMD_MDB_INVALID_ADDR;
>
> Seems weird to set devmem_phys_off here since it's only used for
> PCI_VENDOR_ID_XILINX. Couldn't this be done in
> dw_edma_pcie_get_xilinx_dma_data()?
>
This will be moved to dw_edma_pcie_get_xilinx_dma_data() function.
> > - dw_edma_pcie_get_vsec_dma_data(pdev, vsec_data);
> > + dw_edma_pcie_get_synopsys_dma_data(pdev, vsec_data);
> > + dw_edma_pcie_get_xilinx_dma_data(pdev, vsec_data);
> > +
> > + if (pdev->vendor == PCI_VENDOR_ID_XILINX) {
> > + /*
> > + * There is no valid address found for the LL memory
> > + * space on the device side.
> > + */
> > + if (vsec_data->devmem_phys_off ==
> DW_PCIE_AMD_MDB_INVALID_ADDR)
> > + return -ENOMEM;
> > +
> > + /*
> > + * Configure the channel LL and data blocks if number of
> > + * channels enabled in VSEC capability are more than the
> > + * channels configured in amd_mdb_data.
> > + */
> > + dw_edma_set_chan_region_offset(vsec_data, BAR_2, 0,
> > + DW_PCIE_XILINX_LL_OFF_GAP,
> > + DW_PCIE_XILINX_LL_SIZE,
> > + DW_PCIE_XILINX_DT_OFF_GAP,
> > + DW_PCIE_XILINX_DT_SIZE);
> > + }
>
> This PCI_VENDOR_ID_XILINX block looks like maybe it would make sense
> inside dw_edma_pcie_get_xilinx_dma_data()? That function could look
> like:
>
> dw_edma_pcie_get_xilinx_dma_data(...)
> {
> if (pdev->vendor != PCI_VENDOR_ID_XILINX)
> return;
>
> pdata->devmem_phys_off = DW_PCIE_XILINX_MDB_INVALID_ADDR;
> ...
>
In the above suggestion, having dw_edma_set_chan_region_offset() in the
current function makes sense as when checked along patch 2/2 of the same
series. Moreover, the function is setting up some offsets, doing so in a *get*
function does not look justified just because they are related to same vendor.
Similar thing is being done when setting up the ll and dt virt / phys addresses
In the same probe() function which is after *getting* the vsec data and
then *setting* up the next data based on the retrieved info. I also followed
the similar approach, for Xilinx, of *getting* vsec data, *setting* up offsets
and then move ahead with final *settings* of ll and dt regions.
> > static const struct pci_device_id dw_edma_pcie_id_table[] = {
> > { PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
> > + { PCI_VDEVICE(XILINX, PCI_DEVICE_ID_AMD_MDB_B054),
> > + (kernel_ulong_t)&amd_mdb_data },
Powered by blists - more mailing lists