qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH uq/master 2/2] KVM: make XSAVE support more robu


From: Gleb Natapov
Subject: Re: [Qemu-devel] [PATCH uq/master 2/2] KVM: make XSAVE support more robust
Date: Mon, 9 Sep 2013 13:41:55 +0300

On Mon, Sep 09, 2013 at 11:50:03AM +0200, Paolo Bonzini wrote:
> Il 09/09/2013 11:18, Gleb Natapov ha scritto:
> > On Mon, Sep 09, 2013 at 10:51:58AM +0200, Paolo Bonzini wrote:
> >> Il 08/09/2013 13:52, Gleb Natapov ha scritto:
> >>> On Thu, Sep 05, 2013 at 03:06:22PM +0200, Paolo Bonzini wrote:
> >>>> QEMU moves state from CPUArchState to struct kvm_xsave and back when it
> >>>> invokes the KVM_*_XSAVE ioctls.  Because it doesn't treat the XSAVE
> >>>> region as an opaque blob, it might be impossible to set some state on
> >>>> the destination if migrating to an older version.
> >>>>
> >>>> This patch blocks migration if it finds that unsupported bits are set
> >>>> in the XSTATE_BV header field.  To make this work robustly, QEMU should
> >>>> only report in env->xstate_bv those fields that will actually end up
> >>>> in the migration stream.
> >>>
> >>> We usually handle host cpu differences in cpuid layer, not by trying to
> >>> validate migration data.
> >>
> >> Actually we do both.  QEMU for example detects invalid subsections and
> >> blocks migration, and CPU differences also result in subsections that
> >> the destination does not know.
> >>
> > That's different from what you do here though. If xstate_bv was in its
> > separate subsection things would be easier, but it is not.
> 
> I agree.  And also if YMM was in its separate subsections; future XSAVE
> states will likely use subsections (whose presence is keyed off bits in
> env->xstate_bv).
> 
> >> However, KVM_GET/SET_XSAVE should still return all values supported by
> >> the hypervisor, independent of the supported CPUID bits.
> >
> > Why?
> 
> Because this is not talking to the guest, it is talking to userspace.
> 
> The VCPU state is more than what is visible to the guest, and returning
If a state does not affect guest in any way there is not reason to
migrate it.

> all of it seems more consistent with the rest of the KVM API.  For
> example, KVM_GET_FPU always returns SSE state even if the CPUID lacks
> SSE and/or FXSR.
There are counter examples too :) If APIC is not created we do not return
fake information on GET_IRQCHIP.  I think nobody expected FPU state to
grow indefinitely, so fixed, inflexible API was introduced, but now,
when CPU state has flexible extended state management it make sense to
model it in the API too.

> 
> >> A well-behaved guest should not have modified that state anyway, since:
> >>
> >> * the source and destination machines should have the same CPU
> >>
> >> * since the destination QEMU does not support the feature, the source
> >> should have masked it as well
> >>
> >> * the guest should always probe CPUID before using a feature
> >>
> > The I fail to see what is the purpose of the patch. I see two cases:
> > 1. Each extended state has separate CPUID bit (is this guarantied?)
> 
> Not guaranteed, but it has always happened so far (AVX, AVX-512, MPX).
> 
OK, So for now no need to make 0D configurable, but we need to provide
correct one according to those flags, not to passthrough host values.
 
> >   - In this case, as you say, by matching CPUID on src and dst we guaranty
> >     that migration data is good.
> 
> But we don't match CPUID on src and destination.  This is something that
Yes, I was saying that management infrastructure already knows how to handle
it.

> the user should do, but it's better if we can test it too.  Subsections
> do that for us; I am, in some sense, emulating subsections for the XSAVE
> states that are not stored in subsections.
We do not do it for other bits. It is possible currently to migrate to a
slightly different cpu without failure and it may cause guest to crash,
but we are not trying actively to catch those situations. Why XSAVE is
different?

> 
> >> In fact, perhaps even XSTATE_SUPPORTED is not restrictive enough here,
> >> and we should hide all features that are not visible in CPUID.  It is
> >> okay, however, to test it in cpu_post_load.
> > 
> > The kernel should not even return state that is not visible in CPUID.
> 
> That's an interesting point of view that I hadn't considered.  But just
> like you asked me why it should return state that is not visible in
> CPUID, I'm asking you why it should not...
> 
For number of reasons. First because since a sate is not used there is no
point in migrating it. Second to make interface more deterministic for
QEMU. i.e QEMU configures only features it supports and gets
exactly same state from the kernel no matter what host cpu is and what
kernel version is. This patch will not be needed since kernel will do
the job.

--
                        Gleb.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]