Re: [Xen-devel] [PATCH v9 04/19] amd64: introduce hook for custom preload metadata parsers

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <machine/reg.h>
>>  #include <machine/sigframe.h>
>>  #include <machine/specialreg.h>
>> +#include <machine/sysarch.h>
>>  #ifdef PERFMON
>>  #include <machine/perfmon.h>
>>  #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 <x86/sysarch.h>
>> +
>> +/*
>> + * 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 <vm/vm_pager.h>
>>  #include <vm/vm_param.h>
>>  
>> +#include <machine/sysarch.h>
>> +
>>  #include <xen/xen-os.h>
>>  #include <xen/hypervisor.h>
>>  
>> @@ -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@xxxxxxxxxxxxx
>> http://lists.xen.org/xen-devel


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