On 9/14/23 2:11 AM, Tyler Stachecki wrote:
> On Thu, Sep 14, 2023 at 04:15:54AM -0300, Leonardo Bras wrote:
>> So, IIUC, the xfeatures from the source guest will be different than the
>> xfeatures of the target (destination) guest. Is that correct?
>
> Correct.
>
>> It does not seem right to me. I mean, from the guest viewpoint, some
>> features will simply vanish during execution, and this could lead to major
>> issues in the guest.
I fully agree with this.
I think the original commit ad856280ddea ("x86/kvm/fpu: Limit guest
user_xfeatures to supported bits of XCR0") is for the source server, not
destination server.
That is:
1. Without the commit (src and dst), something bad may happen.
2. With the commit on src, issue is fixed.
3. With the commit only dst, it is expected that issue is not fixed.
Therefore, from administrator's perspective, the bugfix should always be applied
no the source server, in order to succeed the migration.
BTW, we may not be able to use commit ad856280ddea in the Fixes tag.
>
> My assumption is that the guest CPU model should confine access to registers
> that make sense for that (guest) CPU.
>
> e.g., take a host CPU capable of AVX-512 running a guest CPU model that only
> has AVX-256. If the guest suddenly loses the top 256 bits of %zmm*, it should
> not really be perceivable as %ymm architecturally remains unchanged.
>
> Though maybe I'm being too rash here? Is there a case where this assumption
> breaks down?
>
>> The idea here is that if the target (destination) host can't provide those
>> features for the guest, then migration should fail.
>>
>> I mean, qemu should fail the migration, and that's correct behavior.
>> Is it what is happening?
>
> Unfortunately, no, it is not... and that is biggest concern right now.
>
> I do see some discussion between Peter and you on this topic and see that
> there was an RFC to implement such behavior stemming from it, here:
> https://lore.kernel.org/qemu-devel/20220607230645.53950-1-peterx@xxxxxxxxxx/
I agree that bug is at QEMU side, not KVM side.
It is better to improve at QEMU side.
4508 int kvm_arch_put_registers(CPUState *cpu, int level)
4509 {
4510 X86CPU *x86_cpu = X86_CPU(cpu);
4511 int ret;
... ...
4546 ret = kvm_put_xsave(x86_cpu);
4547 if (ret < 0) {
4548 return ret;
4549 }
... ...--> the rest of kvm_arch_put_registers() won't execute !!!
Thank you very much!
Dongli Zhang
>
> ... though I do not believe that work ever landed in the tree. Looking at
> qemu's master branch now, the error from kvm_arch_put_registers is just
> discarded in do_kvm_cpu_synchronize_post_init...
>
> ```
> static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, run_on_cpu_data arg)
> {
> kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
> cpu->vcpu_dirty = false;
> }
> ```
>
> Best,
> Tyler
