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
