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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

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.

[...]

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