Re: [Xen-devel] [PATCH for-next 5/7] x86_64: move PV specific code under pv/x86_64

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Apr 21, 2017 at 03:33:11AM -0600, Jan Beulich wrote:
> >>> On 06.04.17 at 19:14, <wei.liu2@xxxxxxxxxx> wrote:
> > No functional change.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/pv/Makefile                    |   1 +
> >  xen/arch/x86/pv/x86_64/Makefile             |   1 +
> >  xen/arch/x86/{ => pv}/x86_64/compat/traps.c |   0
> >  xen/arch/x86/pv/x86_64/traps.c              | 398 
> > ++++++++++++++++++++++++++++
> >  xen/arch/x86/x86_64/traps.c                 | 377 
> > --------------------------
> >  5 files changed, 400 insertions(+), 377 deletions(-)
> >  create mode 100644 xen/arch/x86/pv/x86_64/Makefile
> >  rename xen/arch/x86/{ => pv}/x86_64/compat/traps.c (100%)
> >  create mode 100644 xen/arch/x86/pv/x86_64/traps.c
> 
> Please don't - we really should try to remove all those x86_64/
> subdirectories (and I must have overlooked this same aspect
> earlier on). There's no reason to believe the current leftovers
> from the 32-/64-bit split are going to be appropriate for a
> hypothetical successor architecture to x86-64.
> 

Fine with me to remove x86_64 directories.

> > -void subarch_percpu_traps_init(void)
> > -{
> > -    unsigned long stack_bottom = get_stack_bottom();
> > -    unsigned long stub_va = this_cpu(stubs.addr);
> > -    unsigned char *stub_page;
> > -    unsigned int offset;
> > -
> > -    /* IST_MAX IST pages + 1 syscall page + 1 guard page + primary stack. 
> > */
> > -    BUILD_BUG_ON((IST_MAX + 2) * PAGE_SIZE + PRIMARY_STACK_SIZE > 
> > STACK_SIZE);
> 
> Even if it's only this one line - this isn't PV-specific, and hence shouldn't
> be moved. It is also inappropriate for a function with this name to live
> in PV-specific code.
> 

I see your point for this BUILD_BUG_ON. But the rest of this function is
setting up trampoline stub for syscall and sysenter, which, AFAICT, are
only ever going to be used by PV guests. What did I miss? May be I
should rename this function?

> > -void hypercall_page_initialise(struct domain *d, void *hypercall_page)
> > -{
> > -    memset(hypercall_page, 0xCC, PAGE_SIZE);
> > -    if ( is_hvm_domain(d) )
> > -        hvm_hypercall_page_initialise(d, hypercall_page);
> > -    else if ( !is_pv_32bit_domain(d) )
> > -        hypercall_page_initialise_ring3_kernel(hypercall_page);
> > -    else
> > -        hypercall_page_initialise_ring1_kernel(hypercall_page);
> > -}
> 
> Same for this function, the more that it calls a HVM one.
> 

Yes this one needs to stay in commmon code.

Wei.

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel