Re: [PATCH v3 3/6] powerpc/powernv: use one M64 BAR in Single PE mode for one VF BAR

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 08/14/2015 01:54 PM, Wei Yang wrote:
> 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 <weiyang@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?


Is this map only valid for a VF and PF won't have/need one?
If so, yes, kmalloc() it.


>
>> 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];


Is not here an extra space before "m64_map"?

Also the commit log does not explain why symbols were renamed (what since 
you are renaming them - say a couple of words what they are).


>>> #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.
>
>


-- 
Alexey