[Xen-devel,RFC,for-4.13,03/10] xen/arm: traps: Rework entry/exit from the guest path

Message ID 20190926183808.11630-4-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, enter_hypervisor_head() and leave_hypervisor_tail() are
used to deal with actions to be done before/after any guest request is
handled.

While they are meant to work in pair, the former is called for most of
the traps, including traps from the same exception level (i.e.
hypervisor) whilst the latter will only be called when returning to the
guest.

As pointed out, the enter_hypervisor_head() is not called from all the
traps, so this makes potentially difficult to extend it for the dealing
with same exception level.

Furthermore, some assembly only path will require to call
enter_hypervisor_tail(). So the function is now directly call by
assembly in for guest vector only. This means that the check whether we
are called in a guest trap can now be removed.

Take the opportunity to rename enter_hypervisor_tail() and
leave_hypervisor_tail() to something more meaningful and document them.
This should help everyone to understand the purpose of the two
functions.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

I haven't done the 32-bits part yet. I wanted to gather feedback before
looking in details how to integrate that with Arm32.
---
 xen/arch/arm/arm64/entry.S |  4 ++-
 xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
 2 files changed, 37 insertions(+), 38 deletions(-)

Comments

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

Julien Grall writes:

> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
> used to deal with actions to be done before/after any guest request is
> handled.
>
> While they are meant to work in pair, the former is called for most of
> the traps, including traps from the same exception level (i.e.
> hypervisor) whilst the latter will only be called when returning to the
> guest.
>
> As pointed out, the enter_hypervisor_head() is not called from all the
> traps, so this makes potentially difficult to extend it for the dealing
> with same exception level.
>
> Furthermore, some assembly only path will require to call
> enter_hypervisor_tail(). So the function is now directly call by
> assembly in for guest vector only. This means that the check whether we
> are called in a guest trap can now be removed.
>
> Take the opportunity to rename enter_hypervisor_tail() and
> leave_hypervisor_tail() to something more meaningful and document them.
> This should help everyone to understand the purpose of the two
> functions.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> ---
>
> I haven't done the 32-bits part yet. I wanted to gather feedback before
> looking in details how to integrate that with Arm32.
I'm looking at patches one by one and it is looking okay so far.


> ---
>  xen/arch/arm/arm64/entry.S |  4 ++-
>  xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
>  2 files changed, 37 insertions(+), 38 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 40d9f3ec8c..9eafae516b 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -147,7 +147,7 @@
>
>          .if \hyp == 0         /* Guest mode */
>
> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
>
>          exit_guest \compat
>
> @@ -175,6 +175,8 @@
>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>          msr     daifclr, \iflags
>          mov     x0, sp
Looks like this mov can be removed (see commend below).

> +        bl      enter_hypervisor_from_guest
> +        mov     x0, sp
>          bl      do_trap_\trap
>  1:
>          exit    hyp=0, compat=\compat
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index a3b961bd06..20ba34ec91 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>               cpu_require_ssbd_mitigation();
>  }
>
> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
> +/*
> + * Actions that needs to be done after exiting the guest and before any
> + * request from it is handled.
Maybe it is me only, but the phrasing is confusing. I had to read it two
times before I get it. What about "Actions that needs to be done when
raising exception level"? Or maybe "Actions that needs to be done when
switching from guest to hypervisor mode" ?

> + */
> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
With the guest_mode(regs) check removal , this function does not use regs
anymore.

>  {
> -    if ( guest_mode(regs) )
> -    {
> -        struct vcpu *v = current;
> +    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);
> +    /* 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);
>
> -        /*
> -         * If we pended a virtual abort, preserve it until it gets cleared.
> -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> -         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> -         * (alias of HCR.VA) is cleared to 0."
> -         */
> -        if ( v->arch.hcr_el2 & HCR_VA )
> -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> +    /*
> +     * If we pended a virtual abort, preserve it until it gets cleared.
> +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> +     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> +     * (alias of HCR.VA) is cleared to 0."
> +     */
> +    if ( v->arch.hcr_el2 & HCR_VA )
> +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>
>  #ifdef CONFIG_NEW_VGIC
> -        /*
> -         * We need to update the state of our emulated devices using level
> -         * triggered interrupts before syncing back the VGIC state.
> -         *
> -         * TODO: Investigate whether this is necessary to do on every
> -         * trap and how it can be optimised.
> -         */
> -        vtimer_update_irqs(v);
> -        vcpu_update_evtchn_irq(v);
> +    /*
> +     * We need to update the state of our emulated devices using level
> +     * triggered interrupts before syncing back the VGIC state.
> +     *
> +     * TODO: Investigate whether this is necessary to do on every
> +     * trap and how it can be optimised.
> +     */
> +    vtimer_update_irqs(v);
> +    vcpu_update_evtchn_irq(v);
>  #endif
>
> -        vgic_sync_from_lrs(v);
> -    }
> +    vgic_sync_from_lrs(v);
>  }
>
>  void do_trap_guest_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>
> -    enter_hypervisor_head(regs);
> -
>      switch ( hsr.ec )
>      {
>      case HSR_EC_WFI_WFE:
> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>
> -    enter_hypervisor_head(regs);
> -
>      switch ( hsr.ec )
>      {
>  #ifdef CONFIG_ARM_64
> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>
>  void do_trap_hyp_serror(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
> -
>      __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
>  }
>
>  void do_trap_guest_serror(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
> -
>      __do_trap_serror(regs, true);
>  }
>
>  void do_trap_irq(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
>      gic_interrupt(regs, 0);
>  }
>
>  void do_trap_fiq(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
>      gic_interrupt(regs, 1);
>  }
>
> @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void)
>      local_irq_disable();
>  }
>
> -void leave_hypervisor_tail(void)
> +/*
> + * Actions that needs to be done before entering the guest. This is the
> + * last thing executed before the guest context is fully restored.
> + *
> + * The function will return with interrupts disabled.
> + */
> +void leave_hypervisor_to_guest(void)
>  {
>      local_irq_disable();


--
Volodymyr Babchuk at EPAM
Julien Grall Sept. 27, 2019, 12:16 p.m. | #2
On 27/09/2019 12:45, Volodymyr Babchuk wrote:
> 
> Julien,

Hi...

> Julien Grall writes:
> 
>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
>> used to deal with actions to be done before/after any guest request is
>> handled.
>>
>> While they are meant to work in pair, the former is called for most of
>> the traps, including traps from the same exception level (i.e.
>> hypervisor) whilst the latter will only be called when returning to the
>> guest.
>>
>> As pointed out, the enter_hypervisor_head() is not called from all the
>> traps, so this makes potentially difficult to extend it for the dealing
>> with same exception level.
>>
>> Furthermore, some assembly only path will require to call
>> enter_hypervisor_tail(). So the function is now directly call by
>> assembly in for guest vector only. This means that the check whether we
>> are called in a guest trap can now be removed.
>>
>> Take the opportunity to rename enter_hypervisor_tail() and
>> leave_hypervisor_tail() to something more meaningful and document them.
>> This should help everyone to understand the purpose of the two
>> functions.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>> looking in details how to integrate that with Arm32.
> I'm looking at patches one by one and it is looking okay so far.
> 
> 
>> ---
>>   xen/arch/arm/arm64/entry.S |  4 ++-
>>   xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
>>   2 files changed, 37 insertions(+), 38 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>> index 40d9f3ec8c..9eafae516b 100644
>> --- a/xen/arch/arm/arm64/entry.S
>> +++ b/xen/arch/arm/arm64/entry.S
>> @@ -147,7 +147,7 @@
>>
>>           .if \hyp == 0         /* Guest mode */
>>
>> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
>> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
>>
>>           exit_guest \compat
>>
>> @@ -175,6 +175,8 @@
>>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>           msr     daifclr, \iflags
>>           mov     x0, sp
> Looks like this mov can be removed (see commend below).
> 
>> +        bl      enter_hypervisor_from_guest
>> +        mov     x0, sp
>>           bl      do_trap_\trap
>>   1:
>>           exit    hyp=0, compat=\compat
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index a3b961bd06..20ba34ec91 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>>                cpu_require_ssbd_mitigation();
>>   }
>>
>> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
>> +/*
>> + * Actions that needs to be done after exiting the guest and before any
>> + * request from it is handled.
> Maybe it is me only, but the phrasing is confusing. I had to read it two
> times before I get it. What about "Actions that needs to be done when
> raising exception level"? Or maybe "Actions that needs to be done when
> switching from guest to hypervisor mode" ?

Is it a suggestion to replace the full sentence or just the first before (i.e. 
before 'and')?

> 
>> + */
>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> With the guest_mode(regs) check removal , this function does not use regs
> anymore.

I have nearly done it while working on the series, but then I thought that it 
would be better keep all the functions called from the entry path in assembly 
similar.

