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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan



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