Re: [Xen-devel] [PATCH v3 3/7] x86: make PV hypercall entry points work with !CONFIG_PV

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Nov 08, 2018 at 09:22:08AM -0700, Jan Beulich wrote:
> >>> On 08.11.18 at 17:09, <wei.liu2@xxxxxxxxxx> wrote:
> > On Thu, Nov 08, 2018 at 08:49:44AM -0700, Jan Beulich wrote:
> >> >>> On 08.11.18 at 16:36, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> > On 08/11/18 15:33, Wei Liu wrote:
> >> >> On Mon, Nov 05, 2018 at 09:11:44AM -0700, Jan Beulich wrote:
> >> >>>>>> On 05.11.18 at 16:49, <andrew.cooper3@xxxxxxxxxx> wrote:
> >> >>>> On 05/11/18 15:48, Wei Liu wrote:
> >> >>>>> On Mon, Nov 05, 2018 at 08:04:37AM -0700, Jan Beulich wrote:
> >> >>>>>>>>> On 02.11.18 at 16:55, <wei.liu2@xxxxxxxxxx> wrote:
> >> >>>>>>> --- a/xen/arch/x86/x86_64/traps.c
> >> >>>>>>> +++ b/xen/arch/x86/x86_64/traps.c
> >> >>>>>>> @@ -298,8 +298,21 @@ static unsigned int write_stub_trampoline(
> >> >>>>>>>  }
> >> >>>>>>>  
> >> >>>>>>>  DEFINE_PER_CPU(struct stubs, stubs);
> >> >>>>>>> +
> >> >>>>>>> +#ifdef CONFIG_PV
> >> >>>>>>>  void lstar_enter(void);
> >> >>>>>>>  void cstar_enter(void);
> >> >>>>>>> +#else
> >> >>>>>>> +static inline void lstar_enter(void)
> >> >>>>>>> +{
> >> >>>>>>> +    panic("%s called", __func__);
> >> >>>>>>> +}
> >> >>>>>>> +
> >> >>>>>>> +static inline void cstar_enter(void)
> >> >>>>>>> +{
> >> >>>>>>> +    panic("%s called", __func__);
> >> >>>>>>> +}
> >> >>>>>>> +#endif /* CONFIG_PV */
> >> >>>>>> Do we really need two separate stubs (and two separate string 
> >> >>>>>> literals)
> >> >>>>>> here?
> >> >>>>> I think it is clearer if we have two distinct messages. But I'm not 
> >> >>>>> too
> >> >>>>> fussed either way really. If you feel strongly about this, I'm happy 
> >> >>>>> to
> >> >>>>> change it to only one function.
> >> >>>> This is the correct way to do it.  __func__ will already be in the
> >> >>>> string table, and the format string (being identical) will be merged.
> >> >>> Why would __func__ be in the string table already, for functions
> >> >>> containing no other references to it?
> >> >> What is the way forward? Do we really care if there is one more string
> >> >> literal in the binary?
> >> > 
> >> > No.  One extra string like this is not something which needs caring
> >> > about in the slightest.
> >> 
> >> So just to clarify - the unnecessary redundancy in the source is of more
> >> relevance to me than the resulting slightly larger binary.
> > 
> > So you would rather have
> > 
> >     static inline void lcstar_enter(void)
> >     {
> >         panic("lstar/cstar\n");
> >     }
> > 
> > ?
> 
> Yes. Let's not forget that this function is never to be invoked
> anyway, so the less dead code and data, the better. (As a
> side note, I don't understand why they're inline when you need
> a pointer to them anyway. Instead they should go into
> .text.cold or whatever it is.)

OK. I will submit a followup patch for that.

Wei.

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