Re: [Xen-devel] [PATCH 3/9] x86: enclose hypercall page initialisation code in CONFIG_{HVM, PV}

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Aug 07, 2018 at 10:18:36AM -0600, Jan Beulich wrote:
> >>> On 07.08.18 at 18:08, <wei.liu2@xxxxxxxxxx> wrote:
> > On Tue, Aug 07, 2018 at 04:44:13AM -0600, Jan Beulich wrote:
> >> >>> On 07.08.18 at 12:00, <wei.liu2@xxxxxxxxxx> wrote:
> >> > --- a/xen/arch/x86/x86_64/traps.c
> >> > +++ b/xen/arch/x86/x86_64/traps.c
> >> > @@ -352,12 +352,19 @@ void subarch_percpu_traps_init(void)
> >> >  void hypercall_page_initialise(struct domain *d, void *hypercall_page)
> >> >  {
> >> >      memset(hypercall_page, 0xCC, PAGE_SIZE);
> >> > +#if CONFIG_HVM
> >> >      if ( is_hvm_domain(d) )
> >> >          hvm_hypercall_page_initialise(d, hypercall_page);
> >> > -    else if ( !is_pv_32bit_domain(d) )
> >> > -        hypercall_page_initialise_ring3_kernel(hypercall_page);
> >> > -    else
> >> > -        hypercall_page_initialise_ring1_kernel(hypercall_page);
> >> > +#endif
> >> > +#if CONFIG_PV
> >> > +    if ( is_pv_domain(d) )
> >> > +    {
> >> > +        if ( !is_pv_32bit_domain(d) )
> >> > +            hypercall_page_initialise_ring3_kernel(hypercall_page);
> >> > +        else
> >> > +            hypercall_page_initialise_ring1_kernel(hypercall_page);
> >> > +    }
> >> > +#endif
> >> >  }
> >> 
> >> I'm not convinced: This is uglier to read, and things like is_hvm_domain()
> >> should evaluate to constant false anyway without the respective
> >> CONFIG_* setting enabled.
> > 
> > I take it you are against adding CONFIG_* here?
> > 
> > The addition of is_pv_domain branch is still needed because both sets
> > might get compiled out.
> 
> But that can be done by making the sequence
> 
>       if ( is_hvm_... )
>       else if ( is_pv_32bit_... )
>       else if ( is_pv_... )
> 
> I.e. in particular without any changes to the nesting.

Yeah, sure. I just wanted to make sure PV is explicitly tested. The
above structure is fine by me.

Wei.

> 
> Jan
> 
> 

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