Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi,

On 27/09/2019 18:58, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi,
>>
>> On 27/09/2019 15:21, Volodymyr Babchuk wrote:
>>>
>>> Julien Grall writes:
>>>
>>>> On 27/09/2019 14:33, Volodymyr Babchuk wrote:
>>>>> Julien Grall writes:
>>>>>> On 27/09/2019 13:39, Volodymyr Babchuk wrote:
>>>>>>> Julien Grall writes:
>>>>>>>> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>>>>>>>> Julien Grall writes:
>>>>>>> Or maybe, we should not split the function at all? Instead, we enable
>>>>>>> interrupts right in the middle of it.
>>>>>>
>>>>>> I thought about this but I didn't much like the resulting code.
>>>>>>
>>>>>> The instruction to unmask interrupts requires to take an immediate
>>>>>> (indicates which interrupts to unmask). As not all the traps require
>>>>>> to unmask the same interrupts, we would end up to have to a bunch of
>>>>>> if in the code to select the right unmasking.
>>>>> Ah, yes, this is the problem. We can provide callback to
>>>>> enter_hypervisor_from_guest().
>>>>
>>>> I am not sure what you mean by this. Do you mean a callback that will
>>>> unmask the interrupts?
>>> Yes. You can pass function pointer to enter_hypervisor_from_guest(). To
>>> a function, that will unmask the interrupts. I'm sure that guest_vector
>>> macro can generate it for you. Something like this:
>>>
>>>           .macro  guest_vector compat, iflags, trap, save_x0_x1=1
>>>           entry   hyp=0, compat=\compat, save_x0_x1=\save_x0_x1
>>>           /*
>>>            * The vSError will be checked while 
>>> SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
>>>            * is not set. If a vSError took place, the initial exception 
>>> will be
>>>            * skipped. Exit ASAP
>>>            */
>>>           ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>>>                       "nop; nop",
>>>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>>           ldr     x0, =1f
>>>           bl      enter_hypervisor_from_guest
>>>           mov     x0, sp
>>>           bl      do_trap_\trap
>>>           b       1f
>>> 2:
>>>           msr     daifclr, \iflags
>>>           ret
>>> 1:
>>>           exit    hyp=0, compat=\compat
>>>           .endm
>>
>> TBH, I don't see what's the point you are trying to make here. Yes,
>> there are many way to write a code and possibility to make one
>> function. You could also create a skeleton macro for
>> enter_hypervisor_from_guest and generate N of them (one per set of
>> unmask interrupts) and call them.
>>
>> But are they really worth it?
> The point is that you are trying to split one entity into two.
> As I see it: semantically we have one function:
> enter_hypervisor_from_guest(). The purpose of this function is clear:
> finish transition from guest mode to hypervisor mode.
> 
> But because of some architectural limitation (daifclr requires only
> immediate argument) you are forced to divide this function in two.
> I don't like this, because entry path now is more complex. To follow
> what is going you need to bounce between head.S and traps.c one extra time.

Ok. If I understand correctly, this is mostly a matter of taste. Right?

I am going to ignore the "matter of taste" and just focus on the code 
itself. While I quite like the idea of a single function, I have two 
concerns with this:
    1) Because this is a callback, you will use an indirect branch. The 
address used is loaded from the literal pool (ldr x0, =...), therefore 
your branch will depend on a load. Such construction may stall the 
pipeline for a long time as most likely you will have to fetch the 
address from memory and not the cache (the cache is likely to be 
populated with mostly guest stuff). Depending on the core, this may have 
quite an impact. I am aware that we have been using in quite a few 
places such pattern within Xen but we are trying to get away. For 
instance, on x86 they recently introduced a way to converting indirect 
branch to direct branch if the address is fixed after boot (see the 
alternative_call macro).

    2) With the split functions, it is easier to spot in a diff if 
someone is trying to add code before the interrupts are unmasked. So I 
feel this is more maintainable as I have one less thing to worry when 
reviewing.

The second one is borderline matter of taste, so it is less important. 
But the first one is important to me.

So any solution should address this.

Cheers,

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