Re: [PATCH v5 04/17] common-user: Move syscall error detection into safe_syscall_base

[Richard Henderson]

On 11/22/21 12:55 PM, Peter Maydell wrote:
> > -       /* This is the entry point for making a system call. The calling
> > +       /*
> > +        * This is the entry point for making a system call. The calling
> >           * convention here is that of a C varargs function with the
> >           * first argument an 'int *' to the signal_pending flag, the
> >           * second one the system call number (as a 'long'), and all further
> >           * arguments being syscall arguments (also 'long').
>
> This comment text needs updating to mention the new errnop argument.
> (Applies to all the similar comments in the files for the other archs.)

Yep.

> > +       /*
> > +         * The syscall calling convention isn't the same as the C one:
>
> Looks like the indent here is wrong ?

Irritatingly, these files are a mix of tabs/spaces.

> >           * we enter with x0 == *signal_pending
> > -        *               x1 == syscall number
> > -        *               x2 ... x7, (stack) == syscall arguments
> > +        *               x1 == errno
>
> "int* address of errno"

Arg, fixed some of these, but clearly. not all.

> >          /* code path for having successfully executed the syscall */
> > +       cmn     x0, #4095
> > +       b.cs    1f
>
> Shouldn't this be going to label 0f ? We need to do the 'neg',
> and unless I'm misreading the diff there's currently no path
> of execution that gets to that.

Oops, rebase error, where the fix landed in the next patch.

> > +        *               r2 == syscall number
> > +        *               r3, [sp+0] ... [sp+16] == syscall arguments
> >           *               and return the result in r0
>
> Don't we wind up with a potential issue here with 64-bit arguments
> due to the calling convention wanting to put those in aligned
> memory/register locations? Previously because we had just two
> extra arguments the arguments started at r2 and had the same
> alignment behaviour as the syscall wants for them starting at
> r0; but now we start at r3 so if for instance the first argument
> is 64-bit it will be in [sp+0][sp+4] but should go in r0:r1
> I think...
>
> (Stopped reviewing here because if we need to change the
> way we call these functions there's no point my reviewing
> the fine detail of the asm.)

Oof.  I missed that detail.  Yes, that is a problem (I think arm is the only such 
supported host).  I think the best solution would be to *not* pass in &errno, but to have 
the assembly tail-call to

long safe_syscall_errno_tail(int value)
{
    errno = value;
    return -1;
}

Which is probably more efficient in any case.  I'll re-work this.


r~