One question to compat model

One question to compat model

[Jiang, Yunhong]

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

Re: One question to compat model

[Jan Beulich]

>>> "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

RE: One question to compat model

[Jiang, Yunhong]

>-----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

RE: One question to compat model

[Jan Beulich]

>>> "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.

>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

RE: One question to compat model

[Jiang, Yunhong]

>-----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

RE: One question to compat model

[Jan Beulich]

>>> "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

RE: One question to compat model

[Jiang, Yunhong]

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

Re: One question to compat model

[Keir Fraser]

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

RE: One question to compat model

[Jiang, Yunhong]

>-----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
>

Re: One question to compat model

[Keir Fraser]

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

RE: One question to compat model

[Jiang, Yunhong]

>-----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
>

Re: One question to compat model

[Keir Fraser]

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