thr3ads.net - Xen devel - [PATCH]: PVH: specify xen features strings cleany for PVH [Jan 2013]

If this information is useful, please help other people find it:
Share via:

Mukesh Rathor

2013-Jan-19 01:35 UTC

[PATCH]: PVH: specify xen features strings cleany for PVH

On Thu, 17 Jan 2013 22:22:47 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> Jan had some comments about that patch:
> 
> https://patchwork.kernel.org/patch/1745041/
> 
> Please fix it up so I can put it in the Linux tree.
Please see below.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>

Thanks,
Mukesh


diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 1a6bca1..543877d 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -14,12 +14,17 @@
 #include <asm/xen/interface.h>
 
 #ifdef CONFIG_XEN_X86_PVH
-#define FEATURES_PVH "|writable_descriptor_tables" \
-		     "|auto_translated_physmap" \
-		     "|supervisor_mode_kernel" \
-		     "|hvm_callback_vector"
+#define XEN_FEATURES_STR .ascii  "!writable_page_tables"       \
+                                 "|pae_pgdir_above_4gb"        \
+                                 "|writable_descriptor_tables" \
+                                 "|auto_translated_physmap"    \
+                                 "|supervisor_mode_kernel"     \
+                                 "|hvm_callback_vector"        \
+                                 "\0"
 #else
-#define FEATURES_PVH /* Not supported */
+#define XEN_FEATURES_STR .ascii "!writable_page_tables"        \
+                                "|pae_pgdir_above_4gb"         \
+                                 "\0"
 #endif
 
 	__INIT
@@ -104,7 +109,7 @@ NEXT_HYPERCALL(arch_6)
 #endif
 	ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
 	ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page)
-	ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz
"!writable_page_tables|pae_pgdir_above_4gb"FEATURES_PVH)
+        ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       XEN_FEATURES_STR)
 	ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
 	ELFNOTE(Xen, XEN_ELFNOTE_LOADER,         .asciz "generic")
 	ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,

Jan Beulich

2013-Jan-21 12:06 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> On 19.01.13 at 02:35, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Thu, 17 Jan 2013 22:22:47 -0500
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
>> Jan had some comments about that patch:
>> 
>> https://patchwork.kernel.org/patch/1745041/ 
>> 
>> Please fix it up so I can put it in the Linux tree.
> 
> Please see below.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> Thanks,
> Mukesh
> 
> 
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index 1a6bca1..543877d 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -14,12 +14,17 @@
>  #include <asm/xen/interface.h>
>  
>  #ifdef CONFIG_XEN_X86_PVH
> -#define FEATURES_PVH "|writable_descriptor_tables" \
> -		     "|auto_translated_physmap" \
> -		     "|supervisor_mode_kernel" \
> -		     "|hvm_callback_vector"
> +#define XEN_FEATURES_STR .ascii  "!writable_page_tables"       \
> +                                 "|pae_pgdir_above_4gb"        \
> +                                 "|writable_descriptor_tables" \
> +                                 "|auto_translated_physmap"    \
> +                                 "|supervisor_mode_kernel"     \
> +                                 "|hvm_callback_vector"        \
> +                                 "\0"
>  #else
> -#define FEATURES_PVH /* Not supported */
> +#define XEN_FEATURES_STR .ascii "!writable_page_tables"        \
> +                                "|pae_pgdir_above_4gb"         \
> +                                 "\0"
>  #endif
>  
>  	__INIT
> @@ -104,7 +109,7 @@ NEXT_HYPERCALL(arch_6)
>  #endif
>  	ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
>  	ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page)
> -	ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz 
> "!writable_page_tables|pae_pgdir_above_4gb"FEATURES_PVH)
> +        ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       XEN_FEATURES_STR)
>  	ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
>  	ELFNOTE(Xen, XEN_ELFNOTE_LOADER,         .asciz "generic")
>  	ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,
That''s not really better. What I''d like you to do is keep the
common
part common (i.e. not redundantly defined) and add the PVH-specific
bits (with what amounts to an empty string as the non-PVH
replacement) on top. Meaning you will likely want a mixture of
.ascii and .asciz.

Jan

Mukesh Rathor

2013-Jan-22 23:12 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Mon, 21 Jan 2013 12:06:42 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 19.01.13 at 02:35, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> 
> That''s not really better. What I''d like you to do is keep
the common
> part common (i.e. not redundantly defined) and add the PVH-specific
> bits (with what amounts to an empty string as the non-PVH
> replacement) on top. Meaning you will likely want a mixture of
> .ascii and .asciz.
         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz
"!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);

Will put NULL char before PVH_FEATURES_STR, so we need:

         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii
"!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);

This means PVH_FEATURES_STR has to be defined as:

#define PVH_FEATURES_STR 
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|hvm_callback_vector\0"

because 
#define PVH_FEATURES_STR "|writable_descriptor_tables" \
                         "|auto_translated_physmap"       
                         .....

will put null char after writable_descriptor_tables. Putting .ascii
above will not work either with concatenation later.

So, I think what I proposed earlier is the cleanest. Alternately:

#ifdef CONFIG_XEN_X86_PVH

#define PVH_FEATURES_STR 
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|hvm_callback_vector\0"

#else
#define PVH_FEATURES_STR "\0"
#endif

Then:
        ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii
"!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);


