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

From: Julien Grall <julien.grall@arm.com>
To: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Andrii Anisov <Andrii_Anisov@epam.com>,
	"andrii.anisov@gmail.com" <andrii.anisov@gmail.com>
Subject: Re: [Xen-devel] [PATCH RFC for-4.13 04/10] xen/arm: Ensure the SSBD workaround is re-enabled right after exiting a guest
Date: Fri, 27 Sep 2019 14:16:14 +0100	[thread overview]
Message-ID: <df6b891c-2670-47d9-ae0d-223161edc225@arm.com> (raw)
In-Reply-To: <87r2419am7.fsf@epam.com>

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.

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

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

So the split solution was the best I had in mind. I am open to better suggestion 
here.

Cheers,

-- 
Julien Grall

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