[PATCH] x86/pv: Drop HYPERVISOR_COMPAT_VIRT_START()

[PATCH] x86/pv: Drop HYPERVISOR_COMPAT_VIRT_START()

[Andrew Cooper]

This is pure obfuscation (in particular, hiding the two locations where the
variable gets set), and is longer than the code it replaces.

Also fix MACH2PHYS_COMPAT_VIRT_START to be expressed as a 1-parameter macro,
which is how it is used.  The current construct only works because
HYPERVISOR_COMPAT_VIRT_START was also a 1-parameter macro.

No functional change, but does create easier-to-follow logic.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/domain.c                | 2 +-
 xen/arch/x86/pv/descriptor-tables.c  | 2 +-
 xen/arch/x86/pv/dom0_build.c         | 4 ++--
 xen/arch/x86/x86_64/mm.c             | 4 ++--
 xen/common/compat/kernel.c           | 2 +-
 xen/include/asm-x86/config.h         | 9 ++-------
 xen/include/asm-x86/x86_64/uaccess.h | 2 +-
 7 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 4dc27f798e..5d8b864718 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -791,7 +791,7 @@ int arch_domain_create(struct domain *d,
     d->arch.emulation_flags = emflags;
 
 #ifdef CONFIG_PV32
-    HYPERVISOR_COMPAT_VIRT_START(d) =
+    d->arch.hv_compat_vstart =
         is_pv_domain(d) ? __HYPERVISOR_COMPAT_VIRT_START : ~0u;
 #endif
 
diff --git a/xen/arch/x86/pv/descriptor-tables.c b/xen/arch/x86/pv/descriptor-tables.c
index 5e84704400..0d22759820 100644
--- a/xen/arch/x86/pv/descriptor-tables.c
+++ b/xen/arch/x86/pv/descriptor-tables.c
@@ -235,7 +235,7 @@ static bool check_descriptor(const struct domain *dom, seg_desc_t *d)
             if ( b & _SEGMENT_G )
                 limit <<= 12;
 
-            if ( (base == 0) && (limit > HYPERVISOR_COMPAT_VIRT_START(dom)) )
+            if ( (base == 0) && (limit > dom->arch.hv_compat_vstart) )
             {
                 a |= 0x0000ffff;
                 b |= 0x000f0000;
diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index e0801a9e6d..5e70422678 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -432,7 +432,7 @@ int __init dom0_construct_pv(struct domain *d,
                 printk("Dom0 expects too high a hypervisor start address\n");
                 return -ERANGE;
             }
-            HYPERVISOR_COMPAT_VIRT_START(d) =
+            d->arch.hv_compat_vstart =
                 max_t(unsigned int, m2p_compat_vstart, value);
         }
 
@@ -603,7 +603,7 @@ int __init dom0_construct_pv(struct domain *d,
 
     /* Overlap with Xen protected area? */
     if ( compat
-         ? v_end > HYPERVISOR_COMPAT_VIRT_START(d)
+         ? v_end > d->arch.hv_compat_vstart
          : (v_start < HYPERVISOR_VIRT_END) && (v_end > HYPERVISOR_VIRT_START) )
     {
         printk("DOM0 image overlaps with Xen private area.\n");
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index c41ce847b3..a51c2b52d9 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -1029,7 +1029,7 @@ int pagefault_by_memadd(unsigned long addr, struct cpu_user_regs *regs)
     struct domain *d = current->domain;
 
     return mem_hotplug && guest_mode(regs) && is_pv_32bit_domain(d) &&
-           (addr >= HYPERVISOR_COMPAT_VIRT_START(d)) &&
+           (addr >= d->arch.hv_compat_vstart) &&
            (addr < MACH2PHYS_COMPAT_VIRT_END);
 }
 
@@ -1048,7 +1048,7 @@ int handle_memadd_fault(unsigned long addr, struct cpu_user_regs *regs)
     if (!is_pv_32bit_domain(d))
         return 0;
 
-    if ( (addr < HYPERVISOR_COMPAT_VIRT_START(d)) ||
+    if ( (addr < d->arch.hv_compat_vstart) ||
          (addr >= MACH2PHYS_COMPAT_VIRT_END) )
         return 0;
 
diff --git a/xen/common/compat/kernel.c b/xen/common/compat/kernel.c
index 804b919bdc..57845a6c07 100644
--- a/xen/common/compat/kernel.c
+++ b/xen/common/compat/kernel.c
@@ -27,7 +27,7 @@ CHECK_TYPE(capabilities_info);
 #define xen_platform_parameters compat_platform_parameters
 #define xen_platform_parameters_t compat_platform_parameters_t
 #undef HYPERVISOR_VIRT_START
-#define HYPERVISOR_VIRT_START HYPERVISOR_COMPAT_VIRT_START(current->domain)
+#define HYPERVISOR_VIRT_START current->domain->arch.hv_compat_vstart
 
 #define xen_changeset_info compat_changeset_info
 #define xen_changeset_info_t compat_changeset_info_t
diff --git a/xen/include/asm-x86/config.h b/xen/include/asm-x86/config.h
index 883c2ef0df..130db90b5c 100644
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128];
 
 /* This is not a fixed value, just a lower limit. */
 #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000
-#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart)
-
-#else /* !CONFIG_PV32 */
-
-#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0)
 
 #endif /* CONFIG_PV32 */
 
-#define MACH2PHYS_COMPAT_VIRT_START    HYPERVISOR_COMPAT_VIRT_START
+#define MACH2PHYS_COMPAT_VIRT_START(d) (d)->arch.hv_compat_vstart
 #define MACH2PHYS_COMPAT_VIRT_END      0xFFE00000
 #define MACH2PHYS_COMPAT_NR_ENTRIES(d) \
     ((MACH2PHYS_COMPAT_VIRT_END-MACH2PHYS_COMPAT_VIRT_START(d))>>2)
 
 #define COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(d) \
-    l2_table_offset(HYPERVISOR_COMPAT_VIRT_START(d))
+    l2_table_offset((d)->arch.hv_compat_vstart)
 #define COMPAT_L2_PAGETABLE_LAST_XEN_SLOT  l2_table_offset(~0U)
 #define COMPAT_L2_PAGETABLE_XEN_SLOTS(d) \
     (COMPAT_L2_PAGETABLE_LAST_XEN_SLOT - COMPAT_L2_PAGETABLE_FIRST_XEN_SLOT(d) + 1)
diff --git a/xen/include/asm-x86/x86_64/uaccess.h b/xen/include/asm-x86/x86_64/uaccess.h
index ba79f950fb..8c7df060d5 100644
--- a/xen/include/asm-x86/x86_64/uaccess.h
+++ b/xen/include/asm-x86/x86_64/uaccess.h
@@ -55,7 +55,7 @@ extern void *xlat_malloc(unsigned long *xlat_page_current, size_t size);
      access_ok(addr, (count) * (size)))
 
 #define __compat_addr_ok(d, addr) \
-    ((unsigned long)(addr) < HYPERVISOR_COMPAT_VIRT_START(d))
+    ((unsigned long)(addr) < (d)->arch.hv_compat_vstart)
 
 #define __compat_access_ok(d, addr, size) \
     __compat_addr_ok(d, (unsigned long)(addr) + ((size) ? (size) - 1 : 0))
-- 
2.11.0

Re: [PATCH] x86/pv: Drop HYPERVISOR_COMPAT_VIRT_START()

[Jan Beulich]

On 27.04.2021 15:02, Andrew Cooper wrote:
> This is pure obfuscation (in particular, hiding the two locations where the
> variable gets set), and is longer than the code it replaces.

Obfuscation - not just; see below.

> Also fix MACH2PHYS_COMPAT_VIRT_START to be expressed as a 1-parameter macro,
> which is how it is used.  The current construct only works because
> HYPERVISOR_COMPAT_VIRT_START was also a 1-parameter macro.

I don't mind this getting changed, but I don't think there's any
"fixing" involved here. Omitting macro parameters from macros
forwarding to other macros (or functions) is well defined and imo
not a problem at all. In fact, if at the end of all expansions a
macro evaluates to a function, it may be necessary to do so in case
covering not just function invocations is intended, but also taking
of its address.

> --- a/xen/arch/x86/pv/descriptor-tables.c
> +++ b/xen/arch/x86/pv/descriptor-tables.c
> @@ -235,7 +235,7 @@ static bool check_descriptor(const struct domain *dom, seg_desc_t *d)
>              if ( b & _SEGMENT_G )
>                  limit <<= 12;
>  
> -            if ( (base == 0) && (limit > HYPERVISOR_COMPAT_VIRT_START(dom)) )
> +            if ( (base == 0) && (limit > dom->arch.hv_compat_vstart) )

I expect this (and a few more instances) to fail to build when !PV32.
It was the purpose of ...

> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128];
>  
>  /* This is not a fixed value, just a lower limit. */
>  #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000
> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart)
> -
> -#else /* !CONFIG_PV32 */
> -
> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0)