Let me know what you prefer, whats right above, or what I had originally
last week. I can''t figure any other way.

Thanks,
Mukesh

Jan Beulich

2013-Jan-23 08:22 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> On 23.01.13 at 00:12, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Mon, 21 Jan 2013 12:06:42 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 19.01.13 at 02:35, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >>> wrote:
>> 
>> That''s not really better. What I''d like you to do is
keep the common
>> part common (i.e. not redundantly defined) and add the PVH-specific
>> bits (with what amounts to an empty string as the non-PVH
>> replacement) on top. Meaning you will likely want a mixture of
>> .ascii and .asciz.
> 
>          ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz 
> "!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);
> 
> Will put NULL char before PVH_FEATURES_STR, so we need:
> 
>          ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> "!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);
> 
> This means PVH_FEATURES_STR has to be defined as:
> 
> #define PVH_FEATURES_STR  
>
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|h
> vm_callback_vector\0"
> 
> because 
> #define PVH_FEATURES_STR "|writable_descriptor_tables" \
>                          "|auto_translated_physmap"       
>                          .....
> 
> will put null char after writable_descriptor_tables. Putting .ascii
> above will not work either with concatenation later.
> 
> So, I think what I proposed earlier is the cleanest. Alternately:
> 
> #ifdef CONFIG_XEN_X86_PVH
> 
> #define PVH_FEATURES_STR  
>
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|h
> vm_callback_vector\0"
> 
> #else
> #define PVH_FEATURES_STR "\0"
> #endif
> 
> Then:
>         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> "!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);
> 
> 
> Let me know what you prefer, whats right above, or what I had originally
> last week. I can''t figure any other way.
I continue to think that the cleanest is to use something like

ELFNOTE(Xen, XEN_ELFNOTE_FEATURES, .ascii
"!writable_page_tables|pae_pgdir_above_4gb"; .asciz PVH_FEATURES_STR);

Jan

Mukesh Rathor

2013-Jan-23 22:43 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Wed, 23 Jan 2013 08:22:47 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 23.01.13 at 00:12, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > On Mon, 21 Jan 2013 12:06:42 +0000
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> > So, I think what I proposed earlier is the cleanest. Alternately:
> > 
> > #ifdef CONFIG_XEN_X86_PVH
> > 
> > #define PVH_FEATURES_STR  
> >
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|h
> > vm_callback_vector\0"
> > 
> > #else
> > #define PVH_FEATURES_STR "\0"
> > #endif
> > 
> > Then:
> >         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> >
"!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);
> > 
> 
> ELFNOTE(Xen, XEN_ELFNOTE_FEATURES, .ascii
> "!writable_page_tables|pae_pgdir_above_4gb"; .asciz
PVH_FEATURES_STR);
Not that different from what I have above since PVH_FEATURES_STR still
has to be defined as null string for non PVH case. Anyways, below is
the latest patch. Konard, kindly apply if Jan is OK.

Thanks,
Mukesh


PVH: Use .ascii and .asciz to define xen feature string. Note, the PVH
string must be in a single line (not multiple lines with \) to keep the
assembler from putting null char after each string before \.


Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>

diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 1a6bca1..45226cb 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -14,12 +14,11 @@
 #include <asm/xen/interface.h>
 
 #ifdef CONFIG_XEN_X86_PVH
-#define FEATURES_PVH "|writable_descriptor_tables" \
-		     "|auto_translated_physmap" \
-		     "|supervisor_mode_kernel" \
-		     "|hvm_callback_vector"
+
+#define PVH_FEATURES_STR 
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|hvm_callback_vector"
+
 #else
-#define FEATURES_PVH /* Not supported */
+#define PVH_FEATURES_STR  ""
 #endif
 
 	__INIT
@@ -104,7 +103,7 @@ NEXT_HYPERCALL(arch_6)
 #endif
 	ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
 	ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page)
-	ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz
"!writable_page_tables|pae_pgdir_above_4gb"FEATURES_PVH)
+        ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii
"!writable_page_tables|pae_pgdir_above_4gb"; .asciz PVH_FEATURES_STR);
 	ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
 	ELFNOTE(Xen, XEN_ELFNOTE_LOADER,         .asciz "generic")
 	ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,

Jan Beulich

2013-Jan-24 09:16 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> On 23.01.13 at 23:43, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Wed, 23 Jan 2013 08:22:47 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 23.01.13 at 00:12, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >>> wrote:
>> > On Mon, 21 Jan 2013 12:06:42 +0000
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> > 
>> > So, I think what I proposed earlier is the cleanest. Alternately:
>> > 
>> > #ifdef CONFIG_XEN_X86_PVH
>> > 
>> > #define PVH_FEATURES_STR  
>> > 
>
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|h
>> > vm_callback_vector\0"
>> > 
>> > #else
>> > #define PVH_FEATURES_STR "\0"
>> > #endif
>> > 
>> > Then:
>> >         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
>> >
"!writable_page_tables|pae_pgdir_above_4gb"PVH_FEATURES_STR);
>> > 
>> 
>> ELFNOTE(Xen, XEN_ELFNOTE_FEATURES, .ascii
>> "!writable_page_tables|pae_pgdir_above_4gb"; .asciz
PVH_FEATURES_STR);
> 
> Not that different from what I have above since PVH_FEATURES_STR still
> has to be defined as null string for non PVH case. Anyways, below is
> the latest patch. Konard, kindly apply if Jan is OK.
> 
> Thanks,
> Mukesh
> 
> 
> PVH: Use .ascii and .asciz to define xen feature string. Note, the PVH
> string must be in a single line (not multiple lines with \) to keep the
> assembler from putting null char after each string before \.
This looks to be an incremental patch on top of something I
don''t think I''ve seen - there''s neither an embedded
\0 nor any
redundancy being removed here, yet I''m sure that was so in the
prior patch, and these two were what I disliked most. So perhaps
sending out the original patch this one goes on top of was simply
never done?

