| Message ID | 20200611213748.1967029-4-maxime.coquelin@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series |
|
| Related | show |
| Context | Check | Description |
|---|---|---|
| ci/Intel-compilation | success | Compilation OK |
| ci/checkpatch | success | coding style OK |
On 11/06/20 23:37 +0200, Maxime Coquelin wrote: > This patch introduces vDPA device class. It will enable > application to iterate over the vDPA devices. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > lib/librte_vhost/vdpa.c | 116 +++++++++++++++++++++++++++++++++------- > 1 file changed, 98 insertions(+), 18 deletions(-) > > diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c > index b2b2a105f1..61ab9aadb4 100644 > --- a/lib/librte_vhost/vdpa.c > +++ b/lib/librte_vhost/vdpa.c > @@ -10,11 +10,12 @@ > > #include <stdbool.h> > > +#include <rte_class.h> > #include <rte_malloc.h> > #include "rte_vdpa.h" > #include "vhost.h" > > -static struct rte_vdpa_device *vdpa_devices[MAX_VHOST_DEVICE]; > +static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE]; > static uint32_t vdpa_device_num; > > static bool > @@ -46,35 +47,28 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, > struct rte_vdpa_dev_ops *ops) > { > struct rte_vdpa_device *dev; > - char device_name[MAX_VDPA_NAME_LEN]; > int i; > > if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL) > return -1; > > for (i = 0; i < MAX_VHOST_DEVICE; i++) { > - dev = vdpa_devices[i]; > - if (dev && is_same_vdpa_device(&dev->addr, addr)) > + dev = &vdpa_devices[i]; > + if (dev->ops && is_same_vdpa_device(&dev->addr, addr)) > return -1; > } > > for (i = 0; i < MAX_VHOST_DEVICE; i++) { > - if (vdpa_devices[i] == NULL) > + if (vdpa_devices[i].ops == NULL) > break; > } > > if (i == MAX_VHOST_DEVICE) > return -1; > > - snprintf(device_name, sizeof(device_name), "vdpa-dev-%d", i); > - dev = rte_zmalloc(device_name, sizeof(struct rte_vdpa_device), > - RTE_CACHE_LINE_SIZE); > - if (!dev) > - return -1; > - > + dev = &vdpa_devices[i]; > memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr)); > dev->ops = ops; > - vdpa_devices[i] = dev; > vdpa_device_num++; > > return i; > @@ -83,11 +77,10 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, > int > rte_vdpa_unregister_device(int did) > { > - if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did] == NULL) > + if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL) > return -1; > > - rte_free(vdpa_devices[did]); > - vdpa_devices[did] = NULL; > + memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device)); > vdpa_device_num--; > > return did; > @@ -103,8 +96,11 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr) > return -1; > > for (i = 0; i < MAX_VHOST_DEVICE; ++i) { > - dev = vdpa_devices[i]; > - if (dev && is_same_vdpa_device(&dev->addr, addr)) > + dev = &vdpa_devices[i]; > + if (dev->ops == NULL) > + continue; > + > + if (is_same_vdpa_device(&dev->addr, addr)) > return i; > } > > @@ -117,7 +113,7 @@ rte_vdpa_get_device(int did) > if (did < 0 || did >= MAX_VHOST_DEVICE) > return NULL; > > - return vdpa_devices[did]; > + return &vdpa_devices[did]; > } > > int > @@ -227,3 +223,87 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m) > free_ind_table(idesc); > return -1; > } > + > +static uint16_t > +vdpa_dev_to_id(const struct rte_vdpa_device *dev) > +{ > + if (dev == NULL) > + return MAX_VHOST_DEVICE; I know it comes from ethdev, this helper was introduced by 214ed1acd12 and changed the comparison type from ptrdiff_t to uin16_t. This is incorrect, ideally it should instead be a ptrdiff_t, then if within acceptable range it should be casted to uin16_t to be used as an index. > + return dev - vdpa_devices; > +} > + > +static int > +vdpa_dev_match(struct rte_vdpa_device *dev, > + const struct rte_device *rte_dev) > +{ > + struct rte_vdpa_dev_addr addr; > + > + /* Only PCI bus supported for now */ > + if (strcmp(rte_dev->bus->name, "pci") != 0) > + return -1; > + > + addr.type = VDPA_ADDR_PCI; > + > + if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0) > + return -1; > + > + if (!is_same_vdpa_device(&dev->addr, &addr)) > + return -1; > + > + return 0; > +} > + > +/* Generic rte_vdpa_dev comparison function. */ > +typedef int (*rte_vdpa_cmp_t)(struct rte_vdpa_device *, > + const struct rte_device *rte_dev); > + > +static struct rte_vdpa_device * > +vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp, > + struct rte_device *rte_dev) > +{ > + struct rte_vdpa_device *dev; > + ptrdiff_t idx; The ptrdiff_t was used in ethdev due to the original start - &rte_eth_devices[0] + 1; The diff was removed by commit 214ed1acd12 . 'idx' should be the proper type to hold a device id, here probably uint16_t. Ethdev class should be cleaned up as well. > + > + /* Avoid Undefined Behaviour */ > + if (start != NULL && > + (start < &vdpa_devices[0] || > + start >= &vdpa_devices[MAX_VHOST_DEVICE])) > + return NULL; Same as before, this guard made sense mostly for the diff following. It is coupled to the "random ptr to static array" comparison happening, it would be better in vpda_dev_to_id() I think. > + > + if (start != NULL) > + idx = vdpa_dev_to_id(start) + 1; > + else > + idx = 0; > + for (; idx < MAX_VHOST_DEVICE; idx++) { > + dev = &vdpa_devices[idx]; > + /* > + * ToDo: Certainly better to introduce a state field, > + * but rely on ops being set for now. > + */ > + if (dev->ops == NULL) > + continue; > + if (cmp(dev, rte_dev) == 0) > + return dev; > + } > + return NULL; > +} > + > +static void * > +vdpa_dev_iterate(const void *start, > + const char *str, > + const struct rte_dev_iterator *it) > +{ > + struct rte_vdpa_device *vdpa_dev = NULL; > + > + RTE_SET_USED(str); > + > + vdpa_dev = vdpa_find_device(start, vdpa_dev_match, it->device); > + > + return vdpa_dev; > +} > + > +static struct rte_class rte_class_vdpa = { > + .dev_iterate = vdpa_dev_iterate, > +}; > + > +RTE_REGISTER_CLASS(vdpa, rte_class_vdpa); > -- > 2.26.2 >
On 6/12/20 2:37 PM, Gaëtan Rivet wrote: > On 11/06/20 23:37 +0200, Maxime Coquelin wrote: >> This patch introduces vDPA device class. It will enable >> application to iterate over the vDPA devices. >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> lib/librte_vhost/vdpa.c | 116 +++++++++++++++++++++++++++++++++------- >> 1 file changed, 98 insertions(+), 18 deletions(-) >> >> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c >> index b2b2a105f1..61ab9aadb4 100644 >> --- a/lib/librte_vhost/vdpa.c >> +++ b/lib/librte_vhost/vdpa.c >> @@ -10,11 +10,12 @@ >> >> #include <stdbool.h> >> >> +#include <rte_class.h> >> #include <rte_malloc.h> >> #include "rte_vdpa.h" >> #include "vhost.h" >> >> -static struct rte_vdpa_device *vdpa_devices[MAX_VHOST_DEVICE]; >> +static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE]; >> static uint32_t vdpa_device_num; >> >> static bool >> @@ -46,35 +47,28 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, >> struct rte_vdpa_dev_ops *ops) >> { >> struct rte_vdpa_device *dev; >> - char device_name[MAX_VDPA_NAME_LEN]; >> int i; >> >> if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL) >> return -1; >> >> for (i = 0; i < MAX_VHOST_DEVICE; i++) { >> - dev = vdpa_devices[i]; >> - if (dev && is_same_vdpa_device(&dev->addr, addr)) >> + dev = &vdpa_devices[i]; >> + if (dev->ops && is_same_vdpa_device(&dev->addr, addr)) >> return -1; >> } >> >> for (i = 0; i < MAX_VHOST_DEVICE; i++) { >> - if (vdpa_devices[i] == NULL) >> + if (vdpa_devices[i].ops == NULL) >> break; >> } >> >> if (i == MAX_VHOST_DEVICE) >> return -1; >> >> - snprintf(device_name, sizeof(device_name), "vdpa-dev-%d", i); >> - dev = rte_zmalloc(device_name, sizeof(struct rte_vdpa_device), >> - RTE_CACHE_LINE_SIZE); >> - if (!dev) >> - return -1; >> - >> + dev = &vdpa_devices[i]; >> memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr)); >> dev->ops = ops; >> - vdpa_devices[i] = dev; >> vdpa_device_num++; >> >> return i; >> @@ -83,11 +77,10 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, >> int >> rte_vdpa_unregister_device(int did) >> { >> - if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did] == NULL) >> + if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL) >> return -1; >> >> - rte_free(vdpa_devices[did]); >> - vdpa_devices[did] = NULL; >> + memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device)); >> vdpa_device_num--; >> >> return did; >> @@ -103,8 +96,11 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr) >> return -1; >> >> for (i = 0; i < MAX_VHOST_DEVICE; ++i) { >> - dev = vdpa_devices[i]; >> - if (dev && is_same_vdpa_device(&dev->addr, addr)) >> + dev = &vdpa_devices[i]; >> + if (dev->ops == NULL) >> + continue; >> + >> + if (is_same_vdpa_device(&dev->addr, addr)) >> return i; >> } >> >> @@ -117,7 +113,7 @@ rte_vdpa_get_device(int did) >> if (did < 0 || did >= MAX_VHOST_DEVICE) >> return NULL; >> >> - return vdpa_devices[did]; >> + return &vdpa_devices[did]; >> } >> >> int >> @@ -227,3 +223,87 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m) >> free_ind_table(idesc); >> return -1; >> } >> + >> +static uint16_t >> +vdpa_dev_to_id(const struct rte_vdpa_device *dev) >> +{ >> + if (dev == NULL) >> + return MAX_VHOST_DEVICE; > > I know it comes from ethdev, this helper was introduced by 214ed1acd12 > and changed the comparison type from ptrdiff_t to uin16_t. > > This is incorrect, ideally it should instead be a ptrdiff_t, then > if within acceptable range it should be casted to uin16_t to be used as > an index. > >> + return dev - vdpa_devices; >> +} >> + >> +static int >> +vdpa_dev_match(struct rte_vdpa_device *dev, >> + const struct rte_device *rte_dev) >> +{ >> + struct rte_vdpa_dev_addr addr; >> + >> + /* Only PCI bus supported for now */ >> + if (strcmp(rte_dev->bus->name, "pci") != 0) >> + return -1; >> + >> + addr.type = VDPA_ADDR_PCI; >> + >> + if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0) >> + return -1; >> + >> + if (!is_same_vdpa_device(&dev->addr, &addr)) >> + return -1; >> + >> + return 0; >> +} >> + >> +/* Generic rte_vdpa_dev comparison function. */ >> +typedef int (*rte_vdpa_cmp_t)(struct rte_vdpa_device *, >> + const struct rte_device *rte_dev); >> + >> +static struct rte_vdpa_device * >> +vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp, >> + struct rte_device *rte_dev) >> +{ >> + struct rte_vdpa_device *dev; >> + ptrdiff_t idx; > > The ptrdiff_t was used in ethdev due to the original > > start - &rte_eth_devices[0] + 1; > > The diff was removed by commit 214ed1acd12 . 'idx' should be > the proper type to hold a device id, here probably uint16_t. Ok, makes sense. > Ethdev class should be cleaned up as well. > >> + >> + /* Avoid Undefined Behaviour */ >> + if (start != NULL && >> + (start < &vdpa_devices[0] || >> + start >= &vdpa_devices[MAX_VHOST_DEVICE])) >> + return NULL; > > Same as before, this guard made sense mostly for the diff following. > It is coupled to the "random ptr to static array" comparison happening, > it would be better in vpda_dev_to_id() I think. OK, I will clean that in next version, but but this part moves away later in the series, when switching to linked-list. Thanks, Maxime >> + >> + if (start != NULL) >> + idx = vdpa_dev_to_id(start) + 1; >> + else >> + idx = 0; >> + for (; idx < MAX_VHOST_DEVICE; idx++) { >> + dev = &vdpa_devices[idx]; >> + /* >> + * ToDo: Certainly better to introduce a state field, >> + * but rely on ops being set for now. >> + */ >> + if (dev->ops == NULL) >> + continue; >> + if (cmp(dev, rte_dev) == 0) >> + return dev; >> + } >> + return NULL; >> +} >> + >> +static void * >> +vdpa_dev_iterate(const void *start, >> + const char *str, >> + const struct rte_dev_iterator *it) >> +{ >> + struct rte_vdpa_device *vdpa_dev = NULL; >> + >> + RTE_SET_USED(str); >> + >> + vdpa_dev = vdpa_find_device(start, vdpa_dev_match, it->device); >> + >> + return vdpa_dev; >> +} >> + >> +static struct rte_class rte_class_vdpa = { >> + .dev_iterate = vdpa_dev_iterate, >> +}; >> + >> +RTE_REGISTER_CLASS(vdpa, rte_class_vdpa); >> -- >> 2.26.2 >> >
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c index b2b2a105f1..61ab9aadb4 100644 --- a/lib/librte_vhost/vdpa.c +++ b/lib/librte_vhost/vdpa.c @@ -10,11 +10,12 @@ #include <stdbool.h> +#include <rte_class.h> #include <rte_malloc.h> #include "rte_vdpa.h" #include "vhost.h" -static struct rte_vdpa_device *vdpa_devices[MAX_VHOST_DEVICE]; +static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE]; static uint32_t vdpa_device_num; static bool @@ -46,35 +47,28 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, struct rte_vdpa_dev_ops *ops) { struct rte_vdpa_device *dev; - char device_name[MAX_VDPA_NAME_LEN]; int i; if (vdpa_device_num >= MAX_VHOST_DEVICE || addr == NULL || ops == NULL) return -1; for (i = 0; i < MAX_VHOST_DEVICE; i++) { - dev = vdpa_devices[i]; - if (dev && is_same_vdpa_device(&dev->addr, addr)) + dev = &vdpa_devices[i]; + if (dev->ops && is_same_vdpa_device(&dev->addr, addr)) return -1; } for (i = 0; i < MAX_VHOST_DEVICE; i++) { - if (vdpa_devices[i] == NULL) + if (vdpa_devices[i].ops == NULL) break; } if (i == MAX_VHOST_DEVICE) return -1; - snprintf(device_name, sizeof(device_name), "vdpa-dev-%d", i); - dev = rte_zmalloc(device_name, sizeof(struct rte_vdpa_device), - RTE_CACHE_LINE_SIZE); - if (!dev) - return -1; - + dev = &vdpa_devices[i]; memcpy(&dev->addr, addr, sizeof(struct rte_vdpa_dev_addr)); dev->ops = ops; - vdpa_devices[i] = dev; vdpa_device_num++; return i; @@ -83,11 +77,10 @@ rte_vdpa_register_device(struct rte_vdpa_dev_addr *addr, int rte_vdpa_unregister_device(int did) { - if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did] == NULL) + if (did < 0 || did >= MAX_VHOST_DEVICE || vdpa_devices[did].ops == NULL) return -1; - rte_free(vdpa_devices[did]); - vdpa_devices[did] = NULL; + memset(&vdpa_devices[did], 0, sizeof(struct rte_vdpa_device)); vdpa_device_num--; return did; @@ -103,8 +96,11 @@ rte_vdpa_find_device_id(struct rte_vdpa_dev_addr *addr) return -1; for (i = 0; i < MAX_VHOST_DEVICE; ++i) { - dev = vdpa_devices[i]; - if (dev && is_same_vdpa_device(&dev->addr, addr)) + dev = &vdpa_devices[i]; + if (dev->ops == NULL) + continue; + + if (is_same_vdpa_device(&dev->addr, addr)) return i; } @@ -117,7 +113,7 @@ rte_vdpa_get_device(int did) if (did < 0 || did >= MAX_VHOST_DEVICE) return NULL; - return vdpa_devices[did]; + return &vdpa_devices[did]; } int @@ -227,3 +223,87 @@ rte_vdpa_relay_vring_used(int vid, uint16_t qid, void *vring_m) free_ind_table(idesc); return -1; } + +static uint16_t +vdpa_dev_to_id(const struct rte_vdpa_device *dev) +{ + if (dev == NULL) + return MAX_VHOST_DEVICE; + return dev - vdpa_devices; +} + +static int +vdpa_dev_match(struct rte_vdpa_device *dev, + const struct rte_device *rte_dev) +{ + struct rte_vdpa_dev_addr addr; + + /* Only PCI bus supported for now */ + if (strcmp(rte_dev->bus->name, "pci") != 0) + return -1; + + addr.type = VDPA_ADDR_PCI; + + if (rte_pci_addr_parse(rte_dev->name, &addr.pci_addr) != 0) + return -1; + + if (!is_same_vdpa_device(&dev->addr, &addr)) + return -1; + + return 0; +} + +/* Generic rte_vdpa_dev comparison function. */ +typedef int (*rte_vdpa_cmp_t)(struct rte_vdpa_device *, + const struct rte_device *rte_dev); + +static struct rte_vdpa_device * +vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp, + struct rte_device *rte_dev) +{ + struct rte_vdpa_device *dev; + ptrdiff_t idx; + + /* Avoid Undefined Behaviour */ + if (start != NULL && + (start < &vdpa_devices[0] || + start >= &vdpa_devices[MAX_VHOST_DEVICE])) + return NULL; + + if (start != NULL) + idx = vdpa_dev_to_id(start) + 1; + else + idx = 0; + for (; idx < MAX_VHOST_DEVICE; idx++) { + dev = &vdpa_devices[idx]; + /* + * ToDo: Certainly better to introduce a state field, + * but rely on ops being set for now. + */ + if (dev->ops == NULL) + continue; + if (cmp(dev, rte_dev) == 0) + return dev; + } + return NULL; +} + +static void * +vdpa_dev_iterate(const void *start, + const char *str, + const struct rte_dev_iterator *it) +{ + struct rte_vdpa_device *vdpa_dev = NULL; + + RTE_SET_USED(str); + + vdpa_dev = vdpa_find_device(start, vdpa_dev_match, it->device); + + return vdpa_dev; +} + +static struct rte_class rte_class_vdpa = { + .dev_iterate = vdpa_dev_iterate, +}; + +RTE_REGISTER_CLASS(vdpa, rte_class_vdpa);
This patch introduces vDPA device class. It will enable application to iterate over the vDPA devices. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vdpa.c | 116 +++++++++++++++++++++++++++++++++------- 1 file changed, 98 insertions(+), 18 deletions(-)