[PATCH 0/5] qrap: Fix a number of issues

[PATCH 0/5] qrap: Fix a number of issues

[Andrea Bolognani]

Andrea Bolognani (5):
  qrap: Fix limits for PCI addresses
  qrap: Fix support for pc machines
  qrap: Drop args in JSON format
  qrap: Introduce machine-specific PCI address base
  qrap: Generate -netdev as JSON

 qrap.c | 47 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 35 insertions(+), 12 deletions(-)

-- 
2.39.2

[PATCH 1/5] qrap: Fix limits for PCI addresses

[Andrea Bolognani]

The pci.0 bus on a pc machine has 32 slots.

For q35 machines, we don't expect devices to be plugged into
pcie.0 directly, so technically we could have a very large
number of slots by adding many pcie-root-ports, but even in
this scenario 32 is a reasonable number.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 qrap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qrap.c b/qrap.c
index 287198e..2443fa3 100644
--- a/qrap.c
+++ b/qrap.c
@@ -89,13 +89,13 @@ static const struct pci_dev {
 		"pc-q35", "virtio-net-pci",
 		"bus=pci.", ",addr=0x0",
 		"\"bus\":\"pci.", ",\"addr\":\"0x0\"",
-		3, /* 2: hotplug bus */ 16
+		3, /* 2: hotplug bus */ 31
 	},
 	{
 		"pc-", "virtio-net-pci",
 		"bus=pci.0,addr=0x", "",
 		"\"bus\":\"pci.0\",\"addr=0x", "",
-		2, /* 1: ISA bridge */ 16
+		2, /* 1: ISA bridge */ 31
 	},
 	{
 		"s390-ccw", "virtio-net-ccw",
-- 
@@ -89,13 +89,13 @@ static const struct pci_dev {
 		"pc-q35", "virtio-net-pci",
 		"bus=pci.", ",addr=0x0",
 		"\"bus\":\"pci.", ",\"addr\":\"0x0\"",
-		3, /* 2: hotplug bus */ 16
+		3, /* 2: hotplug bus */ 31
 	},
 	{
 		"pc-", "virtio-net-pci",
 		"bus=pci.0,addr=0x", "",
 		"\"bus\":\"pci.0\",\"addr=0x", "",
-		2, /* 1: ISA bridge */ 16
+		2, /* 1: ISA bridge */ 31
 	},
 	{
 		"s390-ccw", "virtio-net-ccw",
-- 
2.39.2

[PATCH 2/5] qrap: Fix support for pc machines

[Andrea Bolognani]

The JSON version of the template is incorrect, making qrap
completely unusable when JSON arguments are used together
with the pc machine type.

Fixes: b944ca185587 ("qrap: Support JSON syntax for -device")
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 qrap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qrap.c b/qrap.c
index 2443fa3..5c18cec 100644
--- a/qrap.c
+++ b/qrap.c
@@ -94,7 +94,7 @@ static const struct pci_dev {
 	{
 		"pc-", "virtio-net-pci",
 		"bus=pci.0,addr=0x", "",
-		"\"bus\":\"pci.0\",\"addr=0x", "",
+		"\"bus\":\"pci.0\",\"addr\":\"0x", "",
 		2, /* 1: ISA bridge */ 31
 	},
 	{
-- 
@@ -94,7 +94,7 @@ static const struct pci_dev {
 	{
 		"pc-", "virtio-net-pci",
 		"bus=pci.0,addr=0x", "",
-		"\"bus\":\"pci.0\",\"addr=0x", "",
+		"\"bus\":\"pci.0\",\"addr\":\"0x", "",
 		2, /* 1: ISA bridge */ 31
 	},
 	{
-- 
2.39.2

[PATCH 3/5] qrap: Drop args in JSON format

[Andrea Bolognani]

When JSON support was introduced, the drop_args array has
not been updated accordingly.

Fixes: b944ca185587 ("qrap: Support JSON syntax for -device")
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 qrap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/qrap.c b/qrap.c
index 5c18cec..3077944 100644
--- a/qrap.c
+++ b/qrap.c
@@ -57,10 +57,15 @@ static const struct drop_arg {
 	{ "-netdev",	NULL },
 	{ "-net",	NULL },
 	{ "-device",	"virtio-net-pci," },
+	{ "-device",	"{\"driver\":\"virtio-net-pci\"," },
 	{ "-device",	"virtio-net-ccw," },
+	{ "-device",	"{\"driver\":\"virtio-net-ccw\"," },
 	{ "-device",	"e1000," },
+	{ "-device",	"{\"driver\":\"e1000\"," },
 	{ "-device",	"e1000e," },
+	{ "-device",	"{\"driver\":\"e1000e\"," },
 	{ "-device",	"rtl8139," },
+	{ "-device",	"{\"driver\":\"rtl8139\"," },
 	{ 0 },
 };
 
-- 
@@ -57,10 +57,15 @@ static const struct drop_arg {
 	{ "-netdev",	NULL },
 	{ "-net",	NULL },
 	{ "-device",	"virtio-net-pci," },
+	{ "-device",	"{\"driver\":\"virtio-net-pci\"," },
 	{ "-device",	"virtio-net-ccw," },
+	{ "-device",	"{\"driver\":\"virtio-net-ccw\"," },
 	{ "-device",	"e1000," },
+	{ "-device",	"{\"driver\":\"e1000\"," },
 	{ "-device",	"e1000e," },
+	{ "-device",	"{\"driver\":\"e1000e\"," },
 	{ "-device",	"rtl8139," },
+	{ "-device",	"{\"driver\":\"rtl8139\"," },
 	{ 0 },
 };
 
-- 
2.39.2

[PATCH 4/5] qrap: Introduce machine-specific PCI address base

[Andrea Bolognani]

For pc machines, devices are placed directly on pci.0 with
addresses like

  bus=pci.0,addr=0xa

and in this case the existing code works correctly.

For q35 machines, however, a separate PCI bus is created for
each devices using a pcie-root-port, and the resulting
addresses look like

  bus=pci.9,addr=0x0

In this case, we need to treat PCI addresses as decimal, not
hexadecimal, both when parsing and generating them.

This issue has gone unnoticed for a long time because it only
shows up when enough PCI devices are present: for small
numbers, decimal and hexadecimal overlap, masking the issue.

Reported-by: Alona Paz <alkaplan@redhat.com>
Fixes: 5307faa05997 ("qrap: Strip network devices from command line, set them up according to machine")
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 qrap.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/qrap.c b/qrap.c
index 3077944..27c12ed 100644
--- a/qrap.c
+++ b/qrap.c
@@ -77,6 +77,7 @@ static const struct drop_arg {
  * @template_post:	Suffix for device specification (last part of address)
  * @template_json:		Device prefix for when JSON is used
  * @template_json_post:	Device suffix for when JSON is used
+ * @base:		Base used for PCI addresses
  * @first:		First usable PCI address
  * @last:		Last usable PCI address
  */
@@ -87,6 +88,7 @@ static const struct pci_dev {
 	char *template_post;
 	char *template_json;
 	char *template_json_post;
+	int base;
 	int first;
 	int last;
 } pci_devs[] = {
@@ -94,19 +96,19 @@ static const struct pci_dev {
 		"pc-q35", "virtio-net-pci",
 		"bus=pci.", ",addr=0x0",
 		"\"bus\":\"pci.", ",\"addr\":\"0x0\"",
-		3, /* 2: hotplug bus */ 31
+		10, 3, /* 2: hotplug bus */ 31
 	},
 	{
 		"pc-", "virtio-net-pci",
 		"bus=pci.0,addr=0x", "",
 		"\"bus\":\"pci.0\",\"addr\":\"0x", "",
-		2, /* 1: ISA bridge */ 31
+		16, 2, /* 1: ISA bridge */ 31
 	},
 	{
 		"s390-ccw", "virtio-net-ccw",
 		"devno=fe.0.", "",
 		"\"devno\":\"fe.0.", "",
-		1, 16
+		16, 1, 16
 	},
 	{ 0 },
 };
@@ -264,7 +266,7 @@ int main(int argc, char **argv)
 			if (template) {
 				long n;
 
-				n = strtol(p + strlen(template), NULL, 16);
+				n = strtol(p + strlen(template), NULL, dev->base);
 				if (!errno)
 					addr_map |= (1 << n);
 			}
@@ -285,13 +287,25 @@ int main(int argc, char **argv)
 	if (has_dev) {
 		qemu_argv[qemu_argc++] = "-device";
 		if (!has_json) {
-			snprintf(dev_str, ARG_MAX,
-			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
-			         dev->name, dev->template, i, dev->template_post);
+			if (dev->base == 16) {
+				snprintf(dev_str, ARG_MAX,
+				         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
+				         dev->name, dev->template, i, dev->template_post);
+			} else if (dev->base == 10) {
+				snprintf(dev_str, ARG_MAX,
+				         "%s,%s%d%s,netdev=hostnet0,x-txburst=4096",
+				         dev->name, dev->template, i, dev->template_post);
+			}
 		} else {
-			snprintf(dev_str, ARG_MAX,
-			         "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
-			         dev->name, dev->template_json, i, dev->template_json_post);
+			if (dev->base == 16) {
+				snprintf(dev_str, ARG_MAX,
+				         "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
+				         dev->name, dev->template_json, i, dev->template_json_post);
+			} else if (dev->base == 10) {
+				snprintf(dev_str, ARG_MAX,
+				         "{\"driver\":\"%s\",%s%d\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
+				         dev->name, dev->template_json, i, dev->template_json_post);
+			}
 		}
 		qemu_argv[qemu_argc++] = dev_str;
 	}
-- 
@@ -77,6 +77,7 @@ static const struct drop_arg {
  * @template_post:	Suffix for device specification (last part of address)
  * @template_json:		Device prefix for when JSON is used
  * @template_json_post:	Device suffix for when JSON is used
+ * @base:		Base used for PCI addresses
  * @first:		First usable PCI address
  * @last:		Last usable PCI address
  */
@@ -87,6 +88,7 @@ static const struct pci_dev {
 	char *template_post;
 	char *template_json;
 	char *template_json_post;
+	int base;
 	int first;
 	int last;
 } pci_devs[] = {
@@ -94,19 +96,19 @@ static const struct pci_dev {
 		"pc-q35", "virtio-net-pci",
 		"bus=pci.", ",addr=0x0",
 		"\"bus\":\"pci.", ",\"addr\":\"0x0\"",
-		3, /* 2: hotplug bus */ 31
+		10, 3, /* 2: hotplug bus */ 31
 	},
 	{
 		"pc-", "virtio-net-pci",
 		"bus=pci.0,addr=0x", "",
 		"\"bus\":\"pci.0\",\"addr\":\"0x", "",
-		2, /* 1: ISA bridge */ 31
+		16, 2, /* 1: ISA bridge */ 31
 	},
 	{
 		"s390-ccw", "virtio-net-ccw",
 		"devno=fe.0.", "",
 		"\"devno\":\"fe.0.", "",
-		1, 16
+		16, 1, 16
 	},
 	{ 0 },
 };
@@ -264,7 +266,7 @@ int main(int argc, char **argv)
 			if (template) {
 				long n;
 
-				n = strtol(p + strlen(template), NULL, 16);
+				n = strtol(p + strlen(template), NULL, dev->base);
 				if (!errno)
 					addr_map |= (1 << n);
 			}
@@ -285,13 +287,25 @@ int main(int argc, char **argv)
 	if (has_dev) {
 		qemu_argv[qemu_argc++] = "-device";
 		if (!has_json) {
-			snprintf(dev_str, ARG_MAX,
-			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
-			         dev->name, dev->template, i, dev->template_post);
+			if (dev->base == 16) {
+				snprintf(dev_str, ARG_MAX,
+				         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
+				         dev->name, dev->template, i, dev->template_post);
+			} else if (dev->base == 10) {
+				snprintf(dev_str, ARG_MAX,
+				         "%s,%s%d%s,netdev=hostnet0,x-txburst=4096",
+				         dev->name, dev->template, i, dev->template_post);
+			}
 		} else {
-			snprintf(dev_str, ARG_MAX,
-			         "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
-			         dev->name, dev->template_json, i, dev->template_json_post);
+			if (dev->base == 16) {
+				snprintf(dev_str, ARG_MAX,
+				         "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
+				         dev->name, dev->template_json, i, dev->template_json_post);
+			} else if (dev->base == 10) {
+				snprintf(dev_str, ARG_MAX,
+				         "{\"driver\":\"%s\",%s%d\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
+				         dev->name, dev->template_json, i, dev->template_json_post);
+			}
 		}
 		qemu_argv[qemu_argc++] = dev_str;
 	}
-- 
2.39.2

[PATCH 5/5] qrap: Generate -netdev as JSON

[Andrea Bolognani]

While generating -device as JSON when JSON is in use is
mandatory, because not doing so can often prevent the VM from
starting up, using JSON for -netdev simply makes things a bit
nicer. No reason not to do it, though.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 qrap.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qrap.c b/qrap.c
index 27c12ed..d0e2fb2 100644
--- a/qrap.c
+++ b/qrap.c
@@ -311,7 +311,11 @@ int main(int argc, char **argv)
 	}
 
 	qemu_argv[qemu_argc++] = "-netdev";
-	qemu_argv[qemu_argc++] = "socket,fd=" STR(DEFAULT_FD) ",id=hostnet0";
+	if (!has_json) {
+		qemu_argv[qemu_argc++] = "socket,fd=" STR(DEFAULT_FD) ",id=hostnet0";
+	} else {
+		qemu_argv[qemu_argc++] = "{\"type\":\"socket\",\"fd\":\"" STR(DEFAULT_FD) "\",\"id\":\"hostnet0\"}";
+	}
 	qemu_argv[qemu_argc] = NULL;
 
 valid_args:
-- 
@@ -311,7 +311,11 @@ int main(int argc, char **argv)
 	}
 
 	qemu_argv[qemu_argc++] = "-netdev";
-	qemu_argv[qemu_argc++] = "socket,fd=" STR(DEFAULT_FD) ",id=hostnet0";
+	if (!has_json) {
+		qemu_argv[qemu_argc++] = "socket,fd=" STR(DEFAULT_FD) ",id=hostnet0";
+	} else {
+		qemu_argv[qemu_argc++] = "{\"type\":\"socket\",\"fd\":\"" STR(DEFAULT_FD) "\",\"id\":\"hostnet0\"}";
+	}
 	qemu_argv[qemu_argc] = NULL;
 
 valid_args:
-- 
2.39.2

Re: [PATCH 0/5] qrap: Fix a number of issues

[Andrea Bolognani]

On Fri, Feb 24, 2023 at 07:49:44PM +0100, Andrea Bolognani wrote:
> Andrea Bolognani (5):
>   qrap: Fix limits for PCI addresses
>   qrap: Fix support for pc machines
>   qrap: Drop args in JSON format
>   qrap: Introduce machine-specific PCI address base
>   qrap: Generate -netdev as JSON
>
>  qrap.c | 47 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 12 deletions(-)

Some information about testing.

In order to convince myself that the changes I was working on were
correct, I have created a few quick and dirty test scripts. You can
find them here:

  https://gitlab.com/abologna/passt/-/commits/qrap-tests

The commit contains a few YAML files:

  vmi-small-q35-passt.yaml # q35 VM with few devices
  vmi-small-pc-passt.yaml  #  pc VM with few devices
  vmi-big-q35-passt.yaml   # q35 VM with many devices
  vmi-big-pc-passt.yaml    #  pc VM with many devices

Each one of those, fed into KubeVirt, results in qrap being called
with a bunch of command line arguments. These are normally passed to
the actual QEMU binary, but in my case I've hacked qrap so that
they're simply printed out.

For each of the scenarios, there are three files:

  vm.in  # arguments passed to qrap
  vm.exp # arguments generated by the current version of qrap
  vm.sh  # test script

Running vm.sh results in qrap being called with the same arguments
contained in vm.in (but quoted for shell consumption), with the
output collected into the temporary file vm.out. This file is then
compared with both vm.in (to highlight changes made by qrap) and with
vm.exp (to highlight changes in behavior compared to the current
version of qrap).


Running these tests before applying my patches should result in
vm.out being identical to vm.ext whereas, after my patches have been
applied, the following important changes will be visible:

  --- pc-small.exp	2023-02-24 20:11:14.559711296 +0100
  +++ pc-small.out	2023-02-24 20:26:53.557307727 +0100
  @@ -75,6 +75,6 @@
   -msg
   timestamp=on
   -device
  -virtio-net-pci,bus=pci.0,addr=0x2,netdev=hostnet0,x-txburst=4096
  +{"driver":"virtio-net-pci","bus":"pci.0","addr":"0x8","netdev":"hostnet0","x-txburst":4096}

(addresses of other devices are now parsed correctly, so qrap's
interface will be assigned to slot 8, which is available, instead of
slot 2, which isn't)

  --- q35-big.exp	2023-02-24 20:11:14.560711288 +0100
  +++ q35-big.out	2023-02-24 20:27:20.632094269 +0100
  @@ -159,6 +159,6 @@
   -msg
   timestamp=on
   -device
  -{"driver":"virtio-net-pci","bus":"pci.a","addr":"0x0","netdev":"hostnet0","x-txburst":4096}
  +{"driver":"virtio-net-pci","bus":"pci.15","addr":"0x0","netdev":"hostnet0","x-txburst":4096}

(pci.15 is correctly identified as the bus where qrap's interface
should be placed instead of the non-existing pci.a bus)

  --- pc-big.exp	2023-02-24 20:11:14.559711296 +0100
  +++ pc-big.out	2023-02-24 20:27:37.124964240 +0100
  @@ -129,6 +129,6 @@
   -msg
   timestamp=on
   -device
  -virtio-net-pci,bus=pci.0,addr=0x2,netdev=hostnet0,x-txburst=4096
  +{"driver":"virtio-net-pci","bus":"pci.0","addr":"0x11","netdev":"hostnet0","x-txburst":4096}

(again, an available slot will be picked)

Note how the q35-small test sees no differences. That's the one
scenario that was already working, so the lack of changes there is a
very good thing :)


In addition to these crude test scripts, I've also made a custom
RPM build of passt with these patches applied and integrated it into
KubeVirt. Using it, I was able to successfully submit all the YAML
files mentioned earlier and get working VMs, with functional network
connection, as a result.

I also ran the passt tests from KubeVirt's own functional test suite,
and they all passed except for one. That one's about IPv6
connectivity, and I think the lack of IPv6 connectivity in my own
machine / network is to blame for the failure. This specific test
fails with the non-patched version of passt too, further validating
this theory.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 1/5] qrap: Fix limits for PCI addresses

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1311 bytes --]

On Fri, Feb 24, 2023 at 07:49:45PM +0100, Andrea Bolognani wrote:
> The pci.0 bus on a pc machine has 32 slots.
> 
> For q35 machines, we don't expect devices to be plugged into
> pcie.0 directly, so technically we could have a very large
> number of slots by adding many pcie-root-ports, but even in
> this scenario 32 is a reasonable number.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  qrap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qrap.c b/qrap.c
> index 287198e..2443fa3 100644
> --- a/qrap.c
> +++ b/qrap.c
> @@ -89,13 +89,13 @@ static const struct pci_dev {
>  		"pc-q35", "virtio-net-pci",
>  		"bus=pci.", ",addr=0x0",
>  		"\"bus\":\"pci.", ",\"addr\":\"0x0\"",
> -		3, /* 2: hotplug bus */ 16
> +		3, /* 2: hotplug bus */ 31
>  	},
>  	{
>  		"pc-", "virtio-net-pci",
>  		"bus=pci.0,addr=0x", "",
>  		"\"bus\":\"pci.0\",\"addr=0x", "",
> -		2, /* 1: ISA bridge */ 16
> +		2, /* 1: ISA bridge */ 31
>  	},
>  	{
>  		"s390-ccw", "virtio-net-ccw",

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] qrap: Fix support for pc machines

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

On Fri, Feb 24, 2023 at 07:49:46PM +0100, Andrea Bolognani wrote:
> The JSON version of the template is incorrect, making qrap
> completely unusable when JSON arguments are used together
> with the pc machine type.
> 
> Fixes: b944ca185587 ("qrap: Support JSON syntax for -device")
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  qrap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qrap.c b/qrap.c
> index 2443fa3..5c18cec 100644
> --- a/qrap.c
> +++ b/qrap.c
> @@ -94,7 +94,7 @@ static const struct pci_dev {
>  	{
>  		"pc-", "virtio-net-pci",
>  		"bus=pci.0,addr=0x", "",
> -		"\"bus\":\"pci.0\",\"addr=0x", "",
> +		"\"bus\":\"pci.0\",\"addr\":\"0x", "",
>  		2, /* 1: ISA bridge */ 31
>  	},
>  	{

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/5] qrap: Drop args in JSON format

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1321 bytes --]

On Fri, Feb 24, 2023 at 07:49:47PM +0100, Andrea Bolognani wrote:
> When JSON support was introduced, the drop_args array has
> not been updated accordingly.
> 
> Fixes: b944ca185587 ("qrap: Support JSON syntax for -device")
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>

Oof, drop_args is such a hack.  But I guess all of qrap is.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  qrap.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/qrap.c b/qrap.c
> index 5c18cec..3077944 100644
> --- a/qrap.c
> +++ b/qrap.c
> @@ -57,10 +57,15 @@ static const struct drop_arg {
>  	{ "-netdev",	NULL },
>  	{ "-net",	NULL },
>  	{ "-device",	"virtio-net-pci," },
> +	{ "-device",	"{\"driver\":\"virtio-net-pci\"," },
>  	{ "-device",	"virtio-net-ccw," },
> +	{ "-device",	"{\"driver\":\"virtio-net-ccw\"," },
>  	{ "-device",	"e1000," },
> +	{ "-device",	"{\"driver\":\"e1000\"," },
>  	{ "-device",	"e1000e," },
> +	{ "-device",	"{\"driver\":\"e1000e\"," },
>  	{ "-device",	"rtl8139," },
> +	{ "-device",	"{\"driver\":\"rtl8139\"," },
>  	{ 0 },
>  };
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/5] qrap: Introduce machine-specific PCI address base

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4370 bytes --]

On Fri, Feb 24, 2023 at 07:49:48PM +0100, Andrea Bolognani wrote:
11;rgb:ffff/ffff/ffff> For pc machines, devices are placed directly on pci.0 with
> addresses like
> 
>   bus=pci.0,addr=0xa
> 
> and in this case the existing code works correctly.
> 
> For q35 machines, however, a separate PCI bus is created for
> each devices using a pcie-root-port, and the resulting
> addresses look like
> 
>   bus=pci.9,addr=0x0
> 
> In this case, we need to treat PCI addresses as decimal, not
> hexadecimal, both when parsing and generating them.
> 
> This issue has gone unnoticed for a long time because it only
> shows up when enough PCI devices are present: for small
> numbers, decimal and hexadecimal overlap, masking the issue.
> 
> Reported-by: Alona Paz <alkaplan@redhat.com>
> Fixes: 5307faa05997 ("qrap: Strip network devices from command line, set them up according to machine")
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>

Clunky, but, whatever, it's qrap.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  qrap.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/qrap.c b/qrap.c
> index 3077944..27c12ed 100644
> --- a/qrap.c
> +++ b/qrap.c
> @@ -77,6 +77,7 @@ static const struct drop_arg {
>   * @template_post:	Suffix for device specification (last part of address)
>   * @template_json:		Device prefix for when JSON is used
>   * @template_json_post:	Device suffix for when JSON is used
> + * @base:		Base used for PCI addresses
>   * @first:		First usable PCI address
>   * @last:		Last usable PCI address
>   */
> @@ -87,6 +88,7 @@ static const struct pci_dev {
>  	char *template_post;
>  	char *template_json;
>  	char *template_json_post;
> +	int base;
>  	int first;
>  	int last;
>  } pci_devs[] = {
> @@ -94,19 +96,19 @@ static const struct pci_dev {
>  		"pc-q35", "virtio-net-pci",
>  		"bus=pci.", ",addr=0x0",
>  		"\"bus\":\"pci.", ",\"addr\":\"0x0\"",
> -		3, /* 2: hotplug bus */ 31
> +		10, 3, /* 2: hotplug bus */ 31
>  	},
>  	{
>  		"pc-", "virtio-net-pci",
>  		"bus=pci.0,addr=0x", "",
>  		"\"bus\":\"pci.0\",\"addr\":\"0x", "",
> -		2, /* 1: ISA bridge */ 31
> +		16, 2, /* 1: ISA bridge */ 31
>  	},
>  	{
>  		"s390-ccw", "virtio-net-ccw",
>  		"devno=fe.0.", "",
>  		"\"devno\":\"fe.0.", "",
> -		1, 16
> +		16, 1, 16
>  	},
>  	{ 0 },
>  };
> @@ -264,7 +266,7 @@ int main(int argc, char **argv)
>  			if (template) {
>  				long n;
>  
> -				n = strtol(p + strlen(template), NULL, 16);
> +				n = strtol(p + strlen(template), NULL, dev->base);
>  				if (!errno)
>  					addr_map |= (1 << n);
>  			}
> @@ -285,13 +287,25 @@ int main(int argc, char **argv)
>  	if (has_dev) {
>  		qemu_argv[qemu_argc++] = "-device";
>  		if (!has_json) {
> -			snprintf(dev_str, ARG_MAX,
> -			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
> -			         dev->name, dev->template, i, dev->template_post);
> +			if (dev->base == 16) {
> +				snprintf(dev_str, ARG_MAX,
> +				         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
> +				         dev->name, dev->template, i, dev->template_post);
> +			} else if (dev->base == 10) {
> +				snprintf(dev_str, ARG_MAX,
> +				         "%s,%s%d%s,netdev=hostnet0,x-txburst=4096",
> +				         dev->name, dev->template, i, dev->template_post);
> +			}
>  		} else {
> -			snprintf(dev_str, ARG_MAX,
> -			         "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
> -			         dev->name, dev->template_json, i, dev->template_json_post);
> +			if (dev->base == 16) {
> +				snprintf(dev_str, ARG_MAX,
> +				         "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
> +				         dev->name, dev->template_json, i, dev->template_json_post);
> +			} else if (dev->base == 10) {
> +				snprintf(dev_str, ARG_MAX,
> +				         "{\"driver\":\"%s\",%s%d\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
> +				         dev->name, dev->template_json, i, dev->template_json_post);
> +			}
>  		}
>  		qemu_argv[qemu_argc++] = dev_str;
>  	}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 5/5] qrap: Generate -netdev as JSON

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On Fri, Feb 24, 2023 at 07:49:49PM +0100, Andrea Bolognani wrote:
> While generating -device as JSON when JSON is in use is
> mandatory, because not doing so can often prevent the VM from
> starting up, using JSON for -netdev simply makes things a bit
> nicer. No reason not to do it, though.

Well, except that it means we now have two cases when before there was
one.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

in the sense that I think the patch is correct.  I don't think there's
much point to this change though.


> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  qrap.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/qrap.c b/qrap.c
> index 27c12ed..d0e2fb2 100644
> --- a/qrap.c
> +++ b/qrap.c
> @@ -311,7 +311,11 @@ int main(int argc, char **argv)
>  	}
>  
>  	qemu_argv[qemu_argc++] = "-netdev";
> -	qemu_argv[qemu_argc++] = "socket,fd=" STR(DEFAULT_FD) ",id=hostnet0";
> +	if (!has_json) {
> +		qemu_argv[qemu_argc++] = "socket,fd=" STR(DEFAULT_FD) ",id=hostnet0";
> +	} else {
> +		qemu_argv[qemu_argc++] = "{\"type\":\"socket\",\"fd\":\"" STR(DEFAULT_FD) "\",\"id\":\"hostnet0\"}";
> +	}
>  	qemu_argv[qemu_argc] = NULL;
>  
>  valid_args:

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/5] qrap: Fix a number of issues

