diff mbox series

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

Message ID 0972ea3c-c2a8-b0f4-ae25-132bdb798f6a@suse.com (mailing list archive)
State Superseded
Headers show
Series x86: M2P maintenance adjustments (step 1) | expand

Commit Message

Jan Beulich Aug. 6, 2020, 9:28 a.m. UTC 
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@suse.com>

Comments

Andrew Cooper Aug. 6, 2020, 2:04 p.m. UTC | #1 
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@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@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
>
Jan Beulich Aug. 7, 2020, 9:34 a.m. UTC | #2 
On 06.08.2020 16:04, Andrew Cooper wrote:
> 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@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> although with some
> further suggestions.

Thanks.

>> @@ -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?

To be honest, I'd rather add a check than drop the printing.
However unlikely it may be to encounter an MSB ELF binary ...
This particular one I'd like to do in a separate, follow-on
patch though.

I've addressed the other comments in what I intend to commit.

Jan
diff mbox series

Patch

--- 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" : "");
     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;
 
-    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));
+            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 ( 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