On Sat, 10 May 2025, Andrew Cooper wrote:
> On 07/05/2025 11:46 pm, victorm.l...@amd.com wrote:
> > From: Nicola Vetrini <nicola.vetr...@bugseng.com>
> >
> > Rule 13.2 states: "The value of an expression and its persistent
> > side effects shall be the same under all permitted evaluation orders".
> >
> > The full expansion of macro "commit_far_branch" contains an assignment to
> > variable "rc", which is also assigned to the result of the GNU statement
> > expression which "commit_far_branch" expands to.
> >
> > To avoid any dependence on the order of evaluation, the latter assignment
> > is moved inside the macro.
>
> The (mostly expanded) and reformatted expression is:
>
> > if ( (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) ||
> > (rc = ({
> > ({
> > if ( sizeof(dst.val) <= 4 )
> > {
> > do {
> > if ( __builtin_expect(!!(!(!ctxt->lma)),0) )
> > ASSERT();
> > } while ( 0 );
> > ({
> > if ( ((dst.val) > (&cs)->limit) )
> > {
> > x86_emul_hw_exception(13, mkec(13, 0, 0),
> > ctxt);
> > rc = 2;
> > goto done;
> > }
> > });
> > }
> > else
> > ({
> > if ( (ctxt->lma && (&cs)->l
> > ? !(((long)(dst.val) >> 47) ==
> > ((long)(dst.val) >> 63))
> > : (dst.val) > (&cs)->limit) )
> > {
> > x86_emul_hw_exception(13, mkec(13, 0, 0),
> > ctxt);
> > rc = 2;
> > goto done;
> > }
> > });
> > });
> > _regs.rip = (dst.val);
> > singlestep = _regs.eflags & 0x00000100;
> > ops->write_segment(x86_seg_cs, &cs, ctxt);
> > })) )
>
> And I can't identify anywhere where there is an ambiguous order of
> evaluation.
>
> The problem with this patch is that, while the existing logic is not
> exactly great, ...
>
> > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> > b/xen/arch/x86/x86_emulate/x86_emulate.c
> > index 8e14ebb35b..7108fe7b30 100644
> > --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> > @@ -337,7 +337,7 @@ do {
> > \
> > validate_far_branch(cs, newip); \
> > _regs.r(ip) = (newip); \
> > singlestep = _regs.eflags & X86_EFLAGS_TF; \
> > - ops->write_segment(x86_seg_cs, cs, ctxt); \
> > + rc = ops->write_segment(x86_seg_cs, cs, ctxt); \
> > })
> >
> > int x86emul_get_fpu(
> > @@ -2382,7 +2382,7 @@ x86_emulate(
> > (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes + src.val),
> > &src.val, op_bytes, ctxt, ops)) ||
> > (rc = load_seg(x86_seg_cs, src.val, 1, &cs, ctxt, ops)) ||
> > - (rc = commit_far_branch(&cs, dst.val)) )
> > + commit_far_branch(&cs, dst.val) )
> > goto done;
>
> ... this is breaking a visual layout pattern where you can always see
> the update to rc beside the "goto done".
>
> This code is complicated enough without making it harder.
>
> One option which might work is this:
>
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index 3350303f8634..dab4373ece1e 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -333,12 +333,14 @@ do
> { \
> : (ip) > (cs)->limit, X86_EXC_GP, 0); \
> })
>
> -#define commit_far_branch(cs, newip) ({ \
> - validate_far_branch(cs, newip); \
> - _regs.r(ip) = (newip); \
> - singlestep = _regs.eflags & X86_EFLAGS_TF; \
> - ops->write_segment(x86_seg_cs, cs, ctxt); \
> -})
> +#define commit_far_branch(cs, newip) ( \
> + ({ \
> + validate_far_branch(cs, newip); \
> + _regs.r(ip) = (newip); \
> + singlestep = _regs.eflags & X86_EFLAGS_TF; \
> + }), \
> + ops->write_segment(x86_seg_cs, cs, ctxt) \
> + )
>
> int x86emul_get_fpu(
> enum x86_emulate_fpu_type type,
>
>
> This rearranges commit_far_branch() to use the comma operator, but
> maintains the previous property that it's still overall an assignment to rc.
>
> However, I don't have much confidence that Eclair is going to like it more.
Eclair likes it:
Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
