Re: svn commit: r367744 - in head/sys: compat/freebsd32 kern sys
On Tue, Nov 17, 2020 at 1:51 PM Brooks Davis wrote:
>
> On Tue, Nov 17, 2020 at 11:59:50AM -0600, Kyle Evans wrote:
> > On Tue, Nov 17, 2020 at 11:11 AM Brooks Davis wrote:
> > >
> > > On Tue, Nov 17, 2020 at 03:36:58AM +, Kyle Evans wrote:
> > > > Modified: head/sys/compat/freebsd32/freebsd32.h
> > > > ==
> > > > --- head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:34:01 2020
> > > > (r367743)
> > > > +++ head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:36:58 2020
> > > > (r367744)
> > > > @@ -94,6 +94,27 @@ struct itimerval32 {
> > > > struct timeval32 it_value;
> > > > };
> > > >
> > > > +struct umtx_time32 {
> > > > + struct timespec32 _timeout;
> > > > + uint32_t_flags;
> > > > + uint32_t_clockid;
> > > > +};
> > > > +
> > > > +struct umtx_robust_lists_params_compat32 {
> > > > + uint32_trobust_list_offset;
> > > > + uint32_trobust_priv_list_offset;
> > > > + uint32_trobust_inact_offset;
> > > > +};
> > > > +
> > > > +struct umutex32 {
> > > > + volatile __lwpid_t m_owner;/* Owner of the mutex */
> > > > + __uint32_t m_flags;/* Flags of the mutex */
> > > > + __uint32_t m_ceilings[2]; /* Priority protect
> > > > ceiling */
> > > > + __uint32_t m_rb_lnk; /* Robust linkage */
> > > > + __uint32_t m_pad;
> > > > + __uint32_t m_spare[2];
> > > > +};
> > > > +
> > > > #define FREEBSD4_MFSNAMELEN 16
> > > > #define FREEBSD4_MNAMELEN(88 - 2 * sizeof(int32_t))
> > > >
> > > >
> > > > Modified: head/sys/compat/freebsd32/freebsd32_misc.c
> > > > ==
> > > > --- head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17
> > > > 03:34:01 2020(r367743)
> > > > +++ head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17
> > > > 03:36:58 2020(r367744)
> > > > @@ -84,6 +84,7 @@ __FBSDID("$FreeBSD$");
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -3764,4 +3765,12 @@ freebsd32_sched_rr_get_interval(struct thread
> > > > *td,
> > > > error = copyout(, uap->interval, sizeof(ts32));
> > > > }
> > > > return (error);
> > > > +}
> > > > +
> > > > +int
> > > > +freebsd32__umtx_op(struct thread *td, struct freebsd32__umtx_op_args
> > > > *uap)
> > > > +{
> > > > +
> > > > + return (kern__umtx_op(td, uap->obj, uap->op, uap->val, uap->uaddr,
> > > > + uap->uaddr2, _native_ops32));
> > > > }
> > > >
> > >
> > > Putting any of this under compat/freebsd32 seems like a somewhat
> > > odd choice since all the work is done in kern_umtx.h. In CheriBSD,
> > > everything just lives there so nothing has to be exposed in headers.
> > >
> >
> > I have no strong opinion here -- my initial impression of the
> > suggestion to move the struct definitions into freebsd32 was that:
> >
> > 1.) One can then quickly reference the definition of, e.g., timespec32
> > when I'm looking at a umtx_time32, and
> > 2.) It'd be 'cleaner', requiring less #ifdef soup in kern_umtx.c
> >
> > The follow-up patch muddies the waters a lot, as we end up using the
> > compat32 definitions on all 64-bit platforms anyways even without
> > compat32. I don't object to moving any/all of this back, if you think
> > that's better.
>
> (1) makes sense to me. I'm less convinced of (2) especially given the
> followup. As a rule, I've been removing compat bits from headers when
> they only need to be defined in a single .c file. If nothing else, I
> don't like that it presents a somewhat-false implication that the
> interfaces are public (and there have been quite a few cases where they
> weren't correctly guarded with _KERNEL).
>
Sure- I've got this queued up:
https://people.freebsd.org/~kevans/umtx32.diff -> the next diff in
line will immediately remove that first COMPAT_FREEBSD32 block after
umtx_copyops is defined.
Thanks,
Kyle Evans
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r367744 - in head/sys: compat/freebsd32 kern sys
On Tue, Nov 17, 2020 at 11:59:50AM -0600, Kyle Evans wrote:
> On Tue, Nov 17, 2020 at 11:11 AM Brooks Davis wrote:
> >
> > On Tue, Nov 17, 2020 at 03:36:58AM +, Kyle Evans wrote:
> > > Modified: head/sys/compat/freebsd32/freebsd32.h
> > > ==
> > > --- head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:34:01 2020
> > > (r367743)
> > > +++ head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:36:58 2020
> > > (r367744)
> > > @@ -94,6 +94,27 @@ struct itimerval32 {
> > > struct timeval32 it_value;
> > > };
> > >
> > > +struct umtx_time32 {
> > > + struct timespec32 _timeout;
> > > + uint32_t_flags;
> > > + uint32_t_clockid;
> > > +};
> > > +
> > > +struct umtx_robust_lists_params_compat32 {
> > > + uint32_trobust_list_offset;
> > > + uint32_trobust_priv_list_offset;
> > > + uint32_trobust_inact_offset;
> > > +};
> > > +
> > > +struct umutex32 {
> > > + volatile __lwpid_t m_owner;/* Owner of the mutex */
> > > + __uint32_t m_flags;/* Flags of the mutex */
> > > + __uint32_t m_ceilings[2]; /* Priority protect ceiling
> > > */
> > > + __uint32_t m_rb_lnk; /* Robust linkage */
> > > + __uint32_t m_pad;
> > > + __uint32_t m_spare[2];
> > > +};
> > > +
> > > #define FREEBSD4_MFSNAMELEN 16
> > > #define FREEBSD4_MNAMELEN(88 - 2 * sizeof(int32_t))
> > >
> > >
> > > Modified: head/sys/compat/freebsd32/freebsd32_misc.c
> > > ==
> > > --- head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17 03:34:01
> > > 2020(r367743)
> > > +++ head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17 03:36:58
> > > 2020(r367744)
> > > @@ -84,6 +84,7 @@ __FBSDID("$FreeBSD$");
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -3764,4 +3765,12 @@ freebsd32_sched_rr_get_interval(struct thread *td,
> > > error = copyout(, uap->interval, sizeof(ts32));
> > > }
> > > return (error);
> > > +}
> > > +
> > > +int
> > > +freebsd32__umtx_op(struct thread *td, struct freebsd32__umtx_op_args
> > > *uap)
> > > +{
> > > +
> > > + return (kern__umtx_op(td, uap->obj, uap->op, uap->val, uap->uaddr,
> > > + uap->uaddr2, _native_ops32));
> > > }
> > >
> >
> > Putting any of this under compat/freebsd32 seems like a somewhat
> > odd choice since all the work is done in kern_umtx.h. In CheriBSD,
> > everything just lives there so nothing has to be exposed in headers.
> >
>
> I have no strong opinion here -- my initial impression of the
> suggestion to move the struct definitions into freebsd32 was that:
>
> 1.) One can then quickly reference the definition of, e.g., timespec32
> when I'm looking at a umtx_time32, and
> 2.) It'd be 'cleaner', requiring less #ifdef soup in kern_umtx.c
>
> The follow-up patch muddies the waters a lot, as we end up using the
> compat32 definitions on all 64-bit platforms anyways even without
> compat32. I don't object to moving any/all of this back, if you think
> that's better.
(1) makes sense to me. I'm less convinced of (2) especially given the
followup. As a rule, I've been removing compat bits from headers when
they only need to be defined in a single .c file. If nothing else, I
don't like that it presents a somewhat-false implication that the
interfaces are public (and there have been quite a few cases where they
weren't correctly guarded with _KERNEL).
-- Brooks
signature.asc
Description: PGP signature
Re: svn commit: r367744 - in head/sys: compat/freebsd32 kern sys
On Tue, Nov 17, 2020 at 11:11 AM Brooks Davis wrote:
>
> On Tue, Nov 17, 2020 at 03:36:58AM +, Kyle Evans wrote:
> > Modified: head/sys/compat/freebsd32/freebsd32.h
> > ==
> > --- head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:34:01 2020
> > (r367743)
> > +++ head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:36:58 2020
> > (r367744)
> > @@ -94,6 +94,27 @@ struct itimerval32 {
> > struct timeval32 it_value;
> > };
> >
> > +struct umtx_time32 {
> > + struct timespec32 _timeout;
> > + uint32_t_flags;
> > + uint32_t_clockid;
> > +};
> > +
> > +struct umtx_robust_lists_params_compat32 {
> > + uint32_trobust_list_offset;
> > + uint32_trobust_priv_list_offset;
> > + uint32_trobust_inact_offset;
> > +};
> > +
> > +struct umutex32 {
> > + volatile __lwpid_t m_owner;/* Owner of the mutex */
> > + __uint32_t m_flags;/* Flags of the mutex */
> > + __uint32_t m_ceilings[2]; /* Priority protect ceiling */
> > + __uint32_t m_rb_lnk; /* Robust linkage */
> > + __uint32_t m_pad;
> > + __uint32_t m_spare[2];
> > +};
> > +
> > #define FREEBSD4_MFSNAMELEN 16
> > #define FREEBSD4_MNAMELEN(88 - 2 * sizeof(int32_t))
> >
> >
> > Modified: head/sys/compat/freebsd32/freebsd32_misc.c
> > ==
> > --- head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17 03:34:01
> > 2020(r367743)
> > +++ head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17 03:36:58
> > 2020(r367744)
> > @@ -84,6 +84,7 @@ __FBSDID("$FreeBSD$");
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -3764,4 +3765,12 @@ freebsd32_sched_rr_get_interval(struct thread *td,
> > error = copyout(, uap->interval, sizeof(ts32));
> > }
> > return (error);
> > +}
> > +
> > +int
> > +freebsd32__umtx_op(struct thread *td, struct freebsd32__umtx_op_args *uap)
> > +{
> > +
> > + return (kern__umtx_op(td, uap->obj, uap->op, uap->val, uap->uaddr,
> > + uap->uaddr2, _native_ops32));
> > }
> >
>
> Putting any of this under compat/freebsd32 seems like a somewhat
> odd choice since all the work is done in kern_umtx.h. In CheriBSD,
> everything just lives there so nothing has to be exposed in headers.
>
I have no strong opinion here -- my initial impression of the
suggestion to move the struct definitions into freebsd32 was that:
1.) One can then quickly reference the definition of, e.g., timespec32
when I'm looking at a umtx_time32, and
2.) It'd be 'cleaner', requiring less #ifdef soup in kern_umtx.c
The follow-up patch muddies the waters a lot, as we end up using the
compat32 definitions on all 64-bit platforms anyways even without
compat32. I don't object to moving any/all of this back, if you think
that's better.
Thanks,
Kyle Evans
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r367744 - in head/sys: compat/freebsd32 kern sys
On Tue, Nov 17, 2020 at 03:36:58AM +, Kyle Evans wrote:
> Modified: head/sys/compat/freebsd32/freebsd32.h
> ==
> --- head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:34:01 2020
> (r367743)
> +++ head/sys/compat/freebsd32/freebsd32.h Tue Nov 17 03:36:58 2020
> (r367744)
> @@ -94,6 +94,27 @@ struct itimerval32 {
> struct timeval32 it_value;
> };
>
> +struct umtx_time32 {
> + struct timespec32 _timeout;
> + uint32_t_flags;
> + uint32_t_clockid;
> +};
> +
> +struct umtx_robust_lists_params_compat32 {
> + uint32_trobust_list_offset;
> + uint32_trobust_priv_list_offset;
> + uint32_trobust_inact_offset;
> +};
> +
> +struct umutex32 {
> + volatile __lwpid_t m_owner;/* Owner of the mutex */
> + __uint32_t m_flags;/* Flags of the mutex */
> + __uint32_t m_ceilings[2]; /* Priority protect ceiling */
> + __uint32_t m_rb_lnk; /* Robust linkage */
> + __uint32_t m_pad;
> + __uint32_t m_spare[2];
> +};
> +
> #define FREEBSD4_MFSNAMELEN 16
> #define FREEBSD4_MNAMELEN(88 - 2 * sizeof(int32_t))
>
>
> Modified: head/sys/compat/freebsd32/freebsd32_misc.c
> ==
> --- head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17 03:34:01
> 2020(r367743)
> +++ head/sys/compat/freebsd32/freebsd32_misc.cTue Nov 17 03:36:58
> 2020(r367744)
> @@ -84,6 +84,7 @@ __FBSDID("$FreeBSD$");
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -3764,4 +3765,12 @@ freebsd32_sched_rr_get_interval(struct thread *td,
> error = copyout(, uap->interval, sizeof(ts32));
> }
> return (error);
> +}
> +
> +int
> +freebsd32__umtx_op(struct thread *td, struct freebsd32__umtx_op_args *uap)
> +{
> +
> + return (kern__umtx_op(td, uap->obj, uap->op, uap->val, uap->uaddr,
> + uap->uaddr2, _native_ops32));
> }
>
Putting any of this under compat/freebsd32 seems like a somewhat
odd choice since all the work is done in kern_umtx.h. In CheriBSD,
everything just lives there so nothing has to be exposed in headers.
-- Brooks
signature.asc
Description: PGP signature
