diff mbox series

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

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

Commit Message

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

-- 
2.7.4

Comments

Laurent Vivier Oct. 12, 2017, 4:49 p.m. UTC | #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. UTC | #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. UTC | #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. UTC | #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. UTC | #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. UTC | #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. UTC | #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
diff mbox series

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