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

[Jan Beulich <jbeulich@xxxxxxxx>]

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