Date: Mon, 14 Apr 2008 19:35:45 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Chagin Dmitry <chagin.dmitry@gmail.com> Cc: freebsd-emulation@freebsd.org Subject: Re: Call for review && test: linux_kdump-1.6 Message-ID: <20080414163545.GJ18958@deviant.kiev.zoral.com.ua> In-Reply-To: <20080414194041.Q6842@ora.chd.net> References: <20080412145401.GA4139@freebsd.org> <20080413214624.S7426@ora.chd.net> <20080413183248.GA68642@freebsd.org> <20080413183659.GA18958@deviant.kiev.zoral.com.ua> <20080413231135.K1079@ora.chd.net> <20080413192614.GC18958@deviant.kiev.zoral.com.ua> <20080413234359.H1165@ora.chd.net> <20080414123137.GH18958@deviant.kiev.zoral.com.ua> <20080414134935.GI18958@deviant.kiev.zoral.com.ua> <20080414194041.Q6842@ora.chd.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--QxN5xOWGsmh5a4wb
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Mon, Apr 14, 2008 at 08:18:22PM +0400, Chagin Dmitry wrote:
> On Mon, 14 Apr 2008, Kostik Belousov wrote:
>=20
> >On Mon, Apr 14, 2008 at 03:31:37PM +0300, Kostik Belousov wrote:
> >>On Mon, Apr 14, 2008 at 12:42:29AM +0400, Chagin Dmitry wrote:
> >>>On Sun, 13 Apr 2008, Kostik Belousov wrote:
> >>>
> >>>>On Sun, Apr 13, 2008 at 11:11:55PM +0400, Chagin Dmitry wrote:
> >>>>>On Sun, 13 Apr 2008, Kostik Belousov wrote:
> >>>>>
> >>>>>>On Sun, Apr 13, 2008 at 08:32:48PM +0200, Roman Divacky wrote:
> >>>>>>>On Sun, Apr 13, 2008 at 09:58:08PM +0400, Chagin Dmitry wrote:
> >>>>>>>>On Sat, 12 Apr 2008, Roman Divacky wrote:
> >>>>>>>>
> >>>>>>>>>>And question: whether i can add to linuxolator some ktr_struct
> >>>>>>>>>> functionality?
> >>>>>>>>>
> >>>>>>>>>sure... please provide a patch and I'll take care about it.
> >>>>>>>>
> >>>>>>>>ok, thnx :)
> >>>>>>>>what about EJUSTRETURN?
> >>>>>>>>i attached simple patch for demo only (not tested).
> >>>>>>>
> >>>>>>>uh... can you provide diff -u ? I dont understand the diff at all =
;)
> >>>>>>
> >>>>>>Also, please note that the ML software strips your attachments. Eit=
her
> >>>>>>inline the patch, or use the plain-text content-type for it.
> >>>>>>
> >>>>>
> >>>>>ups... ah google ))
> >>>>>i have understood, sorry and thnx.
> >>>>>Speech about that in linux_kdump it is impossible to distinguish
> >>>>>EJUSTRETURN from a real error. look:
> >>>>>
> >>>>>--- sys/i386/i386/trap.c.orig 2008-04-13 21:39:18.000000000 +0400
> >>>>>+++ sys/i386/i386/trap.c 2008-04-13 22:35:25.000000000 +0400
> >>>>>@@ -1091,8 +1091,12 @@
> >>>>> td->td_proc->p_pid, td->td_name, code);
> >>>>>
> >>>>> #ifdef KTRACE
> >>>>>- if (KTRPOINT(td, KTR_SYSRET))
> >>>>>- ktrsysret(code, error, td->td_retval[0]);
> >>>>>+ if (KTRPOINT(td, KTR_SYSRET)) {
> >>>>>+ if (error =3D=3D EJUSTRETURN)
> >>>>>+ ktrsysret(code, 0, td->td_retval[0]);
> >>>>>+ else
> >>>>>+ ktrsysret(code, error, td->td_retval[0]);
> >>>>>+ }
> >>>>> #endif
> >>>>>
> >>>>> /*
> >>>>>@@ -1104,4 +1108,3 @@
> >>>>>
> >>>>> PTRACESTOP_SC(p, td, S_PT_SCX);
> >>>>> }
> >>>>>-
> >>>>
> >>>>I do not quite understand the intent of this change.
> >>>>
> >>>>EJUSTRETURN is used for two different purposes in the kernel.
> >>>>1. The sigreturn family of the syscalls use it after the interrupted
> >>>>frame is restored to avoid the normal syscall return sequence to modi=
fy
> >>>>the machine state.
> >>>>2. It is used by the kernel to notify the in-kernel caller code about
> >>>>some special condition, that nonetheless shall not be returned to the
> >>>>userspace.
> >>>>
> >>>>Only the first case is applicable to the kdump, and IMHO you actually
> >>>>destroy some information, since error =3D=3D EJUSTRETURN is reported =
as 0.
> >>>>
> >>>>Could you, please, provide some more arguments in the support of your
> >>>>proposed change ?
> >>>>
> >>>
> >>>Thanks for you informative reply Kostya.
> >>>The problem arises only in linux_kdump. Because linux error
> >>>codes negative and EJUSTRETURN coincides with ENOENT.
> >>>Before a call ktr_sysret we decode return codes of emulators syscalls.
> >>
> >>Ah, I see. Then, we shall never dump the ERESTART and EJUSTRETURN
> >>for the emulated ABIs. At least, this is true for Linux, I am not
> >>so sure about iBCS2 and SVR4.
> >>
>=20
> I do not absolutely agree with this statement. If the emulated syscalls=
=20
> should return native FreeBSD errno, that why to not write them to a=20
> ktrace file without conversion? Current linux_kdump port uses strerror=20
> because expects it. At least, it is convenient :)
Again, could you, please, elaborate ? ABI emulation shall return the
translated errors. And, the current behaviour is to dump translated
error codes, so linux_kdump must cope with it already.
>=20
> Your patch is correct for my version of linux_kdump, but does not solve=
=20
> a problem with the current port version.
I think we could commit the trap.c patch simultaneously with
the new linux_kdump. Even better, two versions of the linux_kdump
could coexist in the ports, with the right one being selected based
on the __FreeBSD_version. But I would leave this to the emulation@.
>=20
> If it's possible, explain please, that is not correct in my last patch?
Your previous patch (cited above) prevents the dumping of the
EJUSTRETURN for the native FreeBSD syscalls, that is also wrong, IMHO.
Assuming that your last patch is the one that moved the ktrsysret()
before the switch (error), I see two problems:
1. It dumps the error before the ABI compat has translated the error.
This is definitely huge deviation with the present behaviour, see
above.
2. It missed the amd64 trap.c.
#1 is corrected in my version. #2 is a trivial overlook.
>=20
> thnx
>=20
> >>Could you test the patch below, instead ?
> >
> >>diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c
> >>index e7de579..55642d1 100644
> >>--- a/sys/i386/i386/trap.c
> >>+++ b/sys/i386/i386/trap.c
> >
> >The patch is obviously wrong, it just prevents the Linux ENOENT to be
> >dumped. Please, try this one instead.
^^^^^^^^This statement is about _my_ first patch.
> >
> >diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
> >index b6a454d..3b1368a 100644
> >--- a/sys/amd64/amd64/trap.c
> >+++ b/sys/amd64/amd64/trap.c
> >@@ -861,9 +861,18 @@ syscall(struct trapframe *frame)
> > frame->tf_rip -=3D frame->tf_err;
> > frame->tf_r10 =3D frame->tf_rcx;
> > td->td_pcb->pcb_flags |=3D PCB_FULLCTX;
> >- break;
> >-
> >+ /* FALLTHROUGH */
> > case EJUSTRETURN:
> >+#ifdef KTRACE
> >+ /*
> >+ * The ABIs that use the negative error codes, like
> >+ * Linux, would confuse the in-kernel errno values
> >+ * with proper userspace errno. Clean these values to
> >+ * avoid a confusion in the kdump.
> >+ */
> >+ if (p->p_sysent->sv_errsize)
> >+ error =3D 0;
> >+#endif
> > break;
> >
> > default:
> >diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c
> >index e7de579..6ec04b0 100644
> >--- a/sys/i386/i386/trap.c
> >+++ b/sys/i386/i386/trap.c
> >@@ -1040,9 +1040,18 @@ syscall(struct trapframe *frame)
> > * int 0x80 is 2 bytes. We saved this in tf_err.
> > */
> > frame->tf_eip -=3D frame->tf_err;
> >- break;
> >-
> >+ /* FALLTHROUGH */
> > case EJUSTRETURN:
> >+#ifdef KTRACE
> >+ /*
> >+ * The ABIs that use the negative error codes, like
> >+ * Linux, would confuse the in-kernel errno values
> >+ * with proper userspace errno. Clean these values to
> >+ * avoid a confusion in the kdump.
> >+ */
> >+ if (p->p_sysent->sv_errsize)
> >+ error =3D 0;
> >+#endif
> > break;
> >
> > default:
> >
>=20
> --=20
> Have fun!
> chd
--QxN5xOWGsmh5a4wb
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.8 (FreeBSD)
iEYEARECAAYFAkgDh+AACgkQC3+MBN1Mb4i2mwCfWQa1YpRicz+1F0u2qMpLNZGx
RxAAoMWmPsqQbn3uRKrZiaGkAvYBtZ4h
=2ByX
-----END PGP SIGNATURE-----
--QxN5xOWGsmh5a4wb--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080414163545.GJ18958>