Re: [PATCH 1/3] x86: slightly re-arrange 32-bit handling in dom0_construct_pv()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/08/2020 10:28, Jan Beulich wrote:
> Add #ifdef-s (the 2nd one will be needed in particular, to guard the
> uses of m2p_compat_vstart and HYPERVISOR_COMPAT_VIRT_START()) and fold
> duplicate uses of elf_32bit().
>
> Also adjust what gets logged: Avoid "compat32" when support isn't built
> in, and don't assume ELF class <> ELFCLASS64 means ELFCLASS32.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> although with some
further suggestions.

>
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -300,7 +300,6 @@ int __init dom0_construct_pv(struct doma
>      struct page_info *page = NULL;
>      start_info_t *si;
>      struct vcpu *v = d->vcpu[0];
> -    unsigned long long value;
>      void *image_base = bootstrap_map(image);
>      unsigned long image_len = image->mod_end;
>      void *image_start = image_base + image_headroom;
> @@ -357,27 +356,36 @@ int __init dom0_construct_pv(struct doma
>          goto out;
>  
>      /* compatibility check */
> +    printk(" Xen  kernel: 64-bit, lsb%s\n",
> +           IS_ENABLED(CONFIG_PV32) ? ", compat32" : "");

Here, and below, we print out lsb/msb for the ELF file.  However, we
don't use or check that it is actually lsb, and blindly assume that it is.

Why bother printing it then?

>      compatible = 0;
>      machine = elf_uval(&elf, elf.ehdr, e_machine);
> -    printk(" Xen  kernel: 64-bit, lsb, compat32\n");
> -    if ( elf_32bit(&elf) && parms.pae == XEN_PAE_BIMODAL )
> -        parms.pae = XEN_PAE_EXTCR3;
> -    if ( elf_32bit(&elf) && parms.pae && machine == EM_386 )
> +
> +#ifdef CONFIG_PV32
> +    if ( elf_32bit(&elf) )
>      {
> -        if ( unlikely(rc = switch_compat(d)) )
> +        if ( parms.pae == XEN_PAE_BIMODAL )
> +            parms.pae = XEN_PAE_EXTCR3;
> +        if ( parms.pae && machine == EM_386 )
>          {
> -            printk("Dom0 failed to switch to compat: %d\n", rc);
> -            return rc;
> -        }
> +            if ( unlikely(rc = switch_compat(d)) )
> +            {
> +                printk("Dom0 failed to switch to compat: %d\n", rc);
> +                return rc;
> +            }
>  
> -        compatible = 1;
> +            compatible = 1;
> +        }
>      }
> -    if (elf_64bit(&elf) && machine == EM_X86_64)
> +#endif
> +
> +    if ( elf_64bit(&elf) && machine == EM_X86_64 )
>          compatible = 1;
> -    printk(" Dom0 kernel: %s%s, %s, paddr %#" PRIx64 " -> %#" PRIx64 "\n",
> -           elf_64bit(&elf) ? "64-bit" : "32-bit",
> -           parms.pae       ? ", PAE"  : "",
> -           elf_msb(&elf)   ? "msb"    : "lsb",
> +
> +    printk(" Dom0 kernel: %s-bit%s, %s, paddr %#" PRIx64 " -> %#" PRIx64 
> "\n",
> +           elf_64bit(&elf) ? "64" : elf_32bit(&elf) ? "32" : "??",
> +           parms.pae       ? ", PAE" : "",
> +           elf_msb(&elf)   ? "msb"   : "lsb",
>             elf.pstart, elf.pend);
>      if ( elf.bsd_symtab_pstart )
>          printk(" Dom0 symbol map %#" PRIx64 " -> %#" PRIx64 "\n",
> @@ -405,23 +413,28 @@ int __init dom0_construct_pv(struct doma
>      if ( parms.pae == XEN_PAE_EXTCR3 )
>              set_bit(VMASST_TYPE_pae_extended_cr3, &d->vm_assist);
>  
> -    if ( !pv_shim && (parms.virt_hv_start_low != UNSET_ADDR) &&
> -         elf_32bit(&elf) )
> +#ifdef CONFIG_PV32
> +    if ( elf_32bit(&elf) )
>      {
> -        unsigned long mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
> -        value = (parms.virt_hv_start_low + mask) & ~mask;
> -        BUG_ON(!is_pv_32bit_domain(d));
> -        if ( value > __HYPERVISOR_COMPAT_VIRT_START )
> -            panic("Domain 0 expects too high a hypervisor start address\n");
> -        HYPERVISOR_COMPAT_VIRT_START(d) =
> -            max_t(unsigned int, m2p_compat_vstart, value);
> -    }
> +        if ( !pv_shim && (parms.virt_hv_start_low != UNSET_ADDR) )
> +        {
> +            unsigned long mask = (1UL << L2_PAGETABLE_SHIFT) - 1;
> +            unsigned long value = (parms.virt_hv_start_low + mask) & ~mask;

ROUNDUP() instead of opencoding it?

>  
> -    if ( (parms.p2m_base != UNSET_ADDR) && elf_32bit(&elf) )
> -    {
> -        printk(XENLOG_WARNING "P2M table base ignored\n");
> -        parms.p2m_base = UNSET_ADDR;
> +            BUG_ON(!is_pv_32bit_domain(d));

This BUG_ON() is useless.  I suspect it is a vestigial safety measure
from when the switch to compat was opencoded rather than using
switch_compat() directly.

> +            if ( value > __HYPERVISOR_COMPAT_VIRT_START )
> +                panic("Domain 0 expects too high a hypervisor start 
> address\n");

It would be better to printk() and return -EINVAL, to be consistent with
how other fatal errors are reported to the user.

~Andrew

> +            HYPERVISOR_COMPAT_VIRT_START(d) =
> +                max_t(unsigned int, m2p_compat_vstart, value);
> +        }
> +
> +        if ( parms.p2m_base != UNSET_ADDR )
> +        {
> +            printk(XENLOG_WARNING "P2M table base ignored\n");
> +            parms.p2m_base = UNSET_ADDR;
> +        }
>      }
> +#endif
>  
>      /*
>       * Why do we need this? The number of page-table frames depends on the
>