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

Julien Grall writes:

> On 27/09/2019 12:56, Volodymyr Babchuk wrote:
>>
>> Julien,
>
> Hi...
>
>>
>> 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 :)

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.

maybe something like enter_hypervisor_from_guest_pt1() and
enter_hypervisor_from_guest_pt2()?

Or maybe, we should not split the function at all? Instead, we enable
interrupts right in the middle of it.

>
>>
>>>      2) enter_hypervisor_from_guest() called with interrupts unmasked.
>>>
>>> Note that while enter_hypervisor_from_guest_noirq() does not use the
>>> on-stack context registers, it is still passed as parameter to match the
>>> rest of the C functions called from the entry path.
>> As I pointed in the previous email, enter_hypervisor_from_guest() does
>> not use on-stack registers as well.
>
> I am well aware of this, hence my comment here in the commit message
> ;). The reason it is like that is because I wanted to keep the
> prototype the same for all functions called from the entry path (this
> includes do_trap_*).
Let's continue those discussion in the other thread.
[...]

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