Re: [Xen-devel] [PATCH 07/17] x86/traps: put PV code handlers under CONFIG_PV

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Fri, Oct 12, 2018 at 08:08:19AM -0600, Jan Beulich wrote:
> >>> On 04.10.18 at 17:43, <wei.liu2@xxxxxxxxxx> wrote:
> > The trap handlers in hypervisor need to forward events to PV guests if
> > necessary. If there is no PV guest relevant code should be excluded.
> > 
> > Put code under CONFIG_PV and add ASSERT_UNREACHABLE.
> 
> -ETOOMANYIFDEFS
> 
> What's wrong with an inline stub for pv_inject_event()? This won't
> eliminate all #ifdef-s, but quite a few of them afaics.

Nothing wrong with that. I have provided a stub in my next version.

> 
> > @@ -1140,6 +1152,7 @@ static int handle_ldt_mapping_fault(unsigned int 
> > offset,
> >          if ( !guest_mode(regs) )
> >              return 0;
> >  
> > +#ifdef CONFIG_PV
> >          /* Access would have become non-canonical? Pass #GP[sel] back. */
> >          if ( unlikely(!is_canonical_address(curr->arch.pv.ldt_base + 
> > offset)) )
> >          {
> > @@ -1151,6 +1164,9 @@ static int handle_ldt_mapping_fault(unsigned int 
> > offset,
> >              /* else pass the #PF back, with adjusted %cr2. */
> >              pv_inject_page_fault(regs->error_code,
> >                                   curr->arch.pv.ldt_base + offset);
> > +#else
> > +        ASSERT_UNREACHABLE();
> > +#endif
> 
> I think here it should be the call site to gain #ifdef (around the
> entire if()), and the function as a whole should then be put in
> another #ifdef, or be moved to pv/traps.c.
> 

Done. Relevant code and call site are now put under CONFIG_PV. The
movement will be done later.

> > @@ -1494,7 +1514,9 @@ void __init do_early_page_fault(struct cpu_user_regs 
> > *regs)
> >  
> >  void do_general_protection(struct cpu_user_regs *regs)
> >  {
> > +#ifdef CONFIG_PV
> >      struct vcpu *v = current;
> > +#endif
> 
> I think this would better be resolved by moving the declaration into
> the block which has multiple references, and use plain "current" in
> the subsequent "else if()". But it's a matter of taste, I agree.

To me calling current more than necessary is bad because the resulting
machine code can end up calculating the address of cpu_info multiple
times.

Wei.

> 
> Jan
> 
> 

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