Jan
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index 1a6bca1..45226cb 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -14,12 +14,11 @@
>  #include <asm/xen/interface.h>
>  
>  #ifdef CONFIG_XEN_X86_PVH
> -#define FEATURES_PVH "|writable_descriptor_tables" \
> -		     "|auto_translated_physmap" \
> -		     "|supervisor_mode_kernel" \
> -		     "|hvm_callback_vector"
> +
> +#define PVH_FEATURES_STR  
>
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|h
> vm_callback_vector"
> +
>  #else
> -#define FEATURES_PVH /* Not supported */
> +#define PVH_FEATURES_STR  ""
>  #endif
>  
>  	__INIT
> @@ -104,7 +103,7 @@ NEXT_HYPERCALL(arch_6)
>  #endif
>  	ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
>  	ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page)
> -	ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz 
> "!writable_page_tables|pae_pgdir_above_4gb"FEATURES_PVH)
> +        ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> "!writable_page_tables|pae_pgdir_above_4gb"; .asciz
PVH_FEATURES_STR);
>  	ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
>  	ELFNOTE(Xen, XEN_ELFNOTE_LOADER,         .asciz "generic")
>  	ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,

Ian Campbell

2013-Jan-24 09:27 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Thu, 2013-01-24 at 09:16 +0000, Jan Beulich wrote:> This looks to be an incremental patch on top of something I
> don''t think I''ve seen - there''s neither an
embedded \0 nor any
> redundancy being removed here, yet I''m sure that was so in the
> prior patch, and these two were what I disliked most. So perhaps
> sending out the original patch this one goes on top of was simply
> never done?
The baseline of this patch wouldn''t have worked because it has .asciz
as
the first (global) features, so that would be NULL terminated and the
PVH features wouldn''t appear.

This one one of the first revisions I think it''s probably what is
current in Konrad''s tree.

Ian.

Konrad Rzeszutek Wilk

2013-Jan-24 15:10 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Thu, Jan 24, 2013 at 09:27:05AM +0000, Ian Campbell
wrote:> On Thu, 2013-01-24 at 09:16 +0000, Jan Beulich wrote:
> > This looks to be an incremental patch on top of something I
> > don''t think I''ve seen - there''s neither an
embedded \0 nor any
> > redundancy being removed here, yet I''m sure that was so in
the
> > prior patch, and these two were what I disliked most. So perhaps
> > sending out the original patch this one goes on top of was simply
> > never done?
> 
> The baseline of this patch wouldn''t have worked because it has
.asciz as
> the first (global) features, so that would be NULL terminated and the
> PVH features wouldn''t appear.
> 
> This one one of the first revisions I think it''s probably what is
> current in Konrad''s tree.
Yup.> 
> Ian.
>

Mukesh Rathor

2013-Jan-24 23:13 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Thu, 24 Jan 2013 09:16:56 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> This looks to be an incremental patch on top of something I
> don''t think I''ve seen - there''s neither an
embedded \0 nor any
> redundancy being removed here, yet I''m sure that was so in the
> prior patch, and these two were what I disliked most. So perhaps
> sending out the original patch this one goes on top of was simply
> never done?
This is on top of what konrad already has in his linux tree so he
can just apply it. Here''s the final version of the file, just in case:



/* Xen-specific pieces of head.S, intended to be included in the right
	place in head.S */

#ifdef CONFIG_XEN

#include <linux/elfnote.h>
#include <linux/init.h>

#include <asm/boot.h>
#include <asm/asm.h>
#include <asm/page_types.h>

#include <xen/interface/elfnote.h>
#include <asm/xen/interface.h>

#ifdef CONFIG_XEN_X86_PVH

#define PVH_FEATURES_STR 
"|writable_descriptor_tables|auto_translated_physmap|supervisor_mode_kernel|hvm_callback_vector"

#else
#define PVH_FEATURES_STR  ""
#endif

	__INIT
ENTRY(startup_xen)
	cld
#ifdef CONFIG_X86_32
	mov %esi,xen_start_info
	mov $init_thread_union+THREAD_SIZE,%esp
#else
	mov %rsi,xen_start_info
	mov $init_thread_union+THREAD_SIZE,%rsp
#endif
	jmp xen_start_kernel

	__FINIT

.pushsection .text
	.balign PAGE_SIZE