[Stefano Brivio]

On Fri, 24 Feb 2023 11:36:51 -0800
Andrea Bolognani <abologna@redhat.com> wrote:

> On Fri, Feb 24, 2023 at 07:49:44PM +0100, Andrea Bolognani wrote:
> > Andrea Bolognani (5):
> >   qrap: Fix limits for PCI addresses
> >   qrap: Fix support for pc machines
> >   qrap: Drop args in JSON format
> >   qrap: Introduce machine-specific PCI address base
> >   qrap: Generate -netdev as JSON
> >
> >  qrap.c | 47 +++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 35 insertions(+), 12 deletions(-)  
> 
> Some information about testing.
> 
> In order to convince myself that the changes I was working on were
> correct, I have created a few quick and dirty test scripts. You can
> find them here:
> 
>   https://gitlab.com/abologna/passt/-/commits/qrap-tests
> 
> The commit contains a few YAML files:
> 
>   vmi-small-q35-passt.yaml # q35 VM with few devices
>   vmi-small-pc-passt.yaml  #  pc VM with few devices
>   vmi-big-q35-passt.yaml   # q35 VM with many devices
>   vmi-big-pc-passt.yaml    #  pc VM with many devices
> 
> Each one of those, fed into KubeVirt, results in qrap being called
> with a bunch of command line arguments. These are normally passed to
> the actual QEMU binary, but in my case I've hacked qrap so that
> they're simply printed out.

