| Message ID | 20231211063355.2630028-3-srasheed@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add PF-VF mailbox support | expand |
On Sun, Dec 10, 2023 at 10:33:53PM -0800, Shinas Rasheed wrote: > Add PF-VF mailbox initial version support > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com> > --- <...> > @@ -28,10 +28,18 @@ static void octep_pfvf_validate_version(struct octep_device *oct, u32 vf_id, > { > u32 vf_version = (u32)cmd.s_version.version; > > - if (vf_version <= OCTEP_PFVF_MBOX_VERSION_V1) > - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; > + dev_dbg(&oct->pdev->dev, "VF id:%d VF version:%d PF version:%d\n", > + vf_id, vf_version, OCTEP_PFVF_MBOX_VERSION_CURRENT); > + if (vf_version < OCTEP_PFVF_MBOX_VERSION_CURRENT) > + rsp->s_version.version = vf_version; > else > - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_NACK; > + rsp->s_version.version = OCTEP_PFVF_MBOX_VERSION_CURRENT; > + > + oct->vf_info[vf_id].mbox_version = rsp->s_version.version; > + dev_dbg(&oct->pdev->dev, "VF id:%d negotiated VF version:%d\n", > + vf_id, oct->vf_info[vf_id].mbox_version); > + > + rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; > } <...> > +#define OCTEP_PFVF_MBOX_VERSION_CURRENT OCTEP_PFVF_MBOX_VERSION_V1 This architecture design is unlikely to work in the real world unless you control both PF and VF environment. Mostly PF is running some old legacy distribution while VFs run more modern OS and this check will prevent to run new driver in VF. Thanks
Hi Leon, > > @@ -28,10 +28,18 @@ static void octep_pfvf_validate_version(struct > octep_device *oct, u32 vf_id, > > { > > u32 vf_version = (u32)cmd.s_version.version; > > > > - if (vf_version <= OCTEP_PFVF_MBOX_VERSION_V1) > > - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; > > + dev_dbg(&oct->pdev->dev, "VF id:%d VF version:%d PF > version:%d\n", > > + vf_id, vf_version, OCTEP_PFVF_MBOX_VERSION_CURRENT); > > + if (vf_version < OCTEP_PFVF_MBOX_VERSION_CURRENT) > > + rsp->s_version.version = vf_version; > > else > > - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_NACK; > > + rsp->s_version.version = > OCTEP_PFVF_MBOX_VERSION_CURRENT; > > + > > + oct->vf_info[vf_id].mbox_version = rsp->s_version.version; > > + dev_dbg(&oct->pdev->dev, "VF id:%d negotiated VF version:%d\n", > > + vf_id, oct->vf_info[vf_id].mbox_version); > > + > > + rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; > > } > > <...> > > > +#define OCTEP_PFVF_MBOX_VERSION_CURRENT > OCTEP_PFVF_MBOX_VERSION_V1 > > This architecture design is unlikely to work in the real world unless > you control both PF and VF environment. Mostly PF is running some old > legacy distribution while VFs run more modern OS and this check will > prevent to run new driver in VF. > > Thanks Thanks for the review. This version validation only concerns regarding the control net API layer (which is used to communicate with the firmware). In the case you have described, this instead enables new VF drivers to atleast work atop legacy PF drivers (note legacy here still refers to PF drivers which support this backward compatibility), although they might not be able to use the latest control net functionalities that they have been enabled for. In the absence of such a backward compatibility, VF drivers would issue control net requests which PF drivers wouldn't know, only leading to logs of incompatibility errors and erroneous usage. Also again please note that this version compatibility only concerns the control net infrastructure and API (the control plane).
On Mon, Dec 11, 2023 at 10:31:32AM +0000, Shinas Rasheed wrote: > Hi Leon, > > > > @@ -28,10 +28,18 @@ static void octep_pfvf_validate_version(struct > > octep_device *oct, u32 vf_id, > > > { > > > u32 vf_version = (u32)cmd.s_version.version; > > > > > > - if (vf_version <= OCTEP_PFVF_MBOX_VERSION_V1) > > > - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; > > > + dev_dbg(&oct->pdev->dev, "VF id:%d VF version:%d PF > > version:%d\n", > > > + vf_id, vf_version, OCTEP_PFVF_MBOX_VERSION_CURRENT); > > > + if (vf_version < OCTEP_PFVF_MBOX_VERSION_CURRENT) > > > + rsp->s_version.version = vf_version; > > > else > > > - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_NACK; > > > + rsp->s_version.version = > > OCTEP_PFVF_MBOX_VERSION_CURRENT; > > > + > > > + oct->vf_info[vf_id].mbox_version = rsp->s_version.version; > > > + dev_dbg(&oct->pdev->dev, "VF id:%d negotiated VF version:%d\n", > > > + vf_id, oct->vf_info[vf_id].mbox_version); > > > + > > > + rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; > > > } > > > > <...> > > > > > +#define OCTEP_PFVF_MBOX_VERSION_CURRENT > > OCTEP_PFVF_MBOX_VERSION_V1 > > > > This architecture design is unlikely to work in the real world unless > > you control both PF and VF environment. Mostly PF is running some old > > legacy distribution while VFs run more modern OS and this check will > > prevent to run new driver in VF. > > > > Thanks > > Thanks for the review. This version validation only concerns regarding the control net API layer (which is used to communicate with > the firmware). In the case you have described, this instead enables new VF drivers to atleast work atop legacy PF drivers (note legacy here still > refers to PF drivers which support this backward compatibility), although they might not be able to use the latest control net functionalities that they > have been enabled for. The question what will be in X years from now, when you will have v100? Will you fallback to v0 for backward compatibility? > > In the absence of such a backward compatibility, VF drivers would issue control net requests which PF drivers wouldn't know, only leading to logs of > incompatibility errors and erroneous usage. > > Also again please note that this version compatibility only concerns the control net infrastructure and API (the control plane). It doesn't matter, even in best scenario, you can't guarantee that code in VM actually implements version Y fully and will need to check correctness per-command anyway. Thanks >
> > > This architecture design is unlikely to work in the real world unless > > > you control both PF and VF environment. Mostly PF is running some old > > > legacy distribution while VFs run more modern OS and this check will > > > prevent to run new driver in VF. > > > > > > Thanks > > > > Thanks for the review. This version validation only concerns regarding the > >control net API layer (which is used to communicate with > > the firmware). In the case you have described, this instead enables new VF > >drivers to atleast work atop legacy PF drivers (note legacy here still > > refers to PF drivers which support this backward compatibility), although > >they might not be able to use the latest control net functionalities that they > > have been enabled for. > > The question what will be in X years from now, when you will have v100? > Will you fallback to v0 for backward compatibility? > > > > > In the absence of such a backward compatibility, VF drivers would issue > >control net requests which PF drivers wouldn't know, only leading to logs of > > incompatibility errors and erroneous usage. > > > > Also again please note that this version compatibility only concerns the > > control net infrastructure and API (the control plane). > > It doesn't matter, even in best scenario, you can't guarantee that code in VM > actually > implements version Y fully and will need to check correctness per-command > anyway. > > Thanks I'm afraid as to what else can be an alternative? The control net commands have to be decoded and passed by the PF driver for the VFs, as only the PFs have access to talk to firmware directly. The VF drivers do not have an alternative way to query control net APIs, and may fail if the control net APIs they have are not even recognized by the PF to decode them. Either VF commands which the PF can't support can be blocked at the source (by the equivalent PF-VF backward compatibility which will exist in VF drivers) by this negotiation, or we have to let commands come through and fail them, leading to just redundancy in terms of running code. I don't see how this negotiation in any way 'limit' the VF drivers. As you said, in essence the VF drivers will have to fallback to v0 for backward compatibility if the native host uses some old OS having older PF drivers. If not, the commands would come and fail anyways at the PF. Either way, it's an error case and this negotiation is just to decide if we are going to allow letting such commands in. Thanks for your time
On Mon, Dec 11, 2023 at 02:51:19PM +0000, Shinas Rasheed wrote: > > > > This architecture design is unlikely to work in the real world unless > > > > you control both PF and VF environment. Mostly PF is running some old > > > > legacy distribution while VFs run more modern OS and this check will > > > > prevent to run new driver in VF. > > > > > > > > Thanks > > > > > > Thanks for the review. This version validation only concerns regarding the > > >control net API layer (which is used to communicate with > > > the firmware). In the case you have described, this instead enables new VF > > >drivers to atleast work atop legacy PF drivers (note legacy here still > > > refers to PF drivers which support this backward compatibility), although > > >they might not be able to use the latest control net functionalities that they > > > have been enabled for. > > > > The question what will be in X years from now, when you will have v100? > > Will you fallback to v0 for backward compatibility? > > > > > > > > In the absence of such a backward compatibility, VF drivers would issue > > >control net requests which PF drivers wouldn't know, only leading to logs of > > > incompatibility errors and erroneous usage. > > > > > > Also again please note that this version compatibility only concerns the > > > control net infrastructure and API (the control plane). > > > > It doesn't matter, even in best scenario, you can't guarantee that code in VM > > actually > > implements version Y fully and will need to check correctness per-command > > anyway. > > > > Thanks > > I'm afraid as to what else can be an alternative? The control net commands have to be decoded and passed by the PF driver for the VFs, > as only the PFs have access to talk to firmware directly. The VF drivers do not have an alternative way to query control net APIs, and may fail > if the control net APIs they have are not even recognized by the PF to decode them. > > Either VF commands which the PF can't support can be blocked at the source (by the equivalent PF-VF backward compatibility which will exist in VF drivers) > by this negotiation, or we have to let commands come through and fail them, leading to just redundancy in terms of running code. I don't see how this negotiation in > any way 'limit' the VF drivers. > > As you said, in essence the VF drivers will have to fallback to v0 for backward compatibility if the native host uses some old OS having older PF drivers. If not, the > commands would come and fail anyways at the PF. Either way, it's an error case and this negotiation is just to decide if we are going to allow letting such commands in. I don't know what netdev maintainers will do with this code, I just pointed to this architecture/HW troublesome design. Thanks > > Thanks for your time
> > I'm afraid as to what else can be an alternative? The control net commands > have to be decoded and passed by the PF driver for the VFs, > > as only the PFs have access to talk to firmware directly. The VF drivers do > not have an alternative way to query control net APIs, and may fail > > if the control net APIs they have are not even recognized by the PF to > decode them. > > > > Either VF commands which the PF can't support can be blocked at the > source (by the equivalent PF-VF backward compatibility which will exist in VF > drivers) > > by this negotiation, or we have to let commands come through and fail > them, leading to just redundancy in terms of running code. I don't see how > this negotiation in > > any way 'limit' the VF drivers. > > > > As you said, in essence the VF drivers will have to fallback to v0 for > backward compatibility if the native host uses some old OS having older PF > drivers. If not, the > > commands would come and fail anyways at the PF. Either way, it's an error > case and this negotiation is just to decide if we are going to allow letting such > commands in. > > I don't know what netdev maintainers will do with this code, I just > pointed to this architecture/HW troublesome design. > > Thanks I understand, thanks for your scrutiny. As I understand, its a case for letting errors run its course by letting unsupported commands in, or negotiate for a common version and stop earlier on control plane commands which the PF driver just doesn't know. Again, thanks for your constructive insight.
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h index 3223bb6f95ea..fee59e0e0138 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.h +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.h @@ -220,6 +220,7 @@ struct octep_iface_link_info { /* The Octeon VF device specific info data structure.*/ struct octep_pfvf_info { u8 mac_addr[ETH_ALEN]; + u32 mbox_version; }; /* The Octeon device specific private data structure. diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c index 43b40e91f7bf..baffe298a2a0 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.c @@ -28,10 +28,18 @@ static void octep_pfvf_validate_version(struct octep_device *oct, u32 vf_id, { u32 vf_version = (u32)cmd.s_version.version; - if (vf_version <= OCTEP_PFVF_MBOX_VERSION_V1) - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; + dev_dbg(&oct->pdev->dev, "VF id:%d VF version:%d PF version:%d\n", + vf_id, vf_version, OCTEP_PFVF_MBOX_VERSION_CURRENT); + if (vf_version < OCTEP_PFVF_MBOX_VERSION_CURRENT) + rsp->s_version.version = vf_version; else - rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_NACK; + rsp->s_version.version = OCTEP_PFVF_MBOX_VERSION_CURRENT; + + oct->vf_info[vf_id].mbox_version = rsp->s_version.version; + dev_dbg(&oct->pdev->dev, "VF id:%d negotiated VF version:%d\n", + vf_id, oct->vf_info[vf_id].mbox_version); + + rsp->s_version.type = OCTEP_PFVF_MBOX_TYPE_RSP_ACK; } static void octep_pfvf_get_link_status(struct octep_device *oct, u32 vf_id, @@ -167,6 +175,7 @@ int octep_setup_pfvf_mbox(struct octep_device *oct) goto free_mbox; memset(oct->mbox[ring], 0, sizeof(struct octep_mbox)); + memset(&oct->vf_info[i], 0, sizeof(struct octep_pfvf_info)); mutex_init(&oct->mbox[ring]->lock); INIT_WORK(&oct->mbox[ring]->wk.work, octep_pfvf_mbox_work); oct->mbox[ring]->wk.ctxptr = oct->mbox[ring]; diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h index 34feeb559b0d..af4dcf5ef7f1 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_pfvf_mbox.h @@ -13,11 +13,15 @@ #define OCTEON_SDP_16K_HW_FRS 16380UL #define OCTEON_SDP_64K_HW_FRS 65531UL +/* When a new command is implemented,PF Mbox version should be bumped. + */ enum octep_pfvf_mbox_version { OCTEP_PFVF_MBOX_VERSION_V0, OCTEP_PFVF_MBOX_VERSION_V1, }; +#define OCTEP_PFVF_MBOX_VERSION_CURRENT OCTEP_PFVF_MBOX_VERSION_V1 + enum octep_pfvf_mbox_opcode { OCTEP_PFVF_MBOX_CMD_VERSION, OCTEP_PFVF_MBOX_CMD_SET_MTU, @@ -30,7 +34,7 @@ enum octep_pfvf_mbox_opcode { OCTEP_PFVF_MBOX_CMD_GET_LINK_STATUS, OCTEP_PFVF_MBOX_CMD_GET_MTU, OCTEP_PFVF_MBOX_CMD_DEV_REMOVE, - OCTEP_PFVF_MBOX_CMD_LAST, + OCTEP_PFVF_MBOX_CMD_MAX, }; enum octep_pfvf_mbox_word_type { @@ -79,7 +83,6 @@ enum octep_pfvf_link_autoneg { #define OCTEP_PFVF_MBOX_TIMEOUT_MS 500 #define OCTEP_PFVF_MBOX_MAX_RETRIES 2 -#define OCTEP_PFVF_MBOX_VERSION 0 #define OCTEP_PFVF_MBOX_MAX_DATA_SIZE 6 #define OCTEP_PFVF_MBOX_MORE_FRAG_FLAG 1 #define OCTEP_PFVF_MBOX_WRITE_WAIT_TIME msecs_to_jiffies(1)
Add PF-VF mailbox initial version support Signed-off-by: Shinas Rasheed <srasheed@marvell.com> --- V3: - No changes V2: https://lore.kernel.org/all/20231209081450.2613561-3-srasheed@marvell.com/ - No changes V1: https://lore.kernel.org/all/20231208070352.2606192-3-srasheed@marvell.com/ .../net/ethernet/marvell/octeon_ep/octep_main.h | 1 + .../ethernet/marvell/octeon_ep/octep_pfvf_mbox.c | 15 ++++++++++++--- .../ethernet/marvell/octeon_ep/octep_pfvf_mbox.h | 7 +++++-- 3 files changed, 18 insertions(+), 5 deletions(-)