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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.01.2020 15:42, Wei Liu wrote:
> On Tue, Jan 28, 2020 at 04:38:42PM +0100, Jan Beulich wrote:
>> 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".
> 
> Going back to this, not moving stubs will make the change to
> alloc_stub_page become unnecessary (one line); on the other hand it
> makes FIX_X_ADDR_START become XEN_VIRT_END - NR_CPUS * PAGE_SIZE -
> PAGE_SIZE.
> 
> Are you really concerned about this? I can make the change if you really
> want that, but it is just work with no apparent benefit.

Hmm, indeed, it's just one line. Not sure why I thought there
would be more of an effect. Leave it as is, and sorry for the
noise.

>>>>> --- 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.
> 
> Even if I use absolute symbol I would still need to define a macro for
> it. There is no way around it, because enum can't be used in asm or
> linker script.

I'm afraid I don't understand. Why a macro? The absolute symbol would
be there to communicate the relevant (enum-derived) value to the
linker script. I.e. with

enum { e0, e1, e2 };

in some C file

asm ( ".equ GBL_e2, %c0; .global GBL_e2" :: "i" (e2) );

which I then hope would allow you to use GBL_e2 in the linker
script ASSERT().

> I want to keep using enum because that would allow us to size the area
> according to Kconfig.

Of course, I fully agree with this goal.

Jan

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