Cheers,
Volodymyr Babchuk Sept. 27, 2019, 12:27 p.m. | #3
Julien Grall writes:

> On 27/09/2019 12:45, Volodymyr Babchuk wrote:
>>
>> Julien,
>
> Hi...
>
>> Julien Grall writes:
>>
>>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
>>> used to deal with actions to be done before/after any guest request is
>>> handled.
>>>
>>> While they are meant to work in pair, the former is called for most of
>>> the traps, including traps from the same exception level (i.e.
>>> hypervisor) whilst the latter will only be called when returning to the
>>> guest.
>>>
>>> As pointed out, the enter_hypervisor_head() is not called from all the
>>> traps, so this makes potentially difficult to extend it for the dealing
>>> with same exception level.
>>>
>>> Furthermore, some assembly only path will require to call
>>> enter_hypervisor_tail(). So the function is now directly call by
>>> assembly in for guest vector only. This means that the check whether we
>>> are called in a guest trap can now be removed.
>>>
>>> Take the opportunity to rename enter_hypervisor_tail() and
>>> leave_hypervisor_tail() to something more meaningful and document them.
>>> This should help everyone to understand the purpose of the two
>>> functions.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>
>>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>>> looking in details how to integrate that with Arm32.
>> I'm looking at patches one by one and it is looking okay so far.
>>
>>
>>> ---
>>>   xen/arch/arm/arm64/entry.S |  4 ++-
>>>   xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
>>>   2 files changed, 37 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>> index 40d9f3ec8c..9eafae516b 100644
>>> --- a/xen/arch/arm/arm64/entry.S
>>> +++ b/xen/arch/arm/arm64/entry.S
>>> @@ -147,7 +147,7 @@
>>>
>>>           .if \hyp == 0         /* Guest mode */
>>>
>>> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
>>> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
>>>
>>>           exit_guest \compat
>>>
>>> @@ -175,6 +175,8 @@
>>>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>>           msr     daifclr, \iflags
>>>           mov     x0, sp
>> Looks like this mov can be removed (see commend below).
>>
>>> +        bl      enter_hypervisor_from_guest
>>> +        mov     x0, sp
>>>           bl      do_trap_\trap
>>>   1:
>>>           exit    hyp=0, compat=\compat
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index a3b961bd06..20ba34ec91 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>>>                cpu_require_ssbd_mitigation();
>>>   }
>>>
>>> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
>>> +/*
>>> + * Actions that needs to be done after exiting the guest and before any
>>> + * request from it is handled.
>> Maybe it is me only, but the phrasing is confusing. I had to read it two
>> times before I get it. What about "Actions that needs to be done when
>> raising exception level"? Or maybe "Actions that needs to be done when
>> switching from guest to hypervisor mode" ?
>
> Is it a suggestion to replace the full sentence or just the first
> before (i.e. before 'and')?
This is a suggestion for the first part.

>>
>>> + */
>>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>> With the guest_mode(regs) check removal , this function does not use regs
>> anymore.
>
> I have nearly done it while working on the series, but then I thought
> that it would be better keep all the functions called from the entry
> path in assembly similar.
This can save one assembly instruction in the entry path. But I'm not
sure if it is worth it. So it is up to you.
Julien Grall Sept. 27, 2019, 12:44 p.m. | #4
Hi,

On 27/09/2019 13:27, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> On 27/09/2019 12:45, Volodymyr Babchuk wrote:
>>>
>>> Julien,
>>
>> Hi...
>>
>>> Julien Grall writes:
>>>
>>>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
>>>> used to deal with actions to be done before/after any guest request is
>>>> handled.
>>>>
>>>> While they are meant to work in pair, the former is called for most of
>>>> the traps, including traps from the same exception level (i.e.
>>>> hypervisor) whilst the latter will only be called when returning to the
>>>> guest.
>>>>
>>>> As pointed out, the enter_hypervisor_head() is not called from all the
>>>> traps, so this makes potentially difficult to extend it for the dealing
>>>> with same exception level.
>>>>
>>>> Furthermore, some assembly only path will require to call
>>>> enter_hypervisor_tail(). So the function is now directly call by
>>>> assembly in for guest vector only. This means that the check whether we
>>>> are called in a guest trap can now be removed.
>>>>
>>>> Take the opportunity to rename enter_hypervisor_tail() and
>>>> leave_hypervisor_tail() to something more meaningful and document them.
>>>> This should help everyone to understand the purpose of the two
>>>> functions.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>
>>>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>>>> looking in details how to integrate that with Arm32.
>>> I'm looking at patches one by one and it is looking okay so far.
>>>
>>>
>>>> ---
>>>>    xen/arch/arm/arm64/entry.S |  4 ++-
>>>>    xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
>>>>    2 files changed, 37 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>>> index 40d9f3ec8c..9eafae516b 100644
>>>> --- a/xen/arch/arm/arm64/entry.S
>>>> +++ b/xen/arch/arm/arm64/entry.S
>>>> @@ -147,7 +147,7 @@
>>>>
>>>>            .if \hyp == 0         /* Guest mode */
>>>>
>>>> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
>>>> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
>>>>
>>>>            exit_guest \compat
>>>>
>>>> @@ -175,6 +175,8 @@
>>>>                        SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>>>            msr     daifclr, \iflags
>>>>            mov     x0, sp
>>> Looks like this mov can be removed (see commend below).
>>>
>>>> +        bl      enter_hypervisor_from_guest
>>>> +        mov     x0, sp
>>>>            bl      do_trap_\trap
>>>>    1:
>>>>            exit    hyp=0, compat=\compat
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index a3b961bd06..20ba34ec91 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>>>>                 cpu_require_ssbd_mitigation();
>>>>    }
>>>>
>>>> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
>>>> +/*
>>>> + * Actions that needs to be done after exiting the guest and before any
>>>> + * request from it is handled.
>>> Maybe it is me only, but the phrasing is confusing. I had to read it two
>>> times before I get it. What about "Actions that needs to be done when
>>> raising exception level"? Or maybe "Actions that needs to be done when
>>> switching from guest to hypervisor mode" ?
>>
>> Is it a suggestion to replace the full sentence or just the first
>> before (i.e. before 'and')?
> This is a suggestion for the first part.

How about:

"Actions that needs to be done after entering the hypervisor from the guest and 
before we handle any request."

> 
>>>
>>>> + */
>>>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>>> With the guest_mode(regs) check removal , this function does not use regs
>>> anymore.
>>
>> I have nearly done it while working on the series, but then I thought
>> that it would be better keep all the functions called from the entry
>> path in assembly similar.
> This can save one assembly instruction in the entry path. But I'm not
> sure if it is worth it. So it is up to you.

My concern is user may decide to use guest_cpu_user_regs() when the 'regs' 
parameter could have been used. But I guess, we can notice it during review.

So I will drop it.

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

