[PATCH 0/2] xen-pciback: correct and clean up BAR handling

[PATCH 0/2] xen-pciback: correct and clean up BAR handling

[Jan Beulich]

1: return proper values during BAR sizing
2: clean up read_dev_bar()

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Side note: I had always assumed Konrad is the maintainer of pciback,
but according to ./MAINTAINERS he isn't: drivers/pci/*xen*, while
using wild cards, doesn't match drivers/xen/xen-pciback/*. Is that
meant to be that way?

Jan

[PATCH 1/2] xen-pciback: return proper values during BAR sizing

[Jan Beulich]

Reads following writes with all address bits set to 1 should return all
changeable address bits as one, not the BAR size (nor, as was the case
for the upper half of 64-bit BARs, the high half of the region's end
address). Presumably this didn't cause any problems so far because
consumers use the value to calculate the size (usually via val & -val),
and do nothing else with it.

But also consider the exception here: Unimplemented BARs should always
return all zeroes.

And finally, the check for whether to return the sizing address on read
for the ROM BAR should ignore all non-address bits, not just the ROM
Enable one.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 drivers/xen/xen-pciback/conf_space_header.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

--- 4.7-rc2-xen-pciback-BAR.orig/drivers/xen/xen-pciback/conf_space_header.c
+++ 4.7-rc2-xen-pciback-BAR/drivers/xen/xen-pciback/conf_space_header.c
@@ -145,7 +145,7 @@ static int rom_write(struct pci_dev *dev
 	/* A write to obtain the length must happen as a 32-bit write.
 	 * This does not (yet) support writing individual bytes
 	 */
-	if (value == ~PCI_ROM_ADDRESS_ENABLE)
+	if ((value | ~PCI_ROM_ADDRESS_MASK) == ~0)
 		bar->which = 1;
 	else {
 		u32 tmpval;
@@ -225,38 +225,42 @@ static inline void read_dev_bar(struct p
 			   (PCI_BASE_ADDRESS_SPACE_MEMORY |
 				PCI_BASE_ADDRESS_MEM_TYPE_64))) {
 			bar_info->val = res[pos - 1].start >> 32;
-			bar_info->len_val = res[pos - 1].end >> 32;
+			bar_info->len_val = -resource_size(&res[pos - 1]) >> 32;
 			return;
 		}
 	}
 
+	if (!res[pos].flags ||
+	    (res[pos].flags & (IORESOURCE_DISABLED | IORESOURCE_UNSET |
+			       IORESOURCE_BUSY)))
+		return;
+
 	bar_info->val = res[pos].start |
 			(res[pos].flags & PCI_REGION_FLAG_MASK);
-	bar_info->len_val = resource_size(&res[pos]);
+	bar_info->len_val = -resource_size(&res[pos]) |
+			    (res[pos].flags & PCI_REGION_FLAG_MASK);
 }
 
 static void *bar_init(struct pci_dev *dev, int offset)
 {
-	struct pci_bar_info *bar = kmalloc(sizeof(*bar), GFP_KERNEL);
+	struct pci_bar_info *bar = kzalloc(sizeof(*bar), GFP_KERNEL);
 
 	if (!bar)
 		return ERR_PTR(-ENOMEM);
 
 	read_dev_bar(dev, bar, offset, ~0);
-	bar->which = 0;
 
 	return bar;
 }
 
 static void *rom_init(struct pci_dev *dev, int offset)
 {
-	struct pci_bar_info *bar = kmalloc(sizeof(*bar), GFP_KERNEL);
+	struct pci_bar_info *bar = kzalloc(sizeof(*bar), GFP_KERNEL);
 
 	if (!bar)
 		return ERR_PTR(-ENOMEM);
 
 	read_dev_bar(dev, bar, offset, ~PCI_ROM_ADDRESS_ENABLE);
-	bar->which = 0;
 
 	return bar;
 }

Re: [PATCH 1/2] xen-pciback: return proper values during BAR sizing

[Boris Ostrovsky]

On 06/06/2016 04:11 AM, Jan Beulich wrote:
> @@ -225,38 +225,42 @@ static inline void read_dev_bar(struct p
>  			   (PCI_BASE_ADDRESS_SPACE_MEMORY |
>  				PCI_BASE_ADDRESS_MEM_TYPE_64))) {
>  			bar_info->val = res[pos - 1].start >> 32;
> -			bar_info->len_val = res[pos - 1].end >> 32;
> +			bar_info->len_val = -resource_size(&res[pos - 1]) >> 32;
>  			return;
>  		}
>  	}
>  
> +	if (!res[pos].flags ||
> +	    (res[pos].flags & (IORESOURCE_DISABLED | IORESOURCE_UNSET |
> +			       IORESOURCE_BUSY)))
> +		return;

