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
