| Message ID | 20190926183808.11630-10-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: XSA-201 and XSA-263 fixes | expand |
Julien Grall writes: > Using alternative_if makes the code a bit more streamlined. > > Take the opportunity to use the new auto-nop infrastructure to avoid > counting the number of nop in the else part for arch/arm/arm64/entry.S > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > This is pretty much a matter of taste, but at least for arm64 this > allows us to use the auto-nop infrastructure. So the arm32 is more > to keep inline with arm64. > --- > xen/arch/arm/arm32/entry.S | 9 ++++++--- > xen/arch/arm/arm64/entry.S | 8 +++++--- > 2 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S > index 0b4cd19abd..1428cd3583 100644 > --- a/xen/arch/arm/arm32/entry.S > +++ b/xen/arch/arm/arm32/entry.S > @@ -65,9 +65,12 @@ save_guest_regs: > * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu > * feature, the checking of pending SErrors will be skipped. > */ > - ALTERNATIVE("nop", > - "b skip_check", > - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > + nop > + alternative_else > + b skip_check > + alternative_endif > + for the arm32 code you can have my r-b: Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > /* > * Start to check pending virtual abort in the gap of Guest -> HYP > * world switch. > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 458d12f188..91cf6ee6f4 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -170,9 +170,11 @@ > * is not set. If a vSError took place, the initial exception will be > * skipped. Exit ASAP > */ > - ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", > - "nop; nop", > - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > + bl check_pending_vserror > + cbnz x0, 1f > + alternative_else_nop_endif > + You asked other people to do not introduce new code in one patch and rewrite it in the following patch. But there you are doing exactly the same. I believe, it is possible to move all "alternative" patches to the very beginning of the patch series and only then introduce macro guest_vector. > mov x0, sp > bl enter_hypervisor_from_guest_noirq > msr daifclr, \iflags -- Volodymyr Babchuk at EPAM
Hi, On 27/09/2019 13:11, Volodymyr Babchuk wrote: > > > Julien Grall writes: > >> Using alternative_if makes the code a bit more streamlined. >> >> Take the opportunity to use the new auto-nop infrastructure to avoid >> counting the number of nop in the else part for arch/arm/arm64/entry.S >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> This is pretty much a matter of taste, but at least for arm64 this >> allows us to use the auto-nop infrastructure. So the arm32 is more >> to keep inline with arm64. >> --- >> xen/arch/arm/arm32/entry.S | 9 ++++++--- >> xen/arch/arm/arm64/entry.S | 8 +++++--- >> 2 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S >> index 0b4cd19abd..1428cd3583 100644 >> --- a/xen/arch/arm/arm32/entry.S >> +++ b/xen/arch/arm/arm32/entry.S >> @@ -65,9 +65,12 @@ save_guest_regs: >> * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu >> * feature, the checking of pending SErrors will be skipped. >> */ >> - ALTERNATIVE("nop", >> - "b skip_check", >> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >> + nop >> + alternative_else >> + b skip_check >> + alternative_endif >> + > for the arm32 code you can have my r-b: > Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com> > >> /* >> * Start to check pending virtual abort in the gap of Guest -> HYP >> * world switch. >> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S >> index 458d12f188..91cf6ee6f4 100644 >> --- a/xen/arch/arm/arm64/entry.S >> +++ b/xen/arch/arm/arm64/entry.S >> @@ -170,9 +170,11 @@ >> * is not set. If a vSError took place, the initial exception will be >> * skipped. Exit ASAP >> */ >> - ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", >> - "nop; nop", >> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >> + bl check_pending_vserror >> + cbnz x0, 1f >> + alternative_else_nop_endif >> + > You asked other people to do not introduce new code in one patch and > rewrite it in the following patch. But there you are doing exactly the > same. This is a fairly borderline comment knowing that I usually don't request clean-up and code consolidation in the same patch. > I believe, it is possible to move all "alternative" patches to the > very beginning of the patch series and only then introduce macro > guest_vector. For a first, the first patch is definitely not new code. This is code consolidation and therefore I don't tend to mix the two for clarity. So this should have been a patch before the first patch. Secondly, the first 4 patches are candidate for backport. The rest of the series would be good to backport but I am not aware of a critical issue in previous Xen release to strongly push for it. Cheers,
Julien Grall writes: > Hi, > > On 27/09/2019 13:11, Volodymyr Babchuk wrote: >> >> >> Julien Grall writes: >> >>> Using alternative_if makes the code a bit more streamlined. >>> >>> Take the opportunity to use the new auto-nop infrastructure to avoid >>> counting the number of nop in the else part for arch/arm/arm64/entry.S >>> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> >>> --- >>> This is pretty much a matter of taste, but at least for arm64 this >>> allows us to use the auto-nop infrastructure. So the arm32 is more >>> to keep inline with arm64. >>> --- >>> xen/arch/arm/arm32/entry.S | 9 ++++++--- >>> xen/arch/arm/arm64/entry.S | 8 +++++--- >>> 2 files changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S >>> index 0b4cd19abd..1428cd3583 100644 >>> --- a/xen/arch/arm/arm32/entry.S >>> +++ b/xen/arch/arm/arm32/entry.S >>> @@ -65,9 +65,12 @@ save_guest_regs: >>> * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu >>> * feature, the checking of pending SErrors will be skipped. >>> */ >>> - ALTERNATIVE("nop", >>> - "b skip_check", >>> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >>> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >>> + nop >>> + alternative_else >>> + b skip_check >>> + alternative_endif >>> + >> for the arm32 code you can have my r-b: >> Reviewed-By: Volodymyr Babchuk <volodymyr_babchuk@epam.com> >> >>> /* >>> * Start to check pending virtual abort in the gap of Guest -> HYP >>> * world switch. >>> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S >>> index 458d12f188..91cf6ee6f4 100644 >>> --- a/xen/arch/arm/arm64/entry.S >>> +++ b/xen/arch/arm/arm64/entry.S >>> @@ -170,9 +170,11 @@ >>> * is not set. If a vSError took place, the initial exception will be >>> * skipped. Exit ASAP >>> */ >>> - ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", >>> - "nop; nop", >>> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >>> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >>> + bl check_pending_vserror >>> + cbnz x0, 1f >>> + alternative_else_nop_endif >>> + >> You asked other people to do not introduce new code in one patch and >> rewrite it in the following patch. But there you are doing exactly the >> same. > > This is a fairly borderline comment knowing that I usually don't > request clean-up and code consolidation in the same patch. I understand this. Also I understand why are you asking for clean-up. No one likes to review the same code twice. Anyways, I not wanted to be offensive. Sorry for that. >> I believe, it is possible to move all "alternative" patches to the >> very beginning of the patch series and only then introduce macro >> guest_vector. > > For a first, the first patch is definitely not new code. This is code > consolidation and therefore I don't tend to mix the two for > clarity. So this should have been a patch before the first patch. > > Secondly, the first 4 patches are candidate for backport. The rest of > the series would be good to backport but I am not aware of a critical > issue in previous Xen release to strongly push for it. I see. Yes, I'm always forgetting about backporting :( So, for the rest of the patch: Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
On Thu, 26 Sep 2019, Julien Grall wrote: > Using alternative_if makes the code a bit more streamlined. > > Take the opportunity to use the new auto-nop infrastructure to avoid > counting the number of nop in the else part for arch/arm/arm64/entry.S > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > This is pretty much a matter of taste, but at least for arm64 this > allows us to use the auto-nop infrastructure. So the arm32 is more > to keep inline with arm64. > --- > xen/arch/arm/arm32/entry.S | 9 ++++++--- > xen/arch/arm/arm64/entry.S | 8 +++++--- > 2 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S > index 0b4cd19abd..1428cd3583 100644 > --- a/xen/arch/arm/arm32/entry.S > +++ b/xen/arch/arm/arm32/entry.S > @@ -65,9 +65,12 @@ save_guest_regs: > * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu > * feature, the checking of pending SErrors will be skipped. > */ > - ALTERNATIVE("nop", > - "b skip_check", > - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > + nop > + alternative_else > + b skip_check > + alternative_endif This could be written as: alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT b skip_check alternative_else_nop_endif > /* > * Start to check pending virtual abort in the gap of Guest -> HYP > * world switch. > diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > index 458d12f188..91cf6ee6f4 100644 > --- a/xen/arch/arm/arm64/entry.S > +++ b/xen/arch/arm/arm64/entry.S > @@ -170,9 +170,11 @@ > * is not set. If a vSError took place, the initial exception will be > * skipped. Exit ASAP > */ > - ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", > - "nop; nop", > - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > + bl check_pending_vserror > + cbnz x0, 1f > + alternative_else_nop_endif > + > mov x0, sp > bl enter_hypervisor_from_guest_noirq > msr daifclr, \iflags > -- > 2.11.0 >
Hi Stefano, On 01/10/2019 23:19, Stefano Stabellini wrote: > On Thu, 26 Sep 2019, Julien Grall wrote: >> Using alternative_if makes the code a bit more streamlined. >> >> Take the opportunity to use the new auto-nop infrastructure to avoid >> counting the number of nop in the else part for arch/arm/arm64/entry.S >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> This is pretty much a matter of taste, but at least for arm64 this >> allows us to use the auto-nop infrastructure. So the arm32 is more >> to keep inline with arm64. >> --- >> xen/arch/arm/arm32/entry.S | 9 ++++++--- >> xen/arch/arm/arm64/entry.S | 8 +++++--- >> 2 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S >> index 0b4cd19abd..1428cd3583 100644 >> --- a/xen/arch/arm/arm32/entry.S >> +++ b/xen/arch/arm/arm32/entry.S >> @@ -65,9 +65,12 @@ save_guest_regs: >> * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu >> * feature, the checking of pending SErrors will be skipped. >> */ >> - ALTERNATIVE("nop", >> - "b skip_check", >> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT >> + nop >> + alternative_else >> + b skip_check >> + alternative_endif > > This could be written as: > > alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > b skip_check > alternative_else_nop_endif Actually my implementation is wrong :/. We want to skip the check if the cap is set. So this should be: alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT b skip_check alternative_else_nop_endif > > >> /* >> * Start to check pending virtual abort in the gap of Guest -> HYP >> * world switch. >> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S >> index 458d12f188..91cf6ee6f4 100644 >> --- a/xen/arch/arm/arm64/entry.S >> +++ b/xen/arch/arm/arm64/entry.S >> @@ -170,9 +170,11 @@ >> * is not set. If a vSError took place, the initial exception will be >> * skipped. Exit ASAP >> */ >> - ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", >> - "nop; nop", >> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) >> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT This would need to be alternative_if_not as want to call the function when the cap is not set. >> + bl check_pending_vserror >> + cbnz x0, 1f >> + alternative_else_nop_endif >> + >> mov x0, sp >> bl enter_hypervisor_from_guest_noirq >> msr daifclr, \iflags >> -- >> 2.11.0 >> Cheeers, -- Julien Grall
On Tue, 1 Oct 2019, Julien Grall wrote: > Hi Stefano, > > On 01/10/2019 23:19, Stefano Stabellini wrote: > > On Thu, 26 Sep 2019, Julien Grall wrote: > >> Using alternative_if makes the code a bit more streamlined. > >> > >> Take the opportunity to use the new auto-nop infrastructure to avoid > >> counting the number of nop in the else part for arch/arm/arm64/entry.S > >> > >> Signed-off-by: Julien Grall <julien.grall@arm.com> > >> > >> --- > >> This is pretty much a matter of taste, but at least for arm64 this > >> allows us to use the auto-nop infrastructure. So the arm32 is more > >> to keep inline with arm64. > >> --- > >> xen/arch/arm/arm32/entry.S | 9 ++++++--- > >> xen/arch/arm/arm64/entry.S | 8 +++++--- > >> 2 files changed, 11 insertions(+), 6 deletions(-) > >> > >> diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S > >> index 0b4cd19abd..1428cd3583 100644 > >> --- a/xen/arch/arm/arm32/entry.S > >> +++ b/xen/arch/arm/arm32/entry.S > >> @@ -65,9 +65,12 @@ save_guest_regs: > >> * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu > >> * feature, the checking of pending SErrors will be skipped. > >> */ > >> - ALTERNATIVE("nop", > >> - "b skip_check", > >> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > >> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > >> + nop > >> + alternative_else > >> + b skip_check > >> + alternative_endif > > > > This could be written as: > > > > alternative_if_not SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > > b skip_check > > alternative_else_nop_endif > > Actually my implementation is wrong :/. We want to skip the check if the > cap is set. So this should be: > > alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > b skip_check > alternative_else_nop_endif Ah, yes of course, like the name suggests > > > >> /* > >> * Start to check pending virtual abort in the gap of Guest -> HYP > >> * world switch. > >> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S > >> index 458d12f188..91cf6ee6f4 100644 > >> --- a/xen/arch/arm/arm64/entry.S > >> +++ b/xen/arch/arm/arm64/entry.S > >> @@ -170,9 +170,11 @@ > >> * is not set. If a vSError took place, the initial exception will be > >> * skipped. Exit ASAP > >> */ > >> - ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", > >> - "nop; nop", > >> - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) > >> + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT > > This would need to be alternative_if_not as want to call the function > when the cap is not set. Yep > >> + bl check_pending_vserror > >> + cbnz x0, 1f > >> + alternative_else_nop_endif > >> + > >> mov x0, sp > >> bl enter_hypervisor_from_guest_noirq > >> msr daifclr, \iflags > >> -- > >> 2.11.0 > >> > > Cheeers, > > -- > Julien Grall >
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S index 0b4cd19abd..1428cd3583 100644 --- a/xen/arch/arm/arm32/entry.S +++ b/xen/arch/arm/arm32/entry.S @@ -65,9 +65,12 @@ save_guest_regs: * If the SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT has been set in the cpu * feature, the checking of pending SErrors will be skipped. */ - ALTERNATIVE("nop", - "b skip_check", - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT + nop + alternative_else + b skip_check + alternative_endif + /* * Start to check pending virtual abort in the gap of Guest -> HYP * world switch. diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S index 458d12f188..91cf6ee6f4 100644 --- a/xen/arch/arm/arm64/entry.S +++ b/xen/arch/arm/arm64/entry.S @@ -170,9 +170,11 @@ * is not set. If a vSError took place, the initial exception will be * skipped. Exit ASAP */ - ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f", - "nop; nop", - SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT) + alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT + bl check_pending_vserror + cbnz x0, 1f + alternative_else_nop_endif + mov x0, sp bl enter_hypervisor_from_guest_noirq msr daifclr, \iflags
Using alternative_if makes the code a bit more streamlined. Take the opportunity to use the new auto-nop infrastructure to avoid counting the number of nop in the else part for arch/arm/arm64/entry.S Signed-off-by: Julien Grall <julien.grall@arm.com> --- This is pretty much a matter of taste, but at least for arm64 this allows us to use the auto-nop infrastructure. So the arm32 is more to keep inline with arm64. --- xen/arch/arm/arm32/entry.S | 9 ++++++--- xen/arch/arm/arm64/entry.S | 8 +++++--- 2 files changed, 11 insertions(+), 6 deletions(-)