Why are you not making this check first thing in the routine?

-boris

Re: [PATCH 1/2] xen-pciback: return proper values during BAR sizing

[Jan Beulich]

>>> On 06.06.16 at 15:03, <boris.ostrovsky@oracle.com> wrote:
> On 06/06/2016 04:11 AM, Jan Beulich wrote:
>> @@ -225,38 +225,42 @@ static inline void read_dev_bar(struct p
>>  			   (PCI_BASE_ADDRESS_SPACE_MEMORY |
>>  				PCI_BASE_ADDRESS_MEM_TYPE_64))) {
>>  			bar_info->val = res[pos - 1].start >> 32;
>> -			bar_info->len_val = res[pos - 1].end >> 32;
>> +			bar_info->len_val = -resource_size(&res[pos - 1]) >> 32;
>>  			return;
>>  		}
>>  	}
>>  
>> +	if (!res[pos].flags ||
>> +	    (res[pos].flags & (IORESOURCE_DISABLED | IORESOURCE_UNSET |
>> +			       IORESOURCE_BUSY)))
>> +		return;
> 
> Why are you not making this check first thing in the routine?

For one, pos isn't set there yet. And I'd also rather avoid the
complications resulting from 64-bit memory resources spanning
two entries.

Jan

Re: [PATCH 1/2] xen-pciback: return proper values during BAR sizing

[Boris Ostrovsky]

On 06/06/2016 09:51 AM, Jan Beulich wrote:
>>>> On 06.06.16 at 15:03, <boris.ostrovsky@oracle.com> wrote:
>> On 06/06/2016 04:11 AM, Jan Beulich wrote:
>>> @@ -225,38 +225,42 @@ static inline void read_dev_bar(struct p
>>>  			   (PCI_BASE_ADDRESS_SPACE_MEMORY |
>>>  				PCI_BASE_ADDRESS_MEM_TYPE_64))) {
>>>  			bar_info->val = res[pos - 1].start >> 32;
>>> -			bar_info->len_val = res[pos - 1].end >> 32;
>>> +			bar_info->len_val = -resource_size(&res[pos - 1]) >> 32;
>>>  			return;
>>>  		}
>>>  	}
>>>  
>>> +	if (!res[pos].flags ||
>>> +	    (res[pos].flags & (IORESOURCE_DISABLED | IORESOURCE_UNSET |
>>> +			       IORESOURCE_BUSY)))
>>> +		return;
>> Why are you not making this check first thing in the routine?
> For one, pos isn't set there yet. And I'd also rather avoid the
> complications resulting from 64-bit memory resources spanning
> two entries.

