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@xxxxxxxx>]

Hi,

Julien Grall writes:

> Hi,
>
> 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.

>>
>> 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()?

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

Or switch() instead of multiple ifs. Maybe in some helper function.

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