Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Andrea Bolognani]

On Wed, 2018-06-06 at 12:04 +0200, Peter Krempa wrote:
> On Wed, Jun 06, 2018 at 12:02:12 +0200, Andrea Bolognani wrote:
> > On Wed, 2018-06-06 at 11:14 +1000, David Gibson wrote:
> > > TBH, if the user is already thinking about page sizes at this low
> > > level, I don't think doing it by shift is much of a stretch.
> > 
> > I disagree that it's such a low level setting that only people who
> > can do bitwise shifts off the top of their heads will want to
> > tweak: if it were, then libvirt will probably not need to expose
> > it in the first place :)
> 
> Well, it will most probably be configured by some policy code from a
> upper layer mgmt tool, so I don't think users will actually ever touch
> it.

Probably. Still not a good reason not to ensure it's human-friendly
at every layer of the stack IMHO.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Peter Krempa]

On Wed, Jun 06, 2018 at 12:02:12 +0200, Andrea Bolognani wrote:
> On Wed, 2018-06-06 at 11:14 +1000, David Gibson wrote:
> > Personally I think the shift is *more* usable than a raw page size,
> > since the latter is inevitably going to involve counting a bunch of
> > zeroes to see if it's the number you meant.  Allowing forms like "16M"
> > / "16G" could be nicer; not sure if we have existing helper functions
> > which will make that easy.
> 
> I was indeed thinking about the latter.
> 
> > TBH, if the user is already thinking about page sizes at this low
> > level, I don't think doing it by shift is much of a stretch.
> 
> I disagree that it's such a low level setting that only people who
> can do bitwise shifts off the top of their heads will want to
> tweak: if it were, then libvirt will probably not need to expose
> it in the first place :)

Well, it will most probably be configured by some policy code from a
upper layer mgmt tool, so I don't think users will actually ever touch
it.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Andrea Bolognani]

On Wed, 2018-06-06 at 11:14 +1000, David Gibson wrote:
> Personally I think the shift is *more* usable than a raw page size,
> since the latter is inevitably going to involve counting a bunch of
> zeroes to see if it's the number you meant.  Allowing forms like "16M"
> / "16G" could be nicer; not sure if we have existing helper functions
> which will make that easy.

I was indeed thinking about the latter.

> TBH, if the user is already thinking about page sizes at this low
> level, I don't think doing it by shift is much of a stretch.

I disagree that it's such a low level setting that only people who
can do bitwise shifts off the top of their heads will want to
tweak: if it were, then libvirt will probably not need to expose
it in the first place :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[David Gibson]

On Tue, Jun 05, 2018 at 04:05:38PM +0200, Andrea Bolognani wrote:
> On Mon, 2018-06-04 at 11:33 +1000, David Gibson wrote:
> > On Thu, May 24, 2018 at 07:34:25AM +0200, Peter Krempa wrote:
> > > On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
> > > > To be fair, it would perhaps make sense to perform the conversion
> > > > directly inside QEMU, in order to make it more convenient not only
> > > > for libvirt but for for people driving it directly as well.
> > > 
> > > If strictly only powers of two make sense for this knob then this gives
> > > you input validation for free. On the other hand, specifying a big
> > > number can overflow internally if it is ever used in the non-exponent
> > > form. I think the format does not matter much, since libvirt's job is to
> > > shield users from such weirdness.
> > 
> > Yeah, the above is basically my reasoning for using the exponent, not
> > the final page size.
> 
> As Peter mentioned, what format is used doesn't matter much from
> libvirt's point of view, and in fact this RFC already implements
> the necessary format conversion; however, it might be convenient
> for people spawning QEMU directly to be able to specify the page
> size in a more human-friendly format.
> 
> How do you feel about that? If that's off the table, we'll just
> go ahead with the current implementation.

So, I need to keep the internal representation as a shift, since we
only have 8 bits in the capability field.  But that doesn't prevent
changing the command line representation, since we could convert in
the getter/setter functions.

Personally I think the shift is *more* usable than a raw page size,
since the latter is inevitably going to involve counting a bunch of
zeroes to see if it's the number you meant.  Allowing forms like "16M"
/ "16G" could be nicer; not sure if we have existing helper functions
which will make that easy.

TBH, if the user is already thinking about page sizes at this low
level, I don't think doing it by shift is much of a stretch.

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


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Andrea Bolognani]

