[libvirt] [PATCH 2/2] qemu: Remove host-passthrough validation check for host-phys-bits=on

[Jim Fehlig]

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