Re: [Xen-devel] [PATCH RFC for-4.13 09/10] xen/arm: asm: Replace use of ALTERNATIVE with alternative_if

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxx>
> >
> > ---
> >      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@xxxxxxxx>
>
> >           /*
> >            * 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

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