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

[Wei Liu <wl@xxxxxxx>]

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.

> 
> > @@ -5763,6 +5765,13 @@ void __set_fixmap(
> >      map_pages_to_xen(__fix_to_virt(idx), _mfn(mfn), 1, flags);
> >  }
> >  
> > +void __set_fixmap_x(
> > +    enum fixed_addresses_x idx, unsigned long mfn, unsigned long flags)
> > +{
> > +    BUG_ON(idx >= __end_of_fixed_addresses_x);
> 
> Also check for FIX_X_RESERVED?

Ack. In that case the same should be done for __set_fixmap.

> 
> > --- 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.

> > @@ -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.

Wei.

> 
> Jan

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