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 <jbeul...@suse.com>
Acked-by: Andrew Cooper <andrew.coop...@citrix.com> 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
>
