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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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