> Hi,
>
> On 27/09/2019 13:27, Volodymyr Babchuk wrote:
>>
>> Julien Grall writes:
>>
>>> On 27/09/2019 12:45, Volodymyr Babchuk wrote:
>>>>
>>>> Julien,
>>>
>>> Hi...
>>>
>>>> Julien Grall writes:
>>>>
>>>>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
>>>>> used to deal with actions to be done before/after any guest request is
>>>>> handled.
>>>>>
>>>>> While they are meant to work in pair, the former is called for most of
>>>>> the traps, including traps from the same exception level (i.e.
>>>>> hypervisor) whilst the latter will only be called when returning to the
>>>>> guest.
>>>>>
>>>>> As pointed out, the enter_hypervisor_head() is not called from all the
>>>>> traps, so this makes potentially difficult to extend it for the dealing
>>>>> with same exception level.
>>>>>
>>>>> Furthermore, some assembly only path will require to call
>>>>> enter_hypervisor_tail(). So the function is now directly call by
>>>>> assembly in for guest vector only. This means that the check whether we
>>>>> are called in a guest trap can now be removed.
>>>>>
>>>>> Take the opportunity to rename enter_hypervisor_tail() and
>>>>> leave_hypervisor_tail() to something more meaningful and document them.
>>>>> This should help everyone to understand the purpose of the two
>>>>> functions.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>
>>>>> ---
>>>>>
>>>>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>>>>> looking in details how to integrate that with Arm32.
>>>> I'm looking at patches one by one and it is looking okay so far.
>>>>
>>>>
>>>>> ---
>>>>>    xen/arch/arm/arm64/entry.S |  4 ++-
>>>>>    xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
>>>>>    2 files changed, 37 insertions(+), 38 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>>>> index 40d9f3ec8c..9eafae516b 100644
>>>>> --- a/xen/arch/arm/arm64/entry.S
>>>>> +++ b/xen/arch/arm/arm64/entry.S
>>>>> @@ -147,7 +147,7 @@
>>>>>
>>>>>            .if \hyp == 0         /* Guest mode */
>>>>>
>>>>> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
>>>>> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
>>>>>
>>>>>            exit_guest \compat
>>>>>
>>>>> @@ -175,6 +175,8 @@
>>>>>                        SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>>>>            msr     daifclr, \iflags
>>>>>            mov     x0, sp
>>>> Looks like this mov can be removed (see commend below).
>>>>
>>>>> +        bl      enter_hypervisor_from_guest
>>>>> +        mov     x0, sp
>>>>>            bl      do_trap_\trap
>>>>>    1:
>>>>>            exit    hyp=0, compat=\compat
>>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>>> index a3b961bd06..20ba34ec91 100644
>>>>> --- a/xen/arch/arm/traps.c
>>>>> +++ b/xen/arch/arm/traps.c
>>>>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>>>>>                 cpu_require_ssbd_mitigation();
>>>>>    }
>>>>>
>>>>> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
>>>>> +/*
>>>>> + * Actions that needs to be done after exiting the guest and before any
>>>>> + * request from it is handled.
>>>> Maybe it is me only, but the phrasing is confusing. I had to read it two
>>>> times before I get it. What about "Actions that needs to be done when
>>>> raising exception level"? Or maybe "Actions that needs to be done when
>>>> switching from guest to hypervisor mode" ?
>>>
>>> Is it a suggestion to replace the full sentence or just the first
>>> before (i.e. before 'and')?
>> This is a suggestion for the first part.
>
> How about:
>
> "Actions that needs to be done after entering the hypervisor from the
> guest and before we handle any request."
Sound perfect.

[...]
Stefano Stabellini Oct. 1, 2019, 8:12 p.m. | #6
On Thu, 26 Sep 2019, Julien Grall wrote:
> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
> used to deal with actions to be done before/after any guest request is
> handled.
> 
> While they are meant to work in pair, the former is called for most of
> the traps, including traps from the same exception level (i.e.
> hypervisor) whilst the latter will only be called when returning to the
> guest.
> 
> As pointed out, the enter_hypervisor_head() is not called from all the
> traps, so this makes potentially difficult to extend it for the dealing
> with same exception level.
> 
> Furthermore, some assembly only path will require to call
> enter_hypervisor_tail(). So the function is now directly call by
> assembly in for guest vector only. This means that the check whether we
> are called in a guest trap can now be removed.
> 
> Take the opportunity to rename enter_hypervisor_tail() and
> leave_hypervisor_tail() to something more meaningful and document them.
> This should help everyone to understand the purpose of the two
> functions.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
> I haven't done the 32-bits part yet. I wanted to gather feedback before
> looking in details how to integrate that with Arm32.
> ---
>  xen/arch/arm/arm64/entry.S |  4 ++-
>  xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
>  2 files changed, 37 insertions(+), 38 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 40d9f3ec8c..9eafae516b 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -147,7 +147,7 @@
>  
>          .if \hyp == 0         /* Guest mode */
>  
> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
>  
>          exit_guest \compat
>  
> @@ -175,6 +175,8 @@
>                      SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>          msr     daifclr, \iflags
>          mov     x0, sp
> +        bl      enter_hypervisor_from_guest
> +        mov     x0, sp
>          bl      do_trap_\trap
>  1:
>          exit    hyp=0, compat=\compat
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index a3b961bd06..20ba34ec91 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>               cpu_require_ssbd_mitigation();
>  }
>  
> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
> +/*
> + * Actions that needs to be done after exiting the guest and before any
> + * request from it is handled.
> + */
> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>  {
> -    if ( guest_mode(regs) )
> -    {
> -        struct vcpu *v = current;
> +    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);
> +    /* 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);
>  
> -        /*
> -         * If we pended a virtual abort, preserve it until it gets cleared.
> -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> -         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> -         * (alias of HCR.VA) is cleared to 0."
> -         */
> -        if ( v->arch.hcr_el2 & HCR_VA )
> -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> +    /*
> +     * If we pended a virtual abort, preserve it until it gets cleared.
> +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> +     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> +     * (alias of HCR.VA) is cleared to 0."
> +     */
> +    if ( v->arch.hcr_el2 & HCR_VA )
> +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>  
>  #ifdef CONFIG_NEW_VGIC
> -        /*
> -         * We need to update the state of our emulated devices using level
> -         * triggered interrupts before syncing back the VGIC state.
> -         *
> -         * TODO: Investigate whether this is necessary to do on every
> -         * trap and how it can be optimised.
> -         */
> -        vtimer_update_irqs(v);
> -        vcpu_update_evtchn_irq(v);
> +    /*
> +     * We need to update the state of our emulated devices using level
> +     * triggered interrupts before syncing back the VGIC state.
> +     *
> +     * TODO: Investigate whether this is necessary to do on every
> +     * trap and how it can be optimised.
> +     */
> +    vtimer_update_irqs(v);
> +    vcpu_update_evtchn_irq(v);
>  #endif
>  
> -        vgic_sync_from_lrs(v);
> -    }
> +    vgic_sync_from_lrs(v);
>  }
>  
>  void do_trap_guest_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>  
> -    enter_hypervisor_head(regs);
> -
>      switch ( hsr.ec )
>      {
>      case HSR_EC_WFI_WFE:
> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>  {
>      const union hsr hsr = { .bits = regs->hsr };
>  
> -    enter_hypervisor_head(regs);
> -
>      switch ( hsr.ec )
>      {
>  #ifdef CONFIG_ARM_64
> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>  
>  void do_trap_hyp_serror(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
> -
>      __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
>  }
>  
>  void do_trap_guest_serror(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
> -
>      __do_trap_serror(regs, true);
>  }
>  
>  void do_trap_irq(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
>      gic_interrupt(regs, 0);
>  }
>  
>  void do_trap_fiq(struct cpu_user_regs *regs)
>  {
> -    enter_hypervisor_head(regs);
>      gic_interrupt(regs, 1);
>  }

I am OK with the general approach but one thing to note is that the fiq
handler doesn't use the guest_vector macro at the moment.


> @@ -2281,7 +2272,13 @@ static void check_for_vcpu_work(void)
>      local_irq_disable();
>  }
>  
> -void leave_hypervisor_tail(void)
> +/*
> + * Actions that needs to be done before entering the guest. This is the
> + * last thing executed before the guest context is fully restored.
> + *
> + * The function will return with interrupts disabled.
> + */
> +void leave_hypervisor_to_guest(void)
>  {
>      local_irq_disable();
>  
> -- 
> 2.11.0
>
Julien Grall Oct. 1, 2019, 9:06 p.m. | #7
Hi,

On 01/10/2019 21:12, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Julien Grall wrote:

>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are

>> used to deal with actions to be done before/after any guest request is

>> handled.

>>

>> While they are meant to work in pair, the former is called for most of

>> the traps, including traps from the same exception level (i.e.

>> hypervisor) whilst the latter will only be called when returning to the

>> guest.

>>

>> As pointed out, the enter_hypervisor_head() is not called from all the

>> traps, so this makes potentially difficult to extend it for the dealing

>> with same exception level.

>>

>> Furthermore, some assembly only path will require to call

>> enter_hypervisor_tail(). So the function is now directly call by

>> assembly in for guest vector only. This means that the check whether we

>> are called in a guest trap can now be removed.

>>

>> Take the opportunity to rename enter_hypervisor_tail() and

>> leave_hypervisor_tail() to something more meaningful and document them.

>> This should help everyone to understand the purpose of the two

>> functions.

>>

>> Signed-off-by: Julien Grall <julien.grall@arm.com>

>>

>> ---

>>

>> I haven't done the 32-bits part yet. I wanted to gather feedback before

>> looking in details how to integrate that with Arm32.

>> ---

>>   xen/arch/arm/arm64/entry.S |  4 ++-

>>   xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------

>>   2 files changed, 37 insertions(+), 38 deletions(-)

>>

>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S

>> index 40d9f3ec8c..9eafae516b 100644

>> --- a/xen/arch/arm/arm64/entry.S

>> +++ b/xen/arch/arm/arm64/entry.S

>> @@ -147,7 +147,7 @@

>>   

>>           .if \hyp == 0         /* Guest mode */

>>   

>> -        bl      leave_hypervisor_tail /* Disables interrupts on return */

>> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */

>>   

>>           exit_guest \compat

>>   

>> @@ -175,6 +175,8 @@

>>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)

>>           msr     daifclr, \iflags

>>           mov     x0, sp

>> +        bl      enter_hypervisor_from_guest

>> +        mov     x0, sp

>>           bl      do_trap_\trap

>>   1:

>>           exit    hyp=0, compat=\compat

>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c

>> index a3b961bd06..20ba34ec91 100644

>> --- a/xen/arch/arm/traps.c

>> +++ b/xen/arch/arm/traps.c

>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)

>>                cpu_require_ssbd_mitigation();

>>   }