I thought that both words of a 64-bit BAR would result in a return under
this check so both would be zero. But yes, pos needs to be initialized
anyway (I didn't see this in the diff).

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

[PATCH 2/2] xen-pciback: clean up read_dev_bar()

[Jan Beulich]

- drop unused function parameter
- simplify determination of 64-bit memory resource
- use const and unsigned

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 drivers/xen/xen-pciback/conf_space_header.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

--- 4.7-rc2-xen-pciback-BAR.orig/drivers/xen/xen-pciback/conf_space_header.c
+++ 4.7-rc2-xen-pciback-BAR/drivers/xen/xen-pciback/conf_space_header.c
@@ -209,21 +209,18 @@ static int bar_read(struct pci_dev *dev,
 	return 0;
 }
 
-static inline void read_dev_bar(struct pci_dev *dev,
-				struct pci_bar_info *bar_info, int offset,
-				u32 len_mask)
+static inline void read_dev_bar(const struct pci_dev *dev,
+				struct pci_bar_info *bar_info,
+				unsigned int offset)
 {
-	int	pos;
-	struct resource	*res = dev->resource;
+	unsigned int pos;
+	const struct resource *res = dev->resource;
 
 	if (offset == PCI_ROM_ADDRESS || offset == PCI_ROM_ADDRESS1)
 		pos = PCI_ROM_RESOURCE;
 	else {
 		pos = (offset - PCI_BASE_ADDRESS_0) / 4;
-		if (pos && ((res[pos - 1].flags & (PCI_BASE_ADDRESS_SPACE |
-				PCI_BASE_ADDRESS_MEM_TYPE_MASK)) ==
-			   (PCI_BASE_ADDRESS_SPACE_MEMORY |
-				PCI_BASE_ADDRESS_MEM_TYPE_64))) {
+		if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
 			bar_info->val = res[pos - 1].start >> 32;
 			bar_info->len_val = -resource_size(&res[pos - 1]) >> 32;
 			return;
@@ -248,7 +245,7 @@ static void *bar_init(struct pci_dev *de
 	if (!bar)
 		return ERR_PTR(-ENOMEM);
 
-	read_dev_bar(dev, bar, offset, ~0);
+	read_dev_bar(dev, bar, offset);
 
 	return bar;
 }
@@ -260,7 +257,7 @@ static void *rom_init(struct pci_dev *de
 	if (!bar)
 		return ERR_PTR(-ENOMEM);
 
-	read_dev_bar(dev, bar, offset, ~PCI_ROM_ADDRESS_ENABLE);
+	read_dev_bar(dev, bar, offset);
 
 	return bar;
 }

Re: [PATCH 2/2] xen-pciback: clean up read_dev_bar()

[Boris Ostrovsky]

On 06/06/2016 04:12 AM, Jan Beulich wrote:
> - drop unused function parameter
> - simplify determination of 64-bit memory resource
> - use const and unsigned
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

[PATCH 3/2] xen-pciback: drop rom_init()

[Jan Beulich]

It's identical to bar_init() now.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm sorry for this 3/2 - I only now noticed that this additional
simplification is now possible.
---
 drivers/xen/xen-pciback/conf_space_header.c |   14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

--- 4.7-rc2-xen-pciback-BAR.orig/drivers/xen/xen-pciback/conf_space_header.c
+++ 4.7-rc2-xen-pciback-BAR/drivers/xen/xen-pciback/conf_space_header.c
@@ -250,18 +250,6 @@ static void *bar_init(struct pci_dev *de
 	return bar;
 }
 
-static void *rom_init(struct pci_dev *dev, int offset)
-{
-	struct pci_bar_info *bar = kzalloc(sizeof(*bar), GFP_KERNEL);
-
-	if (!bar)
-		return ERR_PTR(-ENOMEM);
-
-	read_dev_bar(dev, bar, offset);
-
-	return bar;
-}
-
 static void bar_reset(struct pci_dev *dev, int offset, void *data)
 {
 	struct pci_bar_info *bar = data;
@@ -380,7 +368,7 @@ static const struct config_field header_
 	{						\
 	.offset     = reg_offset,			\
 	.size       = 4,				\
-	.init       = rom_init,				\
+	.init       = bar_init,				\
 	.reset      = bar_reset,			\
 	.release    = bar_release,			\
 	.u.dw.read  = bar_read,				\

Re: [PATCH 3/2] xen-pciback: drop rom_init()

[Boris Ostrovsky]

On 06/06/2016 04:47 AM, Jan Beulich wrote:
> It's identical to bar_init() now.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I'm sorry for this 3/2 - I only now noticed that this additional
> simplification is now possible.

I wonder whether we should also move content of read_dev_bar() into
bar_init(). Especially given that it's not really reading a BAR but
rather initializing the stashed value.

-boris

Re: [PATCH 3/2] xen-pciback: drop rom_init()

[Jan Beulich]

>>> On 06.06.16 at 15:09, <boris.ostrovsky@oracle.com> wrote:
> On 06/06/2016 04:47 AM, Jan Beulich wrote:
>> It's identical to bar_init() now.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I'm sorry for this 3/2 - I only now noticed that this additional
>> simplification is now possible.
> 
> I wonder whether we should also move content of read_dev_bar() into
> bar_init(). Especially given that it's not really reading a BAR but
> rather initializing the stashed value.

I had considered that too, but then thought the splitting out of
that logic could as well stay. If we were to do that, I'd in fact
prefer merging patches 2 and 3 (plus this additional adjustment).

Jan

Re: [PATCH 3/2] xen-pciback: drop rom_init()

[Boris Ostrovsky]

On 06/06/2016 09:54 AM, Jan Beulich wrote:
>>>> On 06.06.16 at 15:09, <boris.ostrovsky@oracle.com> wrote:
>> On 06/06/2016 04:47 AM, Jan Beulich wrote:
>>> It's identical to bar_init() now.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> I'm sorry for this 3/2 - I only now noticed that this additional
>>> simplification is now possible.
>> I wonder whether we should also move content of read_dev_bar() into
>> bar_init(). Especially given that it's not really reading a BAR but
>> rather initializing the stashed value.
> I had considered that too, but then thought the splitting out of
> that logic could as well stay. If we were to do that, I'd in fact
> prefer merging patches 2 and 3 (plus this additional adjustment).

If you could do that it would be great. (Again, mostly because I think
the name is misleading and renaming it to something like dev_bar_init()
would also not be good since we already have bar_init()).

Thanks.
-boris