ENTRY(hypercall_page)
#define NEXT_HYPERCALL(x) \
	ENTRY(xen_hypercall_##x) \
	.skip 32

NEXT_HYPERCALL(set_trap_table)
NEXT_HYPERCALL(mmu_update)
NEXT_HYPERCALL(set_gdt)
NEXT_HYPERCALL(stack_switch)
NEXT_HYPERCALL(set_callbacks)
NEXT_HYPERCALL(fpu_taskswitch)
NEXT_HYPERCALL(sched_op_compat)
NEXT_HYPERCALL(platform_op)
NEXT_HYPERCALL(set_debugreg)
NEXT_HYPERCALL(get_debugreg)
NEXT_HYPERCALL(update_descriptor)
NEXT_HYPERCALL(ni)
NEXT_HYPERCALL(memory_op)
NEXT_HYPERCALL(multicall)
NEXT_HYPERCALL(update_va_mapping)
NEXT_HYPERCALL(set_timer_op)
NEXT_HYPERCALL(event_channel_op_compat)
NEXT_HYPERCALL(xen_version)
NEXT_HYPERCALL(console_io)
NEXT_HYPERCALL(physdev_op_compat)
NEXT_HYPERCALL(grant_table_op)
NEXT_HYPERCALL(vm_assist)
NEXT_HYPERCALL(update_va_mapping_otherdomain)
NEXT_HYPERCALL(iret)
NEXT_HYPERCALL(vcpu_op)
NEXT_HYPERCALL(set_segment_base)
NEXT_HYPERCALL(mmuext_op)
NEXT_HYPERCALL(xsm_op)
NEXT_HYPERCALL(nmi_op)
NEXT_HYPERCALL(sched_op)
NEXT_HYPERCALL(callback_op)
NEXT_HYPERCALL(xenoprof_op)
NEXT_HYPERCALL(event_channel_op)
NEXT_HYPERCALL(physdev_op)
NEXT_HYPERCALL(hvm_op)
NEXT_HYPERCALL(sysctl)
NEXT_HYPERCALL(domctl)
NEXT_HYPERCALL(kexec_op)
NEXT_HYPERCALL(tmem_op) /* 38 */
ENTRY(xen_hypercall_rsvr)
	.skip 320
NEXT_HYPERCALL(mca) /* 48 */
NEXT_HYPERCALL(arch_1)
NEXT_HYPERCALL(arch_2)
NEXT_HYPERCALL(arch_3)
NEXT_HYPERCALL(arch_4)
NEXT_HYPERCALL(arch_5)
NEXT_HYPERCALL(arch_6)
	.balign PAGE_SIZE
.popsection

	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
	ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION,  .asciz "2.6")
	ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION,    .asciz "xen-3.0")
#ifdef CONFIG_X86_32
	ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR __PAGE_OFFSET)
#else
	ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR __START_KERNEL_map)
#endif
	ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
	ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page)
        ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii
"!writable_page_tables|pae_pgdir_above_4gb"; .asciz PVH_FEATURES_STR);
	ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
	ELFNOTE(Xen, XEN_ELFNOTE_LOADER,         .asciz "generic")
	ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,
		.quad _PAGE_PRESENT; .quad _PAGE_PRESENT)
	ELFNOTE(Xen, XEN_ELFNOTE_SUSPEND_CANCEL, .long 1)
	ELFNOTE(Xen, XEN_ELFNOTE_HV_START_LOW,   _ASM_PTR __HYPERVISOR_VIRT_START)
	ELFNOTE(Xen, XEN_ELFNOTE_PADDR_OFFSET,   _ASM_PTR 0)

#endif /*CONFIG_XEN */

Jan Beulich

2013-Jan-25 08:02 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> On 25.01.13 at 00:13, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> This is on top of what konrad already has in his linux tree so he
> can just apply it. Here''s the final version of the file, just in
case:
Looks okay to me, and I don''t mind whether this
>         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> "!writable_page_tables|pae_pgdir_above_4gb"; .asciz
PVH_FEATURES_STR);
is done using the .ascii/.asciz or the simpler string concatenation
way, as long as the latter works. All I do mind are redundancy
and embedded \0 characters.

Jan

Ian Campbell

2013-Jan-25 10:11 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Fri, 2013-01-25 at 08:02 +0000, Jan Beulich wrote:> >>> On 25.01.13 at 00:13, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> > This is on top of what konrad already has in his linux tree so he
> > can just apply it. Here''s the final version of the file, just
in case:
> 
> Looks okay to me, and I don''t mind whether this
> 
> >         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> > "!writable_page_tables|pae_pgdir_above_4gb"; .asciz
PVH_FEATURES_STR);
> 
> is done using the .ascii/.asciz or the simpler string concatenation
> way, as long as the latter works.
String concat in gas doesn''t work like in C, you get a \0 at the
join...
>  All I do mind are redundancy
> and embedded \0 characters.
> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Jan Beulich

2013-Jan-25 10:27 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> On 25.01.13 at 11:11, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Fri, 2013-01-25 at 08:02 +0000, Jan Beulich wrote:
>> >>> On 25.01.13 at 00:13, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
>> > This is on top of what konrad already has in his linux tree so he
>> > can just apply it. Here''s the final version of the file,
just in case:
>> 
>> Looks okay to me, and I don''t mind whether this
>> 
>> >         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
>> > "!writable_page_tables|pae_pgdir_above_4gb"; .asciz
PVH_FEATURES_STR);
>> 
>> is done using the .ascii/.asciz or the simpler string concatenation
>> way, as long as the latter works.
> 
> String concat in gas doesn''t work like in C, you get a \0 at the
join...
But this is going through the C preprocessor, and that one,
according to all I know, ought to concatenate adjacent strings.
But of course it depends between which translation phases
the preprocessor/compiler boundary is being set, so perhaps
doing it the .ascii/.asciz way is indeed the only universal one.

