* [PATCH 1/2] xen-pciback: return proper values during BAR sizing
[not found] <57554B8D02000078000F1DE4@prv-mh.provo.novell.com>
@ 2016-06-06 8:11 ` Jan Beulich
2016-06-06 8:12 ` [PATCH 2/2] xen-pciback: clean up read_dev_bar() Jan Beulich
` (4 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2016-06-06 8:11 UTC (permalink / raw)
To: David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk,
Jan Beulich, 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>
---
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 2/2] xen-pciback: clean up read_dev_bar()
[not found] <57554B8D02000078000F1DE4@prv-mh.provo.novell.com>
2016-06-06 8:11 ` [PATCH 1/2] xen-pciback: return proper values during BAR sizing Jan Beulich
@ 2016-06-06 8:12 ` Jan Beulich
2016-06-06 8:47 ` [PATCH 3/2] xen-pciback: drop rom_init() Jan Beulich
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2016-06-06 8:12 UTC (permalink / raw)
To: David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk,
Jan Beulich, Juergen Gross
Cc: xen-devel, linux-kernel
- drop unused function parameter
- simplify determination of 64-bit memory resource
- use const and unsigned
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
drivers/xen/xen-pciback/conf_space_header.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 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,21 +209,18 @@ 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 inline void read_dev_bar(const struct pci_dev *dev,
+ struct pci_bar_info *bar_info,
+ unsigned int offset)
{
- int pos;
- struct resource *res = dev->resource;
+ unsigned int pos;
+ const struct resource *res = dev->resource;
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))) {
+ if (pos && (res[pos - 1].flags & IORESOURCE_MEM_64)) {
bar_info->val = res[pos - 1].start >> 32;
bar_info->len_val = -resource_size(&res[pos - 1]) >> 32;
return;
@@ -248,7 +245,7 @@ static void *bar_init(struct pci_dev *de
if (!bar)
return ERR_PTR(-ENOMEM);
- read_dev_bar(dev, bar, offset, ~0);
+ read_dev_bar(dev, bar, offset);
return bar;
}
@@ -260,7 +257,7 @@ static void *rom_init(struct pci_dev *de
if (!bar)
return ERR_PTR(-ENOMEM);
- read_dev_bar(dev, bar, offset, ~PCI_ROM_ADDRESS_ENABLE);
+ read_dev_bar(dev, bar, offset);
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 3/2] xen-pciback: drop rom_init()
[not found] <57554B8D02000078000F1DE4@prv-mh.provo.novell.com>
2016-06-06 8:11 ` [PATCH 1/2] xen-pciback: return proper values during BAR sizing Jan Beulich
2016-06-06 8:12 ` [PATCH 2/2] xen-pciback: clean up read_dev_bar() Jan Beulich
@ 2016-06-06 8:47 ` Jan Beulich
[not found] ` <57554C5D02000078000F1DF1@prv-mh.provo.novell.com>
` (2 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2016-06-06 8:47 UTC (permalink / raw)
To: David Vrabel, Boris Ostrovsky, Konrad Rzeszutek Wilk,
Jan Beulich, Juergen Gross
Cc: xen-devel, linux-kernel
It's identical to bar_init() now.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm sorry for this 3/2 - I only now noticed that this additional
simplification is now possible.
---
drivers/xen/xen-pciback/conf_space_header.c | 14 +-------------
1 file changed, 1 insertion(+), 13 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
@@ -250,18 +250,6 @@ static void *bar_init(struct pci_dev *de
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);
-
- return bar;
-}
-
static void bar_reset(struct pci_dev *dev, int offset, void *data)
{
struct pci_bar_info *bar = data;
@@ -380,7 +368,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: <57554C5D02000078000F1DF1@prv-mh.provo.novell.com>]
* Re: [PATCH 1/2] xen-pciback: return proper values during BAR sizing
[not found] ` <57554C5D02000078000F1DF1@prv-mh.provo.novell.com>
@ 2016-06-06 13:03 ` Boris Ostrovsky
2016-06-06 13:51 ` Jan Beulich
[not found] ` <57559C1D02000078000F20DF@prv-mh.provo.novell.com>
0 siblings, 2 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2016-06-06 13:03 UTC (permalink / raw)
To: Jan Beulich, David Vrabel, Konrad Rzeszutek Wilk, Juergen Gross
Cc: xen-devel, linux-kernel
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
_______________________________________________
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 1/2] xen-pciback: return proper values during BAR sizing
2016-06-06 13:03 ` [PATCH 1/2] xen-pciback: return proper values during BAR sizing Boris Ostrovsky
@ 2016-06-06 13:51 ` Jan Beulich
[not found] ` <57559C1D02000078000F20DF@prv-mh.provo.novell.com>
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2016-06-06 13:51 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: Juergen Gross, xen-devel, David Vrabel, linux-kernel
>>> 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
_______________________________________________
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: <57559C1D02000078000F20DF@prv-mh.provo.novell.com>]
* Re: [PATCH 1/2] xen-pciback: return proper values during BAR sizing
[not found] ` <57559C1D02000078000F20DF@prv-mh.provo.novell.com>
@ 2016-06-06 14:28 ` Boris Ostrovsky
0 siblings, 0 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2016-06-06 14:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: Juergen Gross, xen-devel, David Vrabel, linux-kernel
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>
_______________________________________________
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: <57554C7502000078000F1DF5@prv-mh.provo.novell.com>]
[parent not found: <575554CC02000078000F1E3F@prv-mh.provo.novell.com>]
* Re: [PATCH 3/2] xen-pciback: drop rom_init()
[not found] ` <575554CC02000078000F1E3F@prv-mh.provo.novell.com>
@ 2016-06-06 13:09 ` Boris Ostrovsky
2016-06-06 13:54 ` Jan Beulich
[not found] ` <57559CBD02000078000F20EC@prv-mh.provo.novell.com>
0 siblings, 2 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2016-06-06 13:09 UTC (permalink / raw)
To: Jan Beulich, David Vrabel, Konrad Rzeszutek Wilk, Juergen Gross
Cc: xen-devel, linux-kernel
On 06/06/2016 04:47 AM, Jan Beulich wrote:
> It's identical to bar_init() now.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I'm sorry for this 3/2 - I only now noticed that this additional
> simplification is now possible.
I wonder whether we should also move content of read_dev_bar() into
bar_init(). Especially given that it's not really reading a BAR but
rather initializing the stashed value.
-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
* Re: [PATCH 3/2] xen-pciback: drop rom_init()
2016-06-06 13:09 ` [PATCH 3/2] xen-pciback: drop rom_init() Boris Ostrovsky
@ 2016-06-06 13:54 ` Jan Beulich
[not found] ` <57559CBD02000078000F20EC@prv-mh.provo.novell.com>
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2016-06-06 13:54 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: Juergen Gross, xen-devel, David Vrabel, linux-kernel
>>> On 06.06.16 at 15:09, <boris.ostrovsky@oracle.com> wrote:
> On 06/06/2016 04:47 AM, Jan Beulich wrote:
>> It's identical to bar_init() now.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I'm sorry for this 3/2 - I only now noticed that this additional
>> simplification is now possible.
>
> I wonder whether we should also move content of read_dev_bar() into
> bar_init(). Especially given that it's not really reading a BAR but
> rather initializing the stashed value.
I had considered that too, but then thought the splitting out of
that logic could as well stay. If we were to do that, I'd in fact
prefer merging patches 2 and 3 (plus this additional adjustment).
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: <57559CBD02000078000F20EC@prv-mh.provo.novell.com>]
* Re: [PATCH 3/2] xen-pciback: drop rom_init()
[not found] ` <57559CBD02000078000F20EC@prv-mh.provo.novell.com>
@ 2016-06-06 14:35 ` Boris Ostrovsky
0 siblings, 0 replies; 10+ messages in thread
From: Boris Ostrovsky @ 2016-06-06 14:35 UTC (permalink / raw)
To: Jan Beulich; +Cc: Juergen Gross, xen-devel, David Vrabel, linux-kernel
On 06/06/2016 09:54 AM, Jan Beulich wrote:
>>>> On 06.06.16 at 15:09, <boris.ostrovsky@oracle.com> wrote:
>> On 06/06/2016 04:47 AM, Jan Beulich wrote:
>>> It's identical to bar_init() now.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> I'm sorry for this 3/2 - I only now noticed that this additional
>>> simplification is now possible.
>> I wonder whether we should also move content of read_dev_bar() into
>> bar_init(). Especially given that it's not really reading a BAR but
>> rather initializing the stashed value.
> I had considered that too, but then thought the splitting out of
> that logic could as well stay. If we were to do that, I'd in fact
> prefer merging patches 2 and 3 (plus this additional adjustment).
If you could do that it would be great. (Again, mostly because I think
the name is misleading and renaming it to something like dev_bar_init()
would also not be good since we already have bar_init()).
Thanks.
-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