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 --- 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", -- 2.39.2

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 --- 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 }, { -- 2.39.2

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 --- 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 }, }; -- 2.39.2

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 Fixes: 5307faa05997 ("qrap: Strip network devices from command line, set them up according to machine") Signed-off-by: Andrea Bolognani --- 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; } -- 2.39.2

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 --- 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: -- 2.39.2

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