public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
@ 2023-02-21 19:24 Stefano Brivio
  2023-02-22  0:27 ` David Gibson
  2023-02-22 10:40 ` Andrea Bolognani
  0 siblings, 2 replies; 12+ messages in thread
From: Stefano Brivio @ 2023-02-21 19:24 UTC (permalink / raw)
  To: passt-dev; +Cc: Alona Paz, Andrea Bolognani

Alona reports that when libvirt starts qrap (KubeVirt integration)
and the domain description leads to more than 10 devices, indices of
PCI device descriptors are formatted as hexadecimal, so we end up
with things like "pci.a" instead of "pci.10".

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: Stefano Brivio <sbrivio@redhat.com>
---
 qrap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qrap.c b/qrap.c
index 287198e..ff99c89 100644
--- a/qrap.c
+++ b/qrap.c
@@ -281,11 +281,11 @@ int main(int argc, char **argv)
 		qemu_argv[qemu_argc++] = "-device";
 		if (!has_json) {
 			snprintf(dev_str, ARG_MAX,
-			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
+			         "%s,%s%i%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}",
+			         "{\"driver\":\"%s\",%s%i\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
 			         dev->name, dev->template_json, i, dev->template_json_post);
 		}
 		qemu_argv[qemu_argc++] = dev_str;
-- 
@@ -281,11 +281,11 @@ int main(int argc, char **argv)
 		qemu_argv[qemu_argc++] = "-device";
 		if (!has_json) {
 			snprintf(dev_str, ARG_MAX,
-			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
+			         "%s,%s%i%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}",
+			         "{\"driver\":\"%s\",%s%i\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
 			         dev->name, dev->template_json, i, dev->template_json_post);
 		}
 		qemu_argv[qemu_argc++] = dev_str;
