* [PATCH]: PVH: specify xen features strings cleany for PVH @ 2013-01-19 1:35 Mukesh Rathor 2013-01-21 12:06 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-01-19 1:35 UTC (permalink / raw) To: Xen-devel; +Cc: Jan Beulich, Konrad Rzeszutek Wilk 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, ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-19 1:35 [PATCH]: PVH: specify xen features strings cleany for PVH Mukesh Rathor @ 2013-01-21 12:06 ` Jan Beulich 2013-01-22 23:12 ` Mukesh Rathor 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2013-01-21 12:06 UTC (permalink / raw) To: Mukesh Rathor; +Cc: Konrad Rzeszutek Wilk, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-21 12:06 ` Jan Beulich @ 2013-01-22 23:12 ` Mukesh Rathor 2013-01-23 8:22 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-01-22 23:12 UTC (permalink / raw) To: Jan Beulich; +Cc: Konrad Rzeszutek Wilk, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-22 23:12 ` Mukesh Rathor @ 2013-01-23 8:22 ` Jan Beulich 2013-01-23 22:43 ` Mukesh Rathor 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2013-01-23 8:22 UTC (permalink / raw) To: Mukesh Rathor; +Cc: Konrad Rzeszutek Wilk, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-23 8:22 ` Jan Beulich @ 2013-01-23 22:43 ` Mukesh Rathor 2013-01-24 9:16 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-01-23 22:43 UTC (permalink / raw) To: Jan Beulich; +Cc: Konrad Rzeszutek Wilk, xen-devel 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, ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-23 22:43 ` Mukesh Rathor @ 2013-01-24 9:16 ` Jan Beulich 2013-01-24 9:27 ` Ian Campbell 2013-01-24 23:13 ` Mukesh Rathor 0 siblings, 2 replies; 22+ messages in thread From: Jan Beulich @ 2013-01-24 9:16 UTC (permalink / raw) To: Mukesh Rathor; +Cc: Konrad Rzeszutek Wilk, xen-devel >>> 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, ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-24 9:16 ` Jan Beulich @ 2013-01-24 9:27 ` Ian Campbell 2013-01-24 15:10 ` Konrad Rzeszutek Wilk 2013-01-24 23:13 ` Mukesh Rathor 1 sibling, 1 reply; 22+ messages in thread From: Ian Campbell @ 2013-01-24 9:27 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Konrad Rzeszutek Wilk 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-24 9:27 ` Ian Campbell @ 2013-01-24 15:10 ` Konrad Rzeszutek Wilk 0 siblings, 0 replies; 22+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-01-24 15:10 UTC (permalink / raw) To: Ian Campbell; +Cc: Jan Beulich, xen-devel 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. > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-24 9:16 ` Jan Beulich 2013-01-24 9:27 ` Ian Campbell @ 2013-01-24 23:13 ` Mukesh Rathor 2013-01-25 8:02 ` Jan Beulich 1 sibling, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-01-24 23:13 UTC (permalink / raw) To: Jan Beulich; +Cc: Konrad Rzeszutek Wilk, xen-devel 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 */ ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-24 23:13 ` Mukesh Rathor @ 2013-01-25 8:02 ` Jan Beulich 2013-01-25 10:11 ` Ian Campbell 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2013-01-25 8:02 UTC (permalink / raw) To: Mukesh Rathor; +Cc: Konrad Rzeszutek Wilk, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-25 8:02 ` Jan Beulich @ 2013-01-25 10:11 ` Ian Campbell 2013-01-25 10:27 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Ian Campbell @ 2013-01-25 10:11 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Konrad Rzeszutek Wilk 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-25 10:11 ` Ian Campbell @ 2013-01-25 10:27 ` Jan Beulich 2013-01-25 10:43 ` Ian Campbell 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2013-01-25 10:27 UTC (permalink / raw) To: Ian Campbell; +Cc: Konrad Rzeszutek Wilk, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-25 10:27 ` Jan Beulich @ 2013-01-25 10:43 ` Ian Campbell 2013-01-28 16:26 ` Is: PVH + ARM new hypercalls. Was: " Konrad Rzeszutek Wilk 0 siblings, 1 reply; 22+ messages in thread From: Ian Campbell @ 2013-01-25 10:43 UTC (permalink / raw) To: Jan Beulich; +Cc: Konrad Rzeszutek Wilk, xen-devel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-25 10:43 ` Ian Campbell @ 2013-01-28 16:26 ` Konrad Rzeszutek Wilk 2013-01-29 2:57 ` Mukesh Rathor 0 siblings, 1 reply; 22+ messages in thread From: Konrad Rzeszutek Wilk @ 2013-01-28 16:26 UTC (permalink / raw) To: Ian Campbell; +Cc: xen-devel, Jan Beulich, Konrad Rzeszutek Wilk 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 > ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-28 16:26 ` Is: PVH + ARM new hypercalls. Was: " Konrad Rzeszutek Wilk @ 2013-01-29 2:57 ` Mukesh Rathor 2013-01-29 10:48 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-01-29 2:57 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Konrad Rzeszutek Wilk, Ian Campbell, Jan Beulich, xen-devel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-29 2:57 ` Mukesh Rathor @ 2013-01-29 10:48 ` Jan Beulich 2013-02-01 2:23 ` Mukesh Rathor 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2013-01-29 10:48 UTC (permalink / raw) To: Mukesh Rathor Cc: Konrad Rzeszutek Wilk, KonradRzeszutek Wilk, Ian Campbell, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-01-29 10:48 ` Jan Beulich @ 2013-02-01 2:23 ` Mukesh Rathor 2013-02-01 16:24 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-02-01 2:23 UTC (permalink / raw) To: Jan Beulich Cc: Konrad Rzeszutek Wilk, KonradRzeszutek Wilk, Ian Campbell, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-02-01 2:23 ` Mukesh Rathor @ 2013-02-01 16:24 ` Jan Beulich 2013-02-01 19:27 ` Mukesh Rathor 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2013-02-01 16:24 UTC (permalink / raw) To: mukesh.rathor; +Cc: konrad, konrad.wilk, Ian.Campbell, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-02-01 16:24 ` Jan Beulich @ 2013-02-01 19:27 ` Mukesh Rathor 2013-02-04 10:31 ` Ian Campbell 0 siblings, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-02-01 19:27 UTC (permalink / raw) To: Jan Beulich; +Cc: konrad, konrad.wilk, Ian.Campbell, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-02-01 19:27 ` Mukesh Rathor @ 2013-02-04 10:31 ` Ian Campbell 2013-02-05 1:04 ` Mukesh Rathor 0 siblings, 1 reply; 22+ messages in thread From: Ian Campbell @ 2013-02-04 10:31 UTC (permalink / raw) To: Mukesh Rathor; +Cc: konrad, konrad.wilk, Jan Beulich, xen-devel 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-02-04 10:31 ` Ian Campbell @ 2013-02-05 1:04 ` Mukesh Rathor 2013-02-05 7:53 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: Mukesh Rathor @ 2013-02-05 1:04 UTC (permalink / raw) To: Ian Campbell; +Cc: konrad, konrad.wilk, Jan Beulich, xen-devel 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Is: PVH + ARM new hypercalls. Was: Re: [PATCH]: PVH: specify xen features strings cleany for PVH 2013-02-05 1:04 ` Mukesh Rathor @ 2013-02-05 7:53 ` Jan Beulich 0 siblings, 0 replies; 22+ messages in thread From: Jan Beulich @ 2013-02-05 7:53 UTC (permalink / raw) To: Mukesh Rathor; +Cc: konrad, konrad.wilk, Ian Campbell, xen-devel >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-02-05 7:53 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-01-19 1:35 [PATCH]: PVH: specify xen features strings cleany for PVH Mukesh Rathor 2013-01-21 12:06 ` Jan Beulich 2013-01-22 23:12 ` Mukesh Rathor 2013-01-23 8:22 ` Jan Beulich 2013-01-23 22:43 ` Mukesh Rathor 2013-01-24 9:16 ` Jan Beulich 2013-01-24 9:27 ` Ian Campbell 2013-01-24 15:10 ` Konrad Rzeszutek Wilk 2013-01-24 23:13 ` Mukesh Rathor 2013-01-25 8:02 ` Jan Beulich 2013-01-25 10:11 ` Ian Campbell 2013-01-25 10:27 ` Jan Beulich 2013-01-25 10:43 ` Ian Campbell 2013-01-28 16:26 ` Is: PVH + ARM new hypercalls. Was: " Konrad Rzeszutek Wilk 2013-01-29 2:57 ` Mukesh Rathor 2013-01-29 10:48 ` Jan Beulich 2013-02-01 2:23 ` Mukesh Rathor 2013-02-01 16:24 ` Jan Beulich 2013-02-01 19:27 ` Mukesh Rathor 2013-02-04 10:31 ` Ian Campbell 2013-02-05 1:04 ` Mukesh Rathor 2013-02-05 7:53 ` Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.