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.gr...@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. -- Volodymyr Babchuk at EPAM _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
