From owner-freebsd-current@FreeBSD.ORG Tue Jan 7 11:07:10 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 9749C38E; Tue, 7 Jan 2014 11:07:10 +0000 (UTC) Received: from SMTP02.CITRIX.COM (smtp02.citrix.com [66.165.176.63]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 02A9D1B4C; Tue, 7 Jan 2014 11:07:08 +0000 (UTC) X-IronPort-AV: E=Sophos;i="4.95,618,1384300800"; d="scan'208";a="88227067" Received: from accessns.citrite.net (HELO FTLPEX01CL01.citrite.net) ([10.9.154.239]) by FTLPIPO02.CITRIX.COM with ESMTP; 07 Jan 2014 11:07:01 +0000 Received: from [IPv6:::1] (10.80.16.47) by smtprelay.citrix.com (10.13.107.78) with Microsoft SMTP Server id 14.2.342.4; Tue, 7 Jan 2014 06:07:00 -0500 Message-ID: <52CBDFD2.603@citrix.com> Date: Tue, 7 Jan 2014 12:06:58 +0100 From: =?ISO-8859-1?Q?Roger_Pau_Monn=E9?= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Konrad Rzeszutek Wilk Subject: Re: [Xen-devel] [PATCH v9 04/19] amd64: introduce hook for custom preload metadata parsers References: <1388677433-49525-1-git-send-email-roger.pau@citrix.com> <1388677433-49525-5-git-send-email-roger.pau@citrix.com> <20140103205949.GC2732@phenom.dumpdata.com> In-Reply-To: <20140103205949.GC2732@phenom.dumpdata.com> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-DLP: MIA2 Cc: xen-devel@lists.xen.org, julien.grall@citrix.com, freebsd-xen@freebsd.org, freebsd-current@freebsd.org, kib@freebsd.org, gibbs@freebsd.org X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Jan 2014 11:07:10 -0000 On 03/01/14 21:59, Konrad Rzeszutek Wilk wrote: > On Thu, Jan 02, 2014 at 04:43:38PM +0100, Roger Pau Monne wrote: >> --- >> sys/amd64/amd64/machdep.c | 41 ++++++++++++++++------ >> sys/amd64/include/sysarch.h | 12 ++++++ >> sys/x86/xen/pv.c | 82 +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 124 insertions(+), 11 deletions(-) >> >> diff --git a/sys/amd64/amd64/machdep.c b/sys/amd64/amd64/machdep.c >> index eae657b..e073eea 100644 >> --- a/sys/amd64/amd64/machdep.c >> +++ b/sys/amd64/amd64/machdep.c >> @@ -126,6 +126,7 @@ __FBSDID("$FreeBSD$"); >> #include >> #include >> #include >> +#include >> #ifdef PERFMON >> #include >> #endif >> @@ -165,6 +166,14 @@ static int set_fpcontext(struct thread *td, const mcontext_t *mcp, >> char *xfpustate, size_t xfpustate_len); >> SYSINIT(cpu, SI_SUB_CPU, SI_ORDER_FIRST, cpu_startup, NULL); >> >> +/* Preload data parse function */ >> +static caddr_t native_parse_preload_data(u_int64_t); >> + >> +/* Default init_ops implementation. */ >> +struct init_ops init_ops = { >> + .parse_preload_data = native_parse_preload_data, > > Extra space there. It's for alignment, looks strange now because there's only one hook. >> +}; >> + >> /* >> * The file "conf/ldscript.amd64" defines the symbol "kernphys". Its value is >> * the physical address at which the kernel is loaded. >> @@ -1683,6 +1692,26 @@ do_next: >> msgbufp = (struct msgbuf *)PHYS_TO_DMAP(phys_avail[pa_indx]); >> } >> >> +static caddr_t >> +native_parse_preload_data(u_int64_t modulep) >> +{ >> + caddr_t kmdp; >> + >> + preload_metadata = (caddr_t)(uintptr_t)(modulep + KERNBASE); > > Two casts? Could it be done via one? > >> + preload_bootstrap_relocate(KERNBASE); >> + kmdp = preload_search_by_type("elf kernel"); >> + if (kmdp == NULL) >> + kmdp = preload_search_by_type("elf64 kernel"); >> + boothowto = MD_FETCH(kmdp, MODINFOMD_HOWTO, int); >> + kern_envp = MD_FETCH(kmdp, MODINFOMD_ENVP, char *) + KERNBASE; >> +#ifdef DDB >> + ksym_start = MD_FETCH(kmdp, MODINFOMD_SSYM, uintptr_t); >> + ksym_end = MD_FETCH(kmdp, MODINFOMD_ESYM, uintptr_t); >> +#endif >> + >> + return (kmdp); >> +} >> + >> u_int64_t >> hammer_time(u_int64_t modulep, u_int64_t physfree) >> { >> @@ -1707,17 +1736,7 @@ hammer_time(u_int64_t modulep, u_int64_t physfree) >> */ >> proc_linkup0(&proc0, &thread0); >> >> - preload_metadata = (caddr_t)(uintptr_t)(modulep + KERNBASE); > > Oh, you just moved the code - right, lets not modify it in this patch. Yes, it's just code movement. > >> - preload_bootstrap_relocate(KERNBASE); >> - kmdp = preload_search_by_type("elf kernel"); >> - if (kmdp == NULL) >> - kmdp = preload_search_by_type("elf64 kernel"); >> - boothowto = MD_FETCH(kmdp, MODINFOMD_HOWTO, int); >> - kern_envp = MD_FETCH(kmdp, MODINFOMD_ENVP, char *) + KERNBASE; >> -#ifdef DDB >> - ksym_start = MD_FETCH(kmdp, MODINFOMD_SSYM, uintptr_t); >> - ksym_end = MD_FETCH(kmdp, MODINFOMD_ESYM, uintptr_t); >> -#endif >> + kmdp = init_ops.parse_preload_data(modulep); >> >> /* Init basic tunables, hz etc */ >> init_param1(); >> diff --git a/sys/amd64/include/sysarch.h b/sys/amd64/include/sysarch.h >> index cd380d4..58ac8cd 100644 >> --- a/sys/amd64/include/sysarch.h >> +++ b/sys/amd64/include/sysarch.h >> @@ -4,3 +4,15 @@ >> /* $FreeBSD$ */ >> >> #include >> + >> +/* >> + * Struct containing pointers to init functions whose >> + * implementation is run time selectable. Selection can be made, >> + * for example, based on detection of a BIOS variant or >> + * hypervisor environment. >> + */ >> +struct init_ops { >> + caddr_t (*parse_preload_data)(u_int64_t); >> +}; >> + >> +extern struct init_ops init_ops; >> diff --git a/sys/x86/xen/pv.c b/sys/x86/xen/pv.c >> index db3b7a3..908b50b 100644 >> --- a/sys/x86/xen/pv.c >> +++ b/sys/x86/xen/pv.c >> @@ -46,6 +46,8 @@ __FBSDID("$FreeBSD$"); >> #include >> #include >> >> +#include >> + >> #include >> #include >> >> @@ -54,6 +56,36 @@ extern u_int64_t hammer_time(u_int64_t, u_int64_t); >> /* Xen initial function */ >> extern u_int64_t hammer_time_xen(start_info_t *, u_int64_t); >> >> +/*--------------------------- Forward Declarations ---------------------------*/ >> +static caddr_t xen_pv_parse_preload_data(u_int64_t); >> + >> +static void xen_pv_set_init_ops(void); >> + >> +/*-------------------------------- Global Data -------------------------------*/ >> +/* Xen init_ops implementation. */ >> +struct init_ops xen_init_ops = { >> + .parse_preload_data = xen_pv_parse_preload_data, >> +}; >> + >> +static struct >> +{ >> + const char *ev; >> + int mask; >> +} howto_names[] = { >> + {"boot_askname", RB_ASKNAME}, >> + {"boot_single", RB_SINGLE}, >> + {"boot_nosync", RB_NOSYNC}, >> + {"boot_halt", RB_ASKNAME}, >> + {"boot_serial", RB_SERIAL}, >> + {"boot_cdrom", RB_CDROM}, >> + {"boot_gdb", RB_GDB}, >> + {"boot_gdb_pause", RB_RESERVED1}, >> + {"boot_verbose", RB_VERBOSE}, >> + {"boot_multicons", RB_MULTIPLE}, >> + {NULL, 0} >> +}; >> + >> +/*-------------------------------- Xen PV init -------------------------------*/ >> /* >> * First function called by the Xen PVH boot sequence. >> * >> @@ -118,6 +150,56 @@ hammer_time_xen(start_info_t *si, u_int64_t xenstack) >> } >> load_cr3(((u_int64_t)&PT4[0]) - KERNBASE); >> >> + /* Set the hooks for early functions that diverge from bare metal */ >> + xen_pv_set_init_ops(); >> + >> /* Now we can jump into the native init function */ >> return (hammer_time(0, physfree)); >> } >> + >> +/*-------------------------------- PV specific -------------------------------*/ >> +/* >> + * Functions to convert the "extra" parameters passed by Xen >> + * into FreeBSD boot options (from the i386 Xen port). >> + */ >> +static char * >> +xen_setbootenv(char *cmd_line) >> +{ >> + char *cmd_line_next; >> + >> + /* Skip leading spaces */ >> + for (; *cmd_line == ' '; cmd_line++); > > Spaces? > >> + >> + for (cmd_line_next = cmd_line; strsep(&cmd_line_next, ",") != NULL;); >> + return (cmd_line); >> +} >> + >> +static int >> +xen_boothowto(char *envp) >> +{ >> + int i, howto = 0; >> + >> + /* get equivalents from the environment */ >> + for (i = 0; howto_names[i].ev != NULL; i++) >> + if (getenv(howto_names[i].ev) != NULL) >> + howto |= howto_names[i].mask; > > You don't believe in '{}' do you ? :-) All this code has also been taken from the FreeBSD Xen i386 PV port, but maybe some refactoring wouldn't be bad :). >> + return (howto); >> +} >> + >> +static caddr_t >> +xen_pv_parse_preload_data(u_int64_t modulep) >> +{ >> + /* Parse the extra boot information given by Xen */ >> + if (HYPERVISOR_start_info->cmd_line) >> + kern_envp = xen_setbootenv(HYPERVISOR_start_info->cmd_line); >> + boothowto |= xen_boothowto(kern_envp); >> + >> + return (NULL); >> +} >> + >> +static void >> +xen_pv_set_init_ops(void) >> +{ >> + /* Init ops for Xen PV */ >> + init_ops = xen_init_ops; >> +} >> -- >> 1.7.7.5 (Apple Git-26) >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel