Re: [Xen-devel] [PATCH v5 08/12] x86/hyperv: provide Hyper-V hypercall functions

[Wei Liu <wl@xxxxxxx>]

On Thu, Jan 30, 2020 at 01:32:26PM +0100, Roger Pau Monné wrote:
> On Thu, Jan 30, 2020 at 12:28:36PM +0000, Wei Liu wrote:
> > On Thu, Jan 30, 2020 at 01:08:07PM +0100, Roger Pau Monné wrote:
> > > 
> > > > +}
> > > > +
> > > >  /*
> > > >   * Local variables:
> > > >   * mode: C
> > > > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > > > index 97f9c07891..8e02b4c648 100644
> > > > --- a/xen/arch/x86/xen.lds.S
> > > > +++ b/xen/arch/x86/xen.lds.S
> > > > @@ -329,6 +329,10 @@ SECTIONS
> > > >    efi = .;
> > > >  #endif
> > > >  
> > > > +#ifdef CONFIG_HYPERV_GUEST
> > > > +  hv_hcall_page = ABSOLUTE(__fix_x_to_virt(1));
> > > 
> > > I assume there's no way to use FIX_X_HYPERV_HCALL because it's an
> > > enum?
> > > 
> > 
> > Yes.
> > 
> > And the trick to generate a symbol didn't work either.
> 
> And you must define that symbol in the linker script? It doesn't seem
> to be used at link time.
> 

I don't follow. I wish I could define and use a symbol in the linker
script but couldn't.

As for defining a symbol, see the patch that introduces the executable
fixmap facility, in function __set_fixmap_x.

> > > > +#endif
> > > > +
> > > >    /* Sections to be discarded */
> > > >    /DISCARD/ : {
> > > >         *(.exit.text)
> > > > diff --git a/xen/include/asm-x86/fixmap.h b/xen/include/asm-x86/fixmap.h
> > > > index 8094546b75..a9bcb068cb 100644
> > > > --- a/xen/include/asm-x86/fixmap.h
> > > > +++ b/xen/include/asm-x86/fixmap.h
> > > > @@ -16,6 +16,7 @@
> > > >  
> > > >  #define FIXADDR_TOP (VMAP_VIRT_END - PAGE_SIZE)
> > > >  #define FIXADDR_X_TOP (XEN_VIRT_END - PAGE_SIZE)
> > > > +#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
> > > >  
> > > >  #ifndef __ASSEMBLY__
> > > >  
> > > > @@ -110,8 +111,6 @@ extern void __set_fixmap_x(
> > > >  
> > > >  #define clear_fixmap_x(idx) __set_fixmap_x(idx, 0, 0)
> > > >  
> > > > -#define __fix_x_to_virt(x) (FIXADDR_X_TOP - ((x) << PAGE_SHIFT))
> > > > -
> > > >  #define fix_x_to_virt(x)   ((void *)__fix_x_to_virt(x))
> > > 
> > > This seems like some unrelated code movement?
> > > 
> > 
> > It is required. This section is not supposed to be used in linker
> > script. I have to move that macro ahead.
> 
> Oh, but you introduce that macro in patch #5, can you place it at the
> right position when introduced?

It wasn't needed in the linker script until now. I don't mind doing it
that way, but sometimes I'm told to now introduce something until it is
used. I wish we could be more consistent on this sort of things.

And frankly this sort of change adds no particular value in this series
whatsoever.

Wei.

> 
> Thanks, Roger.

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