... this to allow things to build in the absence of the actual struct
member.

Jan

Re: [PATCH] x86/pv: Drop HYPERVISOR_COMPAT_VIRT_START()

[Andrew Cooper]

On 27/04/2021 14:18, Jan Beulich wrote:
> On 27.04.2021 15:02, Andrew Cooper wrote:
>> This is pure obfuscation (in particular, hiding the two locations where the
>> variable gets set), and is longer than the code it replaces.
> Obfuscation - not just; see below.
>
>> Also fix MACH2PHYS_COMPAT_VIRT_START to be expressed as a 1-parameter macro,
>> which is how it is used.  The current construct only works because
>> HYPERVISOR_COMPAT_VIRT_START was also a 1-parameter macro.
> I don't mind this getting changed, but I don't think there's any
> "fixing" involved here. Omitting macro parameters from macros
> forwarding to other macros (or functions) is well defined and imo
> not a problem at all. In fact, if at the end of all expansions a
> macro evaluates to a function, it may be necessary to do so in case
> covering not just function invocations is intended, but also taking
> of its address.

It might be well formed, but it doesn't help at all with trying to
follow what the code does.

>
>> --- a/xen/arch/x86/pv/descriptor-tables.c
>> +++ b/xen/arch/x86/pv/descriptor-tables.c
>> @@ -235,7 +235,7 @@ static bool check_descriptor(const struct domain *dom, seg_desc_t *d)
>>              if ( b & _SEGMENT_G )
>>                  limit <<= 12;
>>  
>> -            if ( (base == 0) && (limit > HYPERVISOR_COMPAT_VIRT_START(dom)) )
>> +            if ( (base == 0) && (limit > dom->arch.hv_compat_vstart) )
> I expect this (and a few more instances) to fail to build when !PV32.
> It was the purpose of ...
>
>> --- a/xen/include/asm-x86/config.h
>> +++ b/xen/include/asm-x86/config.h
>> @@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128];
>>  
>>  /* This is not a fixed value, just a lower limit. */
>>  #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000
>> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart)
>> -
>> -#else /* !CONFIG_PV32 */
>> -
>> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0)
> ... this to allow things to build in the absence of the actual struct
> member.

