On Fri, Aug 14, 2015 at 10:52:21AM +1000, Gavin Shan wrote:
>On Thu, Aug 13, 2015 at 10:11:08PM +0800, Wei Yang wrote:
>>In current implementation, when VF BAR is bigger than 64MB, it uses 4 M64
>>BARs in Single PE mode to cover the number of VFs required to be enabled.
>>By doing so, several VFs would be in one VF Group and leads to interference
>>between VFs in the same group.
>>
>>This patch changes the design by using one M64 BAR in Single PE mode for
>>one VF BAR. This gives absolute isolation for VFs.
>>
>>Signed-off-by: Wei Yang <weiy...@linux.vnet.ibm.com>
>>---
>> arch/powerpc/include/asm/pci-bridge.h | 6 +-
>> arch/powerpc/platforms/powernv/pci-ioda.c | 163
>> +++++++++++------------------
>> 2 files changed, 62 insertions(+), 107 deletions(-)
>>
>>diff --git a/arch/powerpc/include/asm/pci-bridge.h
>>b/arch/powerpc/include/asm/pci-bridge.h
>>index 712add5..9d33ada 100644
>>--- a/arch/powerpc/include/asm/pci-bridge.h
>>+++ b/arch/powerpc/include/asm/pci-bridge.h
>>@@ -187,6 +187,7 @@ static inline int isa_vaddr_is_ioport(void __iomem
>>*address)
>> */
>> struct iommu_table;
>>
>>+#define MAX_M64_BAR 16
>
>struct pnv_phb::m64_bar_idx is initialized to 15. Another macro is defined here
>as 16. Both of them can be used as maximal M64 BAR number. Obviously, they're
>duplicated. On the other hand, I don't think it's a good idea to have the
>static
>"m64_map" because @pdn is created for every PCI devices, including VFs. non-PF
>don't "m64_map", together other fields like "m64_per_iov" at all. It's
>obviously
>wasting memory. So it would be allocated dynamically when the PF's pdn is
>created
>or in pnv_pci_ioda_fixup_iov_resources().
>
I prefer the dynamic one.
Alexey,
I changed to static defined based on your comments. So do you have some
concern on the dynamic version?
>In long run, it might be reasonable to move all SRIOV related fields in pci_dn
>to another data struct (struct pci_iov_dn?) and allocate that dynamically.
>
>> int flags;
>> #define PCI_DN_FLAG_IOV_VF 0x01
>>@@ -214,10 +215,9 @@ struct pci_dn {
>> u16 vfs_expanded; /* number of VFs IOV BAR expanded */
>> u16 num_vfs; /* number of VFs enabled*/
>> int offset; /* PE# for the first VF PE */
>>-#define M64_PER_IOV 4
>>- int m64_per_iov;
>>+ bool m64_single_mode; /* Use M64 BAR in Single Mode */
>> #define IODA_INVALID_M64 (-1)
>>- int m64_wins[PCI_SRIOV_NUM_BARS][M64_PER_IOV];
>>+ int m64_map[PCI_SRIOV_NUM_BARS][MAX_M64_BAR];
>> #endif /* CONFIG_PCI_IOV */
>> #endif
>> struct list_head child_list;
>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c
>>b/arch/powerpc/platforms/powernv/pci-ioda.c
>>index 67b8f72..4da0f50 100644
>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>@@ -1162,15 +1162,14 @@ static int pnv_pci_vf_release_m64(struct pci_dev
>>*pdev)
>> pdn = pci_get_pdn(pdev);
>>
>> for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
>>- for (j = 0; j < M64_PER_IOV; j++) {
>>- if (pdn->m64_wins[i][j] == IODA_INVALID_M64)
>>+ for (j = 0; j < MAX_M64_BAR; j++) {
>>+ if (pdn->m64_map[i][j] == IODA_INVALID_M64)
>> continue;
>> opal_pci_phb_mmio_enable(phb->opal_id,
>>- OPAL_M64_WINDOW_TYPE, pdn->m64_wins[i][j], 0);
>>- clear_bit(pdn->m64_wins[i][j],
>>&phb->ioda.m64_bar_alloc);
>>- pdn->m64_wins[i][j] = IODA_INVALID_M64;
>>+ OPAL_M64_WINDOW_TYPE, pdn->m64_map[i][j], 0);
>>+ clear_bit(pdn->m64_map[i][j], &phb->ioda.m64_bar_alloc);
>>+ pdn->m64_map[i][j] = IODA_INVALID_M64;
>> }
>>-
>> return 0;
>> }
>>
>>@@ -1187,8 +1186,7 @@ static int pnv_pci_vf_assign_m64(struct pci_dev *pdev,
>>u16 num_vfs)
>> int total_vfs;
>> resource_size_t size, start;
>> int pe_num;
>>- int vf_groups;
>>- int vf_per_group;
>>+ int m64_bars;
>>
>> bus = pdev->bus;
>> hose = pci_bus_to_host(bus);
>>@@ -1196,26 +1194,23 @@ static int pnv_pci_vf_assign_m64(struct pci_dev
>>*pdev, u16 num_vfs)
>> pdn = pci_get_pdn(pdev);
>> total_vfs = pci_sriov_get_totalvfs(pdev);
>>
>>- /* Initialize the m64_wins to IODA_INVALID_M64 */
>>- for (i = 0; i < PCI_SRIOV_NUM_BARS; i++)
>>- for (j = 0; j < M64_PER_IOV; j++)
>>- pdn->m64_wins[i][j] = IODA_INVALID_M64;
>>+ if (pdn->m64_single_mode)
>>+ m64_bars = num_vfs;
>>+ else
>>+ m64_bars = 1;
>>+
>>+ /* Initialize the m64_map to IODA_INVALID_M64 */
>>+ for (i = 0; i < PCI_SRIOV_NUM_BARS ; i++)
>>+ for (j = 0; j < MAX_M64_BAR; j++)
>>+ pdn->m64_map[i][j] = IODA_INVALID_M64;
>
>It would be done in pnv_pci_ioda_fixup_iov_resources(). That means it will
>be done for once if hotplug isn't considered. The code here will be called
>on every attempt to enable SRIOV capability, which isn't necessary, right?
>
I think you are right.
--
Richard Yang
Help you, Help me
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
