Re: [Xen-devel] [PATCH v4 1/7] x86: provide executable fixmap facility

[Jan Beulich <jbeulich@suse.com>]

On 28.01.2020 16:15, Wei Liu wrote:
> On Thu, Jan 23, 2020 at 12:04:00PM +0100, Jan Beulich wrote:
>> On 22.01.2020 21:23, Wei Liu wrote:
>>> This allows us to set aside some address space for executable mapping.
>>> This fixed map range starts from XEN_VIRT_END so that it is within reach
>>> of the .text section.
>>>
>>> Shift the percpu stub range and livepatch range accordingly.
>>
>> Hmm, the livepatch range gets shrunk, not shifted, but yes. Is there
>> a particular reason why you move the stubs area down? It looks as if
>> the patch would be smaller overall if you didn't. (Possibly down
>> the road the stubs area could be made part of the FIXADDR_X range
>> anyway.)
> 
> I think having a well-known fixed address is more useful for debugging.
> 
> Going the other way around would mean the hypercall page location
> becomes dependent on the number of CPUs configured.

Depending on how future insertions are done into
enum fixed_addresses_x, the address also won't be "well-known fixed".

>>> --- a/xen/include/asm-x86/fixmap.h
>>> +++ b/xen/include/asm-x86/fixmap.h
>>> @@ -15,6 +15,9 @@
>>>  #include <asm/page.h>
>>>  
>>>  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
>>> +#define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
>>> +/* This constant is derived from enum fixed_addresses_x below */
>>> +#define MAX_FIXADDR_X_SIZE (2 << PAGE_SHIFT)
>>
>> If this can't be properly derived, then a BUILD_BUG_ON() is needed.
>> But didn't we discuss on irc already possible approaches of how to
>> derive it from the enum? Did none of this work?
> 
> The only option I remember discussing was to define macros instead of
> using enum. I said at the time at would make us lose the ability to
> dynamically size this area.
> 
> If there are other ways that I missed, let me know.

I seem to recall recommending to export absolute symbols from
assembly code. The question is how easily usable they would
be from C, or how clumsy the resulting code would look.

>>> @@ -89,6 +92,31 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr)
>>>      return __virt_to_fix(vaddr);
>>>  }
>>>  
>>> +enum fixed_addresses_x {
>>> +    /* Index 0 is reserved since fix_x_to_virt(0) == FIXADDR_X_TOP. */
>>> +    FIX_X_RESERVED,
>>> +#ifdef CONFIG_HYPERV_GUEST
>>> +    FIX_X_HYPERV_HCALL,
>>> +#endif
>>> +    __end_of_fixed_addresses_x
>>> +};
>>> +
>>> +#define FIXADDR_X_SIZE  (__end_of_fixed_addresses_x << PAGE_SHIFT)
>>> +#define FIXADDR_X_START (FIXADDR_X_TOP - FIXADDR_X_SIZE)
>>> +
>>> +extern void __set_fixmap_x(
>>> +    enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags);
>>> +
>>> +#define set_fixmap_x(idx, phys) \
>>> +    __set_fixmap_x(idx, (phys)>>PAGE_SHIFT, PAGE_HYPERVISOR_RX | MAP_SMALL_PAGES)
>>
>> Can't __set_fixmap() be used here, making its implementation derive
>> which one is mean from whether _PAGE_NX is set in the passed in flags?
> 
> __set_fixmap and __set_fixmap_x take different enum types for their
> first argument. I would prefer type safety and explicitness here.

Well, okay then. Duplication like this simply makes me a little
nervous, and even more so when it extends our set of name space
violations.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel