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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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:
>>>>>>
>>>>>>> At the moment, SSBD workaround is re-enabled for Xen after interrupts
>>>>>>> are unmasked. This means we may end up to execute some part of the
>>>>>>> hypervisor if an interrupt is received before the workaround is
>>>>>>> re-enabled.
>>>>>>>
>>>>>>> As the rest of enter_hypervisor_from_guest() does not require to have
>>>>>>> interrupts masked, the function is now split in two parts:
>>>>>>>        1) enter_hypervisor_from_guest_noirq() called with interrupts
>>>>>>>           masked.
>>>>>> I'm okay with this approach, but I don't like name for
>>>>>> enter_hypervisor_from_guest_noirq(). Right now it is doing exactly one
>>>>>> thing - mitigates SSBD. So, maybe more appropriate name will be
>>>>>> something like "mitigate_ssbd()" ?
>>>>>
>>>>> If I wanted to call it mitigate_ssbd() I would have implemented
>>>>> completely differently. The reason it is like that is because we may
>>>>> need more code to be added here in the future (I have Andrii's series
>>>>> in mind). So I would rather avoid a further renaming later on and some
>>>>> rework.
>>>> Fair enough
>>>>
>>>>>
>>>>> Regarding the name, this is a split of
>>>>> enter_hypervisor_from_guest(). Hence, why the first path is the
>>>>> same. The noirq merely help the user to know what to expect. This is
>>>>> better of yet an __ version. Feel free to suggest a better suffix.
>>>> I'm bad at naming things :)
>>>
>>> Me too ;).
>>>
>>>>
>>>> I understand that is two halves of one function. But func_name_noirq()
>>>> pattern is widely used for other case: when we have func_name_noirq()
>>>> function and some func_name() that disables interrupts like this:
>>>>
>>>> void func_name()
>>>> {
>>>>           disable_irqs();
>>>>           func_name_noirq();
>>>>           enable_irqs();
>>>> }
>>>>
>>>> I like principle of least surprise, so it is better to use some other
>>>> naming pattern there.
>>>
>>> I can't find any function suffixed with _noirq in Xen. So I don't
>>> think this would be a major issue here.
>> Yes, there are no such functions in Xen. But it may confuse developers
>> who come from another projects.
>
> Well, each projects have their own style. So there are always some
> adaptations needed to move to a new project. What matters is the
> documentation clarifies what is the exact use. But...
>
>>
>>>>
>>>> maybe something like enter_hypervisor_from_guest_pt1() and
>>>> enter_hypervisor_from_guest_pt2()?
>>> Hmmm, it reminds me uni when we had to limit function size to 20 lines :).
>>>
>>> I chose _noirq because the other name I had in mind was quite
>>> verbose. I was thinking:
>>> enter_hypervisor_from_guest_before_interrupts().
>> A was thinking about something like this too.
>> What about enter_hypervisor_from_guest_preirq()?
>
> ... this would be indeed better.

>>
>> I think that "_pre" better shows the relation to
>> enter_hypervisor_from_guest()
>>
>>>
>>>>
>>>> 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



-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel