| Message ID | 1507822245-15748-3-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | fix incorrect target ioctl numbers | expand |
Le 12/10/2017 à 17:30, Peter Maydell a écrit : > The TARGET_MTIOCTOP/TARGET_MTIOCGET/TARGET_MTIOCPOS values > were being defined in terms of host struct types, but > these structures are such that their size might differ > on different hosts. Switch to using a target struct > definition instead. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > linux-user/syscall_defs.h | 31 ++++++++++++++++++++++++++++--- > 1 file changed, 28 insertions(+), 3 deletions(-) > > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h > index f7cc9f9..16d56fa 100644 > --- a/linux-user/syscall_defs.h > +++ b/linux-user/syscall_defs.h > @@ -2706,9 +2706,34 @@ struct target_f_owner_ex { > #define TARGET_VFAT_IOCTL_READDIR_BOTH TARGET_IORU('r', 1) > #define TARGET_VFAT_IOCTL_READDIR_SHORT TARGET_IORU('r', 2) > > -#define TARGET_MTIOCTOP TARGET_IOW('m', 1, struct mtop) > -#define TARGET_MTIOCGET TARGET_IOR('m', 2, struct mtget) > -#define TARGET_MTIOCPOS TARGET_IOR('m', 3, struct mtpos) > +struct target_mtop { > + abi_short mt_op; > + abi_int mt_count; > +}; > + > +#if defined(TARGET_SPARC) || defined(TARGET_MIPS) > +typedef abi_long target_kernel_daddr_t; > +#else > +typedef abi_int target_kernel_daddr_t; > +#endif Perhaps you can add these ones into include/exec/user/abitypes.h ? > +struct target_mtget { > + abi_long mt_type; > + abi_long mt_resid; > + abi_long mt_dsreg; > + abi_long mt_gstat; > + abi_long mt_erreg; > + target_kernel_daddr_t mt_fileno; > + target_kernel_daddr_t mt_blkno; > +}; I think you need to update STRUCT(mtget, ...) in linux-user/syscall_types.h to reflect the size difference for MIPS and SPARC. Thanks, Laurent
On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote: > Le 12/10/2017 à 17:30, Peter Maydell a écrit : >> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS) >> +typedef abi_long target_kernel_daddr_t; >> +#else >> +typedef abi_int target_kernel_daddr_t; >> +#endif > > Perhaps you can add these ones into include/exec/user/abitypes.h ? I don't think they belong there -- that file is for basic CPU ABI dependent types, not things which are just part of the kernel interface. >> +struct target_mtget { >> + abi_long mt_type; >> + abi_long mt_resid; >> + abi_long mt_dsreg; >> + abi_long mt_gstat; >> + abi_long mt_erreg; >> + target_kernel_daddr_t mt_fileno; >> + target_kernel_daddr_t mt_blkno; >> +}; > > I think you need to update STRUCT(mtget, ...) in > linux-user/syscall_types.h to reflect the size difference for MIPS and > SPARC. I thought about that but I wasn't feeling too enthusiastic due to not having a test case... I suppose it's better to change them both though. thanks -- PMM
Le 12/10/2017 à 18:53, Peter Maydell a écrit : > On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote: >> Le 12/10/2017 à 17:30, Peter Maydell a écrit : >>> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS) >>> +typedef abi_long target_kernel_daddr_t; >>> +#else >>> +typedef abi_int target_kernel_daddr_t; >>> +#endif >> >> Perhaps you can add these ones into include/exec/user/abitypes.h ? > > I don't think they belong there -- that file is for basic > CPU ABI dependent types, not things which are just part of > the kernel interface. I agree Laurent
On Thu, Oct 12, 2017 at 07:08:55PM +0200, Laurent Vivier wrote: > Le 12/10/2017 à 18:53, Peter Maydell a écrit : > > On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote: > >> Le 12/10/2017 à 17:30, Peter Maydell a écrit : > >>> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS) > >>> +typedef abi_long target_kernel_daddr_t; > >>> +#else > >>> +typedef abi_int target_kernel_daddr_t; > >>> +#endif > >> > >> Perhaps you can add these ones into include/exec/user/abitypes.h ? > > > > I don't think they belong there -- that file is for basic > > CPU ABI dependent types, not things which are just part of > > the kernel interface. > I agree So we should go with the patch as-is? Riku
Le 16/10/2017 à 15:06, Riku Voipio a écrit : > On Thu, Oct 12, 2017 at 07:08:55PM +0200, Laurent Vivier wrote: >> Le 12/10/2017 à 18:53, Peter Maydell a écrit : >>> On 12 October 2017 at 17:49, Laurent Vivier <laurent@vivier.eu> wrote: >>>> Le 12/10/2017 à 17:30, Peter Maydell a écrit : >>>>> +#if defined(TARGET_SPARC) || defined(TARGET_MIPS) >>>>> +typedef abi_long target_kernel_daddr_t; >>>>> +#else >>>>> +typedef abi_int target_kernel_daddr_t; >>>>> +#endif >>>> >>>> Perhaps you can add these ones into include/exec/user/abitypes.h ? >>> >>> I don't think they belong there -- that file is for basic >>> CPU ABI dependent types, not things which are just part of >>> the kernel interface. > >> I agree > > So we should go with the patch as-is? For this part, yes, but I think for the other comment, STRUCT(mtget, ...) needs to be updated. Thanks, Laurent
On 16 October 2017 at 14:09, Laurent Vivier <laurent@vivier.eu> wrote: > For this part, yes, but I think for the other comment, STRUCT(mtget, > ...) needs to be updated. Looking more closely I don't think it is as simple as just adjusting the STRUCT() definition. For a basic type that can be different on host and guest, I think we'd need to handle it the way we do the TYPE_OLDDEVT, with special support in the thunk code for figuring out how large it is and so on. That seems like a lot of work for a magtape ioctl that I suspect nobody's using and which we can't test... Applying this patch as-is would at least fix the ioctl for everything except MIPS and SPARC hosts and guests. thanks -- PMM
Le 16/10/2017 à 15:26, Peter Maydell a écrit : > On 16 October 2017 at 14:09, Laurent Vivier <laurent@vivier.eu> wrote: >> For this part, yes, but I think for the other comment, STRUCT(mtget, >> ...) needs to be updated. > > Looking more closely I don't think it is as simple as just > adjusting the STRUCT() definition. For a basic type that can > be different on host and guest, I think we'd need to handle > it the way we do the TYPE_OLDDEVT, with special support > in the thunk code for figuring out how large it is and so on. > That seems like a lot of work for a magtape ioctl that I > suspect nobody's using and which we can't test... I agree. > Applying this patch as-is would at least fix the ioctl > for everything except MIPS and SPARC hosts and guests. Yes, so I think it can be applied as-is. Thanks, Laurent
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h index f7cc9f9..16d56fa 100644 --- a/linux-user/syscall_defs.h +++ b/linux-user/syscall_defs.h @@ -2706,9 +2706,34 @@ struct target_f_owner_ex { #define TARGET_VFAT_IOCTL_READDIR_BOTH TARGET_IORU('r', 1) #define TARGET_VFAT_IOCTL_READDIR_SHORT TARGET_IORU('r', 2) -#define TARGET_MTIOCTOP TARGET_IOW('m', 1, struct mtop) -#define TARGET_MTIOCGET TARGET_IOR('m', 2, struct mtget) -#define TARGET_MTIOCPOS TARGET_IOR('m', 3, struct mtpos) +struct target_mtop { + abi_short mt_op; + abi_int mt_count; +}; + +#if defined(TARGET_SPARC) || defined(TARGET_MIPS) +typedef abi_long target_kernel_daddr_t; +#else +typedef abi_int target_kernel_daddr_t; +#endif + +struct target_mtget { + abi_long mt_type; + abi_long mt_resid; + abi_long mt_dsreg; + abi_long mt_gstat; + abi_long mt_erreg; + target_kernel_daddr_t mt_fileno; + target_kernel_daddr_t mt_blkno; +}; + +struct target_mtpos { + abi_long mt_blkno; +}; + +#define TARGET_MTIOCTOP TARGET_IOW('m', 1, struct target_mtop) +#define TARGET_MTIOCGET TARGET_IOR('m', 2, struct target_mtget) +#define TARGET_MTIOCPOS TARGET_IOR('m', 3, struct target_mtpos) struct target_sysinfo { abi_long uptime; /* Seconds since boot */
The TARGET_MTIOCTOP/TARGET_MTIOCGET/TARGET_MTIOCPOS values were being defined in terms of host struct types, but these structures are such that their size might differ on different hosts. Switch to using a target struct definition instead. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- linux-user/syscall_defs.h | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-)