Jan

Ian Campbell

2013-Jan-25 10:43 UTC

head link

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Fri, 2013-01-25 at 10:27 +0000, Jan Beulich wrote:> >>> On 25.01.13 at 11:11, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > On Fri, 2013-01-25 at 08:02 +0000, Jan Beulich wrote:
> >> >>> On 25.01.13 at 00:13, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> >> > This is on top of what konrad already has in his linux tree
so he
> >> > can just apply it. Here''s the final version of the
file, just in case:
> >> 
> >> Looks okay to me, and I don''t mind whether this
> >> 
> >> >         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> >> > "!writable_page_tables|pae_pgdir_above_4gb"; .asciz
PVH_FEATURES_STR);
> >> 
> >> is done using the .ascii/.asciz or the simpler string
concatenation
> >> way, as long as the latter works.
> > 
> > String concat in gas doesn''t work like in C, you get a \0 at
the join...
> 
> But this is going through the C preprocessor, and that one,
> according to all I know, ought to concatenate adjacent strings.
I don''t think so, turning
        "Foo" "Bar"
into
        "FooBar"
happens at the C level not the CPP level.

        #define FOO(x) x
        FOO(.asciz "Foo" "Bar")
        
        #define CAT(x,y) x y
        FOO(.asciz CAT("Foo","Bar"))
        
        #define CAT2(x,y) x #y
        FOO(CAT2(.asciz, Foo Bar))
will produce
        .asciz "Foo" "Bar"
        .asciz "Foo" "Bar"
        .asciz "Foo Bar"

(where FOO(x) ~= ELFNOTE(..., x))

The third form might be an alternative fix to this issue but I don''t
think it is any better than the .ascii/.asciz solution.

Ian.

Konrad Rzeszutek Wilk

2013-Jan-28 16:26 UTC

head link

Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Fri, Jan 25, 2013 at 10:43:41AM +0000, Ian Campbell
wrote:> On Fri, 2013-01-25 at 10:27 +0000, Jan Beulich wrote:
> > >>> On 25.01.13 at 11:11, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > > On Fri, 2013-01-25 at 08:02 +0000, Jan Beulich wrote:
> > >> >>> On 25.01.13 at 00:13, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> > >> > This is on top of what konrad already has in his linux
tree so he
> > >> > can just apply it. Here''s the final version of
the file, just in case:
> > >> 
> > >> Looks okay to me, and I don''t mind whether this
> > >> 
> > >> >         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii 
> > >> > "!writable_page_tables|pae_pgdir_above_4gb";
.asciz PVH_FEATURES_STR);
> > >> 
> > >> is done using the .ascii/.asciz or the simpler string
concatenation
> > >> way, as long as the latter works.
> > > 
> > > String concat in gas doesn''t work like in C, you get a
\0 at the join...
> > 
> > But this is going through the C preprocessor, and that one,
> > according to all I know, ought to concatenate adjacent strings.
> 
> I don''t think so, turning
>         "Foo" "Bar"
> into
>         "FooBar"
> happens at the C level not the CPP level.
> 
>         #define FOO(x) x
>         FOO(.asciz "Foo" "Bar")
>         
>         #define CAT(x,y) x y
>         FOO(.asciz CAT("Foo","Bar"))
>         
>         #define CAT2(x,y) x #y
>         FOO(CAT2(.asciz, Foo Bar))
> will produce
>         .asciz "Foo" "Bar"
>         .asciz "Foo" "Bar"
>         .asciz "Foo Bar"
> 
> (where FOO(x) ~= ELFNOTE(..., x))
> 
> The third form might be an alternative fix to this issue but I
don''t
> think it is any better than the .ascii/.asciz solution.
OK. So I stuck Mukesh''s patch in the tree along with yours:

http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=shortlog;h=refs/heads/stable/pvh.v7

and all of that on top of v3.8-rc5:

http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=shortlog;h=refs/heads/linux-next

We still have a couple of weeks before we have to make the
go/no-go decision on the the new hypercalls:


#define XENMEM_add_to_physmap_range 23
struct xen_add_to_physmap_range {
    /* IN */
    /* Which domain to change the mapping for. */
    domid_t domid;
    uint16_t space; /* => enum phys_map_space */

    /* Number of pages to go through */
    uint16_t size;
    domid_t foreign_domid; /* IFF gmfn_foreign */

    /* Indexes into space being mapped. */
    GUEST_HANDLE(xen_ulong_t) idxs;

    /* GPFN in domid where the source mapping page should appear. */
    GUEST_HANDLE(xen_pfn_t) gpfns;

    /* OUT */

    /* Per index error code. */
    GUEST_HANDLE(int) errs;
};


/*
 * Unmaps the page appearing at a particular GPFN from the specified
guest''s
 * pseudophysical address space.
 * arg == addr of xen_remove_from_physmap_t.
 */
#define XENMEM_remove_from_physmap      15
struct xen_remove_from_physmap {
    /* Which domain to change the mapping for. */
    domid_t domid;

    /* GPFN of the current mapping of the page. */
    xen_pfn_t gpfn;
};
DEFINE_GUEST_HANDLE_STRUCT(xen_remove_from_physmap);


