Re: [PATCH V7 02/17] PCI/IOV: Get VF BAR size from hardware directly when platform needs

[Wei Yang <weiyang@linux.vnet.ibm.com>]

On Tue, Aug 19, 2014 at 03:44:59PM -0600, Bjorn Helgaas wrote:
>On Thu, Jul 24, 2014 at 02:22:12PM +0800, Wei Yang wrote:
>> Current implementation calculates VF BAR size from dividing the total size of
>> IOV BAR by total VF number. It won't work on PowerNV platform because we're
>> going to expand IOV BAR size for finely alignment.
>> 
>> The patch enforces getting IOV BAR size from hardware and then calculate
>> the VF BAR size based on that when platform wants so.
>> 
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  drivers/pci/iov.c      |   28 ++++++++++++++++++++++++----
>>  include/linux/ioport.h |    1 +
>>  2 files changed, 25 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index 7566238..ef1c546 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -55,6 +55,9 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
>>  	struct resource *res;
>>  	struct pci_sriov *iov = dev->sriov;
>>  	struct pci_bus *bus;
>> +	struct resource tmp;
>> +	enum pci_bar_type type;
>> +	int reg;
>>  
>>  	mutex_lock(&iov->dev->sriov->lock);
>>  	bus = virtfn_add_bus(dev->bus, pci_iov_virtfn_bus(dev, id));
>> @@ -80,12 +83,29 @@ static int virtfn_add(struct pci_dev *dev, int id, int reset)
>>  			continue;
>>  		virtfn->resource[i].name = pci_name(virtfn);
>>  		virtfn->resource[i].flags = res->flags;
>> -		size = resource_size(res);
>> -		do_div(size, iov->total_VFs);
>> +		/* When res has IORESOURCE_ARCH, retrieve the IOV BAR size
>> +		 * from hardware directly.
>> +		 */
>> +		if (res->flags & IORESOURCE_ARCH) {
>> +			reg = pci_iov_resource_bar(dev, i + PCI_IOV_RESOURCES, &type);
>> +			__pci_read_base(dev, type, &tmp, reg);
>> +			size = resource_size(&tmp);
>> +			/* When __pci_read_base fails, flags is set to 0.
>> +			 * In this case, reset size to 0, which means the VF
>> +			 * will not be enabled.
>> +			 */
>> +			if (!tmp.flags)
>> +				size = 0;
>
>I don't like the IORESOURCE_ARCH flag because it really doesn't have any
>specific meaning.  You're using it to enable some arch-specific code here
>for this specific case.  But there are any number of other places that
>could do something similar, and there's no way to coordinate them all.
>
>I'd rather have some sort of pcibios_*() hook here where powerpc could
>override the default implementation.

Yep, got it. I will write a pcibios_sriov_resource_size() and override it in
powerpc arch.

>
>> +		} else {
>> +			size = resource_size(res);
>> +			do_div(size, iov->total_VFs);
>> +		}
>>  		virtfn->resource[i].start = res->start + size * id;
>>  		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
>> -		rc = request_resource(res, &virtfn->resource[i]);
>> -		BUG_ON(rc);
>> +		if (resource_size(&virtfn->resource[i])) {
>> +			rc = request_resource(res, &virtfn->resource[i]);
>> +			BUG_ON(rc);
>> +		}
>>  	}
>>  
>>  	if (reset)
>> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
>> index 5e3a906..de8b57c 100644
>> --- a/include/linux/ioport.h
>> +++ b/include/linux/ioport.h
>> @@ -48,6 +48,7 @@ struct resource {
>>  #define IORESOURCE_MEM_64	0x00100000
>>  #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
>>  #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
>> +#define IORESOURCE_ARCH		0x00800000	/* Resource arch tagged */
>>
>>  #define IORESOURCE_EXCLUSIVE	0x08000000	/* Userland may not map this resource */
>>  #define IORESOURCE_DISABLED	0x10000000
>> -- 
>> 1.7.9.5
>> 

-- 
Richard Yang
Help you, Help me