Re: [libvirt] [Qemu-devel] [PATCH/QEMU] s390x/kvm: use cpu_model_available for guarded storage on compat machines
On 10/26/2017 10:13 AM, Christian Borntraeger wrote:
On 10/26/2017 01:35 AM, Halil Pasic wrote:
try the most interesting scenarios out.
>
> The idea of the patch is very clear, but I don't understand the bigger gs
> feature context fully.
>
> From what I read in the code, the attempt to enable the gs capability in
> the kernel is made regardless of the cpu model. If the attempt was
> successful kvm_s390_get_gs will keep returning true. That would in turn
> affect migration, as far as I see (usages of kvm_s390_get_gs). I could
> not figure out how does gs being turned off via cpu-model (-cpu
> z14,gs=off) does turn of gs support -- at least not the details. I wanted
> to give a timely review, so I've limited myself there.
When the CPU model turns off gs, facility bit 133 will be turned off.
Then the kernel does the right thing, see priv.c handle_gs.
guarded storage is enabled lazily. Whenever the guest issues a Gs instruction
we will get an exit and only enable GS if facility 133 is set.
>
> From what I see, this patch does what it advertises, and since I think
> it's the right thing to do in the current situation I gonna give it an:
> Acked-by: Halil Pasic <pasic(a)linux.vnet.ibm.com>
>
> At the same time, I would prefer the commit message being reworded. IMHO
> this patch is a good stop-gap measure, but essentially it trades an
> annoying and obvious bug for a subtle and hopefully painless one.
>
> Let me explain this last statement. For starters, I do share some of the
> concerns Boris has voiced. I won't repeat those. Same goes for the
> example Christian paraphrased previously, and the the fear of an implicit
> requirement for having to support a Cartesian product of the advertised
> machine-types and cpu-models (for each qemu binary).
I will try to come up with a patch description that explains the open issues
and it will tell that additional might or might not be necessary depending
on followup discussions.
I would be already happy with adding something like:
During the discussion enabling cpu-model features for preexisting
machine-types came out as at least controversial. We agreed to implement
this patch as a stop-gap measure for the next release. What is a clean
enough solution shall be decided without time pressure.
I will schedule this patch for 2.11 then.
Sounds like a plan.
Cheers,
Halil