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
