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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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

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