Hi, Jan, I have a question for compat model when I try to do some changes to MCA
side.
Followed is part of my patch, I try to add a structure xen_mc_inject_v2 to
xen_mc, with xenctl_cpumap embeded in the xen_mc_inject_v2.
However, in the "include/compat/arch-x86/xen-mca.h", the xenctl_cpumap
is translated to be compat_ctl_cpumap, this is sure not I wanted.
After checking the related code, I find it should be caused by the [
r"(struct|union|enum)\s+(xen_?)?(\w)", r"\1 compat_\3" ] in
tools/compat-build-header.py, which will replace all "struct xen" with
"struct compat". Add the xenctl_cpumap to xlat.lst raise other
warnings.
I can''t find a solution on how to make sure the xenctl_cpumap will not
be changed, can you please give me some information on it?
BTW, are there any guideline on how should we define the interface structure and
handle them for compat model? For example, seems
arch/x86/x86_64/platform_hypercall.c and arch/x86/cpu/mcheck/mce.c has different
method to handle the compat model.
--jyh
----The result in compat/arch-x86/xen-mca.h
struct compat_mc_inject_v2 {
uint32_t flags;
struct compat_ctl_cpumap cpumap; >>> the xenctl_cpumap is
translated :(
};
----Part of my patches:
diff -r ae1913320f05 xen/include/public/arch-x86/xen-mca.h
--- a/xen/include/public/arch-x86/xen-mca.h Wed May 12 11:12:28 2010 +0800
+++ b/xen/include/public/arch-x86/xen-mca.h Wed May 12 11:14:59 2010 +0800
@@ -404,6 +404,18 @@ struct xen_mc_mceinject {
unsigned int mceinj_cpunr; /* target processor id */
};
+#define XEN_MC_inject_v2 6
+#define XEN_MC_INJECT_TYPE_MASK 0x7
+#define XEN_MC_INJECT_TYPE_MCE 0x0
+#define XEN_MC_INJECT_TYPE_CMCI 0x1
+
+#define XEN_MC_INJECT_CPU_BROADCAST 0x8
+
+struct xen_mc_inject_v2 {
+ uint32_t flags;
+ struct xenctl_cpumap cpumap;
+};
+
struct xen_mc {
uint32_t cmd;
uint32_t interface_version; /* XEN_MCA_INTERFACE_VERSION */
@@ -413,6 +425,8 @@ struct xen_mc {
struct xen_mc_physcpuinfo mc_physcpuinfo;
struct xen_mc_msrinject mc_msrinject;
struct xen_mc_mceinject mc_mceinject;
+ struct xen_mc_inject_v2 mc_inject_v2;
+ uint8_t pad[144];
} u;
};
typedef struct xen_mc xen_mc_t;
diff -r ae1913320f05 xen/include/xlat.lst
--- a/xen/include/xlat.lst Wed May 12 11:12:28 2010 +0800
+++ b/xen/include/xlat.lst Wed May 12 15:15:31 2010 +0800
@@ -23,6 +23,7 @@
? mc_info arch-x86/xen-mca.h
? mc_mceinject arch-x86/xen-mca.h
? mc_msrinject arch-x86/xen-mca.h
+? mc_inject_v2 arch-x86/xen-mca.h
? mc_notifydomain arch-x86/xen-mca.h
! mc_physcpuinfo arch-x86/xen-mca.h
? page_offline_action arch-x86/xen-mca.h
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 12.05.10 09:38 >>> >Followed is part of my patch, I try to add a structure xen_mc_inject_v2 to xen_mc, with xenctl_cpumap embeded in the xen_mc_inject_v2. > >However, in the "include/compat/arch-x86/xen-mca.h", the xenctl_cpumap is translated to be compat_ctl_cpumap, this is sure not I wanted.Hmm, XEN_GUEST_HANDLE_64() is not being dealt with by the scripts so far, so I believe support for this will need to be added. No sure though whether simply adding the type as a needs-checking one to xen/include/xlat.lst wouldn''t suffice.>After checking the related code, I find it should be caused by the [ r"(struct|union|enum)\s+(xen_?)?(\w)", r"\1 compat_\3" ] in tools/compat-build-header.py, which will replace all "struct xen" with "struct compat". Add the xenctl_cpumap to xlat.lst raise other warnings.Without knowing which ones I can''t say much.>I can''t find a solution on how to make sure the xenctl_cpumap will not be changed, can you please give me some information on it? > >BTW, are there any guideline on how should we define the interface structure and handle them for compat model? For example, seems arch/x86/x86_64/platform_hypercall.c and arch/x86/cpu/mcheck/mce.c has different method to handle the compat model.Sure - I picked the model that was causing less headache in each place I needed to do conversion and/or checking. In the worst case, if you can make the whole thing build without handling the compat case, I could look into the checking/translation issues after the patch went in. Please just remind me when submitting the patch if you want me to do this. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>-----Original Message----- >From: Jan Beulich [mailto:JBeulich@novell.com] >Sent: Wednesday, May 12, 2010 4:00 PM >To: Jiang, Yunhong >Cc: xen-devel@lists.xensource.com >Subject: Re: One question to compat model > >>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 12.05.10 09:38 >>> >>Followed is part of my patch, I try to add a structure xen_mc_inject_v2 to xen_mc, >with xenctl_cpumap embeded in the xen_mc_inject_v2. >> >>However, in the "include/compat/arch-x86/xen-mca.h", the xenctl_cpumap is >translated to be compat_ctl_cpumap, this is sure not I wanted. > >Hmm, XEN_GUEST_HANDLE_64() is not being dealt with by the scripts so far, so I >believe support for this will need to be added. No sure though whether simply adding >the type as a needs-checking one to xen/include/xlat.lst wouldn''t suffice.The followed small changes cayse compile failed with: can''t read compat/domctl.h: No such file or directory diff -r 20713df6a7fe xen/include/xlat.lst --- a/xen/include/xlat.lst Wed May 12 16:05:08 2010 +0800 +++ b/xen/include/xlat.lst Wed May 12 16:11:55 2010 +0800 @@ -63,6 +63,7 @@ ! sched_poll sched.h ? sched_remote_shutdown sched.h ? sched_shutdown sched.h +? xenctl_cpumap domctl.h ! tmem_op tmem.h ? t_buf trace.h ? vcpu_get_physid vcpu.h> >>After checking the related code, I find it should be caused by the >[ r"(struct|union|enum)\s+(xen_?)?(\w)", r"\1 compat_\3" ] in >tools/compat-build-header.py, which will replace all "struct xen" with "struct >compat". Add the xenctl_cpumap to xlat.lst raise other warnings. > >Without knowing which ones I can''t say much. > >>I can''t find a solution on how to make sure the xenctl_cpumap will not be changed, >can you please give me some information on it? >> >>BTW, are there any guideline on how should we define the interface structure and >handle them for compat model? For example, seems >arch/x86/x86_64/platform_hypercall.c and arch/x86/cpu/mcheck/mce.c has >different method to handle the compat model. > >Sure - I picked the model that was causing less headache in each place I needed to >do conversion and/or checking. > >In the worst case, if you can make the whole thing build without handling the compat >case, I could look into the checking/translation issues after the patch went in. Please >just remind me when submitting the patch if you want me to do this.How can I disable compat model now? I remember that option has been removed. -jyh> >Jan_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 12.05.10 10:15 >>> >The followed small changes cayse compile failed with: > >can''t read compat/domctl.h: No such file or directorySure - domctl.h must not be included from xen-mca.h in any case, you''ll need to move the type declaration if you want to use it outside the domctl/sysctl set.>How can I disable compat model now? I remember that option has been removed.You can''t disable CONFIG_COMPAT anymore, but I think you should be able to tweak the CONFIG_COMPAT section in xen/arch/x86/cpu/mcheck/mce.c in a way to allow your new code to be built without doing anything compat-related for the new structures. But maybe removing the domctl.h dependency already clarifies matters. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>-----Original Message----- >From: Jan Beulich [mailto:JBeulich@novell.com] >Sent: Wednesday, May 12, 2010 4:31 PM >To: Jiang, Yunhong >Cc: xen-devel@lists.xensource.com >Subject: RE: One question to compat model > >>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 12.05.10 10:15 >>> >>The followed small changes cayse compile failed with: >> >>can''t read compat/domctl.h: No such file or directory > >Sure - domctl.h must not be included from xen-mca.h in any case, >you''ll need to move the type declaration if you want to use it outside >the domctl/sysctl set.Thanks for remind. Yes, this header will also be included in kernel although this specific structure will only be used by tools.> >>How can I disable compat model now? I remember that option has been removed. > >You can''t disable CONFIG_COMPAT anymore, but I think you should >be able to tweak the CONFIG_COMPAT section in >xen/arch/x86/cpu/mcheck/mce.c in a way to allow your new code to >be built without doing anything compat-related for the new >structures. But maybe removing the domctl.h dependency already >clarifies matters.Yes, it should be ok. But a curios question is, why the xenctl_cpumap has to be defined in domctl.h. It''s simply a helper function. Now I have to work like XENPF_getidletime, passing two parameters (nr_cpus and the bitmap pointer), combine them in hypervisor to xenctl_cpumap and then call the xenctl_cpumap code. And the same to user space tools. Thanks for your remind. --jyh> >Jan_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 12.05.10 10:50 >>> >Yes, it should be ok. But a curios question is, why the xenctl_cpumap has to be defined in domctl.h. It''s simply a helper function.I guess because it wasn''t used anywhere else... Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 12/05/2010 09:50, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:>> You can''t disable CONFIG_COMPAT anymore, but I think you should >> be able to tweak the CONFIG_COMPAT section in >> xen/arch/x86/cpu/mcheck/mce.c in a way to allow your new code to >> be built without doing anything compat-related for the new >> structures. But maybe removing the domctl.h dependency already >> clarifies matters. > > Yes, it should be ok. But a curios question is, why the xenctl_cpumap has to > be defined in domctl.h. It''s simply a helper function. Now I have to work like > XENPF_getidletime, passing two parameters (nr_cpus and the bitmap pointer), > combine them in hypervisor to xenctl_cpumap and then call the xenctl_cpumap > code. And the same to user space tools.Whoever implemented XENPF_getidletime decided to stuff a fake xenctl_cpumap struct within Xen rather than properly refactor the public headers. There''s no reason not to move xenctl_cpumap out into xen.h. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Yes, the interface is not used in the interface, but it is in fact used by XENPF_getidletime, which try to translate into xenctl_cpumap in xen hypervisor:-) I didn''t check other code. --jyh>-----Original Message----- >From: Jan Beulich [mailto:JBeulich@novell.com] >Sent: Wednesday, May 12, 2010 5:12 PM >To: Keir Fraser; Jiang, Yunhong >Cc: xen-devel@lists.xensource.com >Subject: RE: One question to compat model > >>>> "Jiang, Yunhong" <yunhong.jiang@intel.com> 12.05.10 10:50 >>> >>Yes, it should be ok. But a curios question is, why the xenctl_cpumap has to be >defined in domctl.h. It''s simply a helper function. > >I guess because it wasn''t used anywhere else... > >Jan_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>-----Original Message----- >From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] >Sent: Wednesday, May 12, 2010 5:13 PM >To: Jiang, Yunhong; Jan Beulich >Cc: xen-devel@lists.xensource.com >Subject: Re: One question to compat model > >On 12/05/2010 09:50, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > >>> You can''t disable CONFIG_COMPAT anymore, but I think you should >>> be able to tweak the CONFIG_COMPAT section in >>> xen/arch/x86/cpu/mcheck/mce.c in a way to allow your new code to >>> be built without doing anything compat-related for the new >>> structures. But maybe removing the domctl.h dependency already >>> clarifies matters. >> >> Yes, it should be ok. But a curios question is, why the xenctl_cpumap has to >> be defined in domctl.h. It''s simply a helper function. Now I have to work like >> XENPF_getidletime, passing two parameters (nr_cpus and the bitmap pointer), >> combine them in hypervisor to xenctl_cpumap and then call the xenctl_cpumap >> code. And the same to user space tools. > >Whoever implemented XENPF_getidletime decided to stuff a fake xenctl_cpumap >struct within Xen rather than properly refactor the public headers. There''s >no reason not to move xenctl_cpumap out into xen.h.A curios question. I checked the code, and notice that the XEN_GUEST_HANDLE_64 is only defined for __XEN__ or __XEN_TOOLS__. I can understand it is needed for tools because 32bit tools can be used in 64bit dom0, but why it is forbidden for kernel? To avoid it be passed as hypercall parameter? Sorry for bothering if this is a stupid question :$ Thanks --jyh> > -- Keir >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 13/05/2010 09:41, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:>> Whoever implemented XENPF_getidletime decided to stuff a fake xenctl_cpumap >> struct within Xen rather than properly refactor the public headers. There''s >> no reason not to move xenctl_cpumap out into xen.h. > > A curios question. I checked the code, and notice that the XEN_GUEST_HANDLE_64 > is only defined for __XEN__ or __XEN_TOOLS__. I can understand it is needed > for tools because 32bit tools can be used in 64bit dom0, but why it is > forbidden for kernel? To avoid it be passed as hypercall parameter? Sorry for > bothering if this is a stupid question :$I was probably being overzealous. There''s no good reason not to use GUEST_HANDLE_64 and uint64_aligned_t outside of tools interfaces. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>-----Original Message----- >From: Keir Fraser [mailto:keir.fraser@eu.citrix.com] >Sent: Thursday, May 13, 2010 4:48 PM >To: Jiang, Yunhong; Jan Beulich >Cc: xen-devel@lists.xensource.com >Subject: Re: One question to compat model > >On 13/05/2010 09:41, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote: > >>> Whoever implemented XENPF_getidletime decided to stuff a fake xenctl_cpumap >>> struct within Xen rather than properly refactor the public headers. There''s >>> no reason not to move xenctl_cpumap out into xen.h. >> >> A curios question. I checked the code, and notice that the >XEN_GUEST_HANDLE_64 >> is only defined for __XEN__ or __XEN_TOOLS__. I can understand it is needed >> for tools because 32bit tools can be used in 64bit dom0, but why it is >> forbidden for kernel? To avoid it be passed as hypercall parameter? Sorry for >> bothering if this is a stupid question :$ > >I was probably being overzealous. There''s no good reason not to use >GUEST_HANDLE_64 and uint64_aligned_t outside of tools interfaces.Althoug not related with my current patch, but curiosly, will it avoid the compat model issue if we use XEN_GUEST_HANDLE_64 for hypercall, especially if not performance critical, like struct xen_mc_fetch? (Maybe we still need consider the #pragma pack optoin for the struction?) --jyh> > -- Keir >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 13/05/2010 11:00, "Jiang, Yunhong" <yunhong.jiang@intel.com> wrote:>> I was probably being overzealous. There''s no good reason not to use >> GUEST_HANDLE_64 and uint64_aligned_t outside of tools interfaces. > > Althoug not related with my current patch, but curiosly, will it avoid the > compat model issue if we use XEN_GUEST_HANDLE_64 for hypercall, especially if > not performance critical, like struct xen_mc_fetch? (Maybe we still need > consider the #pragma pack optoin for the struction?)Oh yes, it would be a *very* good idea to use these macros to avoid the need for compat shim code at all. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel