Bus control framework
cover Bus control framework
Probing policy was introduced in the previous release as a configuration item. It was thus added to the generic bus structure, breaking its ABI.
In this release, the IOVA mode can be read from a bus to configure the EAL. This new configuration element also broke the bus ABI when it was added.
As new operators had to be implemented for the probe policy item, these patches were developed to help mitigate this issue.
This control framework allows to expand the rte_bus API without breaking its ABI. It is meant to be used with configuration elements that may only be valid for a few buses, while the others would remain untouched and unaware of the evolution.
A central control operator is used, similarly to the working of rte_flow API in the ether layer. Each driver thus chooses to expose a set of operators relevant to its implementation. The caller is then free to use those if they are available.
Both Probe mode and IOVA mode operators are implemented for the PCI bus.
This patchset depends on:
Move PCI away from the EAL
http://dpdk.org/ml/archives/dev/2017-August/073512.html
Gaetan Rivet (8):
bus: rename scan policy as probe policy
bus: introduce opaque control framework
bus: remove probe mode configuration structure
bus: add probe mode setter
bus/pci: implement ctrl operator
bus: add IOVA mode as a ctrl operation
bus/pci: implement IOVA mode getter
bus: remove redundant IOVA mode getter
drivers/bus/pci/bsd/pci.c | 9 +- drivers/bus/pci/include/rte_bus_pci.h | 12 +-- drivers/bus/pci/linux/pci.c | 20 ++-- drivers/bus/pci/pci_common.c | 53 +++++++++- drivers/bus/pci/private.h | 13 +++ lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 - lib/librte_eal/common/eal_common_bus.c | 57 +++++++++-- lib/librte_eal/common/eal_common_devargs.c | 8 -- lib/librte_eal/common/eal_common_options.c | 17 +--- lib/librte_eal/common/include/rte_bus.h | 127 ++++++++++++++++-------- lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 - 11 files changed, 222 insertions(+), 96 deletions(-)
Signature · 1 line
2.1.4
PATCH 1/8 bus: rename scan policy as probe policy
This bus configuration item is misnamed, as it actually refers to the probing process.
--- drivers/bus/pci/pci_common.c | 2 +- lib/librte_eal/common/eal_common_devargs.c | 6 +++--- lib/librte_eal/common/include/rte_bus.h | 12 ++++++------ 3 files changed, 10 insertions(+), 10 deletions(-)
drivers/bus/pci/pci_common.c +1 -1
@@ -373,7 +373,7 @@ rte_pci_probe(void)int probe_all =0 ;int ret =0 ; -if (rte_pci_bus.bus.conf.scan_mode !=RTE_BUS_SCAN_WHITELIST ) +if (rte_pci_bus.bus.conf.probe_mode !=RTE_BUS_PROBE_WHITELIST ) probe_all =1 ;FOREACH_DEVICE_ON_PCIBUS (dev) {
lib/librte_eal/common/eal_common_devargs.c +3 -3
@@ -170,11 +170,11 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) bus = devargs->bus;if (devargs->type ==RTE_DEVTYPE_BLACKLISTED_PCI ) devargs->policy =RTE_DEV_BLACKLISTED ; -if (bus->conf.scan_mode ==RTE_BUS_SCAN_UNDEFINED ) { +if (bus->conf.probe_mode ==RTE_BUS_PROBE_UNDEFINED ) {if (devargs->policy ==RTE_DEV_WHITELISTED ) - bus->conf.scan_mode =RTE_BUS_SCAN_WHITELIST ; + bus->conf.probe_mode =RTE_BUS_PROBE_WHITELIST ;else if (devargs->policy ==RTE_DEV_BLACKLISTED ) - bus->conf.scan_mode =RTE_BUS_SCAN_BLACKLIST ; + bus->conf.probe_mode =RTE_BUS_PROBE_BLACKLIST ; }TAILQ_INSERT_TAIL (&devargs_list, devargs, next);return 0 ;
lib/librte_eal/common/include/rte_bus.h +6 -6
@@ -168,19 +168,19 @@ typedef int (*rte_bus_unplug_t)(struct rte_device *dev);typedef int (*rte_bus_parse_t )(const char *name,void *addr);/** -* Bus scan policies +* Bus probe policies */ -enum rte_bus_scan_mode { -RTE_BUS_SCAN_UNDEFINED , -RTE_BUS_SCAN_WHITELIST , -RTE_BUS_SCAN_BLACKLIST , +enum rte_bus_probe_mode { +RTE_BUS_PROBE_UNDEFINED , +RTE_BUS_PROBE_WHITELIST , +RTE_BUS_PROBE_BLACKLIST , };/** * A structure used to configure bus operations. */ struct rte_bus_conf { -enum rte_bus_scan_mode scan_mode;/**< Scan policy. */ +enum rte_bus_probe_mode probe_mode;/**< Probe policy. */ };
Signature · 1 line
2.1.4
PATCH 2/8 bus: introduce opaque control framework
In reply to Gaetan RivetNew configuration elements are added to the buses. They make the ABI unstable and will continue to do so.
This new control scheme allows to add new bus operators without breaking the ABI and by only expanding the API.
This helps having more stability in core EAL subsystems, while allowing flexibility for future evolutions.
--- lib/librte_eal/common/eal_common_bus.c | 9 +++++++ lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+)
lib/librte_eal/common/eal_common_bus.c +9 -0
@@ -42,6 +42,13 @@struct rte_bus_list rte_bus_list =TAILQ_HEAD_INITIALIZER (rte_bus_list); +static rte_bus_ctrl_t +rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused, + enum rte_bus_ctrl_item item __rte_unused) +{ +return NULL ; +} +void rte_bus_register (struct rte_bus *bus) {
@@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus)RTE_VERIFY (bus->find_device);/* Buses supporting driver plug also require unplug. */ RTE_VERIFY (!bus->plug || bus->unplug); +if (bus->ctrl ==NULL ) + bus->ctrl = &rte_bus_default_ctrl;TAILQ_INSERT_TAIL (&rte_bus_list, bus, next);RTE_LOG (DEBUG ,EAL ,"Registered [%s] bus.\n" , bus->name);
lib/librte_eal/common/include/rte_bus.h +46 -0
58 lines not on this page · raw message
Signature · 1 line
2.1.4
Re: [PATCH v1 2/8] bus: introduce opaque control framework
Quoted 47 lines · Gaetan Rivet
New configuration elements are added to the buses. They make the ABI unstable and will continue to do so.
This new control scheme allows to add new bus operators without breaking the ABI and by only expanding the API.
This helps having more stability in core EAL subsystems, while allowing flexibility for future evolutions.
Signed-off-by Gaetan Rivet <[email protected]>--- lib/librte_eal/common/eal_common_bus.c | 9 +++++++ lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+)lib/librte_eal/common/eal_common_bus.c +9
-0@@ -42,6 +42,13 @@struct rte_bus_list rte_bus_list =TAILQ_HEAD_INITIALIZER (rte_bus_list); +static rte_bus_ctrl_t +rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused, + enum rte_bus_ctrl_item item __rte_unused) +{ +return NULL ; +} +void rte_bus_register (struct rte_bus *bus) {@@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus)RTE_VERIFY (bus->find_device);/* Buses supporting driver plug also require unplug. */ RTE_VERIFY (!bus->plug || bus->unplug); +if (bus->ctrl ==NULL ) + bus->ctrl = &rte_bus_default_ctrl;TAILQ_INSERT_TAIL (&rte_bus_list, bus, next);RTE_LOG (DEBUG ,EAL ,"Registered [%s] bus.\n" , bus->name);lib/librte_eal/common/include/rte_bus.h +0
-0
lib/librte_eal/common/include/rte_bus.h struct rte_bus_conf { · line 192
Quoted 10 lines
lib/librte_eal/common/include/rte_bus.h +7
-0@@ -183,6 +183,51 @@ struct rte_bus_conf {enum rte_bus_probe_mode probe_mode;/**< Probe policy. */ }; +/** +* Bus configuration items. +*/ +enum rte_bus_ctrl_item { +RTE_BUS_CTRL_PROBE_MODE =0 , +RTE_BUS_CTRL_ITEM_MAX , +};
I am assuming that a driver implementation can take more than ITEM_MAX control knobs. It is opaque to the library. Are we on same page?
For example, a bus driver can implement:
rte_bus_XXX_ctrl_item {
<Leaving space for allowing rte_bus.h implementations>
RTE_BUS_XYZ_KNOB_1 = 100,
RTE_BUS_XYZ_KNOB_2,
RTE_BUS_XYZ_KNOB_3,
};without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX.
I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted the control knob to RTE_BUS_CTRL_ITEM_MAX. I hope that such restrictions would not float to library layer.
If we are on same page, should this be documented as a code comment somewhere? if not, do you think what I am stating makes sense?
lib/librte_eal/common/include/rte_bus.h struct rte_bus_conf { · line 202
Quoted 10 lines
lib/librte_eal/common/include/rte_bus.h +10
-0+ +/** +* Bus configuration operations. +*/ +enum rte_bus_ctrl_op { +RTE_BUS_CTRL_GET =0 , +RTE_BUS_CTRL_SET , +RTE_BUS_CTRL_RESET , +RTE_BUS_CTRL_OP_MAX , +};
Similarly, the driver implementation can choose to implement a operation which is not defined in the above structures. Obviously, the application is expected to know - it being a custom knob.
[...]
Re: [PATCH v1 2/8] bus: introduce opaque control framework
Quoted 70 lines · Shreyansh Jain
Quoted 45 lines · Gaetan Rivet
New configuration elements are added to the buses. They make the ABI unstable and will continue to do so.
This new control scheme allows to add new bus operators without breaking the ABI and by only expanding the API.
This helps having more stability in core EAL subsystems, while allowing flexibility for future evolutions.
Signed-off-by Gaetan Rivet <[email protected]>--- lib/librte_eal/common/eal_common_bus.c | 9 +++++++ lib/librte_eal/common/include/rte_bus.h | 46 +++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+)lib/librte_eal/common/eal_common_bus.c +9
-0@@ -42,6 +42,13 @@struct rte_bus_list rte_bus_list =TAILQ_HEAD_INITIALIZER (rte_bus_list); +static rte_bus_ctrl_t +rte_bus_default_ctrl(enum rte_bus_ctrl_op op __rte_unused, + enum rte_bus_ctrl_item item __rte_unused) +{ +return NULL ; +} +void rte_bus_register (struct rte_bus *bus) {@@ -53,6 +60,8 @@ rte_bus_register(struct rte_bus *bus)RTE_VERIFY (bus->find_device);/* Buses supporting driver plug also require unplug. */ RTE_VERIFY (!bus->plug || bus->unplug); +if (bus->ctrl ==NULL ) + bus->ctrl = &rte_bus_default_ctrl;TAILQ_INSERT_TAIL (&rte_bus_list, bus, next);RTE_LOG (DEBUG ,EAL ,"Registered [%s] bus.\n" , bus->name);lib/librte_eal/common/include/rte_bus.h +0
-0lib/librte_eal/common/include/rte_bus.h struct rte_bus_conf { · line 191
Quoted 9 lines
lib/librte_eal/common/include/rte_bus.h +7
-0@@ -183,6 +183,51 @@ struct rte_bus_conf {enum rte_bus_probe_mode probe_mode;/**< Probe policy. */ }; +/** +* Bus configuration items. +*/ +enum rte_bus_ctrl_item { +RTE_BUS_CTRL_PROBE_MODE =0 , +RTE_BUS_CTRL_ITEM_MAX , +};I am assuming that a driver implementation can take more than
ITEM_MAXcontrol knobs. It is opaque to the library. Are we on same page?For example, a bus driver can implement:
rte_bus_XXX_ctrl_item { <Leaving space for allowing rte_bus.h implementations> RTE_BUS_XYZ_KNOB_1 = 100, RTE_BUS_XYZ_KNOB_2, RTE_BUS_XYZ_KNOB_3, };without the library knowing or restricting the API to
RTE_BUS_CTRL_ITEM_MAX.
Quoted 8 lines
I see that in your code for PCI (Patch 5/8:
pci_ctrl) you have restricted the control knob toRTE_BUS_CTRL_ITEM_MAX.
I hope that such restrictions would not float to library layer.If we are on same page, should this be documented as a code comment somewhere?
if not, do you think what I am stating makes sense?
I see what you mean, but I'm not sure it would be a good thing. Actually, I think proposing this ITEM_MAX was a mistake.
Regarding the specific bus knobs:
- If a single bus needs this knob, then it would be better for the dev to add it as part of the bus' public API, following the correct library versioning processes. This would not break this bus control structure ABI.
- If more than one bus implement this knob, then it should be proposed as part of the library API. Buses adding this new knob would break their ABI, other buses would be left untouched.
This makes me realize that proposing this ITEM_MAX value is not good to the intended purpose of this patchset:
- If a bus implementation use a reference to ITEM_MAX, then the control structure ABI would be broken by any new control knob added, even if the bus does not implement it. Granted, it would not break the driver structure itself, but still. My PCI implementation is thus incorrect.
Therefore I think that it would be best to remove this ITEM_MAX altogether, forcing bus developpers to use other ways that would not break their ABIs every other release.
Signature · 2 lines
Gaëtan Rivet 6WIND
Re: [PATCH v1 2/8] bus: introduce opaque control framework
Quoted 2 lines · Gaëtan Rivet
Quoted 1 line · Shreyansh Jain
On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
[...]
Quoted 41 lines
Quoted 29 lines
Quoted 5 lines
lib/librte_eal/common/include/rte_bus.h +0
-0lib/librte_eal/common/include/rte_bus.h struct rte_bus_conf { · line 191
Quoted 9 lines
lib/librte_eal/common/include/rte_bus.h +7
-0@@ -183,6 +183,51 @@ struct rte_bus_conf {enum rte_bus_probe_mode probe_mode;/**< Probe policy. */ }; +/** +* Bus configuration items. +*/ +enum rte_bus_ctrl_item { +RTE_BUS_CTRL_PROBE_MODE =0 , +RTE_BUS_CTRL_ITEM_MAX , +};I am assuming that a driver implementation can take more than
ITEM_MAXcontrol knobs. It is opaque to the library. Are we on same page?For example, a bus driver can implement:
rte_bus_XXX_ctrl_item { <Leaving space for allowing rte_bus.h implementations> RTE_BUS_XYZ_KNOB_1 = 100, RTE_BUS_XYZ_KNOB_2, RTE_BUS_XYZ_KNOB_3, };without the library knowing or restricting the API to
RTE_BUS_CTRL_ITEM_MAX.Quoted 8 lines
I see that in your code for PCI (Patch 5/8:
pci_ctrl) you have restricted the control knob toRTE_BUS_CTRL_ITEM_MAX.
I hope that such restrictions would not float to library layer.If we are on same page, should this be documented as a code comment somewhere?
if not, do you think what I am stating makes sense?I see what you mean, but I'm not sure it would be a good thing. Actually, I think proposing this
ITEM_MAXwas a mistake.
Quoted 6 lines
Regarding the specific bus knobs:
- If a single bus needs this knob, then it would be better for the dev to add it as part of the bus' public API, following the correct library versioning processes. This would not break this bus control structure ABI.
Sorry, but can you elaborate on "...add it as part of bus' public API"?
This is what I had in mind:
ctrl_fn = rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);
(unlike specific functions like probe_mode_get/set and iova_mode_get/set)
Where ctrl_fn would then point to a method specific to bus for KNOB_1 configuration parameter. Thereafter, ctrl_fn(KNOB_1, void *arg).
What other public API method are you hinting at?
Quoted 4 lines
- If more than one bus implement this knob, then it should be proposed as part of the library API. Buses adding this new knob would break their ABI, other buses would be left untouched.
Agree, if more than one bus implements same operation.
Quoted 8 lines
This makes me realize that proposing this
ITEM_MAXvalue is not good to the intended purpose of this patchset:- If a bus implementation use a reference to
ITEM_MAX, then the control structure ABI would be broken by any new control knob added, even if the bus does not implement it. Granted, it would not break the driver structure itself, but still. My PCI implementation is thus incorrect.
Changes to enum wouldn't break ABI as far as I understand. Adding a new entry only expands it to a new declaration without impacting its size or signature.
Quoted 5 lines
Therefore I think that it would be best to remove this
ITEM_MAXaltogether, forcing bus developpers to use other ways that would not break their ABIs every other release.
Removing ITEM_MAX is OK from my side. It doesn't serve much purpose. But, not for the "ABI break" reason.
Re: [PATCH v1 2/8] bus: introduce opaque control framework
Quoted 60 lines · Shreyansh Jain
Quoted 2 lines · Gaëtan Rivet
Quoted 1 line · Shreyansh Jain
On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
[...]
Quoted 41 lines
Quoted 29 lines
Quoted 5 lines
lib/librte_eal/common/include/rte_bus.h +0
-0lib/librte_eal/common/include/rte_bus.h struct rte_bus_conf { · line 191
Quoted 9 lines
lib/librte_eal/common/include/rte_bus.h +7
-0@@ -183,6 +183,51 @@ struct rte_bus_conf {enum rte_bus_probe_mode probe_mode;/**< Probe policy. */ }; +/** +* Bus configuration items. +*/ +enum rte_bus_ctrl_item { +RTE_BUS_CTRL_PROBE_MODE =0 , +RTE_BUS_CTRL_ITEM_MAX , +};I am assuming that a driver implementation can take more than
ITEM_MAXcontrol knobs. It is opaque to the library. Are we on same page?For example, a bus driver can implement:
rte_bus_XXX_ctrl_item { <Leaving space for allowing rte_bus.h implementations> RTE_BUS_XYZ_KNOB_1 = 100, RTE_BUS_XYZ_KNOB_2, RTE_BUS_XYZ_KNOB_3, };without the library knowing or restricting the API to
RTE_BUS_CTRL_ITEM_MAX.Quoted 8 lines
I see that in your code for PCI (Patch 5/8:
pci_ctrl) you have restricted the control knob toRTE_BUS_CTRL_ITEM_MAX.
I hope that such restrictions would not float to library layer.If we are on same page, should this be documented as a code comment somewhere?
if not, do you think what I am stating makes sense?I see what you mean, but I'm not sure it would be a good thing. Actually, I think proposing this
ITEM_MAXwas a mistake.Quoted 6 lines
Regarding the specific bus knobs:
- If a single bus needs this knob, then it would be better for the dev to add it as part of the bus' public API, following the correct library versioning processes. This would not break this bus control structure ABI.
Sorry, but can you elaborate on "...add it as part of bus' public API"?
This is what I had in mind:
ctrl_fn=rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);
Quoted 9 lines
(unlike specific functions like probe_mode_get/set and iova_mode_get/set)
Where
ctrl_fnwould then point to a method specific to bus forKNOB_1configuration parameter.
Thereafter,ctrl_fn(KNOB_1, void *arg).What other public API method are you hinting at?
I was thinking that buses would simply expose a function
rte_busname_xyz_knob1(void *arg);
as part of their public API. This would not require an ABI break for this bus, as it would only be an API extension and would not use callbacks within the bus structure.
Thus, I think that for buses tempted to propose a specific API, this would be the cleanest way.
The bus proposing it as part of a custom control section would only be interesting when the operation is expected to become a standard API for other buses but was not yet accepted. Applications would be able to use the interface and the ITEM could be added later. But I doubt this is encouraging best practices as far as API evolution go.
Quoted 21 lines
Quoted 4 lines
- If more than one bus implement this knob, then it should be proposed as part of the library API. Buses adding this new knob would break their ABI, other buses would be left untouched.
Agree, if more than one bus implements same operation.
Quoted 8 lines
This makes me realize that proposing this
ITEM_MAXvalue is not good to the intended purpose of this patchset:- If a bus implementation use a reference to
ITEM_MAX, then the control structure ABI would be broken by any new control knob added, even if the bus does not implement it. Granted, it would not break the driver structure itself, but still. My PCI implementation is thus incorrect.Changes to enum wouldn't break ABI as far as I understand. Adding a new entry only expands it to a new declaration without impacting its size or signature.
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Therefore I think that it would be best to remove this
ITEM_MAXaltogether, forcing bus developpers to use other ways that would not break their ABIs every other release.Removing
ITEM_MAXis OK from my side. It doesn't serve much purpose. But, not for the "ABI break" reason.
Adding the enum would not break ABI indeed, but I was thinking about the way the bus control structure would be declared.
However, upon second inspection on the my PCI implementation, I did not actually use ITEM_MAX:
Quoted 6 lines
static
rte_bus_ctrl_tpci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {» [RTE_BUS_CTRL_PROBE_MODE] = { » » [RTE_BUS_CTRL_GET] = pci_probe_mode_get, » » [RTE_BUS_CTRL_SET] = pci_probe_mode_set, » },};
I just thought I had used RTE_BUS_CTRL_ITEM_MAX instead of RTE_BUS_CTRL_OP_MAX. So my line of thought was simply that if any new item was declared, the control structure would then change size.
But I was mistaken, so that's actually not a problem :)
Having ITEM_MAX available would still make those kind of mistakes possible however. It might be better to prevent it completely by removing it. This would however also prevent a custom control section.
Do you think this would be useful enough to justify the slightly more complex maintenance and review of bus implementations?
Signature · 2 lines
Gaëtan Rivet 6WIND
Re: [PATCH v1 2/8] bus: introduce opaque control framework
Quoted 67 lines · Gaëtan Rivet
Quoted 66 lines · Shreyansh Jain
Quoted 2 lines · Gaëtan Rivet
Quoted 1 line · Shreyansh Jain
On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote:
[...]
Quoted 41 lines
Quoted 29 lines
Quoted 5 lines
lib/librte_eal/common/include/rte_bus.h +0
-0lib/librte_eal/common/include/rte_bus.h struct rte_bus_conf { · line 191
Quoted 9 lines
lib/librte_eal/common/include/rte_bus.h +7
-0@@ -183,6 +183,51 @@ struct rte_bus_conf {enum rte_bus_probe_mode probe_mode;/**< Probe policy. */ }; +/** +* Bus configuration items. +*/ +enum rte_bus_ctrl_item { +RTE_BUS_CTRL_PROBE_MODE =0 , +RTE_BUS_CTRL_ITEM_MAX , +};I am assuming that a driver implementation can take more than
ITEM_MAXcontrol knobs. It is opaque to the library. Are we on same page?For example, a bus driver can implement:
rte_bus_XXX_ctrl_item { <Leaving space for allowing rte_bus.h implementations> RTE_BUS_XYZ_KNOB_1 = 100, RTE_BUS_XYZ_KNOB_2, RTE_BUS_XYZ_KNOB_3, };without the library knowing or restricting the API to
RTE_BUS_CTRL_ITEM_MAX.Quoted 8 lines
I see that in your code for PCI (Patch 5/8:
pci_ctrl) you have restricted the control knob toRTE_BUS_CTRL_ITEM_MAX.
I hope that such restrictions would not float to library layer.If we are on same page, should this be documented as a code comment somewhere?
if not, do you think what I am stating makes sense?I see what you mean, but I'm not sure it would be a good thing. Actually, I think proposing this
ITEM_MAXwas a mistake.Quoted 6 lines
Regarding the specific bus knobs:
- If a single bus needs this knob, then it would be better for the dev to add it as part of the bus' public API, following the correct library versioning processes. This would not break this bus control structure ABI.
Sorry, but can you elaborate on "...add it as part of bus' public API"?
This is what I had in mind:
ctrl_fn=rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1);(unlike specific functions like probe_mode_get/set and iova_mode_get/set)
Where
ctrl_fnwould then point to a method specific to bus forKNOB_1configuration parameter.
Thereafter,ctrl_fn(KNOB_1, void *arg).
Quoted 7 lines
Quoted 3 lines
What other public API method are you hinting at?
I was thinking that buses would simply expose a function
rte_busname_xyz_knob1(void *arg);
Yes, that is possible but only for cases where some very specific functionality needs to be exposed which is not expected to be generalized ever.
Quoted 4 lines
as part of their public API. This would not require an ABI break for this bus, as it would only be an API extension and would not use callbacks within the bus structure.
Yes, agree with your point.
As such APIs are outside control of DPDK framework, they are something which will never impact the library layer.
Quoted 8 lines
Thus, I think that for buses tempted to propose a specific API, this would be the cleanest way. >
The bus proposing it as part of a custom control section would only be interesting when the operation is expected to become a standard API for other buses but was not yet accepted. Applications would be able to use the interface and the ITEM could be added later. But I doubt this is encouraging best practices as far as API evolution go.
So, technically both are feasible: 1) having a bus specific API like rte_busname_xyz_knob1 and 2) expanding OPS with bus specific values and allowing application to use them.
But, in either case, if the APIs can be generalized and/or can be used by multiple buses, they can definitely be moved into the library API (e.g. rte_bus_probe_mode_set) and/or can be added to rte_bus_ctrl_item.
To summarize, I am OK with your approach.
Quoted 47 lines
Quoted 22 lines
Quoted 5 lines
- If more than one bus implement this knob, then it should be proposed as part of the library API. Buses adding this new knob would break their ABI, other buses would be left untouched.
Agree, if more than one bus implements same operation.
Quoted 8 lines
This makes me realize that proposing this
ITEM_MAXvalue is not good to the intended purpose of this patchset:- If a bus implementation use a reference to
ITEM_MAX, then the control structure ABI would be broken by any new control knob added, even if the bus does not implement it. Granted, it would not break the driver structure itself, but still. My PCI implementation is thus incorrect.Changes to enum wouldn't break ABI as far as I understand. Adding a new entry only expands it to a new declaration without impacting its size or signature.
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
Therefore I think that it would be best to remove this
ITEM_MAXaltogether, forcing bus developpers to use other ways that would not break their ABIs every other release.Removing
ITEM_MAXis OK from my side. It doesn't serve much purpose. But, not for the "ABI break" reason.Adding the enum would not break ABI indeed, but I was thinking about the way the bus control structure would be declared.
However, upon second inspection on the my PCI implementation, I did not actually use
ITEM_MAX:Quoted 6 lines
static
rte_bus_ctrl_tpci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {» [RTE_BUS_CTRL_PROBE_MODE] = { » » [RTE_BUS_CTRL_GET] = pci_probe_mode_get, » » [RTE_BUS_CTRL_SET] = pci_probe_mode_set, » },};
I just thought I had used
RTE_BUS_CTRL_ITEM_MAXinstead ofRTE_BUS_CTRL_OP_MAX. So my line of thought was simply that if any new item was declared, the control structure would then change size.
Quoted 5 lines
But I was mistaken, so that's actually not a problem :)
Having
ITEM_MAXavailable would still make those kind of mistakes possible however. It might be better to prevent it completely by removing it. This would however also prevent a custom control section.
It might be a naive question, but, why do you think it would prevent a custom control section?
Assuming that only RTE_BUS_CTRL_ITEM_MAX is not available (RTE_BUS_CTRL_OPS_MAX is available), Bus driver can still define:
static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = {
[ITEM_1] = {
{...},
{...},
},
[ITEM_2] = {
{...},
{...},
},
[ITEM_NOT_IN_RTE_BUS.H] = {
{...},
{...},
},
}(I know you disagree with third element of above definition - but somehow I feel it is a good addition for defining a knob which doesn't require an additional API call. Just assume as an example for now, please!)
Quoted 4 lines
Do you think this would be useful enough to justify the slightly more complex maintenance and review of bus implementations?
Having RTE_BUS_CTRL_ITEM_MAX is helpful if one has to iterate over all ctrl_items - but, that might never be required in my perception. Other than that, I don't have a strong reason to say that ITEM_MAX is required. Though, same is not true for OP_MAX - which is required for definitions like above.
PATCH 3/8 bus: remove probe mode configuration structure
In reply to Gaetan RivetThis configuration item will be implemented within the new flexible framework.
--- drivers/bus/pci/pci_common.c | 5 +---- lib/librte_eal/common/eal_common_devargs.c | 8 -------- lib/librte_eal/common/include/rte_bus.h | 8 -------- 3 files changed, 1 insertion(+), 20 deletions(-)
drivers/bus/pci/pci_common.c +1 -4
@@ -370,12 +370,9 @@ rte_pci_probe(void)struct rte_pci_device *dev =NULL ;size_t probed =0 , failed =0 ;struct rte_devargs *devargs; -int probe_all =0 ; +int probe_all =1 ;int ret =0 ; -if (rte_pci_bus.bus.conf.probe_mode !=RTE_BUS_PROBE_WHITELIST ) - probe_all =1 ; -FOREACH_DEVICE_ON_PCIBUS (dev) { probed++;
lib/librte_eal/common/eal_common_devargs.c +0 -8
@@ -156,7 +156,6 @@ int rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str) { struct rte_devargs *devargs = NULL; -struct rte_bus *bus =NULL ;const char *dev = devargs_str;/* use calloc instead of rte_zmalloc as it's called early at init */
@@ -167,15 +166,8 @@ rte_eal_devargs_add(enum rte_devtype devtype, const char *devargs_str)if (rte_eal_devargs_parse (dev, devargs)) goto fail; devargs->type = devtype; - bus = devargs->bus;if (devargs->type ==RTE_DEVTYPE_BLACKLISTED_PCI ) devargs->policy =RTE_DEV_BLACKLISTED ; -if (bus->conf.probe_mode ==RTE_BUS_PROBE_UNDEFINED ) { -if (devargs->policy ==RTE_DEV_WHITELISTED ) - bus->conf.probe_mode =RTE_BUS_PROBE_WHITELIST ; -else if (devargs->policy ==RTE_DEV_BLACKLISTED ) - bus->conf.probe_mode =RTE_BUS_PROBE_BLACKLIST ; - }TAILQ_INSERT_TAIL (&devargs_list, devargs, next);return 0 ;
lib/librte_eal/common/include/rte_bus.h +0 -8
@@ -177,13 +177,6 @@ enum rte_bus_probe_mode { };/** -* A structure used to configure bus operations. -*/ -struct rte_bus_conf { -enum rte_bus_probe_mode probe_mode;/**< Probe policy. */ -}; - -/** * Bus configuration items. */ enum rte_bus_ctrl_item {
@@ -254,7 +247,6 @@ struct rte_bus {rte_bus_plug_t plug;/**< Probe single device for drivers */ rte_bus_unplug_t unplug;/**< Remove single device from driver */ rte_bus_parse_t parse;/**< Parse a device name */ -struct rte_bus_conf conf;/**< Bus configuration */ rte_bus_get_iommu_class_t get_iommu_class;/**< Get iommu class */ rte_bus_ctrl_get_t ctrl;/**< Get control operators */ };
Signature · 1 line
2.1.4
PATCH 4/8 bus: add probe mode setter
In reply to Gaetan RivetIntroduce new rte_bus operation to configure the probe policy.
Implementation is required from buses interested in supporting this configuration element.
--- lib/librte_eal/common/eal_common_bus.c | 24 ++++++++++++++++++++++++ lib/librte_eal/common/eal_common_options.c | 17 ++++------------- lib/librte_eal/common/include/rte_bus.h | 16 ++++++++++++++++ 3 files changed, 44 insertions(+), 13 deletions(-)
lib/librte_eal/common/eal_common_bus.c +24 -0
@@ -120,6 +120,30 @@ rte_bus_probe(void)return 0 ; } +/* Configure the probing policy of a bus */ +int +rte_bus_probe_mode_set (const char *busname, +enum rte_bus_probe_mode mode) +{ +struct rte_bus *bus; +rte_bus_ctrl_t probe_mode_set; + + bus =rte_bus_find_by_name (busname); +if (bus ==NULL ) { +RTE_LOG (ERR ,EAL ,"Bus %s not found.\n" , + busname); +return -EFAULT ; + } + probe_mode_set = bus->ctrl (RTE_BUS_CTRL_SET , +RTE_BUS_CTRL_PROBE_MODE ); +if (probe_mode_set ==NULL ) { +RTE_LOG (ERR ,EAL ,"Bus %s: probe policy cannot be configured.\n" , + busname); +return -ENOTSUP ; + } +return probe_mode_set (&mode); +} +/* Dump information of a single bus */ static int bus_dump_one (FILE *f,struct rte_bus *bus)
lib/librte_eal/common/eal_common_options.c +4 -13
@@ -997,29 +997,24 @@ int eal_parse_common_option(int opt, const char *optarg, struct internal_config *conf) { - static int b_used; -static int w_used; - switch (opt) { /* blacklist */ case 'b': -if (w_used) - goto bw_used; + if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_BLACKLIST) < 0) + return -1; if (eal_option_device_add(RTE_DEVTYPE_BLACKLISTED_PCI, optarg) < 0) { return -1; } - b_used =1 ; break; /* whitelist */ case 'w': -if (b_used) - goto bw_used; + if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_WHITELIST) < 0) + return -1; if (eal_option_device_add(RTE_DEVTYPE_WHITELISTED_PCI, optarg) < 0) { return -1; } - w_used =1 ; break; /* coremask */ case 'c':
@@ -1165,10 +1160,6 @@ eal_parse_common_option(int opt, const char *optarg, }return 0 ; -bw_used: -RTE_LOG (ERR ,EAL ,"Options blacklist (-b) and whitelist (-w) " -"cannot be used at the same time\n" ); -return -1 ; } static void
lib/librte_eal/common/include/rte_bus.h +16 -0
@@ -289,6 +289,22 @@ int rte_bus_scan(void);int rte_bus_probe (void );/** +* Configure bus probe policy. +* +* @param busname +* Name of the bus to configure. +* +* @param mode +* Configure the designated bus probe policy to this value. +* +* @return +* 0 on success +* !0 otherwise +*/ +int rte_bus_probe_mode_set (const char *busname, +enum rte_bus_probe_mode mode); + +/** * Dump information of all the buses registered with EAL. * * @param f
Signature · 1 line
2.1.4
Re: [PATCH v1 4/8] bus: add probe mode setter
Quoted 5 lines · Gaetan Rivet
Introduce new
rte_busoperation to configure the probe policy.Implementation is required from buses interested in supporting this configuration element.
[...]
Quoted 8 lines
lib/librte_eal/common/eal_common_options.c +0
-0@@ -997,29 +997,24 @@ int eal_parse_common_option(int opt, const char *optarg, struct internal_config *conf) {
lib/librte_eal/common/eal_common_options.c int · line 1004
Quoted 10 lines
lib/librte_eal/common/eal_common_options.c +2
-5-static int b_used; -static int w_used; -switch (opt) {/* blacklist */ case 'b' : -if (w_used) - goto bw_used; +if (rte_bus_probe_mode_set ("pci" ,RTE_BUS_PROBE_BLACKLIST ) <0 ) +return -1 ;
Generic layer shouldn't be concerned about "pci" or other bus. Problem would be to find which bus this option needs to be set.
What I can think of as options is:
1. Storing this configuration until we can parse the argument for which <bus> the argument has been created. That would mean changing the way the "-b" and "-w" are passed and to allow non-PCI device identifier to be passed.
2. Call each bus bus->ctrl and let it decide what to do based on the args.
- so, have a wrapper over rte_bus_probe_mode_set for all buses rather
than taking any one bus as option.
(2) sounds most plausible for now as the application will not send the bus name as argument.
And if brute force is not required, we need to split the argument to know the bus - after making the device naming standardized (<bus>:<...>)
Even before that, we need to agree that "-w' and "-b" are not more valid only for PCIs.
Above is more of loud thinging - I don't have concrete thought on this for now. I'll revisit this after reviewing the patches in this series.
Quoted 21 lines
lib/librte_eal/common/eal_common_options.c +2
-4if (eal_option_device_add (RTE_DEVTYPE_BLACKLISTED_PCI , optarg) <0 ) {return -1 ; } - b_used =1 ;break ;/* whitelist */ case 'w' : -if (b_used) - goto bw_used; +if (rte_bus_probe_mode_set ("pci" ,RTE_BUS_PROBE_WHITELIST ) <0 ) +return -1 ;if (eal_option_device_add (RTE_DEVTYPE_WHITELISTED_PCI , optarg) <0 ) {return -1 ; } - w_used =1 ;break ;/* coremask */ case 'c' :@@ -1165,10 +1160,6 @@ eal_parse_common_option(int opt, const char *optarg,
[...]
Re: [PATCH v1 4/8] bus: add probe mode setter
Quoted 37 lines · Shreyansh Jain
Quoted 5 lines · Gaetan Rivet
Introduce new
rte_busoperation to configure the probe policy.Implementation is required from buses interested in supporting this configuration element.
[...]
Quoted 13 lines
lib/librte_eal/common/eal_common_options.c +0
-3@@ -997,29 +997,24 @@ int eal_parse_common_option(int opt, const char *optarg, struct internal_config *conf) { - static int b_used; - static int w_used; - switch (opt) { /* blacklist */Quoted 5 lines
case 'b':
- if (w_used)
- gotobw_used;
+ if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_BLACKLIST)< 0) + return -1;Generic layer shouldn't be concerned about "pci" or other bus. Problem would be to find which bus this option needs to be set.
What I can think of as options is:
1. Storing this configuration until we can parse the argument for which <bus> the argument has been created. That would mean changing the way the "-b" and "-w" are passed and to allow non-PCI device identifier to be passed.
2. Call each busbus->ctrland let it decide what to do based on the args.
- so, have a wrapper overrte_bus_probe_mode_setfor all buses rather
than taking any one bus as option.
Quoted 6 lines
(2) sounds most plausible for now as the application will not send the bus name as argument.
And if brute force is not required, we need to split the argument to know the bus - after making the device naming standardized (<bus>:<...>)Even before that, we need to agree that "-w' and "-b" are not more valid only for PCIs.
Above is more of loud thinging - I don't have concrete thought on this for now. I'll revisit this after reviewing the patches in this series.
Apologies. I see that some work has been done for this in devargs series - I was too quick to reply on this. Let me come back to you on this after reading through that series.
Quoted 26 lines
Quoted 23 lines
if (
eal_option_device_add(RTE_DEVTYPE_BLACKLISTED_PCI, optarg)< 0) {
return -1;
}
-b_used= 1;
break;
/* whitelist */
case 'w':
- if (b_used)
- gotobw_used;
+ if (rte_bus_probe_mode_set("pci", RTE_BUS_PROBE_WHITELIST)< 0) + return -1;
if (eal_option_device_add(RTE_DEVTYPE_WHITELISTED_PCI, optarg)< 0) {
return -1;
}
-w_used= 1;
break;
/* coremask */
case 'c':lib/librte_eal/common/eal_common_options.c +0
-0@@ -1165,10 +1160,6 @@ eal_parse_common_option(int opt, const char *optarg,[...]
PATCH 5/8 bus/pci: implement ctrl operator
In reply to Gaetan RivetAdd the PCI bus control operator.
This operator gives access to the probe policy setting, allowing to read and write this configuration item. The previous existing functionality is thus restored to the same level.
Probe policy is blacklist mode by default for the PCI bus. Configuration is allowed once, and is then considered immutable.
--- drivers/bus/pci/include/rte_bus_pci.h | 1 + drivers/bus/pci/pci_common.c | 52 ++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 1 deletion(-)
drivers/bus/pci/include/rte_bus_pci.h +1 -0
@@ -155,6 +155,7 @@ struct rte_pci_driver {*/ struct rte_pci_bus {struct rte_bus bus;/**< Inherit the generic class */ +enum rte_bus_probe_mode probe_mode;/**< Probe policy */ struct rte_pci_device_list device_list;/**< List of PCI devices */ struct rte_pci_driver_list driver_list;/**< List of PCI drivers */ };
drivers/bus/pci/pci_common.c +51 -1
73 lines not on this page · raw message
Signature · 1 line
2.1.4
PATCH 6/8 bus: add IOVA mode as a ctrl operation
In reply to Gaetan RivetLeverage the new bus control framework for the IOVA mode configuration item.
The previous version is left for the transition in drivers implementation and will be removed afterward.
--- lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 - lib/librte_eal/common/eal_common_bus.c | 24 +++++++++++------ lib/librte_eal/common/include/rte_bus.h | 34 ++++++++++++++----------- lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 - 4 files changed, 35 insertions(+), 25 deletions(-)
lib/librte_eal/bsdapp/eal/rte_eal_version.map +0 -1
@@ -228,7 +228,6 @@ DPDK_17.11 { rte_eal_iova_mode; rte_eal_mbuf_default_mempool_ops; rte_lcore_has_role; - rte_pci_get_iommu_class; rte_pci_match; } DPDK_17.08;
lib/librte_eal/common/eal_common_bus.c +16 -8
@@ -258,18 +258,26 @@ rte_bus_find_by_device_name(const char *str)enum rte_iova_mode rte_bus_get_iommu_class (void ) { -int mode =RTE_IOVA_DC ; +enum rte_iova_mode result;struct rte_bus *bus; + result =RTE_IOVA_DC ; TAILQ_FOREACH(bus, &rte_bus_list, next) { +rte_bus_ctrl_t iova_mode_get; +enum rte_iova_mode mode; -if (bus->get_iommu_class) - mode |= bus->get_iommu_class (); + iova_mode_get = bus->ctrl (RTE_BUS_CTRL_GET , +RTE_BUS_CTRL_IOVA_MODE ); +if (iova_mode_get !=NULL ) { +if (iova_mode_get (&mode)) +RTE_LOG (ERR ,EAL ,"Bus %s: error reading IOMMU class\n" , + bus->name); +else + result |= mode; + } } - -if (mode !=RTE_IOVA_VA ) { +if (result !=RTE_IOVA_VA )/* Use default IOVA mode */ - mode =RTE_IOVA_PA ; - } -return mode; + result =RTE_IOVA_PA ; +return result; }
lib/librte_eal/common/include/rte_bus.h +19 -15
@@ -55,21 +55,6 @@ extern "C" {/** Double linked list of buses */ TAILQ_HEAD (rte_bus_list, rte_bus); - -/** -* IOVA mapping mode. -* -* IOVA mapping mode is iommu programming mode of a device. -* That device (for example: IOMMU backed DMA device) based -* on rte_iova_mode will generate physical or virtual address. -* -*/ -enum rte_iova_mode { -RTE_IOVA_DC =0 ,/* Don't care mode */ -RTE_IOVA_PA = (1 <<0 ),/* DMA using physical address */ -RTE_IOVA_VA = (1 <<1 )/* DMA using virtual address */ -}; -/** * Bus specific scan for devices attached on the bus. * For each bus object, the scan would be responsible for finding devices and
@@ -177,10 +162,29 @@ enum rte_bus_probe_mode { };/** +* IOVA mapping mode. +* +* IOVA mapping mode is iommu programming mode of a device. +* That device (for example: IOMMU backed DMA device) based +* on rte_iova_mode will generate physical or virtual address. +* +*/ +enum rte_iova_mode { +RTE_IOVA_DC =0 ,/* Don't care mode */ +RTE_IOVA_PA = (1 <<0 ),/* DMA using physical address */ +RTE_IOVA_VA = (1 <<1 )/* DMA using virtual address */ +}; + +/** * Bus configuration items. */ enum rte_bus_ctrl_item {RTE_BUS_CTRL_PROBE_MODE =0 , +/** +* IOMMU class common to all devices on the bus. +* If irrelevant, the bus may return RTE_IOVA_DC. +*/ +RTE_BUS_CTRL_IOVA_MODE ,RTE_BUS_CTRL_ITEM_MAX , };
lib/librte_eal/linuxapp/eal/rte_eal_version.map +0 -1
@@ -232,7 +232,6 @@ DPDK_17.11 { rte_eal_iova_mode; rte_eal_mbuf_default_mempool_ops; rte_lcore_has_role; - rte_pci_get_iommu_class; rte_pci_match; } DPDK_17.08;
Signature · 1 line
2.1.4
PATCH 7/8 bus/pci: implement IOVA mode getter
In reply to Gaetan RivetImplement the ctrl operator for the IOVA mode configuration item. The previous functionality is kept identical, only the new control framework is used.
All operators are made private as there is no reason to expose them.
--- drivers/bus/pci/bsd/pci.c | 9 ++++++--- drivers/bus/pci/include/rte_bus_pci.h | 11 ----------- drivers/bus/pci/linux/pci.c | 20 +++++++++++++------- drivers/bus/pci/pci_common.c | 4 +++- drivers/bus/pci/private.h | 13 +++++++++++++ 5 files changed, 35 insertions(+), 22 deletions(-)
drivers/bus/pci/bsd/pci.c +6 -3
@@ -407,11 +407,14 @@ rte_pci_scan(void)/* * Get iommu class of PCI devices on the bus. */ -enum rte_iova_mode -rte_pci_get_iommu_class (void ) +int +pci_iommu_class_get (void *_mode) { +enum rte_iova_mode *mode = _mode; +/* Supports only RTE_KDRV_NIC_UIO */ -return RTE_IOVA_PA ; + *mode =RTE_IOVA_PA ; +return 0 ; }int
drivers/bus/pci/include/rte_bus_pci.h +0 -11
@@ -205,17 +205,6 @@ int rte_pci_match(const struct rte_pci_driver *pci_drv, const struct rte_pci_device *pci_dev); - -/** -* Get iommu class of PCI devices on the bus. -* And return their preferred iova mapping mode. -* -* @return -* - enum rte_iova_mode. -*/ -enum rte_iova_mode -rte_pci_get_iommu_class (void ); -/** * Map the PCI device resources in user space virtual memory address *
drivers/bus/pci/linux/pci.c +13 -7
@@ -553,17 +553,20 @@ pci_one_device_has_iova_va(void)/* * Get iommu class of PCI devices on the bus. */ -enum rte_iova_mode -rte_pci_get_iommu_class (void ) +int +pci_iommu_class_get (void *_mode) { +enum rte_iova_mode *mode = _mode;bool is_bound;bool is_vfio_noiommu_enabled = true;bool has_iova_va;bool is_bound_uio; is_bound =pci_one_device_is_bound (); -if (!is_bound) -return RTE_IOVA_DC ; +if (!is_bound) { + *mode =RTE_IOVA_DC ; +return 0 ; + } has_iova_va =pci_one_device_has_iova_va (); is_bound_uio =pci_one_device_bound_uio ();
@@ -572,8 +575,10 @@ rte_pci_get_iommu_class(void) true : false;#endif -if (has_iova_va && !is_bound_uio && !is_vfio_noiommu_enabled) -return RTE_IOVA_VA ; +if (has_iova_va && !is_bound_uio && !is_vfio_noiommu_enabled) { + *mode =RTE_IOVA_VA ; +return 0 ; + }if (has_iova_va) {RTE_LOG (WARNING ,EAL ,"Some devices want iova as va but pa will be used because.. " );
@@ -583,7 +588,8 @@ rte_pci_get_iommu_class(void)RTE_LOG (WARNING ,EAL ,"few device bound to UIO\n" ); } -return RTE_IOVA_PA ; + *mode =RTE_IOVA_PA ; +return 0 ; }/* Read PCI config space. */
drivers/bus/pci/pci_common.c +3 -1
@@ -550,6 +550,9 @@ static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = { [RTE_BUS_CTRL_GET] = pci_probe_mode_get, [RTE_BUS_CTRL_SET] = pci_probe_mode_set, }, + [RTE_BUS_CTRL_IOVA_MODE] = { + [RTE_BUS_CTRL_GET] = pci_iommu_class_get, + }, }; static rte_bus_ctrl_t
@@ -571,7 +574,6 @@ struct rte_pci_bus rte_pci_bus = { .plug = pci_plug, .unplug = pci_unplug, .parse = pci_parse, - .get_iommu_class = rte_pci_get_iommu_class, .ctrl = pci_ctrl, }, .probe_mode = RTE_BUS_PROBE_BLACKLIST,
drivers/bus/pci/private.h +13 -0
@@ -171,4 +171,17 @@ void pci_uio_free_resource(struct rte_pci_device *dev,int pci_uio_map_resource_by_index (struct rte_pci_device *dev,int res_idx,struct mapped_pci_resource *uio_res,int map_idx); +/** +* Get iommu class of PCI devices on the bus. +* Return their preferred iova mapping mode. +* +* @param _mode +* Generic address to an (enum rte_iova_mode) +* @return +* 0 on success +* !0 otherwise +*/ +int +pci_iommu_class_get (void *_mode); +#endif /* _PCI_PRIVATE_H_ */
Signature · 1 line
2.1.4
PATCH 8/8 bus: remove redundant IOVA mode getter
In reply to Gaetan RivetThis configuration element is now accessible through the bus control framework and can be removed from the generic bus structure.
--- lib/librte_eal/common/include/rte_bus.h | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-)
lib/librte_eal/common/include/rte_bus.h +9 -24
@@ -227,19 +227,6 @@ typedef rte_bus_ctrl_t (*rte_bus_ctrl_get_t)(enum rte_bus_ctrl_op op, enum rte_bus_ctrl_item item);/** -* Get common iommu class of the all the devices on the bus. The bus may -* check that those devices are attached to iommu driver. -* If no devices are attached to the bus. The bus may return with don't care -* (_DC) value. -* Otherwise, The bus will return appropriate _pa or _va iova mode. -* -* @return -* enum rte_iova_mode value. -*/ -typedef enum rte_iova_mode (*rte_bus_get_iommu_class_t )(void ); - - -/** * A structure describing a generic bus. */ struct rte_bus {
@@ -251,7 +238,6 @@ struct rte_bus {rte_bus_plug_t plug;/**< Probe single device for drivers */ rte_bus_unplug_t unplug;/**< Remove single device from driver */ rte_bus_parse_t parse;/**< Parse a device name */ -rte_bus_get_iommu_class_t get_iommu_class;/**< Get iommu class */ rte_bus_ctrl_get_t ctrl;/**< Get control operators */ };
@@ -309,6 +295,15 @@ int rte_bus_probe_mode_set(const char *busname, enum rte_bus_probe_mode mode);/** +* Get the common iommu class of devices bound on to buses available in the +* system. The default mode is PA. +* +* @return +* enum rte_iova_mode value. +*/ +enum rte_iova_mode rte_bus_get_iommu_class (void ); + +/** * Dump information of all the buses registered with EAL. * * @param f
@@ -368,16 +363,6 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);*/ struct rte_bus *rte_bus_find_by_name (const char *busname); - -/** -* Get the common iommu class of devices bound on to buses available in the -* system. The default mode is PA. -* -* @return -* enum rte_iova_mode value. -*/ -enum rte_iova_mode rte_bus_get_iommu_class (void ); -/** * Helper for Bus registration. * The constructor has higher priority than PMD constructors.
Signature · 1 line
2.1.4
Hello Gaetan,
(I am assuming that this series is still valid for 18.02 and you will spin a new version of this.)
Quoted 15 lines · Gaetan Rivet
Probing policy was introduced in the previous release as a configuration item. It was thus added to the generic bus structure, breaking its ABI.
In this release, the IOVA mode can be read from a bus to configure the EAL. This new configuration element also broke the bus ABI when it was added.
As new operators had to be implemented for the probe policy item, these patches were developed to help mitigate this issue.
This control framework allows to expand the
rte_busAPI without breaking its ABI. It is meant to be used with configuration elements that may only be valid for a few buses, while the others would remain untouched and unaware of the evolution.
Quoted 4 lines
A central control operator is used, similarly to the working of
rte_flowAPI in the ether layer. Each driver thus chooses to expose a set of operators relevant to its implementation. The caller is then free to use those if they are available.
I like the overall idea - similar to an ioctl.
It would help extend the control knobs of buses (drivers?) without adding additional dependency on ABI/API.
+1
Quoted 2 lines
Both Probe mode and IOVA mode operators are implemented for the PCI bus. >
[...]