/*
 * Returns the location in virtual address space of the machine_to_phys
 * mapping table. Architectures which do not have a m2p table, or which do not
 * map it by default into guest address space, do not implement this command.
 * arg == addr of xen_machphys_mapping_t.
 */
#define XENMEM_machphys_mapping     12
struct xen_machphys_mapping {
    xen_ulong_t v_start, v_end; /* Start and end virtual addresses.   */
    xen_ulong_t max_mfn;        /* Maximum MFN that can be looked up. */
};


#define PHYSDEVOP_map_iomem        30
struct physdev_map_iomem {
    /* IN */
    uint64_t first_gfn;
    uint64_t first_mfn;
    uint32_t nr_mfns;
    uint32_t add_mapping; /* 1 == add mapping;  0 == unmap */

};

and the modification to one:

commit 50c0b3df91be7a440b50ee7d74fd2042bca173fd
Author: Mukesh Rathor <mukesh.rathor@oracle.com>
Date:   Mon Oct 22 11:37:57 2012 -0400

    xen/pvh: Extend vcpu_guest_context, p2m, event, and XenBus.
    
    Make gdt_frames[]/gdt_ents into a union with {gdtaddr, gdtsz},
    as PVH only needs to send down gdtaddr and gdtsz in the
    vcpu_guest_context structure..
    
    For interrupts, PVH uses native_irq_ops so we can skip most of the
    PV ones. In the future we can support the pirq_eoi_map..
    Also VCPU hotplug is currently not available for PVH.
    
    For events (and IRQs) we follow what PVHVM does - so use callback
    vector.  Lastly, for XenBus we use the same logic that is used in
    the PVHVM case.
    
    Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
    Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
    [v2: Rebased it]
    [v3: Move 64-bit ifdef and based on Stefan add extra comments.]
    [v4: Rebased it once more]
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff --git a/arch/x86/include/asm/xen/interface.h
b/arch/x86/include/asm/xen/interface.h
index fd9cb76..20e738a 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -145,7 +145,16 @@ struct vcpu_guest_context {
     struct cpu_user_regs user_regs;         /* User-level CPU registers     */
     struct trap_info trap_ctxt[256];        /* Virtual IDT                  */
     unsigned long ldt_base, ldt_ents;       /* LDT (linear address, # ents) */
-    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents) */
+    union {
+	struct {
+		/* PV: GDT (machine frames, # ents).*/
+		unsigned long gdt_frames[16], gdt_ents;
+	} pv;
+	struct {
+		/* PVH: GDTR addr and size */
+		unsigned long gdtaddr, gdtsz;
+	} pvh;
+    } u;
     unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only SS1/SP1)   */
     /* NB. User pagetable on x86/64 is placed in ctrlreg[1]. */
     unsigned long ctrlreg[8];               /* CR0-CR7 (control registers) 
*/> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

Mukesh Rathor

2013-Jan-29 02:57 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Mon, 28 Jan 2013 11:26:08 -0500
Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> On Fri, Jan 25, 2013 at 10:43:41AM +0000, Ian Campbell wrote:
> > On Fri, 2013-01-25 at 10:27 +0000, Jan Beulich wrote:
> 
> #define PHYSDEVOP_map_iomem        30
> struct physdev_map_iomem {
>     /* IN */
>     uint64_t first_gfn;
>     uint64_t first_mfn;
>     uint32_t nr_mfns;
>     uint32_t add_mapping; /* 1 == add mapping;  0 == unmap */
> 
> };
Since the new recommendation is to map transparently, we don''t need
this anymore. I''ll submit an incremental patch to you Konrad.

 > and the modification to one:
> 
> commit 50c0b3df91be7a440b50ee7d74fd2042bca173fd
> Author: Mukesh Rathor <mukesh.rathor@oracle.com>
> Date:   Mon Oct 22 11:37:57 2012 -0400
> 
>     xen/pvh: Extend vcpu_guest_context, p2m, event, and XenBus.
>     
>     Make gdt_frames[]/gdt_ents into a union with {gdtaddr, gdtsz},
>     as PVH only needs to send down gdtaddr and gdtsz in the
>     vcpu_guest_context structure..
>     
>     For interrupts, PVH uses native_irq_ops so we can skip most of the
>     PV ones. In the future we can support the pirq_eoi_map..
>     Also VCPU hotplug is currently not available for PVH.
>     
>     For events (and IRQs) we follow what PVHVM does - so use callback
>     vector.  Lastly, for XenBus we use the same logic that is used in
>     the PVHVM case.
>     
>     Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>     Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
>     [v2: Rebased it]
>     [v3: Move 64-bit ifdef and based on Stefan add extra comments.]
>     [v4: Rebased it once more]
>     Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> diff --git a/arch/x86/include/asm/xen/interface.h
> b/arch/x86/include/asm/xen/interface.h index fd9cb76..20e738a 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -145,7 +145,16 @@ struct vcpu_guest_context {
>      struct cpu_user_regs user_regs;         /* User-level CPU
> registers     */ struct trap_info trap_ctxt[256];        /* Virtual
> IDT                  */ unsigned long ldt_base, ldt_ents;       /*
> LDT (linear address, # ents) */
> -    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames,
> # ents) */
> +    union {
> +	struct {
> +		/* PV: GDT (machine frames, # ents).*/
> +		unsigned long gdt_frames[16], gdt_ents;
> +	} pv;
> +	struct {
> +		/* PVH: GDTR addr and size */
> +		unsigned long gdtaddr, gdtsz;
> +	} pvh;
> +    } u;
>      unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only

On xen side I added the ifdef:

#if __XEN_INTERFACE_VERSION__ < 0x00040300
    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents) */
