| Message ID | 73663851c5223b99ed0f23a163a0d44cba0ebe29.1667906228.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Virtio toolstack support for I2C and GPIO on Arm | expand |
On 08.11.22 13:23, Viresh Kumar wrote: Hello Viresh [sorry for the possible format issues if any] > This patch adds basic support for parsing generic Virtio backend. > > An example of domain configuration for mmio based Virtio I2C device is: > virtio = ["type=virtio,device22,transport=mmio"] > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > tools/ocaml/libs/xl/genwrap.py | 1 + > tools/ocaml/libs/xl/xenlight_stubs.c | 1 + > tools/xl/xl_parse.c | 84 ++++++++++++++++++++++++++++ > 3 files changed, 86 insertions(+) > > diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py > index 7bf26bdcd831..b188104299b1 100644 > --- a/tools/ocaml/libs/xl/genwrap.py > +++ b/tools/ocaml/libs/xl/genwrap.py > @@ -36,6 +36,7 @@ DEVICE_LIST = [ ("list", ["ctx", "domid", "t list"]), > functions = { # ( name , [type1,type2,....] ) > "device_vfb": DEVICE_FUNCTIONS, > "device_vkb": DEVICE_FUNCTIONS, > + "device_virtio": DEVICE_FUNCTIONS, > "device_disk": DEVICE_FUNCTIONS + DEVICE_LIST + > [ ("insert", ["ctx", "t", "domid", "?async:'a", "unit", "unit"]), > ("of_vdev", ["ctx", "domid", "string", "t"]), > diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c > index 45b8af61c74a..8e54f95da7c7 100644 > --- a/tools/ocaml/libs/xl/xenlight_stubs.c > +++ b/tools/ocaml/libs/xl/xenlight_stubs.c > @@ -707,6 +707,7 @@ DEVICE_ADDREMOVE(disk) > DEVICE_ADDREMOVE(nic) > DEVICE_ADDREMOVE(vfb) > DEVICE_ADDREMOVE(vkb) > +DEVICE_ADDREMOVE(virtio) > DEVICE_ADDREMOVE(pci) > _DEVICE_ADDREMOVE(disk, cdrom, insert) > > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c > index 1b5381cef033..c6f35c069d2a 100644 > --- a/tools/xl/xl_parse.c > +++ b/tools/xl/xl_parse.c > @@ -1208,6 +1208,87 @@ static void parse_vkb_list(const XLU_Config *config, > if (rc) exit(EXIT_FAILURE); > } > > +static int parse_virtio_config(libxl_device_virtio *virtio, char *token) > +{ > + char *oparg; > + int rc; > + > + if (MATCH_OPTION("backend", token, oparg)) { > + virtio->backend_domname = strdup(oparg); > + } else if (MATCH_OPTION("type", token, oparg)) { > + virtio->type = strdup(oparg); > + } else if (MATCH_OPTION("transport", token, oparg)) { > + rc = libxl_virtio_transport_from_string(oparg, &virtio->transport); > + if (rc) return rc; > + } else if (MATCH_OPTION("irq", token, oparg)) { > + virtio->irq = strtoul(oparg, NULL, 0); > + } else if (MATCH_OPTION("base", token, oparg)) { > + virtio->base = strtoul(oparg, NULL, 0); Interesting, I see you allow user to configure virtio-mmio params (irq and base), as far as I remember for virtio-disk these are internal only (allocated by tools/libs/light/libxl_arm.c). I am not really sure why we need to configure virtio "base", could you please clarify? But if we really want/need to be able to configure virtio "irq" (for example to avoid possible clashing with physical one), I am afraid, this will require more changes that current patch does. Within current series saving virtio->irq here doesn't have any effect as it will be overwritten in libxl__arch_domain_prepare_config()->alloc_virtio_mmio_params() anyway. I presume the code in libxl__arch_domain_prepare_config() shouldn't try to allocate virtio->irq if it is already configured by user, also the allocator should probably take into the account of what is already configured by user, to avoid allocating the same irq for another device assigned for the same guest. Also doc change in the subsequent patch doesn't mention about irq/base configuration. So maybe we should just drop for now? + } else if (MATCH_OPTION("irq", token, oparg)) { + virtio->irq = strtoul(oparg, NULL, 0); + } else if (MATCH_OPTION("base", token, oparg)) { + virtio->base = strtoul(oparg, NULL, 0); > + } else { > + fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token); > + return -1; > + } > + > + return 0; > +} > + > +static void parse_virtio_list(const XLU_Config *config, > + libxl_domain_config *d_config) > +{ > + XLU_ConfigList *virtios; > + const char *item; > + char *buf = NULL, *oparg, *str = NULL; > + int rc; > + > + if (!xlu_cfg_get_list (config, "virtio", &virtios, 0, 0)) { > + int entry = 0; > + while ((item = xlu_cfg_get_listitem(virtios, entry)) != NULL) { > + libxl_device_virtio *virtio; > + char *p; > + > + virtio = ARRAY_EXTEND_INIT(d_config->virtios, d_config->num_virtios, > + libxl_device_virtio_init); > + > + buf = strdup(item); > + > + p = strtok(buf, ","); > + while (p != NULL) > + { > + while (*p == ' ') p++; > + > + // Type may contain a comma, do special handling. > + if (MATCH_OPTION("type", p, oparg)) { > + if (!strncmp(oparg, "virtio", strlen("virtio"))) { > + char *p2 = strtok(NULL, ","); > + str = malloc(strlen(p) + strlen(p2) + 2); > + > + strcpy(str, p); > + strcat(str, ","); > + strcat(str, p2); > + p = str; > + } > + } > + > + rc = parse_virtio_config(virtio, p); > + if (rc) goto out; > + > + free(str); > + str = NULL; > + p = strtok(NULL, ","); > + } > + > + entry++; > + free(buf); > + } > + } > + > + return; > + > +out: > + free(buf); > + if (rc) exit(EXIT_FAILURE); > +} > + > void parse_config_data(const char *config_source, > const char *config_data, > int config_len, > @@ -2309,8 +2390,10 @@ void parse_config_data(const char *config_source, > > d_config->num_vfbs = 0; > d_config->num_vkbs = 0; > + d_config->num_virtios = 0; > d_config->vfbs = NULL; > d_config->vkbs = NULL; > + d_config->virtios = NULL; > > if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) { > while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) { > @@ -2752,6 +2835,7 @@ void parse_config_data(const char *config_source, > } > > parse_vkb_list(config, d_config); > + parse_virtio_list(config, d_config); > > xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat", > &c_info->xend_suspend_evtchn_compat, 0);
On 02-12-22, 19:16, Oleksandr Tyshchenko wrote: > Interesting, I see you allow user to configure virtio-mmio params (irq and > base), as far as I remember for virtio-disk these are internal only > (allocated by tools/libs/light/libxl_arm.c). It is a mistake. Will drop it.
On 05.12.22 08:20, Viresh Kumar wrote: Hello Viresh > On 02-12-22, 19:16, Oleksandr Tyshchenko wrote: >> Interesting, I see you allow user to configure virtio-mmio params (irq and >> base), as far as I remember for virtio-disk these are internal only >> (allocated by tools/libs/light/libxl_arm.c). > > It is a mistake. Will drop it. ok, good. Please don't forget to add a note to idl file that virtio-mmio params are internal only. libxl_device_virtio = Struct("device_virtio", [ ... # Note that virtio-mmio parameters (irq and base) are for internal # use by libxl and can't be modified. ("irq", uint32), ("base", uint64) ]) >
On Tue, Nov 08, 2022 at 04:53:59PM +0530, Viresh Kumar wrote: > diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py > index 7bf26bdcd831..b188104299b1 100644 > --- a/tools/ocaml/libs/xl/genwrap.py > +++ b/tools/ocaml/libs/xl/genwrap.py > @@ -36,6 +36,7 @@ DEVICE_LIST = [ ("list", ["ctx", "domid", "t list"]), > functions = { # ( name , [type1,type2,....] ) > "device_vfb": DEVICE_FUNCTIONS, > "device_vkb": DEVICE_FUNCTIONS, > + "device_virtio": DEVICE_FUNCTIONS, > "device_disk": DEVICE_FUNCTIONS + DEVICE_LIST + > [ ("insert", ["ctx", "t", "domid", "?async:'a", "unit", "unit"]), > ("of_vdev", ["ctx", "domid", "string", "t"]), > diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c > index 45b8af61c74a..8e54f95da7c7 100644 > --- a/tools/ocaml/libs/xl/xenlight_stubs.c > +++ b/tools/ocaml/libs/xl/xenlight_stubs.c > @@ -707,6 +707,7 @@ DEVICE_ADDREMOVE(disk) > DEVICE_ADDREMOVE(nic) > DEVICE_ADDREMOVE(vfb) > DEVICE_ADDREMOVE(vkb) > +DEVICE_ADDREMOVE(virtio) > DEVICE_ADDREMOVE(pci) > _DEVICE_ADDREMOVE(disk, cdrom, insert) I don't think these ocaml changes are necessary, because they don't build. I'm guessing those adds the ability to hotplug devices which virtio device don't have, so function for that are missing. > diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c > index 1b5381cef033..c6f35c069d2a 100644 > --- a/tools/xl/xl_parse.c > +++ b/tools/xl/xl_parse.c > @@ -2309,8 +2390,10 @@ void parse_config_data(const char *config_source, > > d_config->num_vfbs = 0; > d_config->num_vkbs = 0; > + d_config->num_virtios = 0; > d_config->vfbs = NULL; > d_config->vkbs = NULL; > + d_config->virtios = NULL; These look a bit out of place, I think it would be fine to set num_virtios and virtios just before calling parse_virtio_list(), as array are usually initialised just before parsing the associated config option in parse_config_data(). > if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) { > while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) { > @@ -2752,6 +2835,7 @@ void parse_config_data(const char *config_source, > } > > parse_vkb_list(config, d_config); > + parse_virtio_list(config, d_config); > > xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat", > &c_info->xend_suspend_evtchn_compat, 0); Thanks,
Sorry, I've replied to the wrong version, but those comment apply to V7. Cheers,
diff --git a/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py index 7bf26bdcd831..b188104299b1 100644 --- a/tools/ocaml/libs/xl/genwrap.py +++ b/tools/ocaml/libs/xl/genwrap.py @@ -36,6 +36,7 @@ DEVICE_LIST = [ ("list", ["ctx", "domid", "t list"]), functions = { # ( name , [type1,type2,....] ) "device_vfb": DEVICE_FUNCTIONS, "device_vkb": DEVICE_FUNCTIONS, + "device_virtio": DEVICE_FUNCTIONS, "device_disk": DEVICE_FUNCTIONS + DEVICE_LIST + [ ("insert", ["ctx", "t", "domid", "?async:'a", "unit", "unit"]), ("of_vdev", ["ctx", "domid", "string", "t"]), diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c index 45b8af61c74a..8e54f95da7c7 100644 --- a/tools/ocaml/libs/xl/xenlight_stubs.c +++ b/tools/ocaml/libs/xl/xenlight_stubs.c @@ -707,6 +707,7 @@ DEVICE_ADDREMOVE(disk) DEVICE_ADDREMOVE(nic) DEVICE_ADDREMOVE(vfb) DEVICE_ADDREMOVE(vkb) +DEVICE_ADDREMOVE(virtio) DEVICE_ADDREMOVE(pci) _DEVICE_ADDREMOVE(disk, cdrom, insert) diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c index 1b5381cef033..c6f35c069d2a 100644 --- a/tools/xl/xl_parse.c +++ b/tools/xl/xl_parse.c @@ -1208,6 +1208,87 @@ static void parse_vkb_list(const XLU_Config *config, if (rc) exit(EXIT_FAILURE); } +static int parse_virtio_config(libxl_device_virtio *virtio, char *token) +{ + char *oparg; + int rc; + + if (MATCH_OPTION("backend", token, oparg)) { + virtio->backend_domname = strdup(oparg); + } else if (MATCH_OPTION("type", token, oparg)) { + virtio->type = strdup(oparg); + } else if (MATCH_OPTION("transport", token, oparg)) { + rc = libxl_virtio_transport_from_string(oparg, &virtio->transport); + if (rc) return rc; + } else if (MATCH_OPTION("irq", token, oparg)) { + virtio->irq = strtoul(oparg, NULL, 0); + } else if (MATCH_OPTION("base", token, oparg)) { + virtio->base = strtoul(oparg, NULL, 0); + } else { + fprintf(stderr, "Unknown string \"%s\" in virtio spec\n", token); + return -1; + } + + return 0; +} + +static void parse_virtio_list(const XLU_Config *config, + libxl_domain_config *d_config) +{ + XLU_ConfigList *virtios; + const char *item; + char *buf = NULL, *oparg, *str = NULL; + int rc; + + if (!xlu_cfg_get_list (config, "virtio", &virtios, 0, 0)) { + int entry = 0; + while ((item = xlu_cfg_get_listitem(virtios, entry)) != NULL) { + libxl_device_virtio *virtio; + char *p; + + virtio = ARRAY_EXTEND_INIT(d_config->virtios, d_config->num_virtios, + libxl_device_virtio_init); + + buf = strdup(item); + + p = strtok(buf, ","); + while (p != NULL) + { + while (*p == ' ') p++; + + // Type may contain a comma, do special handling. + if (MATCH_OPTION("type", p, oparg)) { + if (!strncmp(oparg, "virtio", strlen("virtio"))) { + char *p2 = strtok(NULL, ","); + str = malloc(strlen(p) + strlen(p2) + 2); + + strcpy(str, p); + strcat(str, ","); + strcat(str, p2); + p = str; + } + } + + rc = parse_virtio_config(virtio, p); + if (rc) goto out; + + free(str); + str = NULL; + p = strtok(NULL, ","); + } + + entry++; + free(buf); + } + } + + return; + +out: + free(buf); + if (rc) exit(EXIT_FAILURE); +} + void parse_config_data(const char *config_source, const char *config_data, int config_len, @@ -2309,8 +2390,10 @@ void parse_config_data(const char *config_source, d_config->num_vfbs = 0; d_config->num_vkbs = 0; + d_config->num_virtios = 0; d_config->vfbs = NULL; d_config->vkbs = NULL; + d_config->virtios = NULL; if (!xlu_cfg_get_list (config, "vfb", &cvfbs, 0, 0)) { while ((buf = xlu_cfg_get_listitem (cvfbs, d_config->num_vfbs)) != NULL) { @@ -2752,6 +2835,7 @@ void parse_config_data(const char *config_source, } parse_vkb_list(config, d_config); + parse_virtio_list(config, d_config); xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat", &c_info->xend_suspend_evtchn_compat, 0);
This patch adds basic support for parsing generic Virtio backend. An example of domain configuration for mmio based Virtio I2C device is: virtio = ["type=virtio,device22,transport=mmio"] Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- tools/ocaml/libs/xl/genwrap.py | 1 + tools/ocaml/libs/xl/xenlight_stubs.c | 1 + tools/xl/xl_parse.c | 84 ++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+)