If you think it's useful, by the way, feel free to submit those as a
patch for something on the lines of "test/kubevirt/qrap-tests", with
explicit licensing terms and something similar to your email as README.

We wouldn't run them automatically, I'm not sure I would personally try
them out, qrap should hopefully go away in a few months (even though
KubeVirt YAMLs could still be helpful), and... I know it's effort, so
I'm not really pushing for it, you know better.

Thanks!

-- 
Stefano

Re: [PATCH 0/5] qrap: Fix a number of issues

[Andrea Bolognani]

On Mon, Feb 27, 2023 at 11:25:27AM +0100, Stefano Brivio wrote:
> If you think it's useful, by the way, feel free to submit those as a
> patch for something on the lines of "test/kubevirt/qrap-tests", with
> explicit licensing terms and something similar to your email as README.
>
> We wouldn't run them automatically, I'm not sure I would personally try
> them out, qrap should hopefully go away in a few months (even though
> KubeVirt YAMLs could still be helpful), and... I know it's effort, so
> I'm not really pushing for it, you know better.

I'm really counting on the next patch that touches qrap being the one
that gets rid of it for good :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH 0/5] qrap: Fix a number of issues

[Stefano Brivio]

On Fri, 24 Feb 2023 19:49:44 +0100
Andrea Bolognani <abologna@redhat.com> wrote:

> Andrea Bolognani (5):
>   qrap: Fix limits for PCI addresses
>   qrap: Fix support for pc machines
>   qrap: Drop args in JSON format
>   qrap: Introduce machine-specific PCI address base
>   qrap: Generate -netdev as JSON
> 
>  qrap.c | 47 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 35 insertions(+), 12 deletions(-)

You really seem to... like qrap. Series applied, thanks.

I agree with David about 5/5 by the way: why should we add one more
case we need to check, with the JSON format for -netdev? 

But you're a qrap connoisseur at this point, so... I just hope you
don't have to deal with that part ever again. :)

-- 
Stefano