#else
    union {
        struct {
            /* GDT (machine frames, # ents) */
            unsigned long gdt_frames[16], gdt_ents;
        } pv;
        struct {
            /* PVH: GDTR addr and size */
            unsigned long gdtaddr, gdtsz;
        } pvh;
    } u;
#endif

but it doesn''t matter on linux side, so up to you.

Jan Beulich

2013-Jan-29 10:48 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> On 29.01.13 at 03:57, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On xen side I added the ifdef:
> 
> #if __XEN_INTERFACE_VERSION__ < 0x00040300
>     unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents)
> */
> #else
>     union {
>         struct {
>             /* GDT (machine frames, # ents) */
>             unsigned long gdt_frames[16], gdt_ents;
>         } pv;
>         struct {
>             /* PVH: GDTR addr and size */
>             unsigned long gdtaddr, gdtsz;
>         } pvh;
>     } u;
> #endif
> 
> but it doesn''t matter on linux side, so up to you.
But I''d still prefer for this to go away again - you could simply use
gdt_frames[0] for gdtaddr and gdt_ents for the (normalized)
gdtsz.

And if you nevertheless go the union route, call it "gdt" instead
of "u" and drop the gdt/gdt_ prefixes from the member names
(yes, I know, grepping and cscoping for such member is more
difficult, but I continue to see more advantage in avoiding the
redundancy).

Jan

Mukesh Rathor

2013-Feb-01 02:23 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Tue, 29 Jan 2013 10:48:12 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 29.01.13 at 03:57, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > On xen side I added the ifdef:
> > 
> > #if __XEN_INTERFACE_VERSION__ < 0x00040300
> >     unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames,
> > # ents) */
> > #else
> >     union {
> >         struct {
> >             /* GDT (machine frames, # ents) */
> >             unsigned long gdt_frames[16], gdt_ents;
> >         } pv;
> >         struct {
> >             /* PVH: GDTR addr and size */
> >             unsigned long gdtaddr, gdtsz;
> >         } pvh;
> >     } u;
> > #endif
> > 
> > but it doesn''t matter on linux side, so up to you.
> 
> But I''d still prefer for this to go away again - you could simply
use
> gdt_frames[0] for gdtaddr and gdt_ents for the (normalized)
> gdtsz.
That was my patch version 1 during linux patch review. Then the reviewer
suggested to make it a union.
> And if you nevertheless go the union route, call it "gdt" instead
> of "u" and drop the gdt/gdt_ prefixes from the member names
> (yes, I know, grepping and cscoping for such member is more
> difficult, but I continue to see more advantage in avoiding the
> redundancy).
That was my patch version 2, where I called it gdt and another reviewer
suggested to change to u. So I changed it to u.

It''s gone thru enough iterations that I''d like to leave as is.
Thank
you in advance for your compromise in helping us mortals grep/cscope 
to learn code.

Thanks,
Mukesh

Jan Beulich

2013-Feb-01 16:24 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> Mukesh Rathor <mukesh.rathor@oracle.com> 02/01/13 3:23 AM
>>>
>On Tue, 29 Jan 2013 10:48:12 +0000 "Jan Beulich"
<JBeulich@suse.com> wrote:
>> >>> On 29.01.13 at 03:57, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >>> wrote:
>> > On xen side I added the ifdef:
>> > 
>> > #if __XEN_INTERFACE_VERSION__ < 0x00040300
>> >     unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
frames,
>> > # ents) */
>> > #else
>> >     union {
>> >         struct {
>> >             /* GDT (machine frames, # ents) */
>> >             unsigned long gdt_frames[16], gdt_ents;
>> >         } pv;
>> >         struct {
>> >             /* PVH: GDTR addr and size */
>> >             unsigned long gdtaddr, gdtsz;
>> >         } pvh;
>> >     } u;
>> > #endif
>> > 
>> > but it doesn''t matter on linux side, so up to you.
>> 
>> But I''d still prefer for this to go away again - you could
simply use
>> gdt_frames[0] for gdtaddr and gdt_ents for the (normalized)
>> gdtsz.
>
>That was my patch version 1 during linux patch review. Then the reviewer
>suggested to make it a union.
>
>> And if you nevertheless go the union route, call it "gdt"
instead
>> of "u" and drop the gdt/gdt_ prefixes from the member names
>> (yes, I know, grepping and cscoping for such member is more
>> difficult, but I continue to see more advantage in avoiding the
>> redundancy).
>
>That was my patch version 2, where I called it gdt and another reviewer
>suggested to change to u. So I changed it to u.
>
>It''s gone thru enough iterations that I''d like to leave as
is. Thank
>you in advance for your compromise in helping us mortals grep/cscope 
>to learn code.
That''s part of the reason why I said from the beginning that doing the
Linux
side first is wrong.

Jan

Mukesh Rathor

