Re: [PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
On Mon, Aug 03, 2020 at 03:57:11PM +1000, Michael Ellerman wrote:
> > I would assume the function should still be generated since those checks
> > are relevant, just the return value is bogus.
>
> Yeah, just sometimes missing warnings boil down to the compiler eliding
> whole sections of code, if it can convince itself they're unreachable.
Including when the compiler considers they must be unreachable because
they would perform undefined behaviour, like, act on uninitialised
values. Such code is removed by cddce ("control dependence dead code
elimination", enabled by -ftree-dce at -O2 or above).
> AFAICS there's nothing weird going on here that should confuse GCC, it's
> about as straight forward as it gets.
Yes. Please open a bug?
> Actually I can reproduce it with:
>
> $ cat > test.c < int foo(void *p)
> {
> unsigned align;
>
> if (!p)
> return align;
>
> return 0;
> }
> EOF
>
> $ gcc -Wall -Wno-maybe-uninitialized -c test.c
> $
>
> No warning.
The whole if() is deleted pretty early.
===
static int foo(void *p)
{
unsigned align;
if (!p)
return align;
return 42;
}
int bork(void) { return foo(0); }
===
doesn't warn either, although that always uses the uninitialised var
(actually, that code is *removed*, and it always does that "return 42").
> But I guess that's behaving as documented. The compiler can't prove that
> foo() will be called with p == NULL, so it doesn't warn.
-Wmaybe-uninitialized doesn't warn for this either, btw.
Segher
Re: [PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
Nathan Chancellor writes:
> On Sun, Aug 02, 2020 at 11:12:23PM +1000, Michael Ellerman wrote:
>> Nathan Chancellor writes:
>> > On Wed, Jul 22, 2020 at 04:57:14PM +1000, Oliver O'Halloran wrote:
>> >> Using single PE BARs to map an SR-IOV BAR is really a choice about what
>> >> strategy to use when mapping a BAR. It doesn't make much sense for this to
>> >> be a global setting since a device might have one large BAR which needs to
>> >> be mapped with single PE windows and another smaller BAR that can be
>> >> mapped
>> >> with a regular segmented window. Make the segmented vs single decision a
>> >> per-BAR setting and clean up the logic that decides which mode to use.
>> >>
>> >> Signed-off-by: Oliver O'Halloran
>> >> ---
>> >> v2: Dropped unused total_vfs variables in
>> >> pnv_pci_ioda_fixup_iov_resources()
>> >> Dropped bar_no from pnv_pci_iov_resource_alignment()
>> >> Minor re-wording of comments.
>> >> ---
>> >> arch/powerpc/platforms/powernv/pci-sriov.c | 131 ++---
>> >> arch/powerpc/platforms/powernv/pci.h | 11 +-
>> >> 2 files changed, 73 insertions(+), 69 deletions(-)
>> >>
>> >> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c
>> >> b/arch/powerpc/platforms/powernv/pci-sriov.c
>> >> index ce8ad6851d73..76215d01405b 100644
>> >> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
>> >> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
>> >> @@ -260,42 +256,40 @@ void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
>> >> resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
>> >> int resno)
>> >> {
>> >> - struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
>> >> struct pnv_iov_data *iov = pnv_iov_get(pdev);
>> >> resource_size_t align;
>> >>
>> >> + /*
>> >> + * iov can be null if we have an SR-IOV device with IOV BAR that can't
>> >> + * be placed in the m64 space (i.e. The BAR is 32bit or non-prefetch).
>> >> + * In that case we don't allow VFs to be enabled since one of their
>> >> + * BARs would not be placed in the correct PE.
>> >> + */
>> >> + if (!iov)
>> >> + return align;
>> >> + if (!iov->vfs_expanded)
>> >> + return align;
>> >> +
>> >> + align = pci_iov_resource_size(pdev, resno);
>>
>> That's, oof.
>>
>> > I am not sure if it has been reported yet but clang points out that
>> > align is initialized after its use:
>> >
>> > arch/powerpc/platforms/powernv/pci-sriov.c:267:10: warning: variable
>> > 'align' is uninitialized when used here [-Wuninitialized]
>> > return align;
>> >^
>> > arch/powerpc/platforms/powernv/pci-sriov.c:258:23: note: initialize the
>> > variable 'align' to silence this warning
>> > resource_size_t align;
>> > ^
>> > = 0
>> > 1 warning generated.
>>
>> But I can't get gcc to warn about it?
>>
>> It produces some code, so it's not like the whole function has been
>> elided or something. I'm confused.
>
> -Wmaybe-uninitialized was disabled in commit 78a5255ffb6a ("Stop the
> ad-hoc games with -Wno-maybe-initialized") upstream so GCC won't warn on
> stuff like this anymore.
Seems so. Just that there's no "maybe" here, it's very uninitialised.
> I would assume the function should still be generated since those checks
> are relevant, just the return value is bogus.
Yeah, just sometimes missing warnings boil down to the compiler eliding
whole sections of code, if it can convince itself they're unreachable.
AFAICS there's nothing weird going on here that should confuse GCC, it's
about as straight forward as it gets.
Actually I can reproduce it with:
$ cat > test.c <
Re: [PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
On Sun, Aug 02, 2020 at 11:12:23PM +1000, Michael Ellerman wrote:
> Nathan Chancellor writes:
> > On Wed, Jul 22, 2020 at 04:57:14PM +1000, Oliver O'Halloran wrote:
> >> Using single PE BARs to map an SR-IOV BAR is really a choice about what
> >> strategy to use when mapping a BAR. It doesn't make much sense for this to
> >> be a global setting since a device might have one large BAR which needs to
> >> be mapped with single PE windows and another smaller BAR that can be mapped
> >> with a regular segmented window. Make the segmented vs single decision a
> >> per-BAR setting and clean up the logic that decides which mode to use.
> >>
> >> Signed-off-by: Oliver O'Halloran
> >> ---
> >> v2: Dropped unused total_vfs variables in
> >> pnv_pci_ioda_fixup_iov_resources()
> >> Dropped bar_no from pnv_pci_iov_resource_alignment()
> >> Minor re-wording of comments.
> >> ---
> >> arch/powerpc/platforms/powernv/pci-sriov.c | 131 ++---
> >> arch/powerpc/platforms/powernv/pci.h | 11 +-
> >> 2 files changed, 73 insertions(+), 69 deletions(-)
> >>
> >> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c
> >> b/arch/powerpc/platforms/powernv/pci-sriov.c
> >> index ce8ad6851d73..76215d01405b 100644
> >> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> >> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> >> @@ -260,42 +256,40 @@ void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
> >> resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
> >> int resno)
> >> {
> >> - struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
> >>struct pnv_iov_data *iov = pnv_iov_get(pdev);
> >>resource_size_t align;
> >>
> >> + /*
> >> + * iov can be null if we have an SR-IOV device with IOV BAR that can't
> >> + * be placed in the m64 space (i.e. The BAR is 32bit or non-prefetch).
> >> + * In that case we don't allow VFs to be enabled since one of their
> >> + * BARs would not be placed in the correct PE.
> >> + */
> >> + if (!iov)
> >> + return align;
> >> + if (!iov->vfs_expanded)
> >> + return align;
> >> +
> >> + align = pci_iov_resource_size(pdev, resno);
>
> That's, oof.
>
> > I am not sure if it has been reported yet but clang points out that
> > align is initialized after its use:
> >
> > arch/powerpc/platforms/powernv/pci-sriov.c:267:10: warning: variable
> > 'align' is uninitialized when used here [-Wuninitialized]
> > return align;
> >^
> > arch/powerpc/platforms/powernv/pci-sriov.c:258:23: note: initialize the
> > variable 'align' to silence this warning
> > resource_size_t align;
> > ^
> > = 0
> > 1 warning generated.
>
> But I can't get gcc to warn about it?
>
> It produces some code, so it's not like the whole function has been
> elided or something. I'm confused.
>
> cheers
-Wmaybe-uninitialized was disabled in commit 78a5255ffb6a ("Stop the
ad-hoc games with -Wno-maybe-initialized") upstream so GCC won't warn on
stuff like this anymore.
I would assume the function should still be generated since those checks
are relevant, just the return value is bogus.
Cheers,
Nathan
Re: [PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
Nathan Chancellor writes:
> On Wed, Jul 22, 2020 at 04:57:14PM +1000, Oliver O'Halloran wrote:
>> Using single PE BARs to map an SR-IOV BAR is really a choice about what
>> strategy to use when mapping a BAR. It doesn't make much sense for this to
>> be a global setting since a device might have one large BAR which needs to
>> be mapped with single PE windows and another smaller BAR that can be mapped
>> with a regular segmented window. Make the segmented vs single decision a
>> per-BAR setting and clean up the logic that decides which mode to use.
>>
>> Signed-off-by: Oliver O'Halloran
>> ---
>> v2: Dropped unused total_vfs variables in pnv_pci_ioda_fixup_iov_resources()
>> Dropped bar_no from pnv_pci_iov_resource_alignment()
>> Minor re-wording of comments.
>> ---
>> arch/powerpc/platforms/powernv/pci-sriov.c | 131 ++---
>> arch/powerpc/platforms/powernv/pci.h | 11 +-
>> 2 files changed, 73 insertions(+), 69 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c
>> b/arch/powerpc/platforms/powernv/pci-sriov.c
>> index ce8ad6851d73..76215d01405b 100644
>> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
>> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
>> @@ -260,42 +256,40 @@ void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
>> resource_size_t pnv_pci_iov_resource_alignment(struct pci_dev *pdev,
>>int resno)
>> {
>> -struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
>> struct pnv_iov_data *iov = pnv_iov_get(pdev);
>> resource_size_t align;
>>
>> +/*
>> + * iov can be null if we have an SR-IOV device with IOV BAR that can't
>> + * be placed in the m64 space (i.e. The BAR is 32bit or non-prefetch).
>> + * In that case we don't allow VFs to be enabled since one of their
>> + * BARs would not be placed in the correct PE.
>> + */
>> +if (!iov)
>> +return align;
>> +if (!iov->vfs_expanded)
>> +return align;
>> +
>> +align = pci_iov_resource_size(pdev, resno);
That's, oof.
> I am not sure if it has been reported yet but clang points out that
> align is initialized after its use:
>
> arch/powerpc/platforms/powernv/pci-sriov.c:267:10: warning: variable 'align'
> is uninitialized when used here [-Wuninitialized]
> return align;
>^
> arch/powerpc/platforms/powernv/pci-sriov.c:258:23: note: initialize the
> variable 'align' to silence this warning
> resource_size_t align;
> ^
> = 0
> 1 warning generated.
But I can't get gcc to warn about it?
It produces some code, so it's not like the whole function has been
elided or something. I'm confused.
cheers
Re: [PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
On Wed, Jul 22, 2020 at 04:57:14PM +1000, Oliver O'Halloran wrote:
> Using single PE BARs to map an SR-IOV BAR is really a choice about what
> strategy to use when mapping a BAR. It doesn't make much sense for this to
> be a global setting since a device might have one large BAR which needs to
> be mapped with single PE windows and another smaller BAR that can be mapped
> with a regular segmented window. Make the segmented vs single decision a
> per-BAR setting and clean up the logic that decides which mode to use.
>
> Signed-off-by: Oliver O'Halloran
> ---
> v2: Dropped unused total_vfs variables in pnv_pci_ioda_fixup_iov_resources()
> Dropped bar_no from pnv_pci_iov_resource_alignment()
> Minor re-wording of comments.
> ---
> arch/powerpc/platforms/powernv/pci-sriov.c | 131 ++---
> arch/powerpc/platforms/powernv/pci.h | 11 +-
> 2 files changed, 73 insertions(+), 69 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c
> b/arch/powerpc/platforms/powernv/pci-sriov.c
> index ce8ad6851d73..76215d01405b 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -146,21 +146,17 @@
> static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
> {
> struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
> - const resource_size_t gate = phb->ioda.m64_segsize >> 2;
> struct resource *res;
> int i;
> - resource_size_t size, total_vf_bar_sz;
> + resource_size_t vf_bar_sz;
> struct pnv_iov_data *iov;
> - int mul, total_vfs;
> + int mul;
>
> iov = kzalloc(sizeof(*iov), GFP_KERNEL);
> if (!iov)
> goto disable_iov;
> pdev->dev.archdata.iov_data = iov;
> -
> - total_vfs = pci_sriov_get_totalvfs(pdev);
> mul = phb->ioda.total_pe_num;
> - total_vf_bar_sz = 0;
>
> for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> res = >resource[i + PCI_IOV_RESOURCES];
> @@ -173,50 +169,50 @@ static void pnv_pci_ioda_fixup_iov_resources(struct
> pci_dev *pdev)
> goto disable_iov;
> }
>
> - total_vf_bar_sz += pci_iov_resource_size(pdev,
> - i + PCI_IOV_RESOURCES);
> + vf_bar_sz = pci_iov_resource_size(pdev, i + PCI_IOV_RESOURCES);
>
> /*
> - * If bigger than quarter of M64 segment size, just round up
> - * power of two.
> + * Generally, one segmented M64 BAR maps one IOV BAR. However,
> + * if a VF BAR is too large we end up wasting a lot of space.
> + * If each VF needs more than 1/4 of the default m64 segment
> + * then each VF BAR should be mapped in single-PE mode to reduce
> + * the amount of space required. This does however limit the
> + * number of VFs we can support.
>*
> - * Generally, one M64 BAR maps one IOV BAR. To avoid conflict
> - * with other devices, IOV BAR size is expanded to be
> - * (total_pe * VF_BAR_size). When VF_BAR_size is half of M64
> - * segment size , the expanded size would equal to half of the
> - * whole M64 space size, which will exhaust the M64 Space and
> - * limit the system flexibility. This is a design decision to
> - * set the boundary to quarter of the M64 segment size.
> + * The 1/4 limit is arbitrary and can be tweaked.
>*/
> - if (total_vf_bar_sz > gate) {
> - mul = roundup_pow_of_two(total_vfs);
> - dev_info(>dev,
> - "VF BAR Total IOV size %llx > %llx, roundup to
> %d VFs\n",
> - total_vf_bar_sz, gate, mul);
> - iov->m64_single_mode = true;
> - break;
> - }
> - }
> + if (vf_bar_sz > (phb->ioda.m64_segsize >> 2)) {
> + /*
> + * On PHB3, the minimum size alignment of M64 BAR in
> + * single mode is 32MB. If this VF BAR is smaller than
> + * 32MB, but still too large for a segmented window
> + * then we can't map it and need to disable SR-IOV for
> + * this device.
> + */
> + if (vf_bar_sz < SZ_32M) {
> + pci_err(pdev, "VF BAR%d: %pR can't be mapped in
> single PE mode\n",
> + i, res);
> + goto disable_iov;
> + }
>
> - for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> - res = >resource[i + PCI_IOV_RESOURCES];
> - if (!res->flags || res->parent)
> + iov->m64_single_mode[i] = true;
> continue;
> + }
>
> -
[PATCH v2 15/16] powerpc/powernv/sriov: Make single PE mode a per-BAR setting
Using single PE BARs to map an SR-IOV BAR is really a choice about what
strategy to use when mapping a BAR. It doesn't make much sense for this to
be a global setting since a device might have one large BAR which needs to
be mapped with single PE windows and another smaller BAR that can be mapped
with a regular segmented window. Make the segmented vs single decision a
per-BAR setting and clean up the logic that decides which mode to use.
Signed-off-by: Oliver O'Halloran
---
v2: Dropped unused total_vfs variables in pnv_pci_ioda_fixup_iov_resources()
Dropped bar_no from pnv_pci_iov_resource_alignment()
Minor re-wording of comments.
---
arch/powerpc/platforms/powernv/pci-sriov.c | 131 ++---
arch/powerpc/platforms/powernv/pci.h | 11 +-
2 files changed, 73 insertions(+), 69 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c
b/arch/powerpc/platforms/powernv/pci-sriov.c
index ce8ad6851d73..76215d01405b 100644
--- a/arch/powerpc/platforms/powernv/pci-sriov.c
+++ b/arch/powerpc/platforms/powernv/pci-sriov.c
@@ -146,21 +146,17 @@
static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
{
struct pnv_phb *phb = pci_bus_to_pnvhb(pdev->bus);
- const resource_size_t gate = phb->ioda.m64_segsize >> 2;
struct resource *res;
int i;
- resource_size_t size, total_vf_bar_sz;
+ resource_size_t vf_bar_sz;
struct pnv_iov_data *iov;
- int mul, total_vfs;
+ int mul;
iov = kzalloc(sizeof(*iov), GFP_KERNEL);
if (!iov)
goto disable_iov;
pdev->dev.archdata.iov_data = iov;
-
- total_vfs = pci_sriov_get_totalvfs(pdev);
mul = phb->ioda.total_pe_num;
- total_vf_bar_sz = 0;
for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
res = >resource[i + PCI_IOV_RESOURCES];
@@ -173,50 +169,50 @@ static void pnv_pci_ioda_fixup_iov_resources(struct
pci_dev *pdev)
goto disable_iov;
}
- total_vf_bar_sz += pci_iov_resource_size(pdev,
- i + PCI_IOV_RESOURCES);
+ vf_bar_sz = pci_iov_resource_size(pdev, i + PCI_IOV_RESOURCES);
/*
-* If bigger than quarter of M64 segment size, just round up
-* power of two.
+* Generally, one segmented M64 BAR maps one IOV BAR. However,
+* if a VF BAR is too large we end up wasting a lot of space.
+* If each VF needs more than 1/4 of the default m64 segment
+* then each VF BAR should be mapped in single-PE mode to reduce
+* the amount of space required. This does however limit the
+* number of VFs we can support.
*
-* Generally, one M64 BAR maps one IOV BAR. To avoid conflict
-* with other devices, IOV BAR size is expanded to be
-* (total_pe * VF_BAR_size). When VF_BAR_size is half of M64
-* segment size , the expanded size would equal to half of the
-* whole M64 space size, which will exhaust the M64 Space and
-* limit the system flexibility. This is a design decision to
-* set the boundary to quarter of the M64 segment size.
+* The 1/4 limit is arbitrary and can be tweaked.
*/
- if (total_vf_bar_sz > gate) {
- mul = roundup_pow_of_two(total_vfs);
- dev_info(>dev,
- "VF BAR Total IOV size %llx > %llx, roundup to
%d VFs\n",
- total_vf_bar_sz, gate, mul);
- iov->m64_single_mode = true;
- break;
- }
- }
+ if (vf_bar_sz > (phb->ioda.m64_segsize >> 2)) {
+ /*
+* On PHB3, the minimum size alignment of M64 BAR in
+* single mode is 32MB. If this VF BAR is smaller than
+* 32MB, but still too large for a segmented window
+* then we can't map it and need to disable SR-IOV for
+* this device.
+*/
+ if (vf_bar_sz < SZ_32M) {
+ pci_err(pdev, "VF BAR%d: %pR can't be mapped in
single PE mode\n",
+ i, res);
+ goto disable_iov;
+ }
- for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
- res = >resource[i + PCI_IOV_RESOURCES];
- if (!res->flags || res->parent)
+ iov->m64_single_mode[i] = true;
continue;
+ }
- size = pci_iov_resource_size(pdev, i + PCI_IOV_RESOURCES);
/*
-* On PHB3, the minimum size