Hmm - I really should have got this change in earlier, shouldn't I...

For this example you've pointed out, feeding 0 into the limit
calculation is not a correct thing to do in the slightest.

~Andrew

Re: [PATCH] x86/pv: Drop HYPERVISOR_COMPAT_VIRT_START()

[Jan Beulich]

On 27.04.2021 20:05, Andrew Cooper wrote:
> On 27/04/2021 14:18, Jan Beulich wrote:
>> On 27.04.2021 15:02, Andrew Cooper wrote:
>>> --- a/xen/include/asm-x86/config.h
>>> +++ b/xen/include/asm-x86/config.h
>>> @@ -254,21 +254,16 @@ extern unsigned char boot_edid_info[128];
>>>  
>>>  /* This is not a fixed value, just a lower limit. */
>>>  #define __HYPERVISOR_COMPAT_VIRT_START 0xF5800000
>>> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((d)->arch.hv_compat_vstart)
>>> -
>>> -#else /* !CONFIG_PV32 */
>>> -
>>> -#define HYPERVISOR_COMPAT_VIRT_START(d) ((void)(d), 0)
>> ... this to allow things to build in the absence of the actual struct
>> member.
> 
> Hmm - I really should have got this change in earlier, shouldn't I...
> 
> For this example you've pointed out, feeding 0 into the limit
> calculation is not a correct thing to do in the slightest.

Does it actually get fed into any such calculation? From looking
around yesterday as well as from when I made that change over
half a year ago I seem to recall that all potentially problematic
uses are already suitably guarded.

Jan