2013-Feb-01 19:27 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Fri, 01 Feb 2013 16:24:57 +0000
"Jan Beulich" <jbeulich@suse.com> wrote:
> >>> Mukesh Rathor <mukesh.rathor@oracle.com> 02/01/13 3:23
AM >>>
> >On Tue, 29 Jan 2013 10:48:12 +0000 "Jan Beulich"
<JBeulich@suse.com>
> >wrote:
> >> >>> On 29.01.13 at 03:57, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >> >>> wrote:
> >> difficult, but I continue to see more advantage in avoiding the
> >> redundancy).
> >
> >That was my patch version 2, where I called it gdt and another
> >reviewer suggested to change to u. So I changed it to u.
> >
> >It''s gone thru enough iterations that I''d like to
leave as is. Thank
> >you in advance for your compromise in helping us mortals grep/cscope 
> >to learn code.
> 
> That''s part of the reason why I said from the beginning that doing
> the Linux side first is wrong.
It was reviewed by xen folks like Ian C, Stefano, etc... 

Thanks,
Mukesh

Ian Campbell

2013-Feb-04 10:31 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Fri, 2013-02-01 at 19:27 +0000, Mukesh Rathor wrote:> On Fri, 01 Feb 2013 16:24:57 +0000
> "Jan Beulich" <jbeulich@suse.com> wrote:
> 
> > >>> Mukesh Rathor <mukesh.rathor@oracle.com> 02/01/13
3:23 AM >>>
> > >On Tue, 29 Jan 2013 10:48:12 +0000 "Jan Beulich"
<JBeulich@suse.com>
> > >wrote:
> > >> >>> On 29.01.13 at 03:57, Mukesh Rathor
<mukesh.rathor@oracle.com>
> > >> >>> wrote:
> > >> difficult, but I continue to see more advantage in avoiding
the
> > >> redundancy).
> > >
> > >That was my patch version 2, where I called it gdt and another
> > >reviewer suggested to change to u. So I changed it to u.
> > >
> > >It''s gone thru enough iterations that I''d like
to leave as is. Thank
> > >you in advance for your compromise in helping us mortals
grep/cscope
> > >to learn code.
> > 
> > That''s part of the reason why I said from the beginning that
doing
> > the Linux side first is wrong.
> 
> It was reviewed by xen folks like Ian C, Stefano, etc... 
Well, that doesn''t in any way mean that Jan''s opinion
isn''t valid or
relevant and where reviewers/authors cannot reach consensus over
something then it is ultimately the maintainer''s call.

I deliberately avoided formally Acking any of the Linux side patches
until the hypervisor side was posted and reviewed, precisely because it
is necessary to review the hypervisor interfaces as part of the
hypervisor changes before committing to them. I believe we explicitly
agreed that the interface would be subject to change when the hypervisor
patches were posted for review.

WRT this particular change I don''t personally like the idea of sharing
gdt_frames[0] and gdt_ents for two semantically different usages, but I
would defer to Jan as a hypervisor maintainer on that point.

I don''t much care about the gdt vs u naming for the union, although I
would probably have gone with the more meaningful gdt if it were me.
cscope-wise in emacs I would just use "C-c s t" to look for
"gdt.size"
etc.

Ian.

Mukesh Rathor

2013-Feb-05 01:04 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

On Mon, 4 Feb 2013 10:31:41 +0000
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2013-02-01 at 19:27 +0000, Mukesh Rathor wrote:
> > On Fri, 01 Feb 2013 16:24:57 +0000
> > "Jan Beulich" <jbeulich@suse.com> wrote:
> > 
> I don''t much care about the gdt vs u naming for the union,
although I
> would probably have gone with the more meaningful gdt if it were me.
> cscope-wise in emacs I would just use "C-c s t" to look for
"gdt.size"
> etc.
Unf vim cscope doesn''t support that. Moreover, I bet if size was
accessed
via pointer, you couldn''t search it that way either. In general, all 
enterprise software I''ve worked on prohibited such generic names for 
the reason people couldn''t easily find their usages. I find it very 
frustrating, and I hope I can continue to be able to read xen code.

Thanks,
Mukesh

Jan Beulich

2013-Feb-05 07:53 UTC

head link

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

>>> On 05.02.13 at 02:04, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Mon, 4 Feb 2013 10:31:41 +0000
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
>> On Fri, 2013-02-01 at 19:27 +0000, Mukesh Rathor wrote:
>> > On Fri, 01 Feb 2013 16:24:57 +0000
>> > "Jan Beulich" <jbeulich@suse.com> wrote:
>> > 
> 
>> I don''t much care about the gdt vs u naming for the union,
although I
>> would probably have gone with the more meaningful gdt if it were me.
>> cscope-wise in emacs I would just use "C-c s t" to look for
"gdt.size"
>> etc.
> 
> Unf vim cscope doesn''t support that. Moreover, I bet if size was
accessed
> via pointer, you couldn''t search it that way either. In general,
all
> enterprise software I''ve worked on prohibited such generic names
for
> the reason people couldn''t easily find their usages. I find it
very
> frustrating, and I hope I can continue to be able to read xen code.
I''m sorry, Mukesh, but if everyone else manages to read Xen
code with untagged field names and the like, then I''m afraid
you''ll need to find a way for yourself to handle this...

Jan

Possibly Parallel Threads

Search for more apparently analagous threads

Xen devel - Jan 2013 - [PATCH]: PVH: specify xen features strings cleany for PVH

[PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH

Possibly Parallel Threads