Re: [Xen-devel] [PATCH RFC v1 21/74] x86/entry: Early PVH boot code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.01.18 at 14:05, <wei.liu2@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -380,7 +380,39 @@ cs32_switch:
>  ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY, .long sym_offs(__pvh_start))
>  
>  __pvh_start:
> -        ud2a
> +        cld
> +        cli
> +
> +        /*
> +         * We need one push/pop to determine load address.  Use the same
> +         * absolute address as the native path, for lack of a better

... stack address ...

> @@ -544,12 +576,18 @@ trampoline_setup:
>          /* Get bottom-most low-memory stack address. */
>          add     $TRAMPOLINE_SPACE,%ecx
>  
> +#ifdef CONFIG_PVH_GUEST
> +        cmpb    $1, sym_fs(pvh_boot)
> +        je      1f

I'd much prefer

        cmpb    $0, sym_fs(pvh_boot)
        jne     1f

in cases like this one.

But then I sort of dislike the addition of such random in-memory
flags. Considering ...

> +#endif
> +
>          /* Save the Multiboot info struct (after relocation) for later use. 
> */
>          push    %ecx                /* Bottom-most low-memory stack address. 
> */
>          push    %ebx                /* Multiboot information address. */
>          push    %eax                /* Multiboot magic. */

... the values used here, couldn't the flag be replaced by setting
one or both of %eax and %ebx to zero before jumping to
trampoline_setup? Or wait, further down I see that this flag is
also being use in C code. Perhaps fine then as is. Otoh, keying
this off of one of the register values would allow the #ifdef to
be dropped.

> --- /dev/null
> +++ b/xen/arch/x86/guest/pvh-boot.c
> @@ -0,0 +1,119 @@
> +/******************************************************************************
> + * arch/x86/guest/pvh-boot.c
> + *
> + * PVH boot time support
> + *
> + * 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
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> + *
> + * Copyright (c) 2017 Citrix Systems Ltd.
> + */
> +#include <xen/init.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +
> +#include <asm/guest.h>
> +
> +#include <public/arch-x86/hvm/start_info.h>
> +
> +/* Initialised in head.S, before .bss is zeroed. */
> +bool pvh_boot __initdata;
> +uint32_t pvh_start_info_pa __initdata;

Would you mind using the more common placement of __initdata,
like you do ...

> +static multiboot_info_t __initdata pvh_mbi;
> +static module_t __initdata pvh_mbi_mods[32];
> +static char *__initdata pvh_loader = "PVH Directboot";

... here?

For the last item

static const char __initconst pvh_loader[] = "PVH Directboot";

please. For mods[] - isn't 32 overly much?

> +static void __init convert_pvh_info(void)
> +{
> +    struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
> +    struct hvm_modlist_entry *entry;

const (twice)

> +    module_t *mod;
> +    unsigned int i;
> +
> +    ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
> +
> +    /*
> +     * Turn hvm_start_info into mbi. Luckily all modules are placed under 4GB
> +     * boundary on x86.

ISTR having that discussion relatively recently in another context:
All the header states is "NB: Xen on x86 will always try to place all
the data below the 4GiB boundary." Note the "try to". Hence I
think ...

> +     */
> +    pvh_mbi.flags = MBI_CMDLINE | MBI_MODULES | MBI_LOADERNAME;
> +
> +    ASSERT(!(pvh_info->cmdline_paddr >> 32));

... this, if we don't want to handle the case, should be BUG_ON() or
panic() (same further down).

> +    pvh_mbi.cmdline = pvh_info->cmdline_paddr;
> +    pvh_mbi.boot_loader_name = __pa(pvh_loader);
> +
> +    ASSERT(pvh_info->nr_modules < 32);

ARRAY_SIZE(pvh_mbi_mods) and perhaps again BUG_ON() or
panic().

> +    pvh_mbi.mods_count = pvh_info->nr_modules;
> +    pvh_mbi.mods_addr = __pa(pvh_mbi_mods);
> +
> +    mod = pvh_mbi_mods;
> +    entry = __va(pvh_info->modlist_paddr);

How come __va() already works at this point in time? And what about
this address being beyond 4Gb?

> +    for ( i = 0; i < pvh_info->nr_modules; i++ )
> +    {
> +        ASSERT(!(entry[i].paddr >> 32));

To relax this condition (in particular to allow huge initrd), how
about ...

> +        mod[i].mod_start = entry[i].paddr;
> +        mod[i].mod_end   = entry[i].paddr + entry[i].size;

... using the EFI approach here and store the PFN in mod_start
and the size in mod_end?

> +        mod[i].string    = entry[i].cmdline_paddr;

No 4Gb check here?

> +void __init pvh_print_info(void)
> +{
> +    struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
> +    struct hvm_modlist_entry *entry;

const (twice) again

> +    unsigned int i;
> +
> +    ASSERT(pvh_info->magic == XEN_HVM_START_MAGIC_VALUE);
> +
> +    printk("PVH start info: (pa %08x)\n", pvh_start_info_pa);
> +    printk("  version:    %u\n", pvh_info->version);
> +    printk("  flags:      %#"PRIx32"\n", pvh_info->flags);
> +    printk("  nr_modules: %u\n", pvh_info->nr_modules);
> +    printk("  modlist_pa: %016"PRIx64"\n", pvh_info->modlist_paddr);
> +    printk("  cmdline_pa: %016"PRIx64"\n", pvh_info->cmdline_paddr);

Considering you assume these to be below 4Gb anyway, how about
just %08?

> +    if ( pvh_info->cmdline_paddr )
> +        printk("  cmdline:    '%s'\n",
> +               (char *)__va(pvh_info->cmdline_paddr));

This appears to fit on one line.

> +    printk("  rsdp_pa:    %016"PRIx64"\n", pvh_info->rsdp_paddr);

This one also unlikely needs 16 digits (and there are more
candidates further down).

> +    entry = __va(pvh_info->modlist_paddr);
> +    for ( i = 0; i < pvh_info->nr_modules; i++ )
> +    {
> +        printk("    mod[%u].pa:         %016"PRIx64"\n", i, entry[i].paddr);
> +        printk("    mod[%u].size:       %016"PRIu64"\n", i, entry[i].size);
> +        printk("    mod[%u].cmdline_pa: %016"PRIx64"\n",
> +               i, entry[i].cmdline_paddr);
> +        if ( entry[i].cmdline_paddr )
> +            printk("    mod[%u].cmdline:    '%s'\n", i,
> +                   (char *)__va(entry[i].cmdline_paddr));

[%2u] perhaps in all cases, unless you decide to shrink the array
size to no more than 10?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel