From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shriram Rajagopalan Subject: Re: xl/xm save -c fails - set_vcpucontext EOPNOTSUPP (was Re: xl save -c issues with Windows 7 Ultimate) Date: Tue, 10 May 2011 10:50:54 -0500 Message-ID: References: <1305016915.26692.261.camel@zakaz.uk.xensource.com> <4DC96FA50200007800040C69@vpn.id2.novell.com> Reply-To: rshriram@cs.ubc.ca Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0764918611==" Return-path: In-Reply-To: <4DC96FA50200007800040C69@vpn.id2.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jan Beulich Cc: "xen-devel@lists.xensource.com" , Ian Campbell List-Id: xen-devel@lists.xenproject.org --===============0764918611== Content-Type: multipart/alternative; boundary=bcaec52d4cfd376ed504a2edee27 --bcaec52d4cfd376ed504a2edee27 Content-Type: text/plain; charset=ISO-8859-1 On Tue, May 10, 2011 at 10:02 AM, Jan Beulich wrote: > >>> On 10.05.11 at 16:52, Shriram Rajagopalan wrote: > > On Tue, May 10, 2011 at 3:41 AM, Ian Campbell >wrote: > >> The most plausible looking EOPNOTSUPP from that code is in > >> xen/arch/x86/domain.c:arch_set_info_guest() but that is on a PV only > >> path. > >> > >> And that rings with the pv guests I am using. It makes perfect sense, > > looking > > at that function and especially at the code that returns EOPNOTSUPP (the > > only > > place in the entire file). > > else > > { > > bool_t fail = v->arch.pv_vcpu.ctrlreg[3] != c(ctrlreg[3]); > > > > #ifdef CONFIG_X86_64 > > fail |= v->arch.pv_vcpu.ctrlreg[1] != c(ctrlreg[1]); > > #endif > > > > for ( i = 0; i < ARRAY_SIZE(v->arch.pv_vcpu.gdt_frames); ++i ) > > fail |= v->arch.pv_vcpu.gdt_frames[i] != c(gdt_frames[i]); > > fail |= v->arch.pv_vcpu.gdt_ents != c(gdt_ents); > > > > fail |= v->arch.pv_vcpu.ldt_base != c(ldt_base); > > fail |= v->arch.pv_vcpu.ldt_ents != c(ldt_ents); > > > > if ( fail ) > > return -EOPNOTSUPP; > > } > > > > This change was introduced by c/s > > changeset: 23142:f5e8d152a565 > > user: Jan Beulich > > date: Tue Apr 05 13:01:25 2011 +0100 > > x86: split struct vcpu > > > > I think I am missing something really obvious in this piece of code. The > > xc_domain_resume code tries to modify the return value of shutdown > hypercall > > (i.e eax register is set to 1) and this code doesnt seem to check those > > registers. > > Correct - the code here checks only for values where the logic > needed to support changing the on an already initialized vCPU isn't > implemented. Previously, actual vCPU state and what was tracked > in struct vcpu could get out of sync in this case, potentially > confusing things further down (including possible security issues). > > You'll want to figure out which part(s) actually differ, and why. > Only then we'll be able to tell whether mentioned c/s introduced > false positives. > > Bit confused. If I understand correctly, this piece of code checks new values of certain registers against old ones, for an already initialized VCPU. And AFAIT, it is checking the gdts, ldts & control registers. The xc_domain_resume code just changes one general purpose register eax. basically, get_vcpucontext() set_field(eax, 1) //to indicate SUSPEND_CANCEL set_vcpucontext() I dont understand what you mean by "which parts actually differ & why". And just a trivial question: is the hypervisor binary always compiled to a 32-bit elf? somehow, the symbols file xen-syms-* is getting compiled to 64 bit ELF binary while the xen binary is getting compiled to 32-bit binary. shriram > Jan > > --bcaec52d4cfd376ed504a2edee27 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
On Tue, May 10, 2011 at 10:02 AM, Jan Beulich <JBeulich@novell.= com> wrote:
>>> On 10.05.11 at 16:52, Shriram Rajagopalan &l= t;rshriram@cs.ubc.ca> wrote: > On Tue, May 10, 2011 at 3:41 AM, Ian Campbell <Ian.Campbell@citrix.com>wrote:
>> The most plausible looking= EOPNOTSUPP from that code is in
>> xen/arch/x86/domain.c:arch_set_info_guest() but that is on a PV on= ly
>> path.
>>
>> And that rings with the pv guests I am using. It makes perfect sen= se,
> looking
> at that function and especially at the code that returns EOPNOTSUPP (t= he
> only
> place in the entire file).
> =A0 =A0else
> =A0 =A0 {
> =A0 =A0 =A0 =A0 bool_t fail =3D v->arch.pv_vcpu.ctrlreg[3] !=3D c(c= trlreg[3]);
>
> #ifdef CONFIG_X86_64
> =A0 =A0 =A0 =A0 fail |=3D v->arch.pv_vcpu.ctrlreg[1] !=3D c(ctrlreg= [1]);
> #endif
>
> =A0 =A0 =A0 =A0 for ( i =3D 0; i < ARRAY_SIZE(v->arch.pv_vcpu.gd= t_frames); ++i )
> =A0 =A0 =A0 =A0 =A0 =A0 fail |=3D v->arch.pv_vcpu.gdt_frames[i] != =3D c(gdt_frames[i]);
> =A0 =A0 =A0 =A0 fail |=3D v->arch.pv_vcpu.gdt_ents !=3D c(gdt_ents)= ;
>
> =A0 =A0 =A0 =A0 fail |=3D v->arch.pv_vcpu.ldt_base !=3D c(ldt_base)= ;
> =A0 =A0 =A0 =A0 fail |=3D v->arch.pv_vcpu.ldt_ents !=3D c(ldt_ents)= ;
>
> =A0 =A0 =A0 =A0 if ( fail )
> =A0 =A0 =A0 =A0 =A0 =A0return -EOPNOTSUPP;
> =A0 =A0 }
>
> This change was introduced by c/s
> changeset: =A0 23142:f5e8d152a565
> user: =A0 =A0 =A0 =A0Jan Beulich <jbeulich@novell.com>
> date: =A0 =A0 =A0 =A0Tue Apr 05 13:01:25 2011 +0100
> x86: split struct vcpu
>
> I think I am missing something really obvious in this piece of code. T= he
> xc_domain_resume code tries to modify the return value of shutdown hyp= ercall
> (i.e eax register is set to 1) and this code doesnt seem to check thos= e
> registers.

Correct - the code here checks only for values where the logic<= br> needed to support changing the on an already initialized vCPU isn't
implemented. Previously, actual vCPU state and what was tracked
in struct vcpu could get out of sync in this case, potentially
confusing things further down (including possible security issues).

You'll want to figure out which part(s) actually differ, and why.
Only then we'll be able to tell whether mentioned c/s introduced
false positives.

Bit confused. If I und= erstand correctly, this piece of code checks new values
of certain regi= sters against old ones, for an already initialized VCPU. And AFAIT,
it i= s checking the gdts, ldts & control registers. The xc_domain_resume cod= e just
changes one general purpose register eax. basically,

=A0get_vcpucont= ext()
=A0set_field(eax, 1) //to indicate SUSPEND_CANCEL
=A0set_vcpuco= ntext()

I dont understand what you mean by "which parts actuall= y differ & why".


And just a trivial question:
=A0is the hypervisor binary always = compiled to a 32-bit elf? somehow, the
symbols file xen-syms-* is getti= ng compiled to 64 bit ELF binary while
the xen binary is getting compil= ed to 32-bit binary.

shriram
Jan


--bcaec52d4cfd376ed504a2edee27-- --===============0764918611== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --===============0764918611==--