Can someone please explain the madness in the else part of this function? The caller magically passes 2 for mask? Is this already documented anywhere by chance for mortals like me :). thanks, Mukesh _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan wrote it and may still understand it. He''s your best hope. If as a result you think you can make it clearer, please send a patch. :-) -- Keir On 22/05/2009 03:57, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:> > Can someone please explain the madness in the else part of this function? The > caller magically passes 2 for mask? Is this already documented anywhere by > chance for mortals like me :). > > thanks, > Mukesh > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ok. Even if I can''t make it clearer, at least I''ll add few lines of comments explaining what''s going on, after (and if) I figure it out. Jan, It seems assumption is made that a 64bit dom0 will not have a 32bit app making hypercall? BUG_ON(*reg != (unsigned int)*reg); <=== can you please explain the rationale? Also, can you please comment on other checks, dwiddling with mask, setting *id to *reg value, etc. in this function? thanks, Mukesh (starting a new campaign against overuse of ## macros in xen) Keir Fraser wrote:> Jan wrote it and may still understand it. He''s your best hope. If as a > result you think you can make it clearer, please send a patch. :-) > > -- Keir > > On 22/05/2009 03:57, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote: > >> Can someone please explain the madness in the else part of this function? The >> caller magically passes 2 for mask? Is this already documented anywhere by >> chance for mortals like me :). >> >> thanks, >> Mukesh >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote:> Ok. Even if I can''t make it clearer, at least I''ll add few lines of comments > explaining what''s going on, after (and if) I figure it out. > > Jan, > > It seems assumption is made that a 64bit dom0 will not have a 32bit app making > hypercall? > > BUG_ON(*reg != (unsigned int)*reg); <===You know that all the ''xlat'' stuff in Xen is for 32-bit guests running on 64-bit hypervisor, right? 64-bit dom0 would never execute this logic. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2009-05-21 at 22:57 -0400, Mukesh Rathor wrote:> Can someone please explain the madness in the else part of this function? The > caller magically passes 2 for mask? Is this already documented anywhere by > chance for mortals like me :).I was forced to understand this at one point, let''s see how much I can remember ;-) The mask argument to hypercall_xlat_continuation indicates which of the 6 potential continuation arguments (corresponding to the up to 6 arguments to a hypercall) need to be translated from a native value to a compat value. The least significant bit == the first argument, the second least is the second argument etc. For each bit that is set the varargs should contain a pair of additional arguments, the native value and the replacement compat value. The native value is compared to the value in the continuation before replacing it with the compat value. I would have thought the native value must always match by design so this might just be a sanity check. For example do_memory_op takes arguments (cmd, nat.hnd) and therefore we pass mask==0x2 followed by varargs (nat.hnd, compat). So if the second continuation argument matches nat.hnd it will be replaced with compat. Similarly do_mmuext_op takes (nat_ops, otherstuff, etc...) and we pass a mask==0x01 with varargs (nat_ops, cmp_uops so if the first continuation argument matches nat_ops we replace it with cmp_uops. In both cases if the native and compat things are the same we ignore the bit set in the mask. I don''t recall what the "BUG_ON(nval == (unsigned int)nval)" is all about. I guess the assumption is that if an argument requires translation it must be too large to fit in a compat sized variable. This seems to be true for the existing cases (which are both XEN_GUEST_HANDLEs), I don''t see why it would be true in general. Maybe the assumption is that only XEN_GUEST_HANDLES ever need translation? The "BUG_ON(*reg != (unsigned int)*reg)" is there because if we didn''t request translation for a given argument it had better be the same in both native and compat form. The two halves of the outermost if-else are just to handle the different location of the continuation arguments in the multicall vs regular hypercall cases. The first argument to hypercall_xlat_continuation (unsigned int *id) is a pointer to an index which, if non-NULL, is replaced with the value of the argument at that index in the continuation, I think it''s just used for sanity checking, I''m not all that convinced it is necessary (maybe it was useful when initially debugging this stuff) It all seems rather complicated and fragile to me, but it does seem to work so I''m not inclined to go poking at it... Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Sat, 2009-05-23 at 06:17 -0400, Keir Fraser wrote:> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote: > > > Ok. Even if I can''t make it clearer, at least I''ll add few lines of comments > > explaining what''s going on, after (and if) I figure it out. > > > > Jan, > > > > It seems assumption is made that a 64bit dom0 will not have a 32bit app making > > hypercall? > > > > BUG_ON(*reg != (unsigned int)*reg); <===> > You know that all the ''xlat'' stuff in Xen is for 32-bit guests running on > 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.It''s worth noting though that I don''t believe a 32 bit dom0 toolstack on a 64 bit kernel on a 64 bit hypervisor will work. In particular the privcmd "make a hypercall" ioctl doesn''t do any compat translation so 32 bit xend and friends can''t make hypercalls that way and I think the MMAPBATCH privcmd doesn''t work either. I''m sure there are other cases too (blktap user<->kernel ring layout maybe?). Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Ian Campbell <Ian.Campbell@citrix.com> 24.05.09 00:36 >>> >The mask argument to hypercall_xlat_continuation indicates which of the >6 potential continuation arguments (corresponding to the up to 6 >arguments to a hypercall) need to be translated from a native value to a >compat value. The least significant bit == the first argument, the >second least is the second argument etc. For each bit that is set the >varargs should contain a pair of additional arguments, the native value >and the replacement compat value. The native value is compared to the >value in the continuation before replacing it with the compat value. I >would have thought the native value must always match by design so this >might just be a sanity check.It indeed is a sanity check, to avoid silently doing the wrong thing.>For example do_memory_op takes arguments (cmd, nat.hnd) and therefore we >pass mask==0x2 followed by varargs (nat.hnd, compat). So if the second >continuation argument matches nat.hnd it will be replaced with compat. > >Similarly do_mmuext_op takes (nat_ops, otherstuff, etc...) and we pass a >mask==0x01 with varargs (nat_ops, cmp_uops so if the first continuation >argument matches nat_ops we replace it with cmp_uops. > >In both cases if the native and compat things are the same we ignore the >bit set in the mask.... the primary purpose of which is to deal with NULL arguments.>I don''t recall what the "BUG_ON(nval == (unsigned int)nval)" is all >about. I guess the assumption is that if an argument requires >translation it must be too large to fit in a compat sized variable. This >seems to be true for the existing cases (which are both >XEN_GUEST_HANDLEs), I don''t see why it would be true in general. Maybe >the assumption is that only XEN_GUEST_HANDLES ever need translation?The intention here is to avoid someone trying to put translated arguments in guest visible space (i.e. va < 4G).>... > >The first argument to hypercall_xlat_continuation (unsigned int *id) is >a pointer to an index which, if non-NULL, is replaced with the value of >the argument at that index in the continuation, I think it''s just used >for sanity checking, I''m not all that convinced it is necessary (maybe >it was useful when initially debugging this stuff)No, this is not just for sanity checking. For an example, look at compat_memory_op()''s use of it: It has no other way of knowing how much of the current batch do_memory_op() actually processed, since the return value is either an error code or __HYPERVISOR_memory_op.>It all seems rather complicated and fragile to me, but it does seem to >work so I''m not inclined to go poking at it...Indeed, I didn''t try to make it artificially complicated, but the main goal was to keep the users of it simple (which I still believe they are). I''d be all for simplifying it *without* imposing more complexity on the callers. Otoh I don''t think it''s *that* complicated... Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 25/05/2009 12:47, "Jan Beulich" <JBeulich@novell.com> wrote:>> It all seems rather complicated and fragile to me, but it does seem to >> work so I''m not inclined to go poking at it... > > Indeed, I didn''t try to make it artificially complicated, but the main goal > was to keep the users of it simple (which I still believe they are). I''d be > all for simplifying it *without* imposing more complexity on the callers. > Otoh I don''t think it''s *that* complicated...Some code comments would be nice. :-) -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser wrote:> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote: > >> Ok. Even if I can''t make it clearer, at least I''ll add few lines of comments >> explaining what''s going on, after (and if) I figure it out. >> >> Jan, >> >> It seems assumption is made that a 64bit dom0 will not have a 32bit app making >> hypercall? >> >> BUG_ON(*reg != (unsigned int)*reg); <===> > You know that all the ''xlat'' stuff in Xen is for 32-bit guests running on > 64-bit hypervisor, right? 64-bit dom0 would never execute this logic.Unless someone like me, in trying to make 32bit apps on 64bit dom0 work, causes it to take compat path if it''s 32bit hcall. Yeah, I know whacky :)... Thanks as always, Mukesh PS: If anyone is curious, if the call is 32bit hcall, in entry.S I make it jmp to compat/entry.S and do compat hcall. So far, other than this xlat stuff, seems ok! I set a register in dom0 to before hcall to indicate it''s from a 32bit app. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Mukesh Rathor <mukesh.rathor@oracle.com> 26.05.09 20:43 >>> >Keir Fraser wrote: >> You know that all the ''xlat'' stuff in Xen is for 32-bit guests running on >> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic. > >Unless someone like me, in trying to make 32bit apps on 64bit dom0 work, >causes it to take compat path if it''s 32bit hcall. Yeah, I know whacky :)...I don''t think that''s a good approach - even when dealing with 32-bit apps, it''s a 64-bit kernel, and hence you should rather translate things to the 64-bit format in the kernel layer that sits in between (after all, apps can''t do hypercalls directly). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 27/05/2009 09:02, "Jan Beulich" <JBeulich@novell.com> wrote:>>> You know that all the ''xlat'' stuff in Xen is for 32-bit guests running on >>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic. >> >> Unless someone like me, in trying to make 32bit apps on 64bit dom0 work, >> causes it to take compat path if it''s 32bit hcall. Yeah, I know whacky :)... > > I don''t think that''s a good approach - even when dealing with 32-bit apps, > it''s a 64-bit kernel, and hence you should rather translate things to the > 64-bit format in the kernel layer that sits in between (after all, apps can''t > do hypercalls directly).Why would you do that when the tedious translation mechanism already exists in the hypervisor? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Keir Fraser <keir.fraser@eu.citrix.com> 27.05.09 10:04 >>> >On 27/05/2009 09:02, "Jan Beulich" <JBeulich@novell.com> wrote: > >>>> You know that all the ''xlat'' stuff in Xen is for 32-bit guests running on >>>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic. >>> >>> Unless someone like me, in trying to make 32bit apps on 64bit dom0 work, >>> causes it to take compat path if it''s 32bit hcall. Yeah, I know whacky :)... >> >> I don''t think that''s a good approach - even when dealing with 32-bit apps, >> it''s a 64-bit kernel, and hence you should rather translate things to the >> 64-bit format in the kernel layer that sits in between (after all, apps can''t >> do hypercalls directly). > >Why would you do that when the tedious translation mechanism already exists >in the hypervisor?Because it''s conceptually wrong. And the necessary extra de-muxing that''s needed (because the int $HYPERCALL_VECTOR path can''t be used) is not going to come without a (performance) price. A secondary reason is that as soon as Xen becomes more easily build-time configurable (like Linux is), which I expect to happen at some point (unless we manage to replace all those build time decisions with runtime ones), a 64-bit guest can''t really rely on the hypervisor having compat mode support (and considering backward compatibility, which might not matter in the context the subject was brought up in, such an assumption would be wrong in the first place). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 27/05/2009 09:22, "Jan Beulich" <JBeulich@novell.com> wrote:> Because it''s conceptually wrong.Nothing conceptually wrong with it at all. Just another hypervisor service being made available to a higher level of software.> And the necessary extra de-muxing that''s > needed (because the int $HYPERCALL_VECTOR path can''t be used) is not > going to come without a (performance) price.We add a compat32 wrapping hypercall, and shift the actual hypercall number to a different register. Noone takes a hit but the 32-bit userspace which has already bounced via ring 1 and this will not add an extra measurable overhead.> A secondary reason is that as soon as Xen becomes more easily build-time > configurable (like Linux is), which I expect to happen at some point (unless > we manage to replace all those build time decisions with runtime ones), a > 64-bit guest can''t really rely on the hypervisor having compat mode support > (and considering backward compatibility, which might not matter in the > context the subject was brought up in, such an assumption would be wrong > in the first place).Firstly, I don''t see us bringing in extensive build-time configuration any time soon. Secondly, if someone wanted this new compat32-for-userspace support, they would simply have to install a new hypervisor configured with compat32 support -- how is that harder/worse than installing a new kernel configured with compat32 support? At the end of the day, the thought of *duplicating* the compat32 stuff and then trying to stuff it upstream to kernel.org does not appeal. I see that as two major concrete negatives versus the much weaker negatives on the other side of the argument. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell wrote:> On Sat, 2009-05-23 at 06:17 -0400, Keir Fraser wrote: >> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote: >> >>> Ok. Even if I can''t make it clearer, at least I''ll add few lines of comments >>> explaining what''s going on, after (and if) I figure it out. >>> >>> Jan, >>> >>> It seems assumption is made that a 64bit dom0 will not have a 32bit app making >>> hypercall? >>> >>> BUG_ON(*reg != (unsigned int)*reg); <===>> You know that all the ''xlat'' stuff in Xen is for 32-bit guests running on >> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic. > > It''s worth noting though that I don''t believe a 32 bit dom0 toolstack on > a 64 bit kernel on a 64 bit hypervisor will work. In particular the > privcmd "make a hypercall" ioctl doesn''t do any compat translation so 32 > bit xend and friends can''t make hypercalls that way and I think the > MMAPBATCH privcmd doesn''t work either. > > I''m sure there are other cases too (blktap user<->kernel ring layout > maybe?). > > Ian.Thanks Ian for good explanation of hypercall_xlat_continuation(). yeah, I''m just exploring that right now. There is MMAPBATCH_32, btw, in dom0 that looks like was implemented by PPC folks. Also, MMAP_32. I was able to start PV guest without network. Not sure if that was because of compatibility or some other issue. I''m just looking at MMAP stuff right now, think I finally figured out the chain of calls from libxc to hyp to dom0 ... ia32_sys_call_table to compat_sys_ioctl to handler to do_ioctl32_pointer .. whew!! Thanks, Mukesh _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Mukesh Rathor wrote:> > > Ian Campbell wrote: >> On Sat, 2009-05-23 at 06:17 -0400, Keir Fraser wrote: >>> On 22/05/2009 22:58, "Mukesh Rathor" <mukesh.rathor@oracle.com> wrote: >>> >>>> Ok. Even if I can''t make it clearer, at least I''ll add few lines of >>>> comments >>>> explaining what''s going on, after (and if) I figure it out. >>>> >>>> Jan, >>>> >>>> It seems assumption is made that a 64bit dom0 will not have a 32bit >>>> app making >>>> hypercall? >>>> >>>> BUG_ON(*reg != (unsigned int)*reg); <===>>> You know that all the ''xlat'' stuff in Xen is for 32-bit guests >>> running on >>> 64-bit hypervisor, right? 64-bit dom0 would never execute this logic. >> >> It''s worth noting though that I don''t believe a 32 bit dom0 toolstack on >> a 64 bit kernel on a 64 bit hypervisor will work. In particular the >> privcmd "make a hypercall" ioctl doesn''t do any compat translation so 32 >> bit xend and friends can''t make hypercalls that way and I think the >> MMAPBATCH privcmd doesn''t work either. >> >> I''m sure there are other cases too (blktap user<->kernel ring layout >> maybe?). >> >> Ian. > > Thanks Ian for good explanation of hypercall_xlat_continuation(). > > yeah, I''m just exploring that right now. There is MMAPBATCH_32, btw, in > dom0 that looks like was implemented by PPC folks. Also, MMAP_32. > I was able to start PV guest without network. Not sure if that was > because of compatibility or some other issue. I''m just looking at MMAP > stuff right now, think I finally figured out the chain of calls from > libxc to hyp to dom0 ... ia32_sys_call_table to compat_sys_ioctl to > handler to do_ioctl32_pointer .. whew!! > > Thanks, > Mukesh >yeah, looks like privcmd_ioctl_32() only fixes the wrapper struct. The PFN array still is 32bit pfn''s, and privcmd_ioctl() expects 64bits. So, in a dilemma now, not sure if I should fix it up in privcmd_ioctl_32() or change privcmd_ioctl() which will take some time to reverse engineer. Not sure how many things I''ll discover, if it''s too many, it may not be worth it in the end. Thanks, Mukesh _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2009-05-28 at 23:01 -0400, Mukesh Rathor wrote:> yeah, looks like privcmd_ioctl_32() only fixes the wrapper struct. The > PFN array still is 32bit pfn''s, and privcmd_ioctl() expects 64bits. So, > in a dilemma now, not sure if I should fix it up in privcmd_ioctl_32() > or change privcmd_ioctl() which will take some time to reverse engineer. > Not sure how many things I''ll discover, if it''s too many, it may > not be worth it in the end.FWIW here are my very skanky patches from ages ago (patch names are the originals if that gives a clue to my opinion of them even then ;-)). I don''t even recall if they worked properly (or at all), I think I remember starting guests so long as they didn''t use blktap (which has issues with the user<->kernel ring protocol in this scenario). There''s an outside change there might be something in them which isn''t complete rubbish. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell wrote:> On Thu, 2009-05-28 at 23:01 -0400, Mukesh Rathor wrote: >> yeah, looks like privcmd_ioctl_32() only fixes the wrapper struct. The >> PFN array still is 32bit pfn''s, and privcmd_ioctl() expects 64bits. So, >> in a dilemma now, not sure if I should fix it up in privcmd_ioctl_32() >> or change privcmd_ioctl() which will take some time to reverse engineer. >> Not sure how many things I''ll discover, if it''s too many, it may >> not be worth it in the end. > > FWIW here are my very skanky patches from ages ago (patch names are the > originals if that gives a clue to my opinion of them even then ;-)). > > I don''t even recall if they worked properly (or at all), I think I > remember starting guests so long as they didn''t use blktap (which has > issues with the user<->kernel ring protocol in this scenario). > > There''s an outside change there might be something in them which isn''t > complete rubbish. > > Ian. >Hi Ian, Looks like you also put in quite a bit of effort into it. I''m realizing that it is more work than it appeared in the beginning, and probably not worth the gains in the end. So I should also give up. learnt few new things along the way tho :). Thanks, Mukesh _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel