[libvirt] [PATCH 0/2] Minor fixes for maxphysaddr
Lin Ma (2): docs: Fix missing slashes in the maxphysaddr example qemu: Remove host-passthrough validation check for host-phys-bits=on docs/formatdomain.rst | 4 ++-- src/qemu/qemu_validate.c | 7 ------- 2 files changed, 2 insertions(+), 9 deletions(-) -- 2.26.2
Signed-off-by: Lin Ma <lma@suse.com> --- docs/formatdomain.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ed0d9c1959..56ca5efe43 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1336,7 +1336,7 @@ following collection of elements. :since:`Since 0.7.5` <vendor>Intel</vendor> <topology sockets='1' dies='1' cores='2' threads='1'/> <cache level='3' mode='emulate'/> - <maxphysaddr mode='emulate' bits='42'> + <maxphysaddr mode='emulate' bits='42'/> <feature policy='disable' name='lahf_lm'/> </cpu> ... @@ -1353,7 +1353,7 @@ following collection of elements. :since:`Since 0.7.5` <cpu mode='host-passthrough' migratable='off'> <cache mode='passthrough'/> - <maxphysaddr mode='passthrough'> + <maxphysaddr mode='passthrough'/> <feature policy='disable' name='lahf_lm'/> ... -- 2.26.2
On 8/17/22 02:19, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- docs/formatdomain.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Thanks for noticing the omission! Reviewed-by: Jim Fehlig <jfehlig@suse.com>
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ed0d9c1959..56ca5efe43 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1336,7 +1336,7 @@ following collection of elements. :since:`Since 0.7.5` <vendor>Intel</vendor> <topology sockets='1' dies='1' cores='2' threads='1'/> <cache level='3' mode='emulate'/> - <maxphysaddr mode='emulate' bits='42'> + <maxphysaddr mode='emulate' bits='42'/> <feature policy='disable' name='lahf_lm'/> </cpu> ... @@ -1353,7 +1353,7 @@ following collection of elements. :since:`Since 0.7.5`
<cpu mode='host-passthrough' migratable='off'> <cache mode='passthrough'/> - <maxphysaddr mode='passthrough'> + <maxphysaddr mode='passthrough'/> <feature policy='disable' name='lahf_lm'/> ...
On 8/17/22 10:19, Lin Ma wrote:
Signed-off-by: Lin Ma <lma@suse.com> --- docs/formatdomain.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index ed0d9c1959..56ca5efe43 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -1336,7 +1336,7 @@ following collection of elements. :since:`Since 0.7.5` <vendor>Intel</vendor> <topology sockets='1' dies='1' cores='2' threads='1'/> <cache level='3' mode='emulate'/> - <maxphysaddr mode='emulate' bits='42'> + <maxphysaddr mode='emulate' bits='42'/> <feature policy='disable' name='lahf_lm'/> </cpu> ... @@ -1353,7 +1353,7 @@ following collection of elements. :since:`Since 0.7.5`
<cpu mode='host-passthrough' migratable='off'> <cache mode='passthrough'/> - <maxphysaddr mode='passthrough'> + <maxphysaddr mode='passthrough'/> <feature policy='disable' name='lahf_lm'/> ...
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Besides the -cpu host, The host-phys-bits=on applies to custom or max cpu model, So the host-passthrough validation check is unnecessary for maxphysaddr with mode='passthrough'. Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_validate.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 6e457f3814..e60575d8a0 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -344,13 +344,6 @@ qemuValidateDomainDefCpu(virQEMUDriver *driver, switch (addr->mode) { case VIR_CPU_MAX_PHYS_ADDR_MODE_PASSTHROUGH: - if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("CPU maximum physical address bits mode '%s' can only be used with '%s' CPUs"), - virCPUMaxPhysAddrModeTypeToString(addr->mode), - virCPUModeTypeToString(VIR_CPU_MODE_HOST_PASSTHROUGH)); - return -1; - } if (addr->bits != -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("CPU maximum physical address bits number specification cannot be used with mode='%s'"), -- 2.26.2
On 8/17/22 02:19, Lin Ma wrote:
Besides the -cpu host, The host-phys-bits=on applies to custom or max cpu model, So the host-passthrough validation check is unnecessary for maxphysaddr with mode='passthrough'.
I wonder why Dario added the check? He even added a test for it: cpu-phys-bits-passthrough2 in qemuxml2argvtest. He is off now, so we'll have to await his return for an answer.
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_validate.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 6e457f3814..e60575d8a0 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -344,13 +344,6 @@ qemuValidateDomainDefCpu(virQEMUDriver *driver,
switch (addr->mode) { case VIR_CPU_MAX_PHYS_ADDR_MODE_PASSTHROUGH: - if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("CPU maximum physical address bits mode '%s' can only be used with '%s' CPUs"), - virCPUMaxPhysAddrModeTypeToString(addr->mode), - virCPUModeTypeToString(VIR_CPU_MODE_HOST_PASSTHROUGH)); - return -1; - }
If the check is removed the above mentioned test needs removed too. Regards, Jim
if (addr->bits != -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("CPU maximum physical address bits number specification cannot be used with mode='%s'"),
On 8/17/22 08:47, Jim Fehlig wrote:
On 8/17/22 02:19, Lin Ma wrote:
Besides the -cpu host, The host-phys-bits=on applies to custom or max cpu model, So the host-passthrough validation check is unnecessary for maxphysaddr with mode='passthrough'.
I wonder why Dario added the check? He even added a test for it: cpu-phys-bits-passthrough2 in qemuxml2argvtest. He is off now, so we'll have to await his return for an answer.
BTW, I'm not doubting your analysis. In fact, I tested with one of the known CPU models (-cpu Cascadelake-Server,...,host-phys-bits=on) and it worked fine with a 7TB VM. I'd still like to know why Dario added the check. Perhaps he encountered some problematic corner cases. Thanks! Jim
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_validate.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 6e457f3814..e60575d8a0 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -344,13 +344,6 @@ qemuValidateDomainDefCpu(virQEMUDriver *driver, switch (addr->mode) { case VIR_CPU_MAX_PHYS_ADDR_MODE_PASSTHROUGH: - if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("CPU maximum physical address bits mode '%s' can only be used with '%s' CPUs"), - virCPUMaxPhysAddrModeTypeToString(addr->mode), - virCPUModeTypeToString(VIR_CPU_MODE_HOST_PASSTHROUGH)); - return -1; - }
If the check is removed the above mentioned test needs removed too.
Regards, Jim
if (addr->bits != -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("CPU maximum physical address bits number specification cannot be used with mode='%s'"),
On Wed, 2022-08-17 at 11:18 -0600, Jim Fehlig wrote:
On 8/17/22 08:47, Jim Fehlig wrote:
On 8/17/22 02:19, Lin Ma wrote:
Besides the -cpu host, The host-phys-bits=on applies to custom or max cpu model, So the host-passthrough validation check is unnecessary for maxphysaddr with mode='passthrough'.
I wonder why Dario added the check? He even added a test for it: cpu-phys-bits-passthrough2 in qemuxml2argvtest. He is off now, so we'll have to await his return for an answer.
BTW, I'm not doubting your analysis. In fact, I tested with one of the known CPU models (-cpu Cascadelake-Server,...,host-phys-bits=on) and it worked fine with a 7TB VM. I'd still like to know why Dario added the check. Perhaps he encountered some problematic corner cases.
Not really, no. IIRC, QEMU was behaving differently than how it work this days, when I wrote this, but even back then host-phys-bits=on was fine with -cpu != host. The check is there just because I thought that, if you're using a specific CPU model, and not host-passthrough, that's probably because you don't want the host details to "leak" into the VM (for migration or whatever). Hence, you shouldn't use phys-bits passthrough either. Like, you need the CPU to be CascadeLake-Server for migration purposes, your host has 52 bits and you know you need a large VM so instead of wasting time doing the math, you just go for passthrough (for the phys bits, of course). Then you migrate on an host with 46 bits, which would also have been fine for the size VM, but now, since you've basically used 52, it's a problem. But maybe this is not something that should be solved at this level, as it's probably the job of `virsh cpu-baseline`? Does that includes and takes into account this aspect (phys-bits) already? So, yeah, if that check was too much policying, I think it's fine to get rid of it. Nothing should explode :-) Thanks and Regards -- Dario Faggioli, Ph.D http://about.me/dario.faggioli Virtualization Software Engineer SUSE Labs, SUSE https://www.suse.com/ ------------------------------------------------------------------- <<This happens because _I_ choose it to happen!>> (Raistlin Majere)
On 9/2/22 08:50, Dario Faggioli wrote:
On Wed, 2022-08-17 at 11:18 -0600, Jim Fehlig wrote:
On 8/17/22 08:47, Jim Fehlig wrote:
On 8/17/22 02:19, Lin Ma wrote:
Besides the -cpu host, The host-phys-bits=on applies to custom or max cpu model, So the host-passthrough validation check is unnecessary for maxphysaddr with mode='passthrough'.
I wonder why Dario added the check? He even added a test for it: cpu-phys-bits-passthrough2 in qemuxml2argvtest. He is off now, so we'll have to await his return for an answer.
BTW, I'm not doubting your analysis. In fact, I tested with one of the known CPU models (-cpu Cascadelake-Server,...,host-phys-bits=on) and it worked fine with a 7TB VM. I'd still like to know why Dario added the check. Perhaps he encountered some problematic corner cases.
Not really, no. IIRC, QEMU was behaving differently than how it work this days, when I wrote this, but even back then host-phys-bits=on was fine with -cpu != host.
The check is there just because I thought that, if you're using a specific CPU model, and not host-passthrough, that's probably because you don't want the host details to "leak" into the VM (for migration or whatever). Hence, you shouldn't use phys-bits passthrough either.
Hmm, an interesting point...
Like, you need the CPU to be CascadeLake-Server for migration purposes, your host has 52 bits and you know you need a large VM so instead of wasting time doing the math, you just go for passthrough (for the phys bits, of course). Then you migrate on an host with 46 bits, which would also have been fine for the size VM, but now, since you've basically used 52, it's a problem.
but at least for the migration case we have that covered by the ABI stability check https://gitlab.com/libvirt/libvirt/-/blob/master/src/conf/cpu_conf.c#L1113
But maybe this is not something that should be solved at this level, as it's probably the job of `virsh cpu-baseline`? Does that includes and takes into account this aspect (phys-bits) already?
No, it doesn't. Could be added as a separate patch if desired.
So, yeah, if that check was too much policying, I think it's fine to get rid of it. Nothing should explode :-)
Based on my somewhat limited testing, I think it is fine too. Hopefully others on the list will speak up if they know of a reason to keep this check. In the meantime, @Lin, I suggest sending a V2 of this patch with the now failing test removed. Thanks, Jim
On 9/2/22 09:24, Jim Fehlig wrote:
On 9/2/22 08:50, Dario Faggioli wrote:
On Wed, 2022-08-17 at 11:18 -0600, Jim Fehlig wrote:
On 8/17/22 08:47, Jim Fehlig wrote:
On 8/17/22 02:19, Lin Ma wrote:
Besides the -cpu host, The host-phys-bits=on applies to custom or max cpu model, So the host-passthrough validation check is unnecessary for maxphysaddr with mode='passthrough'.
I wonder why Dario added the check? He even added a test for it: cpu-phys-bits-passthrough2 in qemuxml2argvtest. He is off now, so we'll have to await his return for an answer.
BTW, I'm not doubting your analysis. In fact, I tested with one of the known CPU models (-cpu Cascadelake-Server,...,host-phys-bits=on) and it worked fine with a 7TB VM. I'd still like to know why Dario added the check. Perhaps he encountered some problematic corner cases.
Not really, no. IIRC, QEMU was behaving differently than how it work this days, when I wrote this, but even back then host-phys-bits=on was fine with -cpu != host.
The check is there just because I thought that, if you're using a specific CPU model, and not host-passthrough, that's probably because you don't want the host details to "leak" into the VM (for migration or whatever). Hence, you shouldn't use phys-bits passthrough either.
Hmm, an interesting point...
Like, you need the CPU to be CascadeLake-Server for migration purposes, your host has 52 bits and you know you need a large VM so instead of wasting time doing the math, you just go for passthrough (for the phys bits, of course). Then you migrate on an host with 46 bits, which would also have been fine for the size VM, but now, since you've basically used 52, it's a problem.
but at least for the migration case we have that covered by the ABI stability check
https://gitlab.com/libvirt/libvirt/-/blob/master/src/conf/cpu_conf.c#L1113
Wait, that just checks the stability of any config changes provided by the user when starting the migration. It doesn't check such differences between hosts involved in the migration. IIRC that's done elsewhere in the qemu driver, but I'll need to refresh my memory... Regards, Jim
On 8/17/22 10:19, Lin Ma wrote:
Besides the -cpu host, The host-phys-bits=on applies to custom or max cpu model, So the host-passthrough validation check is unnecessary for maxphysaddr with mode='passthrough'.
Signed-off-by: Lin Ma <lma@suse.com> --- src/qemu/qemu_validate.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c index 6e457f3814..e60575d8a0 100644 --- a/src/qemu/qemu_validate.c +++ b/src/qemu/qemu_validate.c @@ -344,13 +344,6 @@ qemuValidateDomainDefCpu(virQEMUDriver *driver,
switch (addr->mode) { case VIR_CPU_MAX_PHYS_ADDR_MODE_PASSTHROUGH: - if (def->cpu->mode != VIR_CPU_MODE_HOST_PASSTHROUGH) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("CPU maximum physical address bits mode '%s' can only be used with '%s' CPUs"), - virCPUMaxPhysAddrModeTypeToString(addr->mode), - virCPUModeTypeToString(VIR_CPU_MODE_HOST_PASSTHROUGH)); - return -1; - } if (addr->bits != -1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("CPU maximum physical address bits number specification cannot be used with mode='%s'"),
Hmm what was the reason for this, maybe Dario knows? In my view we definitely do not want to restrict this to VIR_CPU_MODE_HOST_PASSTHROUGH only, but this should be restricted in my view to QEMU hw accelerators that use host cpuid (as per accel_uses_host_cpuid() -> true), which are KVM and HVF. Not sure how to properly check that? Ciao, CLaudio
participants (4)
-
Claudio Fontana -
Dario Faggioli -
Jim Fehlig -
Lin Ma