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

Message ID 20190926183808.11630-5-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm: XSA-201 and XSA-263 fixes
Related show

Commit Message

Julien Grall Sept. 26, 2019, 6:38 p.m.
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.
    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.

Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests")
Reported-by: Andrii Anisov <andrii_anisov@epam.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>

---

Note the Arm32 code has not been changed yet. I am also open on turn
both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from()
to functions not taking any parameters.
---
 xen/arch/arm/arm64/entry.S |  2 ++
 xen/arch/arm/traps.c       | 16 +++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Volodymyr Babchuk Sept. 27, 2019, 11:56 a.m. | #1
Julien,

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

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

> Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests")
> Reported-by: Andrii Anisov <andrii_anisov@epam.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> Note the Arm32 code has not been changed yet. I am also open on turn
> both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from()
> to functions not taking any parameters.
That would be appropriate in my opinion.

> ---
>  xen/arch/arm/arm64/entry.S |  2 ++
>  xen/arch/arm/traps.c       | 16 +++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 9eafae516b..458d12f188 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -173,6 +173,8 @@
>          ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>                      "nop; nop",
>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        mov     x0, sp
> +        bl      enter_hypervisor_from_guest_noirq
>          msr     daifclr, \iflags
>          mov     x0, sp
>          bl      enter_hypervisor_from_guest
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 20ba34ec91..5848dd8399 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2007,16 +2007,26 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>  }
>
>  /*
> - * Actions that needs to be done after exiting the guest and before any
> - * request from it is handled.
> + * Actions that needs to be done after exiting the guest and before the
> + * interrupts are unmasked.
>   */
> -void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> +void enter_hypervisor_from_guest_noirq(struct cpu_user_regs *regs)
>  {
>      struct vcpu *v = current;
>
>      /* If the guest has disabled the workaround, bring it back on. */
>      if ( needs_ssbd_flip(v) )
>          arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
> +}
> +
> +/*
> + * Actions that needs to be done after exiting the guest and before any
> + * request from it is handled. Depending on the exception trap, this may
> + * be called with interrupts unmasked.
> + */
> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *v = current;
>
>      /*
>       * If we pended a virtual abort, preserve it until it gets cleared.


--
Volodymyr Babchuk at EPAM
Julien Grall Sept. 27, 2019, 12:22 p.m. | #2
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.

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.

> 
>>      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_*).

[...]

> 
>> ---
>>   xen/arch/arm/arm64/entry.S |  2 ++
>>   xen/arch/arm/traps.c       | 16 +++++++++++++---
>>   2 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index 9eafae516b..458d12f188 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -173,6 +173,8 @@
>>           ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>>                       "nop; nop",
>>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>> +        mov     x0, sp
>> +        bl      enter_hypervisor_from_guest_noirq
>>           msr     daifclr, \iflags
>>           mov     x0, sp
>>           bl      enter_hypervisor_from_guest
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 20ba34ec91..5848dd8399 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2007,16 +2007,26 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>>   }
>>
>>   /*
>> - * Actions that needs to be done after exiting the guest and before any
>> - * request from it is handled.
>> + * Actions that needs to be done after exiting the guest and before the
>> + * interrupts are unmasked.
>>    */
>> -void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>> +void enter_hypervisor_from_guest_noirq(struct cpu_user_regs *regs)
>>   {
>>       struct vcpu *v = current;
>>
>>       /* If the guest has disabled the workaround, bring it back on. */
>>       if ( needs_ssbd_flip(v) )
>>           arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
>> +}
>> +
>> +/*
>> + * Actions that needs to be done after exiting the guest and before any
>> + * request from it is handled. Depending on the exception trap, this may
>> + * be called with interrupts unmasked.
>> + */
>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>> +{
>> +    struct vcpu *v = current;
>>
>>       /*
>>        * If we pended a virtual abort, preserve it until it gets cleared.
> 
> 
> --
> Volodymyr Babchuk at EPAM
> 

Cheers,
Volodymyr Babchuk Sept. 27, 2019, 12:39 p.m. | #3
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
Julien Grall Sept. 27, 2019, 1:16 p.m. | #4
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,
Volodymyr Babchuk Sept. 27, 2019, 1:33 p.m. | #5
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
Julien Grall Sept. 27, 2019, 2:11 p.m. | #6
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,
Volodymyr Babchuk Sept. 27, 2019, 2:21 p.m. | #7
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
Julien Grall Sept. 27, 2019, 4:24 p.m. | #8
Hi,

On 27/09/2019 15:21, Volodymyr Babchuk wrote:
> 
> 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:
>>>>> 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

TBH, I don't see what's the point you are trying to make here. Yes, there are 
many way to write a code and possibility to make one function. You could also 
create a skeleton macro for enter_hypervisor_from_guest and generate N of them 
(one per set of unmask interrupts) and call them.

But are they really worth it?

Cheers,
Volodymyr Babchuk Sept. 27, 2019, 5:58 p.m. | #9
Julien Grall writes:

> Hi,
>
> On 27/09/2019 15:21, Volodymyr Babchuk wrote:
>>
>> 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:
>>>>>> 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
>
> TBH, I don't see what's the point you are trying to make here. Yes,
> there are many way to write a code and possibility to make one
> function. You could also create a skeleton macro for
> enter_hypervisor_from_guest and generate N of them (one per set of
> unmask interrupts) and call them.
>
> But are they really worth it?
The point is that you are trying to split one entity into two.
As I see it: semantically we have one function:
enter_hypervisor_from_guest(). The purpose of this function is clear:
finish transition from guest mode to hypervisor mode.

But because of some architectural limitation (daifclr requires only
immediate argument) you are forced to divide this function in two.
I don't like this, because entry path now is more complex. To follow
what is going you need to bounce between head.S and traps.c one extra time.

Anyways, this is only my opinion. I'm not forcing you to implement
enter_hypervisor_from_guest() in this way.

I'm okay with enter_hypervisor_from_guest_preirq() as well.
Julien Grall Sept. 27, 2019, 8:31 p.m. | #10
Hi,

On 27/09/2019 18:58, Volodymyr Babchuk wrote:
> 

> Julien Grall writes:

> 

>> Hi,

>>

>> On 27/09/2019 15:21, Volodymyr Babchuk wrote:

>>>

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

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

>>

>> TBH, I don't see what's the point you are trying to make here. Yes,

>> there are many way to write a code and possibility to make one

>> function. You could also create a skeleton macro for

>> enter_hypervisor_from_guest and generate N of them (one per set of

>> unmask interrupts) and call them.

>>

>> But are they really worth it?

> The point is that you are trying to split one entity into two.

> As I see it: semantically we have one function:

> enter_hypervisor_from_guest(). The purpose of this function is clear:

> finish transition from guest mode to hypervisor mode.

> 

> But because of some architectural limitation (daifclr requires only

> immediate argument) you are forced to divide this function in two.

> I don't like this, because entry path now is more complex. To follow

> what is going you need to bounce between head.S and traps.c one extra time.


Ok. If I understand correctly, this is mostly a matter of taste. Right?

I am going to ignore the "matter of taste" and just focus on the code 
itself. While I quite like the idea of a single function, I have two 
concerns with this:
    1) Because this is a callback, you will use an indirect branch. The 
address used is loaded from the literal pool (ldr x0, =...), therefore 
your branch will depend on a load. Such construction may stall the 
pipeline for a long time as most likely you will have to fetch the 
address from memory and not the cache (the cache is likely to be 
populated with mostly guest stuff). Depending on the core, this may have 
quite an impact. I am aware that we have been using in quite a few 
places such pattern within Xen but we are trying to get away. For 
instance, on x86 they recently introduced a way to converting indirect 
branch to direct branch if the address is fixed after boot (see the 
alternative_call macro).

    2) With the split functions, it is easier to spot in a diff if 
someone is trying to add code before the interrupts are unmasked. So I 
feel this is more maintainable as I have one less thing to worry when 
reviewing.

The second one is borderline matter of taste, so it is less important. 
But the first one is important to me.

So any solution should address this.

Cheers,

-- 
Julien Grall
Volodymyr Babchuk Sept. 30, 2019, 12:14 p.m. | #11
Hi Julien,

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.

To summarize our discussion in this mail thread: providing that you'll
rename enter_hypervisor_from_guest_noirq to
enter_hypervisor_from_guest_preirq():

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

>     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.
>
> Fixes: a7898e4c59 ("xen/arm: Add ARCH_WORKAROUND_2 support for guests")
> Reported-by: Andrii Anisov <andrii_anisov@epam.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> Note the Arm32 code has not been changed yet. I am also open on turn
> both enter_hypervisor_from_guest_noirq() and enter_hypervisor_from()
> to functions not taking any parameters.
> ---
>  xen/arch/arm/arm64/entry.S |  2 ++
>  xen/arch/arm/traps.c       | 16 +++++++++++++---
>  2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 9eafae516b..458d12f188 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -173,6 +173,8 @@
>          ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
>                      "nop; nop",
>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> +        mov     x0, sp
> +        bl      enter_hypervisor_from_guest_noirq
>          msr     daifclr, \iflags
>          mov     x0, sp
>          bl      enter_hypervisor_from_guest
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 20ba34ec91..5848dd8399 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2007,16 +2007,26 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>  }
>
>  /*
> - * Actions that needs to be done after exiting the guest and before any
> - * request from it is handled.
> + * Actions that needs to be done after exiting the guest and before the
> + * interrupts are unmasked.
>   */
> -void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> +void enter_hypervisor_from_guest_noirq(struct cpu_user_regs *regs)
>  {
>      struct vcpu *v = current;
>
>      /* If the guest has disabled the workaround, bring it back on. */
>      if ( needs_ssbd_flip(v) )
>          arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
> +}
> +
> +/*
> + * Actions that needs to be done after exiting the guest and before any
> + * request from it is handled. Depending on the exception trap, this may
> + * be called with interrupts unmasked.
> + */
> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> +{
> +    struct vcpu *v = current;
>
>      /*
>       * If we pended a virtual abort, preserve it until it gets cleared.


--
Volodymyr Babchuk at EPAM
Julien Grall Sept. 30, 2019, 12:15 p.m. | #12
On 30/09/2019 13:14, Volodymyr Babchuk wrote:
> 
> Hi Julien,
> 
> 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.
> 
> To summarize our discussion in this mail thread: providing that you'll
> rename enter_hypervisor_from_guest_noirq to
> enter_hypervisor_from_guest_preirq():
> 
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Thank you. I will try to summarize the discussion we had in the commit message. 
So we know the rationale of the split here.

Cheers,

Patch

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 9eafae516b..458d12f188 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -173,6 +173,8 @@ 
         ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
                     "nop; nop",
                     SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
+        mov     x0, sp
+        bl      enter_hypervisor_from_guest_noirq
         msr     daifclr, \iflags
         mov     x0, sp
         bl      enter_hypervisor_from_guest
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 20ba34ec91..5848dd8399 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2007,16 +2007,26 @@  static inline bool needs_ssbd_flip(struct vcpu *v)
 }
 
 /*
- * Actions that needs to be done after exiting the guest and before any
- * request from it is handled.
+ * Actions that needs to be done after exiting the guest and before the
+ * interrupts are unmasked.
  */
-void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
+void enter_hypervisor_from_guest_noirq(struct cpu_user_regs *regs)
 {
     struct vcpu *v = current;
 
     /* If the guest has disabled the workaround, bring it back on. */
     if ( needs_ssbd_flip(v) )
         arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
+}
+
+/*
+ * Actions that needs to be done after exiting the guest and before any
+ * request from it is handled. Depending on the exception trap, this may
+ * be called with interrupts unmasked.
+ */
+void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
+{
+    struct vcpu *v = current;
 
     /*
      * If we pended a virtual abort, preserve it until it gets cleared.