* [PATCH v2 1/2] xen-pciback: return proper values during BAR sizing [not found] <5756857802000078000F268D@prv-mh.provo.novell.com> @ 2016-06-07 6:30 ` Jan Beulich 2016-06-07 6:31 ` [PATCH v2 2/2] xen-pciback: clean up {bar,rom}_init() Jan Beulich ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2016-06-07 6:30 UTC (permalink / raw) To: David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] xen-pciback: clean up {bar,rom}_init() [not found] <5756857802000078000F268D@prv-mh.provo.novell.com> 2016-06-07 6:30 ` [PATCH v2 1/2] xen-pciback: return proper values during BAR sizing Jan Beulich @ 2016-06-07 6:31 ` Jan Beulich [not found] ` <5756864102000078000F269A@prv-mh.provo.novell.com> [not found] ` <5756866F02000078000F269D@prv-mh.provo.novell.com> 3 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2016-06-07 6:31 UTC (permalink / raw) To: David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel - 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <5756864102000078000F269A@prv-mh.provo.novell.com>]
* Re: [PATCH v2 1/2] xen-pciback: return proper values during BAR sizing [not found] ` <5756864102000078000F269A@prv-mh.provo.novell.com> @ 2016-06-07 14:06 ` Boris Ostrovsky [not found] ` <5756D4F4.7070802@oracle.com> ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Boris Ostrovsky @ 2016-06-07 14:06 UTC (permalink / raw) To: Jan Beulich, David Vrabel, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <5756D4F4.7070802@oracle.com>]
* Re: [PATCH v2 1/2] xen-pciback: return proper values during BAR sizing [not found] ` <5756D4F4.7070802@oracle.com> @ 2016-06-07 14:14 ` Jan Beulich 0 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2016-06-07 14:14 UTC (permalink / raw) To: David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel >>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/2] xen-pciback: return proper values during BAR sizing [not found] ` <5756864102000078000F269A@prv-mh.provo.novell.com> 2016-06-07 14:06 ` [PATCH v2 1/2] xen-pciback: return proper values during BAR sizing Boris Ostrovsky [not found] ` <5756D4F4.7070802@oracle.com> @ 2016-06-24 9:13 ` Jan Beulich [not found] ` <576D15DE02000078000F8678@prv-mh.provo.novell.com> 3 siblings, 0 replies; 10+ messages in thread From: Jan Beulich @ 2016-06-24 9:13 UTC (permalink / raw) To: David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <576D15DE02000078000F8678@prv-mh.provo.novell.com>]
* Re: [PATCH v3 1/2] xen-pciback: return proper values during BAR sizing [not found] ` <576D15DE02000078000F8678@prv-mh.provo.novell.com> @ 2016-06-24 14:08 ` David Vrabel 0 siblings, 0 replies; 10+ messages in thread From: David Vrabel @ 2016-06-24 14:08 UTC (permalink / raw) To: Jan Beulich, David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <5756866F02000078000F269D@prv-mh.provo.novell.com>]
* Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init() [not found] ` <5756866F02000078000F269D@prv-mh.provo.novell.com> @ 2016-06-07 14:06 ` Boris Ostrovsky 2016-06-24 15:01 ` David Vrabel [not found] ` <576D4B59.9070906@citrix.com> 2 siblings, 0 replies; 10+ messages in thread From: Boris Ostrovsky @ 2016-06-07 14:06 UTC (permalink / raw) To: Jan Beulich, David Vrabel, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init() [not found] ` <5756866F02000078000F269D@prv-mh.provo.novell.com> 2016-06-07 14:06 ` [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init() Boris Ostrovsky @ 2016-06-24 15:01 ` David Vrabel [not found] ` <576D4B59.9070906@citrix.com> 2 siblings, 0 replies; 10+ messages in thread From: David Vrabel @ 2016-06-24 15:01 UTC (permalink / raw) To: Jan Beulich, David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk, Juergen Gross Cc: xen-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <576D4B59.9070906@citrix.com>]
* Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init() [not found] ` <576D4B59.9070906@citrix.com> @ 2016-06-27 7:24 ` Jan Beulich [not found] ` <5770F0CF02000078000F8DD3@prv-mh.provo.novell.com> 1 sibling, 0 replies; 10+ messages in thread From: Jan Beulich @ 2016-06-27 7:24 UTC (permalink / raw) To: David Vrabel, Boris Ostrovsky; +Cc: Juergen Gross, xen-devel, linux-kernel >>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <5770F0CF02000078000F8DD3@prv-mh.provo.novell.com>]
* Re: [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init() [not found] ` <5770F0CF02000078000F8DD3@prv-mh.provo.novell.com> @ 2016-06-29 12:42 ` David Vrabel 0 siblings, 0 replies; 10+ messages in thread From: David Vrabel @ 2016-06-29 12:42 UTC (permalink / raw) To: Jan Beulich, David Vrabel, Boris Ostrovsky Cc: Juergen Gross, xen-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-29 12:42 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <5756857802000078000F268D@prv-mh.provo.novell.com> 2016-06-07 6:30 ` [PATCH v2 1/2] xen-pciback: return proper values during BAR sizing Jan Beulich 2016-06-07 6:31 ` [PATCH v2 2/2] xen-pciback: clean up {bar,rom}_init() Jan Beulich [not found] ` <5756864102000078000F269A@prv-mh.provo.novell.com> 2016-06-07 14:06 ` [PATCH v2 1/2] xen-pciback: return proper values during BAR sizing Boris Ostrovsky [not found] ` <5756D4F4.7070802@oracle.com> 2016-06-07 14:14 ` Jan Beulich 2016-06-24 9:13 ` [PATCH v3 " Jan Beulich [not found] ` <576D15DE02000078000F8678@prv-mh.provo.novell.com> 2016-06-24 14:08 ` David Vrabel [not found] ` <5756866F02000078000F269D@prv-mh.provo.novell.com> 2016-06-07 14:06 ` [PATCH v2 2/2] xen-pciback: clean up {bar, rom}_init() Boris Ostrovsky 2016-06-24 15:01 ` David Vrabel [not found] ` <576D4B59.9070906@citrix.com> 2016-06-27 7:24 ` Jan Beulich [not found] ` <5770F0CF02000078000F8DD3@prv-mh.provo.novell.com> 2016-06-29 12:42 ` David Vrabel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).