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

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?
> Or switch() instead of multiple ifs. Maybe in some helper function.
Well, my point about "ifs" is that you add a few branch instruction for 
something that can mostly be static (we will always unmask the same interrupts 
for a given exception).
Anyway, such solutions is a no-go for me. This is only muddying the code and I 
care about long-term maintenance.
Cheers,

--
Julien Grall

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