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);
+ } 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);
--
2.31.1.272.g89b43f80a514On 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,
--
Anthony PERARD
Sorry, I've replied to the wrong version, but those comment apply to V7. Cheers, -- Anthony PERARD
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. -- viresh
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)
])
>
© 2016 - 2026 Red Hat, Inc.