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

[PATCH v2 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>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.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;
 }




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v2 2/2] xen-pciback: clean up {bar,rom}_init()

[Jan Beulich]

- drop unused function parameter of read_dev_bar()
- drop rom_init() (now identical to bar_init())
- fold read_dev_bar() into its now single caller
- simplify determination of 64-bit memory resource
- use const and unsigned

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: fold in 3rd patch and drop read_dev_bar() (as requested by Boris)
---
 drivers/xen/xen-pciback/conf_space_header.c |   57 ++++++++--------------------
 1 file changed, 17 insertions(+), 40 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,58 +209,35 @@ 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 void *bar_init(struct pci_dev *dev, int offset)
 {
-	int	pos;
-	struct resource	*res = dev->resource;
+	unsigned int pos;
+	const struct resource *res = dev->resource;
+	struct pci_bar_info *bar = kzalloc(sizeof(*bar), GFP_KERNEL);
+
+	if (!bar)
+		return ERR_PTR(-ENOMEM);
 
 	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))) {
-			bar_info->val = res[pos - 1].start >> 32;
-			bar_info->len_val = -resource_size(&res[pos - 1]) >> 32;
-			return;
+		if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
+			bar->val = res[pos - 1].start >> 32;
+			bar->len_val = -resource_size(&res[pos - 1]) >> 32;
+			return bar;
 		}
 	}
 
 	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]) |
-			    (res[pos].flags & PCI_REGION_FLAG_MASK);
-}
+		return bar;
 
-static void *bar_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, ~0);
-
-	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, ~PCI_ROM_ADDRESS_ENABLE);
+	bar->val = res[pos].start |
+		   (res[pos].flags & PCI_REGION_FLAG_MASK);
+	bar->len_val = -resource_size(&res[pos]) |
+		       (res[pos].flags & PCI_REGION_FLAG_MASK);
 
 	return bar;
 }
@@ -383,7 +360,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,				\




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

On 06/07/2016 02:30 AM, Jan Beulich wrote:
> 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.


Should this go to stable trees as well?

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 07.06.16 at 16:06, <boris.ostrovsky@oracle.com> wrote:
> On 06/07/2016 02:30 AM, Jan Beulich wrote:
>> 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.
> 
> 
> Should this go to stable trees as well?

Not sure - we had no active reports of problems. The context this
was found in did not really have an issue because it was broken.
I guess I'll leave this to you maintainers...

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 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>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
v3: Use ~0U in rom_write(), to account for PCI_ROM_ADDRESS_MASK being
    of unsigned long type (relevant on 64-bit). (Note: Patch 2 is
    unchanged, and hence not being re-sent. I hope that, despite this
    being a bug fix from v2, retaining the R-b is okay.)
---
 drivers/xen/xen-pciback/conf_space_header.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

--- 4.7-rc4-xen-pciback-BAR.orig/drivers/xen/xen-pciback/conf_space_header.c
+++ 4.7-rc4-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) == ~0U)
 		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;
 }




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[David Vrabel]

On 24/06/16 10:13, Jan Beulich wrote:
> 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.

Applied to for-linus-4.7b, thanks.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init()

[Boris Ostrovsky]

On 06/07/2016 02:31 AM, Jan Beulich wrote:
> - drop unused function parameter of read_dev_bar()
> - drop rom_init() (now identical to bar_init())
> - fold read_dev_bar() into its now single caller
> - 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>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init()

[David Vrabel]

On 07/06/16 07:31, Jan Beulich wrote:
> - drop unused function parameter of read_dev_bar()
> - drop rom_init() (now identical to bar_init())
> - fold read_dev_bar() into its now single caller
> - simplify determination of 64-bit memory resource
> - use const and unsigned

Please split this in 5 separate patches for easier review.

Especially as often anyone writing "simplify" means "accidentally break".

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init()

[Jan Beulich]

>>> On 24.06.16 at 17:01, <david.vrabel@citrix.com> wrote:
> On 07/06/16 07:31, Jan Beulich wrote:
>> - drop unused function parameter of read_dev_bar()
>> - drop rom_init() (now identical to bar_init())
>> - fold read_dev_bar() into its now single caller
>> - simplify determination of 64-bit memory resource
>> - use const and unsigned
> 
> Please split this in 5 separate patches for easier review.
> 
> Especially as often anyone writing "simplify" means "accidentally break".

So this is directly opposite of what Boris had asked for - originally
there were two patches, which I folded upon his request (and
which he gave his R-b for already). May I ask the two of you to
first settle on a consistent set of expectations to patches like this?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init()

[David Vrabel]

On 27/06/16 08:24, Jan Beulich wrote:
>>>> On 24.06.16 at 17:01, <david.vrabel@citrix.com> wrote:
>> On 07/06/16 07:31, Jan Beulich wrote:
>>> - drop unused function parameter of read_dev_bar()
>>> - drop rom_init() (now identical to bar_init())
>>> - fold read_dev_bar() into its now single caller
>>> - simplify determination of 64-bit memory resource
>>> - use const and unsigned
>>
>> Please split this in 5 separate patches for easier review.
>>
>> Especially as often anyone writing "simplify" means "accidentally break".
> 
> So this is directly opposite of what Boris had asked for - originally
> there were two patches, which I folded upon his request (and
> which he gave his R-b for already). May I ask the two of you to
> first settle on a consistent set of expectations to patches like this?

SubmittingPatches section 3 is clear on what is required.

If your commit message is a list of bullet points it's a pretty big hint
that none of the changes are related.

David

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel