Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[David Miller]

From: Yuval Mintz 
Date: Tue, 10 May 2016 18:15:08 +

>> > I'm not entirely convinced this is true; If we'll not enforce the
>> > alignment of this 64-bit field, it's possible there will be
>> > differences between 32-bit and 64-bit machines versions of this struct.
>> > You have to recall that this is going to be copied via DMA between PF
>> > and VF, so they must have the exact same representation of the structure.
>> 
>> Then use properly sized types to fill in all the space in the structure, 
>> that's how
>> you guarantee layout, not aligned_u64.  Also, do not use the packed 
>> attribute.
>> 
>> struct foo {
>>  u32 x;
>>  u32 y;
>>  u64 z;
>> };
>> 
>> 'z' will always be 64-bit aligned.
> 
> Perhaps my bit-numeric is a bit weak - why is it so?

foo is 64-bit aligned, therefore a properly aligned struct member will
be so as well.

We absolutely depend upon this for several data structures in the kernel.

RE: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[Yuval Mintz]

> > > I'm not entirely convinced this is true; If we'll not enforce the
> > > alignment of this 64-bit field, it's possible there will be
> > > differences between 32-bit and 64-bit machines versions of this struct.
> > > You have to recall that this is going to be copied via DMA between
> > > PF and VF, so they must have the exact same representation of the 
> > > structure.
> >
> > Then use properly sized types to fill in all the space in the
> > structure, that's how you guarantee layout, not aligned_u64.  Also, do not 
> > use
> the packed attribute.
> >
> > struct foo {
> > u32 x;
> > u32 y;
> > u64 z;
> > };
> >
> > 'z' will always be 64-bit aligned.
> 
> Perhaps my bit-numeric is a bit weak - why is it so?
> I.e., what prevents `z' from only being 32-bit aligned on a 32-bit machine?
> Isn't it possible that ( % 8)  == 4, ( % 8) == 0 and ( % 8) == 4 on 
> such a
> platform?

If struct foo were to have been allocated, would it be guaranteed to be 64-bit
aligned on a 64-bit platform and 32-bit aligned on a 32-bit platform?
Assuming it is the case, is it theoretically possible that you'd have 2 
different
32-bit platforms, where on one the `z' field would be packed while on the other
it will not, introducing a 4-byte gap [assuming `foo' itself was only 32-bit 
aligned]?

RE: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[Yuval Mintz]

> > I'm not entirely convinced this is true; If we'll not enforce the
> > alignment of this 64-bit field, it's possible there will be
> > differences between 32-bit and 64-bit machines versions of this struct.
> > You have to recall that this is going to be copied via DMA between PF
> > and VF, so they must have the exact same representation of the structure.
> 
> Then use properly sized types to fill in all the space in the structure, 
> that's how
> you guarantee layout, not aligned_u64.  Also, do not use the packed attribute.
> 
> struct foo {
>   u32 x;
>   u32 y;
>   u64 z;
> };
> 
> 'z' will always be 64-bit aligned.

Perhaps my bit-numeric is a bit weak - why is it so?
I.e., what prevents `z' from only being 32-bit aligned on a 32-bit machine?
Isn't it possible that ( % 8)  == 4, ( % 8) == 0 and ( % 8) == 4 on such 
a platform?

Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[David Miller]

From: Alexander Duyck 
Date: Tue, 10 May 2016 11:02:09 -0700

> On Tue, May 10, 2016 at 10:16 AM, Yuval Mintz  wrote:
>>  > From: Yuval Mintz 
>>> Date: Mon, 9 May 2016 16:19:10 +0300
>>>
>>> > +   /* bitmap indicating which fields hold valid values */
>>> > +   aligned_u64 valid_bitmap;
>>>
>>> There is absolutely no reason to use aligned_u64 here.  That type is for 
>>> handling
>>> a specific issue in user facing APIs, which this is not.
>>
>> I'm not entirely convinced this is true; If we'll not enforce the alignment
>> of this 64-bit field, it's possible there will be differences between 32-bit
>> and 64-bit machines versions of this struct.
>> You have to recall that this is going to be copied via DMA between PF and VF,
>> so they must have the exact same representation of the structure.
>>
>> [If I'm wrong on the technical part here, please correct me; I vaguely
>> seem to recall that this was already discussed on bnx2x's implementation
>> of the hw-channel which also uses aligned u64 fields]
> 
> I think your change does have an impact, I just don't know if you
> really realize what it will get you.  Specifically what using the
> aligned_u64 is doing is forcing qed_bulletin_content to be u64 aligned
> and introducing two holes in qed_bulletin on 32 bit platforms where
> dma_addr_t might be 32 bit, and adding a 4 bytes of padding after
> size.

dma_addr_t is 32-bits on some 64-bit architectures too.

Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[David Miller]

From: Yuval Mintz 
Date: Tue, 10 May 2016 17:16:16 +

> I'm not entirely convinced this is true; If we'll not enforce the alignment
> of this 64-bit field, it's possible there will be differences between 32-bit
> and 64-bit machines versions of this struct.
> You have to recall that this is going to be copied via DMA between PF and VF,
> so they must have the exact same representation of the structure.

Then use properly sized types to fill in all the space in the
structure, that's how you guarantee layout, not aligned_u64.  Also, do
not use the packed attribute.

struct foo {
u32 x;
u32 y;
u64 z;
};

'z' will always be 64-bit aligned.

Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[Alexander Duyck]

On Tue, May 10, 2016 at 10:16 AM, Yuval Mintz  wrote:
>  > From: Yuval Mintz 
>> Date: Mon, 9 May 2016 16:19:10 +0300
>>
>> > +   /* bitmap indicating which fields hold valid values */
>> > +   aligned_u64 valid_bitmap;
>>
>> There is absolutely no reason to use aligned_u64 here.  That type is for 
>> handling
>> a specific issue in user facing APIs, which this is not.
>
> I'm not entirely convinced this is true; If we'll not enforce the alignment
> of this 64-bit field, it's possible there will be differences between 32-bit
> and 64-bit machines versions of this struct.
> You have to recall that this is going to be copied via DMA between PF and VF,
> so they must have the exact same representation of the structure.
>
> [If I'm wrong on the technical part here, please correct me; I vaguely
> seem to recall that this was already discussed on bnx2x's implementation
> of the hw-channel which also uses aligned u64 fields]

I think your change does have an impact, I just don't know if you
really realize what it will get you.  Specifically what using the
aligned_u64 is doing is forcing qed_bulletin_content to be u64 aligned
and introducing two holes in qed_bulletin on 32 bit platforms where
dma_addr_t might be 32 bit, and adding a 4 bytes of padding after
size.

My advice would be to update phys to be a u64 instead of a dma_addr_t.
That way it won't change size depending on 32 or 64 bit architecture.
Then you could align qed_bulletin to 8 bytes so that the size of the
structure remains constant on both 32 and 64 bit systems.

- Alex

RE: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[Yuval Mintz]

 > From: Yuval Mintz 
> Date: Mon, 9 May 2016 16:19:10 +0300
> 
> > +   /* bitmap indicating which fields hold valid values */
> > +   aligned_u64 valid_bitmap;
> 
> There is absolutely no reason to use aligned_u64 here.  That type is for 
> handling
> a specific issue in user facing APIs, which this is not.

I'm not entirely convinced this is true; If we'll not enforce the alignment
of this 64-bit field, it's possible there will be differences between 32-bit
and 64-bit machines versions of this struct.
You have to recall that this is going to be copied via DMA between PF and VF,
so they must have the exact same representation of the structure.

[If I'm wrong on the technical part here, please correct me; I vaguely
seem to recall that this was already discussed on bnx2x's implementation
of the hw-channel which also uses aligned u64 fields]

Re: [PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[David Miller]

From: Yuval Mintz 
Date: Mon, 9 May 2016 16:19:10 +0300

> + /* bitmap indicating which fields hold valid values */
> + aligned_u64 valid_bitmap;

There is absolutely no reason to use aligned_u64 here.  That type is for
handling a specific issue in user facing APIs, which this is not.

[PATCH net-next 01/14] qed: Add CONFIG_QED_SRIOV

[Yuval Mintz]

Add support for a new Kconfig option for qed* driver which would allow
[eventually] the support in VFs.

This patch adds the necessary logic in the PF to learn about the possible
VFs it will have to support [Based on PCI configuration space and HW],
and prepare a database with an entry per-VF as infrastructure for future
interaction with said VFs.

Signed-off-by: Yuval Mintz 
---
 drivers/net/ethernet/qlogic/Kconfig |  10 +
 drivers/net/ethernet/qlogic/qed/Makefile|   1 +
 drivers/net/ethernet/qlogic/qed/qed.h   |   7 +
 drivers/net/ethernet/qlogic/qed/qed_dev.c   |  19 ++
 drivers/net/ethernet/qlogic/qed/qed_hw.c|  11 +
 drivers/net/ethernet/qlogic/qed/qed_hw.h|  10 +
 drivers/net/ethernet/qlogic/qed/qed_sriov.c | 366 
 drivers/net/ethernet/qlogic/qed/qed_sriov.h | 185 ++
 drivers/net/ethernet/qlogic/qed/qed_vf.h|  41 
 9 files changed, 650 insertions(+)
 create mode 100644 drivers/net/ethernet/qlogic/qed/qed_sriov.c
 create mode 100644 drivers/net/ethernet/qlogic/qed/qed_sriov.h
 create mode 100644 drivers/net/ethernet/qlogic/qed/qed_vf.h

diff --git a/drivers/net/ethernet/qlogic/Kconfig 
b/drivers/net/ethernet/qlogic/Kconfig
index c0a11b5..680d8c7 100644
--- a/drivers/net/ethernet/qlogic/Kconfig
+++ b/drivers/net/ethernet/qlogic/Kconfig
@@ -98,6 +98,16 @@ config QED
---help---
  This enables the support for ...
 
+config QED_SRIOV
+   bool "QLogic QED 25/40/100Gb SR-IOV support"
+   depends on QED && PCI_IOV
+   default y
+   ---help---
+ This configuration parameter enables Single Root Input Output
+ Virtualization support for QED devices.
+ This allows for virtual function acceleration in virtualized
+ environments.
+
 config QEDE
tristate "QLogic QED 25/40/100Gb Ethernet NIC"
depends on QED
diff --git a/drivers/net/ethernet/qlogic/qed/Makefile 
b/drivers/net/ethernet/qlogic/qed/Makefile
index aafa669..e11a809 100644
--- a/drivers/net/ethernet/qlogic/qed/Makefile
+++ b/drivers/net/ethernet/qlogic/qed/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_QED) := qed.o
 qed-y := qed_cxt.o qed_dev.o qed_hw.o qed_init_fw_funcs.o qed_init_ops.o \
 qed_int.o qed_main.o qed_mcp.o qed_sp_commands.o qed_spq.o qed_l2.o \
 qed_selftest.o
+qed-$(CONFIG_QED_SRIOV) += qed_sriov.o
diff --git a/drivers/net/ethernet/qlogic/qed/qed.h 
b/drivers/net/ethernet/qlogic/qed/qed.h
index cceac32..2e067c7 100644
--- a/drivers/net/ethernet/qlogic/qed/qed.h
+++ b/drivers/net/ethernet/qlogic/qed/qed.h
@@ -152,6 +152,7 @@ enum QED_RESOURCES {
 
 enum QED_FEATURE {
QED_PF_L2_QUE,
+   QED_VF,
QED_MAX_FEATURES,
 };
 
@@ -360,6 +361,7 @@ struct qed_hwfn {
/* True if the driver requests for the link */
boolb_drv_link_init;
 
+   struct qed_pf_iov   *pf_iov_info;
struct qed_mcp_info *mcp_info;
 
struct qed_hw_cid_data  *p_tx_cids;
@@ -484,6 +486,10 @@ struct qed_dev {
u8  num_hwfns;
struct qed_hwfn hwfns[MAX_HWFNS_PER_DEVICE];
 
+   /* SRIOV */
+   struct qed_hw_sriov_info *p_iov_info;
+#define IS_QED_SRIOV(cdev)  (!!(cdev)->p_iov_info)
+
unsigned long   tunn_mode;
u32 drv_type;
 
@@ -514,6 +520,7 @@ struct qed_dev {
const struct firmware   *firmware;
 };
 
+#define NUM_OF_VFS(dev) MAX_NUM_VFS_BB
 #define NUM_OF_SBS(dev) MAX_SB_PER_PATH_BB
 #define NUM_OF_ENG_PFS(dev) MAX_NUM_PFS_BB
 
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c 
b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index b500c86..7a359c4 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -30,6 +30,7 @@
 #include "qed_mcp.h"
 #include "qed_reg_addr.h"
 #include "qed_sp.h"
+#include "qed_sriov.h"
 
 /* API common to all protocols */
 enum BAR_ID {
@@ -136,6 +137,7 @@ void qed_resc_free(struct qed_dev *cdev)
qed_eq_free(p_hwfn, p_hwfn->p_eq);
qed_consq_free(p_hwfn, p_hwfn->p_consq);
qed_int_free(p_hwfn);
+   qed_iov_free(p_hwfn);
qed_dmae_info_free(p_hwfn);
}
 }
@@ -316,6 +318,10 @@ int qed_resc_alloc(struct qed_dev *cdev)
if (rc)
goto alloc_err;
 
+   rc = qed_iov_alloc(p_hwfn);
+   if (rc)
+   goto alloc_err;
+
/* EQ */
p_eq = qed_eq_alloc(p_hwfn, 256);
if (!p_eq) {
@@ -373,6 +379,8 @@ void qed_resc_setup(struct qed_dev *cdev)
   p_hwfn->mcp_info->mfw_mb_length);
 
qed_int_setup(p_hwfn, p_hwfn->p_main_ptt);
+
+   qed_iov_setup(p_hwfn, p_hwfn->p_main_ptt);
}
 }
 
@@ -1238,6 +1246,13 @@