Hi Jerin, I will check how to improve this. Will resubmit this patch with your suggested changes.
Regards, S.Amarnath -----Original Message----- From: Jerin Jacob <[email protected]> Sent: Tuesday, October 15, 2019 3:14 PM To: Somalapuram, Amaranath <[email protected]> Cc: [email protected]; [email protected] Subject: Re: [dpdk-dev] [PATCH v1 5/6] crypto/ccp: enable IOMMU for CCP [CAUTION: External Email] On Tue, Oct 15, 2019 at 2:05 PM Somalapuram, Amaranath <[email protected]> wrote: > > Problem statement: As of now vdev device do not support IOMMU. > vdev device used to custom solution, even for software drives like openssl > uses vdev. I spend some time going through the driver. #The ideal architecture of this driver would have been to introduce a bus driver(see drivers/bus/ifpga/) which does all the PCI probes(drivers/crypto/ccp/rte_ccp_pmd.c and drivers/crypto/ccp/ccp_pci.c) and arrange the devices on the bus scan and enable "non bus" specific driver crypto driver. So that no pci probe etc in crypto driver. # On the upside, You DONT need to give vdev= on eal arguments, on AMD machines it can probe the crypto devices automatically and probe the crypto driver # I dont think, it is specific to vdev IOMMU support, If would have been PCI/Any bus driver, you would have similar changes. Right? # Major portion of this patch does following + if (iommu_mode == 2) + pst.src_addr = (phys_addr_t)rte_mem_virt2iova( + (void *) lsb_buf); + else + pst.src_addr = (phys_addr_t)rte_mem_virt2phy( + (void *) lsb_buf); + Since the following check already present common code, Do we need the above check, just calling rte_mem_virt2iova() is enough. Right? rte_iova_t rte_mem_virt2iova(const void *virtaddr) { if (rte_eal_iova_mode() == RTE_IOVA_VA) return (uintptr_t)virtaddr; return rte_mem_virt2phy(virtaddr); } > I feel its not advisable to put iommu in vdev. > moving the changes to vdev will effect rest of the vdev drives. > That will be big efforts. Every vdev drivers has their own implementation. > Need better design to move it to vdev or common place. > > Regards, > S.Amarnath > > -----Original Message----- > From: Jerin Jacob <[email protected]> > Sent: Tuesday, October 15, 2019 1:47 PM > To: Somalapuram, Amaranath <[email protected]> > Cc: [email protected]; [email protected] > Subject: Re: [dpdk-dev] [PATCH v1 5/6] crypto/ccp: enable IOMMU for > CCP > > [CAUTION: External Email] > > On Tue, Oct 15, 2019 at 12:32 PM <[email protected]> wrote: > > > > From: Amaranath Somalapuram <[email protected]> > > > > CCP use vdev framework, and vdev framework don’t support IOMMU. > > Adding custom IOMMU support for AMD CCP drives. > > Cc: [email protected] > > > > + if (iommu_mode == 2) > > + pci->kdrv = RTE_KDRV_VFIO; > > + else if (iommu_mode == 0) > > + pci->kdrv = RTE_KDRV_IGB_UIO; > > + else if (iommu_mode == 1) > > + pci->kdrv = RTE_KDRV_UIO_GENERIC; > > The crypto driver should not have iommu mode-specific handling. > I am not sure about the problem statement. If the problem is, iommu > support for PCI based vdev device then move the solution to common > layer so that everyone can use it. If not, please share the problem > statement
