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

Commit Message

Jan Beulich June 7, 2016, 6:30 a.m. UTC

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 <[e-mail]>
Reviewed-by: Boris Ostrovsky <[e-mail]>
---
 drivers/xen/xen-pciback/conf_space_header.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Boris Ostrovsky June 7, 2016, 2:06 p.m. UTC | #1

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

Jan Beulich June 7, 2016, 2:14 p.m. UTC | #2

>>> On 07.06.16 at 16:06, <[e-mail]> 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

Patch

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