arch_set_info_guest() producing inconsistent state on x86?

arch_set_info_guest() producing inconsistent state on x86?

[Jan Beulich]

The pv guest code path has a v->is_initialised check in the middle,
skipping d->vm_assist assignment, actual GDT construction, actual
page table setup, and the call to update_domain_wallclock_time().

If indeed do_domctl(XEN_DOMCTL_setvcpucontext,...) (the only
code path where v->is_initialised can be false on entry) was called
twice for a vCPU, this would minimally yield stored GDT settings
(in struct vcpu) out of sync with what is in the per-domain page
tables, thus causing arch_get_info_guest() to return values
not actually in use.

In the course of shrinking struct vcpu to below PAGE_SIZE I need
to split the embedded struct vcpu_guest_context in struct vcpu
anyway (as it is by itself larger than a page on x86-64), which
made this so far hidden inconsistency visible.

The question is whether it must be considered legal to issue
XEN_DOMCTL_setvcpucontext on an already initialized vCPU
in the first place. If that isn't necessary, the fix is simple
(just remove that check, and add one at the top rejecting
the attempt). If it is necessary, but the subsequent code is
all legal to be run a second time, simply removing the check
would again be the way to go (and in the splitting patch I'm
working on the copying of the GDT data then could be
dropped altogether, as the calls to set_gdt() would then
fully take care of this.

Otherwise, I would think that at least the GDT setup needs to
be moved ahead of the check, but I would then wonder
whether the remaining stuff really is correct to be skipped.

Jan

Re: arch_set_info_guest() producing inconsistent state on x86?

[Keir Fraser]

On 29/03/2011 09:01, "Jan Beulich" <JBeulich@novell.com> wrote:

> The question is whether it must be considered legal to issue
> XEN_DOMCTL_setvcpucontext on an already initialized vCPU
> in the first place.

It's probably used by debuggers running in dom0? Also see
modify_returncode() in libxc/xc_resume.c -- so it's used on suspend resume
in the failure case.

I doubt anything other than GPRs are ever modified after first
initialisation.

 -- Keir

Re: arch_set_info_guest() producing inconsistent state on x86?

[Jan Beulich]

>>> On 29.03.11 at 11:59, Keir Fraser <keir.xen@gmail.com> wrote:
> On 29/03/2011 09:01, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>> The question is whether it must be considered legal to issue
>> XEN_DOMCTL_setvcpucontext on an already initialized vCPU
>> in the first place.
> 
> It's probably used by debuggers running in dom0? Also see
> modify_returncode() in libxc/xc_resume.c -- so it's used on suspend resume
> in the failure case.
> 
> I doubt anything other than GPRs are ever modified after first
> initialisation.

So should we then perhaps make the function check the bits
it doesn't really update match what is in place already?

Jan

Re: arch_set_info_guest() producing inconsistent state on x86?

[Keir Fraser]

On 29/03/2011 12:45, "Jan Beulich" <JBeulich@novell.com> wrote:

>> It's probably used by debuggers running in dom0? Also see
>> modify_returncode() in libxc/xc_resume.c -- so it's used on suspend resume
>> in the failure case.
>> 
>> I doubt anything other than GPRs are ever modified after first
>> initialisation.
> 
> So should we then perhaps make the function check the bits
> it doesn't really update match what is in place already?

I suppose it would be nice. I can't say I care much one way or the other.

 -- Keir

Re: arch_set_info_guest() producing inconsistent state on x86?

[Keir Fraser]

On 29/03/2011 13:01, "Keir Fraser" <keir.xen@gmail.com> wrote:

> On 29/03/2011 12:45, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
>>> It's probably used by debuggers running in dom0? Also see
>>> modify_returncode() in libxc/xc_resume.c -- so it's used on suspend resume
>>> in the failure case.
>>> 
>>> I doubt anything other than GPRs are ever modified after first
>>> initialisation.
>> 
>> So should we then perhaps make the function check the bits
>> it doesn't really update match what is in place already?
> 
> I suppose it would be nice. I can't say I care much one way or the other.

By which I mean: if you want to make the change, and do it in a way that is
clean and clear (maybe you can improve the function's readability while
there, since it is a bit of a mess) then I'll apply it happily. I don't want
to make the function even worse and more unmaintainable than it already is,
however.

 -- Keir

>  -- Keir
> 
>