On Mon, 2018-06-04 at 11:33 +1000, David Gibson wrote:
> On Thu, May 24, 2018 at 07:34:25AM +0200, Peter Krempa wrote:
> > On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
> > > To be fair, it would perhaps make sense to perform the conversion
> > > directly inside QEMU, in order to make it more convenient not only
> > > for libvirt but for for people driving it directly as well.
> > 
> > If strictly only powers of two make sense for this knob then this gives
> > you input validation for free. On the other hand, specifying a big
> > number can overflow internally if it is ever used in the non-exponent
> > form. I think the format does not matter much, since libvirt's job is to
> > shield users from such weirdness.
> 
> Yeah, the above is basically my reasoning for using the exponent, not
> the final page size.

As Peter mentioned, what format is used doesn't matter much from
libvirt's point of view, and in fact this RFC already implements
the necessary format conversion; however, it might be convenient
for people spawning QEMU directly to be able to specify the page
size in a more human-friendly format.

How do you feel about that? If that's off the table, we'll just
go ahead with the current implementation.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[David Gibson]

On Thu, May 24, 2018 at 07:34:25AM +0200, Peter Krempa wrote:
> On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
> > On Wed, 2018-05-23 at 18:40 +0200, Peter Krempa wrote:
> > > On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
> > > > +/* QEMU expects the argument to be a number of left shifts:
> > > > + * for example, if you wanted to limit the guest to 4 KiB 
> > > > pages,
> > > > + * since 4096 == 1 << 12, you would need to add 
> > > > cap-hpt-mps=12
> > > > + * to the command line.
> > > 
> > > So basically you need to pass the exponent of a power of 2 that yields
> > > this number. The number of left shifts may be slightly confusing ...
> > 
> > I guess it depends on the reader; the two definitions are
> > equivalent anyway, so no harm in having both in the comment :)
> 
> Using both are fine, but without mentioning that it is in fact a power of
> two-only thing I was thinking that the interface is off by one or
> something:
> 
> 4096 == 1 << 12 == 2 << 11
> 
> > In general, I'd say it's not the most user-friendly interface on
> > QEMU's side, but I believe it's dictated by hardware / emulator
> > details, given how it ends up being used: see
> > 
> >   http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg02822.html
> > 
> > To be fair, it would perhaps make sense to perform the conversion
> > directly inside QEMU, in order to make it more convenient not only
> > for libvirt but for for people driving it directly as well.
> 
> If strictly only powers of two make sense for this knob then this gives
> you input validation for free. On the other hand, specifying a big
> number can overflow internally if it is ever used in the non-exponent
> form. I think the format does not matter much, since libvirt's job is to
> shield users from such weirdness.

Yeah, the above is basically my reasoning for using the exponent, not
the final page size.

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


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Peter Krempa]

On Wed, May 23, 2018 at 19:09:59 +0200, Andrea Bolognani wrote:
> On Wed, 2018-05-23 at 18:40 +0200, Peter Krempa wrote:
> > On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
> > > +/* QEMU expects the argument to be a number of left shifts:
> > > + * for example, if you wanted to limit the guest to 4 KiB 
> > > pages,
> > > + * since 4096 == 1 << 12, you would need to add 
> > > cap-hpt-mps=12
> > > + * to the command line.
> > 
> > So basically you need to pass the exponent of a power of 2 that yields
> > this number. The number of left shifts may be slightly confusing ...
> 
> I guess it depends on the reader; the two definitions are
> equivalent anyway, so no harm in having both in the comment :)

Using both are fine, but without mentioning that it is in fact a power of
two-only thing I was thinking that the interface is off by one or
something:

4096 == 1 << 12 == 2 << 11

> In general, I'd say it's not the most user-friendly interface on
> QEMU's side, but I believe it's dictated by hardware / emulator
> details, given how it ends up being used: see
> 
>   http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg02822.html
> 
> To be fair, it would perhaps make sense to perform the conversion
> directly inside QEMU, in order to make it more convenient not only
> for libvirt but for for people driving it directly as well.

If strictly only powers of two make sense for this knob then this gives
you input validation for free. On the other hand, specifying a big
number can overflow internally if it is ever used in the non-exponent
form. I think the format does not matter much, since libvirt's job is to
shield users from such weirdness.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Andrea Bolognani]

On Wed, 2018-05-23 at 18:40 +0200, Peter Krempa wrote:
> On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
> > +/* QEMU expects the argument to be a number of left shifts:
> > + * for example, if you wanted to limit the guest to 4 KiB 
> > pages,
> > + * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12
> > + * to the command line.
> 
> So basically you need to pass the exponent of a power of 2 that yields
> this number. The number of left shifts may be slightly confusing ...

I guess it depends on the reader; the two definitions are
equivalent anyway, so no harm in having both in the comment :)

In general, I'd say it's not the most user-friendly interface on
QEMU's side, but I believe it's dictated by hardware / emulator
details, given how it ends up being used: see

  http://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg02822.html

To be fair, it would perhaps make sense to perform the conversion
directly inside QEMU, in order to make it more convenient not only
for libvirt but for for people driving it directly as well.

CC'ing David so that he can weigh in on the idea.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Peter Krempa]

On Wed, May 23, 2018 at 18:18:02 +0200, Andrea Bolognani wrote:
> This makes the feature fully functional.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1571078
> 
> Signed-off-by: Andrea Bolognani 
> ---
>  src/qemu/qemu_command.c  | 26 
>  tests/qemuxml2argvdata/pseries-features.args |  3 ++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b446a08613..983839e81c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7193,6 +7193,32 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>  
>  virBufferAsprintf(, ",resize-hpt=%s", str);
>  }
> +
> +if (def->hpt_maxpagesize > 0) {
> +unsigned long long tmp = def->hpt_maxpagesize;
> +unsigned int shifts = 0;
> +
> +if (!virQEMUCapsGet(qemuCaps, 
> QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS)) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +   _("Limiting the page size for HPT guest is "
> + "not supported by this QEMU binary"));
> +goto cleanup;
> +}
> +
> +/* QEMU expects the argument to be a number of left shifts:
> + * for example, if you wanted to limit the guest to 4 KiB pages,
> + * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12
> + * to the command line.

So basically you need to pass the exponent of a power of 2 that yields
this number. The number of left shifts may be slightly confusing ...

> + *
> + * Convert from our internal representation, which is bytes,
> + * to the one QEMU expects */
> +while (tmp > 1) {
> +tmp = tmp >> 1;
> +shifts++;
> +}
> +
> +virBufferAsprintf(, ",cap-hpt-mps=%u", shifts);
> +}
>  }
>  
>  if (cpu && cpu->model &&
> diff --git a/tests/qemuxml2argvdata/pseries-features.args 
> b/tests/qemuxml2argvdata/pseries-features.args
> index f5c1959cca..4e581a69a1 100644
> --- a/tests/qemuxml2argvdata/pseries-features.args
> +++ b/tests/qemuxml2argvdata/pseries-features.args
> @@ -7,7 +7,8 @@ QEMU_AUDIO_DRV=none \
>  /usr/bin/qemu-system-ppc64 \
>  -name guest \
>  -S \
> --machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required \
> +-machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\
> +cap-hpt-mps=30 \
>  -m 512 \
>  -smp 1,sockets=1,cores=1,threads=1 \
>  -uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
> -- 
> 2.17.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [RFC PATCH 6/6] qemu: Format pseries.cap-hpt-mps on the command line

[Andrea Bolognani]

This makes the feature fully functional.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1571078

Signed-off-by: Andrea Bolognani 
---
 src/qemu/qemu_command.c  | 26 
 tests/qemuxml2argvdata/pseries-features.args |  3 ++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index b446a08613..983839e81c 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7193,6 +7193,32 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
 
 virBufferAsprintf(, ",resize-hpt=%s", str);
 }
