[2/2] linux-user: Fix TARGET_MTIOCTOP/MTIOCGET/MTIOCPOS values

Message ID 1507822245-15748-3-git-send-email-peter.maydell@linaro.org
State New
Headers show
Series
  • fix incorrect target ioctl numbers
Related show

Commit Message

Peter Maydell Oct. 12, 2017, 3:30 p.m.
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(-)

Comments

Laurent Vivier Oct. 12, 2017, 4:49 p.m. | #1
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
Peter Maydell Oct. 12, 2017, 4:53 p.m. | #2
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
Laurent Vivier Oct. 12, 2017, 5:08 p.m. | #3
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
Riku Voipio Oct. 16, 2017, 1:06 p.m. | #4
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
Laurent Vivier Oct. 16, 2017, 1:09 p.m. | #5
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
Peter Maydell Oct. 16, 2017, 1:26 p.m. | #6
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
Laurent Vivier Oct. 16, 2017, 1:29 p.m. | #7
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

Patch

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 */