Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1415738 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] xen-pciback: correct and clean up BAR handling "Jan Beulich" <JBeulich@suse.com> - 2016-06-07 08:30 +0200
    [PATCH v2 1/2] xen-pciback: return proper values during BAR  sizing "Jan Beulich" <JBeulich@suse.com> - 2016-06-07 08:40 +0200
      Re: [PATCH v2 1/2] xen-pciback: return proper values during BAR  sizing Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-06-07 16:10 +0200
        Re: [PATCH v2 1/2] xen-pciback: return proper values during  BAR sizing "Jan Beulich" <JBeulich@suse.com> - 2016-06-07 16:20 +0200
    [PATCH v2 2/2] xen-pciback: clean up {bar,rom}_init() "Jan Beulich" <JBeulich@suse.com> - 2016-06-07 08:40 +0200
      Re: [PATCH v2 2/2] xen-pciback: clean up {bar,rom}_init() Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-06-07 16:10 +0200

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

1: return proper values during BAR sizing
2: clean up {bar,rom}_init()

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Fold (and extend) patches 2 and 3. Patch 1 is unchanged.

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

- 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,				\

[toc] | [prev] | [next] | [standalone]

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

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>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web