+
+if (def->hpt_maxpagesize > 0) {
+unsigned long long tmp = def->hpt_maxpagesize;
+unsigned int shifts = 0;
+
+if (!virQEMUCapsGet(qemuCaps, 
QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MPS)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+   _("Limiting the page size for HPT guest is "
+ "not supported by this QEMU binary"));
+goto cleanup;
+}
+
+/* QEMU expects the argument to be a number of left shifts:
+ * for example, if you wanted to limit the guest to 4 KiB pages,
+ * since 4096 == 1 << 12, you would need to add cap-hpt-mps=12
+ * to the command line.
+ *
+ * Convert from our internal representation, which is bytes,
+ * to the one QEMU expects */
+while (tmp > 1) {
+tmp = tmp >> 1;
+shifts++;
+}
+
+virBufferAsprintf(, ",cap-hpt-mps=%u", shifts);
+}
 }
 
 if (cpu && cpu->model &&
diff --git a/tests/qemuxml2argvdata/pseries-features.args 
b/tests/qemuxml2argvdata/pseries-features.args
index f5c1959cca..4e581a69a1 100644
--- a/tests/qemuxml2argvdata/pseries-features.args
+++ b/tests/qemuxml2argvdata/pseries-features.args
@@ -7,7 +7,8 @@ QEMU_AUDIO_DRV=none \
 /usr/bin/qemu-system-ppc64 \
 -name guest \
 -S \
--machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required \
+-machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\
+cap-hpt-mps=30 \
 -m 512 \
 -smp 1,sockets=1,cores=1,threads=1 \
 -uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list