Re: [PATCH for-next v3 06/22] x86/traps: move PV hypercall handlers to pv/traps.c

[Wei Liu <wei.liu2@citrix.com>]

On Wed, May 31, 2017 at 05:45:14AM -0600, Jan Beulich wrote:
> >>> On 31.05.17 at 13:14, <wei.liu2@citrix.com> wrote:
> > On Tue, May 30, 2017 at 11:59:31PM -0600, Jan Beulich wrote:
> >> >>> On 30.05.17 at 19:40, <andrew.cooper3@citrix.com> wrote:
> >> > On 29/05/17 16:40, Jan Beulich wrote:
> >> >>>>> On 18.05.17 at 19:09, <wei.liu2@citrix.com> wrote:
> >> >>> The following handlers are moved:
> >> >>> 1. do_set_trap_table
> >> >> This one makes sense to move to pv/traps.c, but ...
> >> >>
> >> >>> 2. do_set_debugreg
> >> >>> 3. do_get_debugreg
> >> >>> 4. do_fpu_taskswitch
> >> >> ... none of these do. I could see them go into pv/hypercall.c,
> >> >> but I could also see that file dealing intentionally only with
> >> >> everything hypercall related except individual handlers. Andrew,
> >> >> do you have any opinion or thoughts here?
> >> > 
> >> > Despite its name, traps.c deals with mostly low level exception
> >> > handling, so I am not completely convinced that do_set_trap_table()
> >> > would logically live in traps.c
> >> 
> >> I can see this being the case for traps.c, but pv/traps.c? There's
> >> not much _low level_ exception handling that's PV-specific. But I
> >> certainly don't mind such relatively small hypercall handlers to be
> >> lumped together into some other file, ...
> >> 
> >> > I'd also prefer not to mix these into hypercall.c.  The best I can
> >> > suggest is pv/domain.c, but even that isn't great.
> >> > 
> >> > Sorry for being unhelpful.  I'm not sure pv/misc-hypercalls.c is a
> >> > suitable name either.
> >> 
> >> ... be it this name or some other one (if we can think of a better
> >> alternative). Thinking of it: The currently present file is named
> >> "hypercall.c" - how about "hypercalls.c"?
> >> 
> > 
> > Well I don't think moving them into hypercall(s).c is nice either.
> > 
> > Since you expressed the idea of using iret.c for do_iret, maybe we can
> > use debugreg.c and fpu_taskswitch.c ?
> 
> I did consider this too, but some of these are really small, and
> while is dislike overly large files, I also don't think files with just
> one or two dozens of actual code lines are very useful to have.
> 

TBH I don't really see a problem with that approach -- we have clear_page.S,
copy_page.S and gpr_switch.S which don't get lumped together into one
file.

But if you don't like that, I will just put those small handlers
into hypercall.c and change the comment of that file to:

diff --git a/xen/arch/x86/pv/hypercall.c b/xen/arch/x86/pv/hypercall.c
index 7c5e5a629d..b0c9e01268 100644
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -1,7 +1,7 @@
 /******************************************************************************
  * arch/x86/pv/hypercall.c
  *
- * PV hypercall dispatching routines
+ * PV hypercall dispatching routines and misc hypercalls
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by

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