-- 
2.39.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-21 19:24 [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16 Stefano Brivio
@ 2023-02-22  0:27 ` David Gibson
  2023-02-22 10:40 ` Andrea Bolognani
  1 sibling, 0 replies; 12+ messages in thread
From: David Gibson @ 2023-02-22  0:27 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev, Alona Paz, Andrea Bolognani

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

On Tue, Feb 21, 2023 at 08:24:25PM +0100, Stefano Brivio wrote:
> Alona reports that when libvirt starts qrap (KubeVirt integration)
> and the domain description leads to more than 10 devices, indices of
> PCI device descriptors are formatted as hexadecimal, so we end up
> with things like "pci.a" instead of "pci.10".
> 
> 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: Stefano Brivio <sbrivio@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..ff99c89 100644
> --- a/qrap.c
> +++ b/qrap.c
> @@ -281,11 +281,11 @@ int main(int argc, char **argv)
>  		qemu_argv[qemu_argc++] = "-device";
>  		if (!has_json) {
>  			snprintf(dev_str, ARG_MAX,
> -			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
> +			         "%s,%s%i%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}",
> +			         "{\"driver\":\"%s\",%s%i\"%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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-21 19:24 [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16 Stefano Brivio
  2023-02-22  0:27 ` David Gibson
@ 2023-02-22 10:40 ` Andrea Bolognani
  2023-02-22 10:45   ` Stefano Brivio
  2023-02-22 22:27   ` David Gibson
  1 sibling, 2 replies; 12+ messages in thread
From: Andrea Bolognani @ 2023-02-22 10:40 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev, Alona Paz

On Tue, Feb 21, 2023 at 08:24:25PM +0100, Stefano Brivio wrote:
>  		if (!has_json) {
>  			snprintf(dev_str, ARG_MAX,
> -			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
> +			         "%s,%s%i%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}",
> +			         "{\"driver\":\"%s\",%s%i\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
>  			         dev->name, dev->template_json, i, dev->template_json_post);
>  		}

I don't think this is going to work.

The problem is that, while PCI buses are indeed named with increasing
numbers in integer format (pci.9, pci.10 and so on), PCI slots are
addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
this naming convention because it matches QEMU's.

So if you have an i440fx machine with many devices, they will all[1]
be put on pci.0, the corresponding arguments will look like

  -device virtio-net-pci,bus=pci.0,addr=0xa

and the original version of the code will work correctly.

If the machine is q35, however, devices will not be put on pcie.0
directly but rather attached to pcie-root-ports, each one of which
creates a separate PCI bus. So the arguments will look like

  -device virtio-net-pci,bus=pci.10,addr=0x0

where it is the new version of the code that would do the right
thing.

What I think needs to happen, is that each pci_dev should contain a
base value (16 for i440fx and 10 for q35), which is used both in the
strtol() call used to parse the command line produced by libvirt and
to decide whether %x or %i should be used with snprintf() to generate
qrap's own arguments.


[1] Up until the point where are too many devices to fit into pci.0
    and adding a pci-bridge becomes necessary. I think qrap wouldn't
    be able to handle this scenario correctly anyway.
-- 
Andrea Bolognani / Red Hat / Virtualization


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-22 10:40 ` Andrea Bolognani
@ 2023-02-22 10:45   ` Stefano Brivio
  2023-02-22 12:47     ` Andrea Bolognani
  2023-02-22 22:27   ` David Gibson
  1 sibling, 1 reply; 12+ messages in thread
From: Stefano Brivio @ 2023-02-22 10:45 UTC (permalink / raw)
  To: Andrea Bolognani; +Cc: passt-dev, Alona Paz

On Wed, 22 Feb 2023 02:40:32 -0800
Andrea Bolognani <abologna@redhat.com> wrote:

> On Tue, Feb 21, 2023 at 08:24:25PM +0100, Stefano Brivio wrote:
> >  		if (!has_json) {
> >  			snprintf(dev_str, ARG_MAX,
> > -			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
> > +			         "%s,%s%i%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}",
> > +			         "{\"driver\":\"%s\",%s%i\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
> >  			         dev->name, dev->template_json, i, dev->template_json_post);
> >  		}  
> 
> I don't think this is going to work.
> 
> The problem is that, while PCI buses are indeed named with increasing
> numbers in integer format (pci.9, pci.10 and so on), PCI slots are
> addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
> this naming convention because it matches QEMU's.
> 
> So if you have an i440fx machine with many devices, they will all[1]
> be put on pci.0, the corresponding arguments will look like
> 
>   -device virtio-net-pci,bus=pci.0,addr=0xa
> 
> and the original version of the code will work correctly.
> 
> If the machine is q35, however, devices will not be put on pcie.0
> directly but rather attached to pcie-root-ports, each one of which
> creates a separate PCI bus. So the arguments will look like
> 
>   -device virtio-net-pci,bus=pci.10,addr=0x0
> 
> where it is the new version of the code that would do the right
> thing.

Oh, oops, I didn't know that.

> What I think needs to happen, is that each pci_dev should contain a
> base value (16 for i440fx and 10 for q35), which is used both in the
> strtol() call used to parse the command line produced by libvirt and
> to decide whether %x or %i should be used with snprintf() to generate
> qrap's own arguments.

Right, I think it makes sense. Will you prepare a patch in this sense
or should I try?

-- 
Stefano


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-22 10:45   ` Stefano Brivio
@ 2023-02-22 12:47     ` Andrea Bolognani
  2023-02-22 13:02       ` Stefano Brivio
  0 siblings, 1 reply; 12+ messages in thread
From: Andrea Bolognani @ 2023-02-22 12:47 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev, Alona Paz

On Wed, Feb 22, 2023 at 11:45:08AM +0100, Stefano Brivio wrote:
> On Wed, 22 Feb 2023 02:40:32 -0800 Andrea Bolognani <abologna@redhat.com> wrote:
> > What I think needs to happen, is that each pci_dev should contain a
> > base value (16 for i440fx and 10 for q35), which is used both in the
> > strtol() call used to parse the command line produced by libvirt and
> > to decide whether %x or %i should be used with snprintf() to generate
> > qrap's own arguments.
>
> Right, I think it makes sense. Will you prepare a patch in this sense
> or should I try?

Sure, I can give it a shot. It might take a bit, because the code is
kinda tricky and there is no test suite (that I'm aware of?) for this
part.

-- 
Andrea Bolognani / Red Hat / Virtualization


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-22 12:47     ` Andrea Bolognani
@ 2023-02-22 13:02       ` Stefano Brivio
  0 siblings, 0 replies; 12+ messages in thread
From: Stefano Brivio @ 2023-02-22 13:02 UTC (permalink / raw)
  To: Andrea Bolognani; +Cc: passt-dev, Alona Paz

On Wed, 22 Feb 2023 04:47:32 -0800
Andrea Bolognani <abologna@redhat.com> wrote:

> On Wed, Feb 22, 2023 at 11:45:08AM +0100, Stefano Brivio wrote:
> > On Wed, 22 Feb 2023 02:40:32 -0800 Andrea Bolognani <abologna@redhat.com> wrote:  
> > > What I think needs to happen, is that each pci_dev should contain a
> > > base value (16 for i440fx and 10 for q35), which is used both in the
> > > strtol() call used to parse the command line produced by libvirt and
> > > to decide whether %x or %i should be used with snprintf() to generate
> > > qrap's own arguments.  
> >
> > Right, I think it makes sense. Will you prepare a patch in this sense
> > or should I try?  
> 
> Sure, I can give it a shot. It might take a bit, because the code is
> kinda tricky and there is no test suite (that I'm aware of?) for this
> part.

Thanks!

Right, sorry, no tests covering qrap at the moment, and the code is
horrible. The KubeVirt integration might help if you have an environment
ready.

-- 
Stefano


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-22 10:40 ` Andrea Bolognani
  2023-02-22 10:45   ` Stefano Brivio
@ 2023-02-22 22:27   ` David Gibson
  2023-02-23 14:06     ` Andrea Bolognani
  1 sibling, 1 reply; 12+ messages in thread
From: David Gibson @ 2023-02-22 22:27 UTC (permalink / raw)
  To: Andrea Bolognani; +Cc: Stefano Brivio, passt-dev, Alona Paz

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

On Wed, Feb 22, 2023 at 02:40:32AM -0800, Andrea Bolognani wrote:
> On Tue, Feb 21, 2023 at 08:24:25PM +0100, Stefano Brivio wrote:
> >  		if (!has_json) {
> >  			snprintf(dev_str, ARG_MAX,
> > -			         "%s,%s%x%s,netdev=hostnet0,x-txburst=4096",
> > +			         "%s,%s%i%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}",
> > +			         "{\"driver\":\"%s\",%s%i\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}",
> >  			         dev->name, dev->template_json, i, dev->template_json_post);
> >  		}
> 
> I don't think this is going to work.
> 
> The problem is that, while PCI buses are indeed named with increasing
> numbers in integer format (pci.9, pci.10 and so on), PCI slots are
> addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
> this naming convention because it matches QEMU's.

Actually, I think we're ok.  PCI slots are addressed in hex by
convention, but AFAICT if you *just* give a slot number, it will
accept either decimal or hex (so addr=10 and addr=0xa are equivalent).
That's *not* true if you use SS.F format to include the function
number - then it expects hex only.  But we're not doing that, so so
always using decimal should be ok here.

Source: set_pci_devfn() in the qemu source

Obviously that's a pretty fragile hack, but that's 'qrap' for you.

-- 
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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-22 22:27   ` David Gibson
@ 2023-02-23 14:06     ` Andrea Bolognani
  2023-02-24  7:14       ` Stefano Brivio
  0 siblings, 1 reply; 12+ messages in thread
From: Andrea Bolognani @ 2023-02-23 14:06 UTC (permalink / raw)
  To: David Gibson; +Cc: Stefano Brivio, passt-dev, Alona Paz

On Thu, Feb 23, 2023 at 09:27:14AM +1100, David Gibson wrote:
> On Wed, Feb 22, 2023 at 02:40:32AM -0800, Andrea Bolognani wrote:
> > I don't think this is going to work.
> >
> > The problem is that, while PCI buses are indeed named with increasing
> > numbers in integer format (pci.9, pci.10 and so on), PCI slots are
> > addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
> > this naming convention because it matches QEMU's.
>
> Actually, I think we're ok.  PCI slots are addressed in hex by
> convention, but AFAICT if you *just* give a slot number, it will
> accept either decimal or hex (so addr=10 and addr=0xa are equivalent).
> That's *not* true if you use SS.F format to include the function
> number - then it expects hex only.  But we're not doing that, so so
> always using decimal should be ok here.
>
> Source: set_pci_devfn() in the qemu source
>
> Obviously that's a pretty fragile hack, but that's 'qrap' for you.

Yeah, even if that happens to work I'd rather not rely on it,
especially since a proper solution doesn't look like it would be a
lot of additional effort.

I've managed to reproduce the original issue in the context of
KubeVirt. I'll hopefully have a patch ready soon.

-- 
Andrea Bolognani / Red Hat / Virtualization


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-23 14:06     ` Andrea Bolognani
@ 2023-02-24  7:14       ` Stefano Brivio
  2023-02-24 19:05         ` Andrea Bolognani
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Brivio @ 2023-02-24  7:14 UTC (permalink / raw)
  To: Andrea Bolognani; +Cc: David Gibson, passt-dev, Alona Paz

On Thu, 23 Feb 2023 06:06:17 -0800
Andrea Bolognani <abologna@redhat.com> wrote:

> On Thu, Feb 23, 2023 at 09:27:14AM +1100, David Gibson wrote:
> > On Wed, Feb 22, 2023 at 02:40:32AM -0800, Andrea Bolognani wrote:  
> > > I don't think this is going to work.
> > >
> > > The problem is that, while PCI buses are indeed named with increasing
> > > numbers in integer format (pci.9, pci.10 and so on), PCI slots are
> > > addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
> > > this naming convention because it matches QEMU's.  
> >
> > Actually, I think we're ok.  PCI slots are addressed in hex by
> > convention, but AFAICT if you *just* give a slot number, it will
> > accept either decimal or hex (so addr=10 and addr=0xa are equivalent).
> > That's *not* true if you use SS.F format to include the function
> > number - then it expects hex only.  But we're not doing that, so so
> > always using decimal should be ok here.
> >
> > Source: set_pci_devfn() in the qemu source
> >
> > Obviously that's a pretty fragile hack, but that's 'qrap' for you.  
> 
> Yeah, even if that happens to work I'd rather not rely on it,
> especially since a proper solution doesn't look like it would be a
> lot of additional effort.
> 
> I've managed to reproduce the original issue in the context of
> KubeVirt. I'll hopefully have a patch ready soon.

Andrea, allow me to do this: I would push this patch meanwhile, along
with the changes for the DNS issue you reported, because that one might
impact many users, and I think it makes sense to have a fix out soon.

I start thinking it's also part of the issue Paul reported for Podman
with pasta here:
  https://github.com/containers/podman/issues/17074

This patch itself can't hurt, and it changes exactly two letters.

As soon as you have something less qrappy we'll go with that (you don't
even need to rebase, I'll revert this one on the tree first).

-- 
Stefano


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-24  7:14       ` Stefano Brivio
@ 2023-02-24 19:05         ` Andrea Bolognani
  2023-02-24 19:32           ` Stefano Brivio
  0 siblings, 1 reply; 12+ messages in thread
From: Andrea Bolognani @ 2023-02-24 19:05 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: David Gibson, passt-dev, Alona Paz

On Fri, Feb 24, 2023 at 08:14:16AM +0100, Stefano Brivio wrote:
> On Thu, 23 Feb 2023 06:06:17 -0800 Andrea Bolognani <abologna@redhat.com> wrote:
> > On Thu, Feb 23, 2023 at 09:27:14AM +1100, David Gibson wrote:
> > > On Wed, Feb 22, 2023 at 02:40:32AM -0800, Andrea Bolognani wrote:
> > > > I don't think this is going to work.
> > > >
> > > > The problem is that, while PCI buses are indeed named with increasing
> > > > numbers in integer format (pci.9, pci.10 and so on), PCI slots are
> > > > addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
> > > > this naming convention because it matches QEMU's.
> > >
> > > Actually, I think we're ok.  PCI slots are addressed in hex by
> > > convention, but AFAICT if you *just* give a slot number, it will
> > > accept either decimal or hex (so addr=10 and addr=0xa are equivalent).
> > > That's *not* true if you use SS.F format to include the function
> > > number - then it expects hex only.  But we're not doing that, so so
> > > always using decimal should be ok here.
> > >
> > > Source: set_pci_devfn() in the qemu source
> > >
> > > Obviously that's a pretty fragile hack, but that's 'qrap' for you.
> >
> > Yeah, even if that happens to work I'd rather not rely on it,
> > especially since a proper solution doesn't look like it would be a
> > lot of additional effort.
> >
> > I've managed to reproduce the original issue in the context of
> > KubeVirt. I'll hopefully have a patch ready soon.
>
> Andrea, allow me to do this: I would push this patch meanwhile, along
> with the changes for the DNS issue you reported, because that one might
> impact many users, and I think it makes sense to have a fix out soon.
>
> I start thinking it's also part of the issue Paul reported for Podman
> with pasta here:
>   https://github.com/containers/podman/issues/17074
>
> This patch itself can't hurt, and it changes exactly two letters.

I strongly disagree with this assessment. This patch merely trades
one set of issues for another one.

In particular, for pc machine types we'd end up producing
bus=pci.0,addr=0x10 for slot 10 instead of bus=pci.0,addr=0xa,
because the addr=0x part is baked into the template. So the QEMU
logic David mentioned above wouldn't kick in at all.

More importantly, for q35 machines we'd start producing decimal bus
numbers while still parsing the ones present in the original command
line as hexadecimal, so things would stop lining up as soon as enough
devices are present, meaning that the issue reported by Alona would
still exist.

> As soon as you have something less qrappy we'll go with that (you don't
> even need to rebase, I'll revert this one on the tree first).

Patches fixing this issue, as well as a few additional ones, are now
on the list. I'll follow up on that thread with some considerations
related to testing the changes.

-- 
Andrea Bolognani / Red Hat / Virtualization


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-24 19:05         ` Andrea Bolognani
@ 2023-02-24 19:32           ` Stefano Brivio
  2023-02-25  1:44             ` David Gibson
  0 siblings, 1 reply; 12+ messages in thread
From: Stefano Brivio @ 2023-02-24 19:32 UTC (permalink / raw)
  To: Andrea Bolognani; +Cc: David Gibson, passt-dev, Alona Paz

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

> On Fri, Feb 24, 2023 at 08:14:16AM +0100, Stefano Brivio wrote:
> > On Thu, 23 Feb 2023 06:06:17 -0800 Andrea Bolognani <abologna@redhat.com> wrote:  
> > > On Thu, Feb 23, 2023 at 09:27:14AM +1100, David Gibson wrote:  
> > > > On Wed, Feb 22, 2023 at 02:40:32AM -0800, Andrea Bolognani wrote:  
> > > > > I don't think this is going to work.
> > > > >
> > > > > The problem is that, while PCI buses are indeed named with increasing
> > > > > numbers in integer format (pci.9, pci.10 and so on), PCI slots are
> > > > > addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
> > > > > this naming convention because it matches QEMU's.  
> > > >
> > > > Actually, I think we're ok.  PCI slots are addressed in hex by
> > > > convention, but AFAICT if you *just* give a slot number, it will
> > > > accept either decimal or hex (so addr=10 and addr=0xa are equivalent).
> > > > That's *not* true if you use SS.F format to include the function
> > > > number - then it expects hex only.  But we're not doing that, so so
> > > > always using decimal should be ok here.
> > > >
> > > > Source: set_pci_devfn() in the qemu source
> > > >
> > > > Obviously that's a pretty fragile hack, but that's 'qrap' for you.  
> > >
> > > Yeah, even if that happens to work I'd rather not rely on it,
> > > especially since a proper solution doesn't look like it would be a
> > > lot of additional effort.
> > >
> > > I've managed to reproduce the original issue in the context of
> > > KubeVirt. I'll hopefully have a patch ready soon.  
> >
> > Andrea, allow me to do this: I would push this patch meanwhile, along
> > with the changes for the DNS issue you reported, because that one might
> > impact many users, and I think it makes sense to have a fix out soon.
> >
> > I start thinking it's also part of the issue Paul reported for Podman
> > with pasta here:
> >   https://github.com/containers/podman/issues/17074
> >
> > This patch itself can't hurt, and it changes exactly two letters.  
> 
> I strongly disagree with this assessment. This patch merely trades
> one set of issues for another one.
> 
> In particular, for pc machine types we'd end up producing
> bus=pci.0,addr=0x10 for slot 10 instead of bus=pci.0,addr=0xa,
> because the addr=0x part is baked into the template. So the QEMU
> logic David mentioned above wouldn't kick in at all.
> 
> More importantly, for q35 machines we'd start producing decimal bus
> numbers while still parsing the ones present in the original command
> line as hexadecimal, so things would stop lining up as soon as enough
> devices are present, meaning that the issue reported by Alona would
> still exist.

Oh, okay, sorry, I thought you and David agreed that it actually
happens to work. But anyway, nice that it doesn't matter now. :)

> > As soon as you have something less qrappy we'll go with that (you don't
> > even need to rebase, I'll revert this one on the tree first).  
> 
> Patches fixing this issue, as well as a few additional ones, are now
> on the list. I'll follow up on that thread with some considerations
> related to testing the changes.

Thanks a lot!

I didn't push out commits or a release today because I hit a false
positive with cppcheck 2.10 (cppcheck bisected but still trying to
grasp the issue), so that will all be for the next week I guess.

-- 
Stefano


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16
  2023-02-24 19:32           ` Stefano Brivio
@ 2023-02-25  1:44             ` David Gibson
  0 siblings, 0 replies; 12+ messages in thread
From: David Gibson @ 2023-02-25  1:44 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: Andrea Bolognani, passt-dev, Alona Paz

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

On Fri, Feb 24, 2023 at 08:32:56PM +0100, Stefano Brivio wrote:
11;rgb:ffff/ffff/ffff> On Fri, 24 Feb 2023 11:05:00 -0800
> Andrea Bolognani <abologna@redhat.com> wrote:
> 
> > On Fri, Feb 24, 2023 at 08:14:16AM +0100, Stefano Brivio wrote:
> > > On Thu, 23 Feb 2023 06:06:17 -0800 Andrea Bolognani <abologna@redhat.com> wrote:  
> > > > On Thu, Feb 23, 2023 at 09:27:14AM +1100, David Gibson wrote:  
> > > > > On Wed, Feb 22, 2023 at 02:40:32AM -0800, Andrea Bolognani wrote:  
> > > > > > I don't think this is going to work.
> > > > > >
> > > > > > The problem is that, while PCI buses are indeed named with increasing
> > > > > > numbers in integer format (pci.9, pci.10 and so on), PCI slots are
> > > > > > addressed using hexadecimal format (0x9, 0xa and so on). libvirt uses
> > > > > > this naming convention because it matches QEMU's.  
> > > > >
> > > > > Actually, I think we're ok.  PCI slots are addressed in hex by
> > > > > convention, but AFAICT if you *just* give a slot number, it will
> > > > > accept either decimal or hex (so addr=10 and addr=0xa are equivalent).
> > > > > That's *not* true if you use SS.F format to include the function
> > > > > number - then it expects hex only.  But we're not doing that, so so
> > > > > always using decimal should be ok here.
> > > > >
> > > > > Source: set_pci_devfn() in the qemu source
> > > > >
> > > > > Obviously that's a pretty fragile hack, but that's 'qrap' for you.  
> > > >
> > > > Yeah, even if that happens to work I'd rather not rely on it,
> > > > especially since a proper solution doesn't look like it would be a
> > > > lot of additional effort.
> > > >
> > > > I've managed to reproduce the original issue in the context of
> > > > KubeVirt. I'll hopefully have a patch ready soon.  
> > >
> > > Andrea, allow me to do this: I would push this patch meanwhile, along
> > > with the changes for the DNS issue you reported, because that one might
> > > impact many users, and I think it makes sense to have a fix out soon.
> > >
> > > I start thinking it's also part of the issue Paul reported for Podman
> > > with pasta here:
> > >   https://github.com/containers/podman/issues/17074
> > >
> > > This patch itself can't hurt, and it changes exactly two letters.  
> > 
> > I strongly disagree with this assessment. This patch merely trades
> > one set of issues for another one.
> > 
> > In particular, for pc machine types we'd end up producing
> > bus=pci.0,addr=0x10 for slot 10 instead of bus=pci.0,addr=0xa,
> > because the addr=0x part is baked into the template. So the QEMU
> > logic David mentioned above wouldn't kick in at all.
> > 
> > More importantly, for q35 machines we'd start producing decimal bus
> > numbers while still parsing the ones present in the original command
> > line as hexadecimal, so things would stop lining up as soon as enough
> > devices are present, meaning that the issue reported by Alona would
> > still exist.
> 
> Oh, okay, sorry, I thought you and David agreed that it actually
> happens to work. But anyway, nice that it doesn't matter now. :)

Sorry, I missed that '0x' was in the template.  Of course that can be
very easily fixed.

> > > As soon as you have something less qrappy we'll go with that (you don't
> > > even need to rebase, I'll revert this one on the tree first).  
> > 
> > Patches fixing this issue, as well as a few additional ones, are now
> > on the list. I'll follow up on that thread with some considerations
> > related to testing the changes.
> 
> Thanks a lot!
> 
> I didn't push out commits or a release today because I hit a false
> positive with cppcheck 2.10 (cppcheck bisected but still trying to
> grasp the issue), so that will all be for the next week I guess.
> 

-- 
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 --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-02-25  1:55 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-21 19:24 [PATCH] qrap: Pass PCI device numbers to qemu in base 10, not in base 16 Stefano Brivio
2023-02-22  0:27 ` David Gibson
2023-02-22 10:40 ` Andrea Bolognani
2023-02-22 10:45   ` Stefano Brivio
2023-02-22 12:47     ` Andrea Bolognani
2023-02-22 13:02       ` Stefano Brivio
2023-02-22 22:27   ` David Gibson
2023-02-23 14:06     ` Andrea Bolognani
2023-02-24  7:14       ` Stefano Brivio
2023-02-24 19:05         ` Andrea Bolognani
2023-02-24 19:32           ` Stefano Brivio
2023-02-25  1:44             ` David Gibson

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).