>>   

>> -static void enter_hypervisor_head(struct cpu_user_regs *regs)

>> +/*

>> + * Actions that needs to be done after exiting the guest and before any

>> + * request from it is handled.

>> + */

>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)

>>   {

>> -    if ( guest_mode(regs) )

>> -    {

>> -        struct vcpu *v = current;

>> +    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);

>> +    /* 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);

>>   

>> -        /*

>> -         * If we pended a virtual abort, preserve it until it gets cleared.

>> -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,

>> -         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE

>> -         * (alias of HCR.VA) is cleared to 0."

>> -         */

>> -        if ( v->arch.hcr_el2 & HCR_VA )

>> -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);

>> +    /*

>> +     * If we pended a virtual abort, preserve it until it gets cleared.

>> +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,

>> +     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE

>> +     * (alias of HCR.VA) is cleared to 0."

>> +     */

>> +    if ( v->arch.hcr_el2 & HCR_VA )

>> +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);

>>   

>>   #ifdef CONFIG_NEW_VGIC

>> -        /*

>> -         * We need to update the state of our emulated devices using level

>> -         * triggered interrupts before syncing back the VGIC state.

>> -         *

>> -         * TODO: Investigate whether this is necessary to do on every

>> -         * trap and how it can be optimised.

>> -         */

>> -        vtimer_update_irqs(v);

>> -        vcpu_update_evtchn_irq(v);

>> +    /*

>> +     * We need to update the state of our emulated devices using level

>> +     * triggered interrupts before syncing back the VGIC state.

>> +     *

>> +     * TODO: Investigate whether this is necessary to do on every

>> +     * trap and how it can be optimised.

>> +     */

>> +    vtimer_update_irqs(v);

>> +    vcpu_update_evtchn_irq(v);

>>   #endif

>>   

>> -        vgic_sync_from_lrs(v);

>> -    }

>> +    vgic_sync_from_lrs(v);

>>   }

>>   

>>   void do_trap_guest_sync(struct cpu_user_regs *regs)

>>   {

>>       const union hsr hsr = { .bits = regs->hsr };

>>   

>> -    enter_hypervisor_head(regs);

>> -

>>       switch ( hsr.ec )

>>       {

>>       case HSR_EC_WFI_WFE:

>> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)

>>   {

>>       const union hsr hsr = { .bits = regs->hsr };

>>   

>> -    enter_hypervisor_head(regs);

>> -

>>       switch ( hsr.ec )

>>       {

>>   #ifdef CONFIG_ARM_64

>> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)

>>   

>>   void do_trap_hyp_serror(struct cpu_user_regs *regs)

>>   {

>> -    enter_hypervisor_head(regs);

>> -

>>       __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));

>>   }

>>   

>>   void do_trap_guest_serror(struct cpu_user_regs *regs)

>>   {

>> -    enter_hypervisor_head(regs);

>> -

>>       __do_trap_serror(regs, true);

>>   }

>>   

>>   void do_trap_irq(struct cpu_user_regs *regs)

>>   {

>> -    enter_hypervisor_head(regs);

>>       gic_interrupt(regs, 0);

>>   }

>>   

>>   void do_trap_fiq(struct cpu_user_regs *regs)

>>   {

>> -    enter_hypervisor_head(regs);

>>       gic_interrupt(regs, 1);

>>   }

> 

> I am OK with the general approach but one thing to note is that the fiq

> handler doesn't use the guest_vector macro at the moment.


do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). 
So I don't see an issue here.

As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler 
does not use guest_vector.

That said, it is interesting to see that we don't deal the same way the 
FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the 
latter will crash the guest.

It would be good if we can have the same behavior accross the two arch 
if possible. I vaguely recall someone (Andre?) mentioning some changes 
around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?

Also, a side effect of not calling enter_hypervisor_head() is workaround 
are not re-enabled. We are going to panic soon after, so it may not be 
that much an issue.

I will have a think about it.

Cheers,

