| Message ID | 57554C5D02000078000F1DF1@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
>>> 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
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>
--- 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; }
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(-)