-- 
Julien Grall
Stefano Stabellini Oct. 2, 2019, 12:16 a.m. | #8
On Tue, 1 Oct 2019, Julien Grall wrote:
> Hi,
> 
> On 01/10/2019 21:12, Stefano Stabellini wrote:
> > On Thu, 26 Sep 2019, Julien Grall wrote:
> >> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
> >> used to deal with actions to be done before/after any guest request is
> >> handled.
> >>
> >> While they are meant to work in pair, the former is called for most of
> >> the traps, including traps from the same exception level (i.e.
> >> hypervisor) whilst the latter will only be called when returning to the
> >> guest.
> >>
> >> As pointed out, the enter_hypervisor_head() is not called from all the
> >> traps, so this makes potentially difficult to extend it for the dealing
> >> with same exception level.
> >>
> >> Furthermore, some assembly only path will require to call
> >> enter_hypervisor_tail(). So the function is now directly call by
> >> assembly in for guest vector only. This means that the check whether we
> >> are called in a guest trap can now be removed.
> >>
> >> Take the opportunity to rename enter_hypervisor_tail() and
> >> leave_hypervisor_tail() to something more meaningful and document them.
> >> This should help everyone to understand the purpose of the two
> >> functions.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@arm.com>
> >>
> >> ---
> >>
> >> I haven't done the 32-bits part yet. I wanted to gather feedback before
> >> looking in details how to integrate that with Arm32.
> >> ---
> >>   xen/arch/arm/arm64/entry.S |  4 ++-
> >>   xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
> >>   2 files changed, 37 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> >> index 40d9f3ec8c..9eafae516b 100644
> >> --- a/xen/arch/arm/arm64/entry.S
> >> +++ b/xen/arch/arm/arm64/entry.S
> >> @@ -147,7 +147,7 @@
> >>   
> >>           .if \hyp == 0         /* Guest mode */
> >>   
> >> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
> >> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
> >>   
> >>           exit_guest \compat
> >>   
> >> @@ -175,6 +175,8 @@
> >>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> >>           msr     daifclr, \iflags
> >>           mov     x0, sp
> >> +        bl      enter_hypervisor_from_guest
> >> +        mov     x0, sp
> >>           bl      do_trap_\trap
> >>   1:
> >>           exit    hyp=0, compat=\compat
> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> >> index a3b961bd06..20ba34ec91 100644
> >> --- a/xen/arch/arm/traps.c
> >> +++ b/xen/arch/arm/traps.c
> >> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
> >>                cpu_require_ssbd_mitigation();
> >>   }
> >>   
> >> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
> >> +/*
> >> + * Actions that needs to be done after exiting the guest and before any
> >> + * request from it is handled.
> >> + */
> >> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> >>   {
> >> -    if ( guest_mode(regs) )
> >> -    {
> >> -        struct vcpu *v = current;
> >> +    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);
> >> +    /* 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);
> >>   
> >> -        /*
> >> -         * If we pended a virtual abort, preserve it until it gets cleared.
> >> -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> >> -         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> >> -         * (alias of HCR.VA) is cleared to 0."
> >> -         */
> >> -        if ( v->arch.hcr_el2 & HCR_VA )
> >> -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> >> +    /*
> >> +     * If we pended a virtual abort, preserve it until it gets cleared.
> >> +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> >> +     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> >> +     * (alias of HCR.VA) is cleared to 0."
> >> +     */
> >> +    if ( v->arch.hcr_el2 & HCR_VA )
> >> +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> >>   
> >>   #ifdef CONFIG_NEW_VGIC
> >> -        /*
> >> -         * We need to update the state of our emulated devices using level
> >> -         * triggered interrupts before syncing back the VGIC state.
> >> -         *
> >> -         * TODO: Investigate whether this is necessary to do on every
> >> -         * trap and how it can be optimised.
> >> -         */
> >> -        vtimer_update_irqs(v);
> >> -        vcpu_update_evtchn_irq(v);
> >> +    /*
> >> +     * We need to update the state of our emulated devices using level
> >> +     * triggered interrupts before syncing back the VGIC state.
> >> +     *
> >> +     * TODO: Investigate whether this is necessary to do on every
> >> +     * trap and how it can be optimised.
> >> +     */
> >> +    vtimer_update_irqs(v);
> >> +    vcpu_update_evtchn_irq(v);
> >>   #endif
> >>   
> >> -        vgic_sync_from_lrs(v);
> >> -    }
> >> +    vgic_sync_from_lrs(v);
> >>   }
> >>   
> >>   void do_trap_guest_sync(struct cpu_user_regs *regs)
> >>   {
> >>       const union hsr hsr = { .bits = regs->hsr };
> >>   
> >> -    enter_hypervisor_head(regs);
> >> -
> >>       switch ( hsr.ec )
> >>       {
> >>       case HSR_EC_WFI_WFE:
> >> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
> >>   {
> >>       const union hsr hsr = { .bits = regs->hsr };
> >>   
> >> -    enter_hypervisor_head(regs);
> >> -
> >>       switch ( hsr.ec )
> >>       {
> >>   #ifdef CONFIG_ARM_64
> >> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
> >>   
> >>   void do_trap_hyp_serror(struct cpu_user_regs *regs)
> >>   {
> >> -    enter_hypervisor_head(regs);
> >> -
> >>       __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
> >>   }
> >>   
> >>   void do_trap_guest_serror(struct cpu_user_regs *regs)
> >>   {
> >> -    enter_hypervisor_head(regs);
> >> -
> >>       __do_trap_serror(regs, true);
> >>   }
> >>   
> >>   void do_trap_irq(struct cpu_user_regs *regs)
> >>   {
> >> -    enter_hypervisor_head(regs);
> >>       gic_interrupt(regs, 0);
> >>   }
> >>   
> >>   void do_trap_fiq(struct cpu_user_regs *regs)
> >>   {
> >> -    enter_hypervisor_head(regs);
> >>       gic_interrupt(regs, 1);
> >>   }
> > 
> > I am OK with the general approach but one thing to note is that the fiq
> > handler doesn't use the guest_vector macro at the moment.
> 
> do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). 
> So I don't see an issue here.
> 
> As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler 
> does not use guest_vector.
> 
> That said, it is interesting to see that we don't deal the same way the 
> FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the 
> latter will crash the guest.
> 
> It would be good if we can have the same behavior accross the two arch 
> if possible. I vaguely recall someone (Andre?) mentioning some changes 
> around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?
> 
> Also, a side effect of not calling enter_hypervisor_head() is workaround 
> are not re-enabled. We are going to panic soon after, so it may not be 
> that much an issue.

Right, that is what I was thinking too, but I wanted to highlight it. At
least it would be worth adding a sentence to the commit message about
it.

> I will have a think about it.
Julien Grall Oct. 2, 2019, 9:12 a.m. | #9
Hi Stefano,

On 10/2/19 1:16 AM, Stefano Stabellini wrote:
> On Tue, 1 Oct 2019, Julien Grall wrote:
>> On 01/10/2019 21:12, Stefano Stabellini wrote:
>>> On Thu, 26 Sep 2019, Julien Grall wrote:
>>> I am OK with the general approach but one thing to note is that the fiq
>>> handler doesn't use the guest_vector macro at the moment.
>>
>> do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode().
>> So I don't see an issue here.
>>
>> As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler
>> does not use guest_vector.
>>
>> That said, it is interesting to see that we don't deal the same way the
>> FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the
>> latter will crash the guest.
>>
>> It would be good if we can have the same behavior accross the two arch
>> if possible. I vaguely recall someone (Andre?) mentioning some changes
>> around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?
>>
>> Also, a side effect of not calling enter_hypervisor_head() is workaround
>> are not re-enabled. We are going to panic soon after, so it may not be
>> that much an issue.
> 
> Right, that is what I was thinking too, but I wanted to highlight it. At
> least it would be worth adding a sentence to the commit message about
> it.
As I pointed out above, this patch does not change anything in this 
particular Arm64 vector so I don't see why I should mention it in the 
commit message.

Cheers,
Stefano Stabellini Oct. 2, 2019, 12:41 p.m. | #10
On Tue, 1 Oct 2019, Stefano Stabellini wrote:
> On Tue, 1 Oct 2019, Julien Grall wrote:
> > Hi,
> > 
> > On 01/10/2019 21:12, Stefano Stabellini wrote:
> > > On Thu, 26 Sep 2019, Julien Grall wrote:
> > >> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
> > >> used to deal with actions to be done before/after any guest request is
> > >> handled.
> > >>
> > >> While they are meant to work in pair, the former is called for most of
> > >> the traps, including traps from the same exception level (i.e.
> > >> hypervisor) whilst the latter will only be called when returning to the
> > >> guest.
> > >>
> > >> As pointed out, the enter_hypervisor_head() is not called from all the
> > >> traps, so this makes potentially difficult to extend it for the dealing
> > >> with same exception level.
> > >>
> > >> Furthermore, some assembly only path will require to call
> > >> enter_hypervisor_tail(). So the function is now directly call by
> > >> assembly in for guest vector only. This means that the check whether we
> > >> are called in a guest trap can now be removed.
> > >>
> > >> Take the opportunity to rename enter_hypervisor_tail() and
> > >> leave_hypervisor_tail() to something more meaningful and document them.
> > >> This should help everyone to understand the purpose of the two
> > >> functions.
> > >>
> > >> Signed-off-by: Julien Grall <julien.grall@arm.com>
> > >>
> > >> ---
> > >>
> > >> I haven't done the 32-bits part yet. I wanted to gather feedback before
> > >> looking in details how to integrate that with Arm32.
> > >> ---
> > >>   xen/arch/arm/arm64/entry.S |  4 ++-
> > >>   xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
> > >>   2 files changed, 37 insertions(+), 38 deletions(-)
> > >>
> > >> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > >> index 40d9f3ec8c..9eafae516b 100644
> > >> --- a/xen/arch/arm/arm64/entry.S
> > >> +++ b/xen/arch/arm/arm64/entry.S
> > >> @@ -147,7 +147,7 @@
> > >>   
> > >>           .if \hyp == 0         /* Guest mode */
> > >>   
> > >> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
> > >> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
> > >>   
> > >>           exit_guest \compat
> > >>   
> > >> @@ -175,6 +175,8 @@
> > >>                       SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> > >>           msr     daifclr, \iflags
> > >>           mov     x0, sp
> > >> +        bl      enter_hypervisor_from_guest
> > >> +        mov     x0, sp
> > >>           bl      do_trap_\trap
> > >>   1:
> > >>           exit    hyp=0, compat=\compat
> > >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > >> index a3b961bd06..20ba34ec91 100644
> > >> --- a/xen/arch/arm/traps.c
> > >> +++ b/xen/arch/arm/traps.c
> > >> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
> > >>                cpu_require_ssbd_mitigation();
> > >>   }
> > >>   
> > >> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
> > >> +/*
> > >> + * Actions that needs to be done after exiting the guest and before any
> > >> + * request from it is handled.
> > >> + */
> > >> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> > >>   {
> > >> -    if ( guest_mode(regs) )
> > >> -    {
> > >> -        struct vcpu *v = current;
> > >> +    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);
> > >> +    /* 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);
> > >>   
> > >> -        /*
> > >> -         * If we pended a virtual abort, preserve it until it gets cleared.
> > >> -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> > >> -         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> > >> -         * (alias of HCR.VA) is cleared to 0."
> > >> -         */
> > >> -        if ( v->arch.hcr_el2 & HCR_VA )
> > >> -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> > >> +    /*
> > >> +     * If we pended a virtual abort, preserve it until it gets cleared.
> > >> +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
> > >> +     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
> > >> +     * (alias of HCR.VA) is cleared to 0."
> > >> +     */
> > >> +    if ( v->arch.hcr_el2 & HCR_VA )
> > >> +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> > >>   
> > >>   #ifdef CONFIG_NEW_VGIC
> > >> -        /*
> > >> -         * We need to update the state of our emulated devices using level
> > >> -         * triggered interrupts before syncing back the VGIC state.
> > >> -         *
> > >> -         * TODO: Investigate whether this is necessary to do on every
> > >> -         * trap and how it can be optimised.
> > >> -         */
> > >> -        vtimer_update_irqs(v);
> > >> -        vcpu_update_evtchn_irq(v);
> > >> +    /*
> > >> +     * We need to update the state of our emulated devices using level
> > >> +     * triggered interrupts before syncing back the VGIC state.
> > >> +     *
> > >> +     * TODO: Investigate whether this is necessary to do on every
> > >> +     * trap and how it can be optimised.
> > >> +     */
> > >> +    vtimer_update_irqs(v);
> > >> +    vcpu_update_evtchn_irq(v);
> > >>   #endif
> > >>   
> > >> -        vgic_sync_from_lrs(v);
> > >> -    }
> > >> +    vgic_sync_from_lrs(v);
> > >>   }
> > >>   
> > >>   void do_trap_guest_sync(struct cpu_user_regs *regs)
> > >>   {
> > >>       const union hsr hsr = { .bits = regs->hsr };
> > >>   
> > >> -    enter_hypervisor_head(regs);
> > >> -
> > >>       switch ( hsr.ec )
> > >>       {
> > >>       case HSR_EC_WFI_WFE:
> > >> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
> > >>   {
> > >>       const union hsr hsr = { .bits = regs->hsr };
> > >>   
> > >> -    enter_hypervisor_head(regs);
> > >> -
> > >>       switch ( hsr.ec )
> > >>       {
> > >>   #ifdef CONFIG_ARM_64
> > >> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
> > >>   
> > >>   void do_trap_hyp_serror(struct cpu_user_regs *regs)
> > >>   {
> > >> -    enter_hypervisor_head(regs);
> > >> -
> > >>       __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
> > >>   }
> > >>   
> > >>   void do_trap_guest_serror(struct cpu_user_regs *regs)
> > >>   {
> > >> -    enter_hypervisor_head(regs);
> > >> -
> > >>       __do_trap_serror(regs, true);
> > >>   }
> > >>   
> > >>   void do_trap_irq(struct cpu_user_regs *regs)
> > >>   {
> > >> -    enter_hypervisor_head(regs);
> > >>       gic_interrupt(regs, 0);
> > >>   }
> > >>   
> > >>   void do_trap_fiq(struct cpu_user_regs *regs)
> > >>   {
> > >> -    enter_hypervisor_head(regs);
> > >>       gic_interrupt(regs, 1);
> > >>   }
> > > 
> > > I am OK with the general approach but one thing to note is that the fiq
> > > handler doesn't use the guest_vector macro at the moment.
> > 
> > do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode(). 
> > So I don't see an issue here.
> > 
> > As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler 
> > does not use guest_vector.
> > 
> > That said, it is interesting to see that we don't deal the same way the 
> > FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the 
> > latter will crash the guest.
> > 
> > It would be good if we can have the same behavior accross the two arch 
> > if possible. I vaguely recall someone (Andre?) mentioning some changes 
> > around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?
> > 
> > Also, a side effect of not calling enter_hypervisor_head() is workaround 
> > are not re-enabled. We are going to panic soon after, so it may not be 
> > that much an issue.
> 
> Right, that is what I was thinking too, but I wanted to highlight it. At
> least it would be worth adding a sentence to the commit message about
> it.

Actually on second thought, maybe we have to apply the workaround anyway
to identify/spot that the guest somehow managed to trigger a serror? I
mean: maybe it is important enough that we should let the user know.
Julien Grall Oct. 2, 2019, 12:47 p.m. | #11
Hi,

On 10/2/19 1:41 PM, Stefano Stabellini wrote:
> On Tue, 1 Oct 2019, Stefano Stabellini wrote:
>> On Tue, 1 Oct 2019, Julien Grall wrote:
>>> Hi,
>>>
>>> On 01/10/2019 21:12, Stefano Stabellini wrote:
>>>> On Thu, 26 Sep 2019, Julien Grall wrote:
>>>>> At the moment, enter_hypervisor_head() and leave_hypervisor_tail() are
>>>>> used to deal with actions to be done before/after any guest request is
>>>>> handled.
>>>>>
>>>>> While they are meant to work in pair, the former is called for most of
>>>>> the traps, including traps from the same exception level (i.e.
>>>>> hypervisor) whilst the latter will only be called when returning to the
>>>>> guest.
>>>>>
>>>>> As pointed out, the enter_hypervisor_head() is not called from all the
>>>>> traps, so this makes potentially difficult to extend it for the dealing
>>>>> with same exception level.
>>>>>
>>>>> Furthermore, some assembly only path will require to call
>>>>> enter_hypervisor_tail(). So the function is now directly call by
>>>>> assembly in for guest vector only. This means that the check whether we
>>>>> are called in a guest trap can now be removed.
>>>>>
>>>>> Take the opportunity to rename enter_hypervisor_tail() and
>>>>> leave_hypervisor_tail() to something more meaningful and document them.
>>>>> This should help everyone to understand the purpose of the two
>>>>> functions.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>
>>>>> ---
>>>>>
>>>>> I haven't done the 32-bits part yet. I wanted to gather feedback before
>>>>> looking in details how to integrate that with Arm32.
>>>>> ---
>>>>>    xen/arch/arm/arm64/entry.S |  4 ++-
>>>>>    xen/arch/arm/traps.c       | 71 ++++++++++++++++++++++------------------------
>>>>>    2 files changed, 37 insertions(+), 38 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
>>>>> index 40d9f3ec8c..9eafae516b 100644
>>>>> --- a/xen/arch/arm/arm64/entry.S
>>>>> +++ b/xen/arch/arm/arm64/entry.S
>>>>> @@ -147,7 +147,7 @@
>>>>>    
>>>>>            .if \hyp == 0         /* Guest mode */
>>>>>    
>>>>> -        bl      leave_hypervisor_tail /* Disables interrupts on return */
>>>>> +        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
>>>>>    
>>>>>            exit_guest \compat
>>>>>    
>>>>> @@ -175,6 +175,8 @@
>>>>>                        SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
>>>>>            msr     daifclr, \iflags
>>>>>            mov     x0, sp
>>>>> +        bl      enter_hypervisor_from_guest
>>>>> +        mov     x0, sp
>>>>>            bl      do_trap_\trap
>>>>>    1:
>>>>>            exit    hyp=0, compat=\compat
>>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>>> index a3b961bd06..20ba34ec91 100644
>>>>> --- a/xen/arch/arm/traps.c
>>>>> +++ b/xen/arch/arm/traps.c
>>>>> @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
>>>>>                 cpu_require_ssbd_mitigation();
>>>>>    }
>>>>>    
>>>>> -static void enter_hypervisor_head(struct cpu_user_regs *regs)
>>>>> +/*
>>>>> + * Actions that needs to be done after exiting the guest and before any
>>>>> + * request from it is handled.
>>>>> + */
>>>>> +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
>>>>>    {
>>>>> -    if ( guest_mode(regs) )
>>>>> -    {
>>>>> -        struct vcpu *v = current;
>>>>> +    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);
>>>>> +    /* 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);
>>>>>    
>>>>> -        /*
>>>>> -         * If we pended a virtual abort, preserve it until it gets cleared.
>>>>> -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
>>>>> -         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
>>>>> -         * (alias of HCR.VA) is cleared to 0."
>>>>> -         */
>>>>> -        if ( v->arch.hcr_el2 & HCR_VA )
>>>>> -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>>>>> +    /*
>>>>> +     * If we pended a virtual abort, preserve it until it gets cleared.
>>>>> +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
>>>>> +     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
>>>>> +     * (alias of HCR.VA) is cleared to 0."
>>>>> +     */
>>>>> +    if ( v->arch.hcr_el2 & HCR_VA )
>>>>> +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
>>>>>    
>>>>>    #ifdef CONFIG_NEW_VGIC
>>>>> -        /*
>>>>> -         * We need to update the state of our emulated devices using level
>>>>> -         * triggered interrupts before syncing back the VGIC state.
>>>>> -         *
>>>>> -         * TODO: Investigate whether this is necessary to do on every
>>>>> -         * trap and how it can be optimised.
>>>>> -         */
>>>>> -        vtimer_update_irqs(v);
>>>>> -        vcpu_update_evtchn_irq(v);
>>>>> +    /*
>>>>> +     * We need to update the state of our emulated devices using level
>>>>> +     * triggered interrupts before syncing back the VGIC state.
>>>>> +     *
>>>>> +     * TODO: Investigate whether this is necessary to do on every
>>>>> +     * trap and how it can be optimised.
>>>>> +     */
>>>>> +    vtimer_update_irqs(v);
>>>>> +    vcpu_update_evtchn_irq(v);
>>>>>    #endif
>>>>>    
>>>>> -        vgic_sync_from_lrs(v);
>>>>> -    }
>>>>> +    vgic_sync_from_lrs(v);
>>>>>    }
>>>>>    
>>>>>    void do_trap_guest_sync(struct cpu_user_regs *regs)
>>>>>    {
>>>>>        const union hsr hsr = { .bits = regs->hsr };
>>>>>    
>>>>> -    enter_hypervisor_head(regs);
>>>>> -
>>>>>        switch ( hsr.ec )
>>>>>        {
>>>>>        case HSR_EC_WFI_WFE:
>>>>> @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>>>>>    {
>>>>>        const union hsr hsr = { .bits = regs->hsr };
>>>>>    
>>>>> -    enter_hypervisor_head(regs);
>>>>> -
>>>>>        switch ( hsr.ec )
>>>>>        {
>>>>>    #ifdef CONFIG_ARM_64
>>>>> @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>>>>>    
>>>>>    void do_trap_hyp_serror(struct cpu_user_regs *regs)
>>>>>    {
>>>>> -    enter_hypervisor_head(regs);
>>>>> -
>>>>>        __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
>>>>>    }
>>>>>    
>>>>>    void do_trap_guest_serror(struct cpu_user_regs *regs)
>>>>>    {
>>>>> -    enter_hypervisor_head(regs);
>>>>> -
>>>>>        __do_trap_serror(regs, true);
>>>>>    }
>>>>>    
>>>>>    void do_trap_irq(struct cpu_user_regs *regs)
>>>>>    {
>>>>> -    enter_hypervisor_head(regs);
>>>>>        gic_interrupt(regs, 0);
>>>>>    }
>>>>>    
>>>>>    void do_trap_fiq(struct cpu_user_regs *regs)
>>>>>    {
>>>>> -    enter_hypervisor_head(regs);
>>>>>        gic_interrupt(regs, 1);
>>>>>    }
>>>>
>>>> I am OK with the general approach but one thing to note is that the fiq
>>>> handler doesn't use the guest_vector macro at the moment.
>>>
>>> do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode().
>>> So I don't see an issue here.
>>>
>>> As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler
>>> does not use guest_vector.
>>>
>>> That said, it is interesting to see that we don't deal the same way the
>>> FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the
>>> latter will crash the guest.
>>>
>>> It would be good if we can have the same behavior accross the two arch
>>> if possible. I vaguely recall someone (Andre?) mentioning some changes
>>> around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?
>>>
>>> Also, a side effect of not calling enter_hypervisor_head() is workaround
>>> are not re-enabled. We are going to panic soon after, so it may not be
>>> that much an issue.
>>
>> Right, that is what I was thinking too, but I wanted to highlight it. At
>> least it would be worth adding a sentence to the commit message about
>> it.
> 
> Actually on second thought, maybe we have to apply the workaround anyway
> to identify/spot that the guest somehow managed to trigger a serror? I
> mean: maybe it is important enough that we should let the user know.

I am sorry but I don't understand how this is related to this patch. 
There are strictly no change in the SError handling here.

Cheers,
Stefano Stabellini Oct. 2, 2019, 10:26 p.m. | #12
On Wed, 2 Oct 2019, Julien Grall wrote:
> Hi,
> 
> On 10/2/19 1:41 PM, Stefano Stabellini wrote:
> > On Tue, 1 Oct 2019, Stefano Stabellini wrote:
> > > On Tue, 1 Oct 2019, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 01/10/2019 21:12, Stefano Stabellini wrote:
> > > > > On Thu, 26 Sep 2019, Julien Grall wrote:
> > > > > > At the moment, enter_hypervisor_head() and leave_hypervisor_tail()
> > > > > > are
> > > > > > used to deal with actions to be done before/after any guest request
> > > > > > is
> > > > > > handled.
> > > > > > 
> > > > > > While they are meant to work in pair, the former is called for most
> > > > > > of
> > > > > > the traps, including traps from the same exception level (i.e.
> > > > > > hypervisor) whilst the latter will only be called when returning to
> > > > > > the
> > > > > > guest.
> > > > > > 
> > > > > > As pointed out, the enter_hypervisor_head() is not called from all
> > > > > > the
> > > > > > traps, so this makes potentially difficult to extend it for the
> > > > > > dealing
> > > > > > with same exception level.
> > > > > > 
> > > > > > Furthermore, some assembly only path will require to call
> > > > > > enter_hypervisor_tail(). So the function is now directly call by
> > > > > > assembly in for guest vector only. This means that the check whether
> > > > > > we
> > > > > > are called in a guest trap can now be removed.
> > > > > > 
> > > > > > Take the opportunity to rename enter_hypervisor_tail() and
> > > > > > leave_hypervisor_tail() to something more meaningful and document
> > > > > > them.
> > > > > > This should help everyone to understand the purpose of the two
> > > > > > functions.
> > > > > > 
> > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > > 
> > > > > > ---
> > > > > > 
> > > > > > I haven't done the 32-bits part yet. I wanted to gather feedback
> > > > > > before
> > > > > > looking in details how to integrate that with Arm32.
> > > > > > ---
> > > > > >    xen/arch/arm/arm64/entry.S |  4 ++-
> > > > > >    xen/arch/arm/traps.c       | 71
> > > > > > ++++++++++++++++++++++------------------------
> > > > > >    2 files changed, 37 insertions(+), 38 deletions(-)
> > > > > > 
> > > > > > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> > > > > > index 40d9f3ec8c..9eafae516b 100644
> > > > > > --- a/xen/arch/arm/arm64/entry.S
> > > > > > +++ b/xen/arch/arm/arm64/entry.S
> > > > > > @@ -147,7 +147,7 @@
> > > > > >               .if \hyp == 0         /* Guest mode */
> > > > > >    -        bl      leave_hypervisor_tail /* Disables interrupts on
> > > > > > return */
> > > > > > +        bl      leave_hypervisor_to_guest /* Disables interrupts on
> > > > > > return */
> > > > > >               exit_guest \compat
> > > > > >    @@ -175,6 +175,8 @@
> > > > > >                        SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
> > > > > >            msr     daifclr, \iflags
> > > > > >            mov     x0, sp
> > > > > > +        bl      enter_hypervisor_from_guest
> > > > > > +        mov     x0, sp
> > > > > >            bl      do_trap_\trap
> > > > > >    1:
> > > > > >            exit    hyp=0, compat=\compat
> > > > > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > > > > index a3b961bd06..20ba34ec91 100644
> > > > > > --- a/xen/arch/arm/traps.c
> > > > > > +++ b/xen/arch/arm/traps.c
> > > > > > @@ -2006,47 +2006,46 @@ static inline bool needs_ssbd_flip(struct
> > > > > > vcpu *v)
> > > > > >                 cpu_require_ssbd_mitigation();
> > > > > >    }
> > > > > >    -static void enter_hypervisor_head(struct cpu_user_regs *regs)
> > > > > > +/*
> > > > > > + * Actions that needs to be done after exiting the guest and before
> > > > > > any
> > > > > > + * request from it is handled.
> > > > > > + */
> > > > > > +void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    if ( guest_mode(regs) )
> > > > > > -    {
> > > > > > -        struct vcpu *v = current;
> > > > > > +    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);
> > > > > > +    /* 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);
> > > > > >    -        /*
> > > > > > -         * If we pended a virtual abort, preserve it until it gets
> > > > > > cleared.
> > > > > > -         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for
> > > > > > details,
> > > > > > -         * but the crucial bit is "On taking a vSError interrupt,
> > > > > > HCR_EL2.VSE
> > > > > > -         * (alias of HCR.VA) is cleared to 0."
> > > > > > -         */
> > > > > > -        if ( v->arch.hcr_el2 & HCR_VA )
> > > > > > -            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> > > > > > +    /*
> > > > > > +     * If we pended a virtual abort, preserve it until it gets
> > > > > > cleared.
> > > > > > +     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for
> > > > > > details,
> > > > > > +     * but the crucial bit is "On taking a vSError interrupt,
> > > > > > HCR_EL2.VSE
> > > > > > +     * (alias of HCR.VA) is cleared to 0."
> > > > > > +     */
> > > > > > +    if ( v->arch.hcr_el2 & HCR_VA )
> > > > > > +        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
> > > > > >       #ifdef CONFIG_NEW_VGIC
> > > > > > -        /*
> > > > > > -         * We need to update the state of our emulated devices
> > > > > > using level
> > > > > > -         * triggered interrupts before syncing back the VGIC state.
> > > > > > -         *
> > > > > > -         * TODO: Investigate whether this is necessary to do on
> > > > > > every
> > > > > > -         * trap and how it can be optimised.
> > > > > > -         */
> > > > > > -        vtimer_update_irqs(v);
> > > > > > -        vcpu_update_evtchn_irq(v);
> > > > > > +    /*
> > > > > > +     * We need to update the state of our emulated devices using
> > > > > > level
> > > > > > +     * triggered interrupts before syncing back the VGIC state.
> > > > > > +     *
> > > > > > +     * TODO: Investigate whether this is necessary to do on every
> > > > > > +     * trap and how it can be optimised.
> > > > > > +     */
> > > > > > +    vtimer_update_irqs(v);
> > > > > > +    vcpu_update_evtchn_irq(v);
> > > > > >    #endif
> > > > > >    -        vgic_sync_from_lrs(v);
> > > > > > -    }
> > > > > > +    vgic_sync_from_lrs(v);
> > > > > >    }
> > > > > >       void do_trap_guest_sync(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > >        const union hsr hsr = { .bits = regs->hsr };
> > > > > >    -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        switch ( hsr.ec )
> > > > > >        {
> > > > > >        case HSR_EC_WFI_WFE:
> > > > > > @@ -2180,8 +2179,6 @@ void do_trap_hyp_sync(struct cpu_user_regs
> > > > > > *regs)
> > > > > >    {
> > > > > >        const union hsr hsr = { .bits = regs->hsr };
> > > > > >    -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        switch ( hsr.ec )
> > > > > >        {
> > > > > >    #ifdef CONFIG_ARM_64
> > > > > > @@ -2218,27 +2215,21 @@ void do_trap_hyp_sync(struct cpu_user_regs
> > > > > > *regs)
> > > > > >       void do_trap_hyp_serror(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
> > > > > >    }
> > > > > >       void do_trap_guest_serror(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > > -
> > > > > >        __do_trap_serror(regs, true);
> > > > > >    }
> > > > > >       void do_trap_irq(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > >        gic_interrupt(regs, 0);
> > > > > >    }
> > > > > >       void do_trap_fiq(struct cpu_user_regs *regs)
> > > > > >    {
> > > > > > -    enter_hypervisor_head(regs);
> > > > > >        gic_interrupt(regs, 1);
> > > > > >    }
> > > > > 
> > > > > I am OK with the general approach but one thing to note is that the
> > > > > fiq
> > > > > handler doesn't use the guest_vector macro at the moment.
> > > > 
> > > > do_trap_fiq() is not called from arm64 Instead, we call do_bad_mode().
> > > > So I don't see an issue here.
> > > > 
> > > > As do_bad_mode() does not call enter_hypervisor_head(), the fiq handler
> > > > does not use guest_vector.
> > > > 
> > > > That said, it is interesting to see that we don't deal the same way the
> > > > FIQ on Arm32 and Arm64. On the former, we will call do_IRQ while the
> > > > latter will crash the guest.
> > > > 
> > > > It would be good if we can have the same behavior accross the two arch
> > > > if possible. I vaguely recall someone (Andre?) mentioning some changes
> > > > around FIQ in KVM recently. Andre, are FIQ meant to work with Guest?
> > > > 
> > > > Also, a side effect of not calling enter_hypervisor_head() is workaround
> > > > are not re-enabled. We are going to panic soon after, so it may not be
> > > > that much an issue.
> > > 
> > > Right, that is what I was thinking too, but I wanted to highlight it. At
> > > least it would be worth adding a sentence to the commit message about
> > > it.
> > 
> > Actually on second thought, maybe we have to apply the workaround anyway
> > to identify/spot that the guest somehow managed to trigger a serror? I
> > mean: maybe it is important enough that we should let the user know.
> 
> I am sorry but I don't understand how this is related to this patch. There are
> strictly no change in the SError handling here.

That was my reflection on whether it would be a good idea or a bad idea
to have a SERROR check on the fiq hypervisor entries. Not necessarely in
this patch. Probably not in this patch.
Julien Grall Oct. 3, 2019, 10:24 a.m. | #13
Hi Stefano,

On 02/10/2019 23:26, Stefano Stabellini wrote:
> On Wed, 2 Oct 2019, Julien Grall wrote:
> That was my reflection on whether it would be a good idea or a bad idea
> to have a SERROR check on the fiq hypervisor entries. Not necessarely in
> this patch. Probably not in this patch.

If you receive a FIQ exception on Arm64, then you are already doomed because the 
hypervisor will crash (see do_bad_mode()). So receiving the SError is going to 
be your last concern here.

Cheers,
Stefano Stabellini Oct. 3, 2019, 5:48 p.m. | #14
On Thu, 3 Oct 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 02/10/2019 23:26, Stefano Stabellini wrote:
> > On Wed, 2 Oct 2019, Julien Grall wrote:
> > That was my reflection on whether it would be a good idea or a bad idea
> > to have a SERROR check on the fiq hypervisor entries. Not necessarely in
> > this patch. Probably not in this patch.
> 
> If you receive a FIQ exception on Arm64, then you are already doomed because
> the hypervisor will crash (see do_bad_mode()). So receiving the SError is
> going to be your last concern here.

I realize that Xen is doomed anyway, but if I was the user, I would
still want to know about the SError: it is not going to save the
platform in any way but it might make me realize there is something
wrong with the guest configuration (in addition to the FIQ problem.)

But because there is no way to get a FIQ in Xen, at least on paper, this
is kind of a theoretical exercise.
Julien Grall Oct. 3, 2019, 5:53 p.m. | #15
Hi,

On 03/10/2019 18:48, Stefano Stabellini wrote:
> On Thu, 3 Oct 2019, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 02/10/2019 23:26, Stefano Stabellini wrote:
>>> On Wed, 2 Oct 2019, Julien Grall wrote:
>>> That was my reflection on whether it would be a good idea or a bad idea
>>> to have a SERROR check on the fiq hypervisor entries. Not necessarely in
>>> this patch. Probably not in this patch.
>>
>> If you receive a FIQ exception on Arm64, then you are already doomed because
>> the hypervisor will crash (see do_bad_mode()). So receiving the SError is
>> going to be your last concern here.
> 
> I realize that Xen is doomed anyway, but if I was the user, I would
> still want to know about the SError: it is not going to save the
> platform in any way but it might make me realize there is something
> wrong with the guest configuration (in addition to the FIQ problem.)

This is not something I am going to look at in the near future. There are more 
concerning problem in arch/arm*/entry.S. Although, patches are welcomed.

Cheers,

Patch

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 40d9f3ec8c..9eafae516b 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -147,7 +147,7 @@ 
 
         .if \hyp == 0         /* Guest mode */
 
-        bl      leave_hypervisor_tail /* Disables interrupts on return */
+        bl      leave_hypervisor_to_guest /* Disables interrupts on return */
 
         exit_guest \compat
 
@@ -175,6 +175,8 @@ 
                     SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT)
         msr     daifclr, \iflags
         mov     x0, sp
+        bl      enter_hypervisor_from_guest
+        mov     x0, sp
         bl      do_trap_\trap
 1:
         exit    hyp=0, compat=\compat
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index a3b961bd06..20ba34ec91 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2006,47 +2006,46 @@  static inline bool needs_ssbd_flip(struct vcpu *v)
              cpu_require_ssbd_mitigation();
 }
 
-static void enter_hypervisor_head(struct cpu_user_regs *regs)
+/*
+ * Actions that needs to be done after exiting the guest and before any
+ * request from it is handled.
+ */
+void enter_hypervisor_from_guest(struct cpu_user_regs *regs)
 {
-    if ( guest_mode(regs) )
-    {
-        struct vcpu *v = current;
+    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);
+    /* 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);
 
-        /*
-         * If we pended a virtual abort, preserve it until it gets cleared.
-         * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
-         * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
-         * (alias of HCR.VA) is cleared to 0."
-         */
-        if ( v->arch.hcr_el2 & HCR_VA )
-            v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
+    /*
+     * If we pended a virtual abort, preserve it until it gets cleared.
+     * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
+     * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
+     * (alias of HCR.VA) is cleared to 0."
+     */
+    if ( v->arch.hcr_el2 & HCR_VA )
+        v->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
 
 #ifdef CONFIG_NEW_VGIC
-        /*
-         * We need to update the state of our emulated devices using level
-         * triggered interrupts before syncing back the VGIC state.
-         *
-         * TODO: Investigate whether this is necessary to do on every
-         * trap and how it can be optimised.
-         */
-        vtimer_update_irqs(v);
-        vcpu_update_evtchn_irq(v);
+    /*
+     * We need to update the state of our emulated devices using level
+     * triggered interrupts before syncing back the VGIC state.
+     *
+     * TODO: Investigate whether this is necessary to do on every
+     * trap and how it can be optimised.
+     */
+    vtimer_update_irqs(v);
+    vcpu_update_evtchn_irq(v);
 #endif
 
-        vgic_sync_from_lrs(v);
-    }
+    vgic_sync_from_lrs(v);
 }
 
 void do_trap_guest_sync(struct cpu_user_regs *regs)
 {
     const union hsr hsr = { .bits = regs->hsr };
 
-    enter_hypervisor_head(regs);
-
     switch ( hsr.ec )
     {
     case HSR_EC_WFI_WFE:
@@ -2180,8 +2179,6 @@  void do_trap_hyp_sync(struct cpu_user_regs *regs)
 {
     const union hsr hsr = { .bits = regs->hsr };
 
-    enter_hypervisor_head(regs);
-
     switch ( hsr.ec )
     {
 #ifdef CONFIG_ARM_64
@@ -2218,27 +2215,21 @@  void do_trap_hyp_sync(struct cpu_user_regs *regs)
 
 void do_trap_hyp_serror(struct cpu_user_regs *regs)
 {
-    enter_hypervisor_head(regs);
-
     __do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
 }
 
 void do_trap_guest_serror(struct cpu_user_regs *regs)
 {
-    enter_hypervisor_head(regs);
-
     __do_trap_serror(regs, true);
 }
 
 void do_trap_irq(struct cpu_user_regs *regs)
 {
-    enter_hypervisor_head(regs);
     gic_interrupt(regs, 0);
 }
 
 void do_trap_fiq(struct cpu_user_regs *regs)
 {
-    enter_hypervisor_head(regs);
     gic_interrupt(regs, 1);
 }
 
@@ -2281,7 +2272,13 @@  static void check_for_vcpu_work(void)
     local_irq_disable();
 }
 
-void leave_hypervisor_tail(void)
+/*
+ * Actions that needs to be done before entering the guest. This is the
+ * last thing executed before the guest context is fully restored.
+ *
+ * The function will return with interrupts disabled.
+ */
+void leave_hypervisor_to_guest(void)
 {
     local_irq_disable();