Re: [Qemu-devel] [PATCH v6 8/8] linux-user: Add support for FDSETEMSGTRESH, FDSETMAXERRS, and FDGETMAXERRS ioctls

[Laurent Vivier]

Le 10/09/2019 à 20:59, Aleksandar Markovic a écrit :
> 
> 06.09.2019. 12.47, "Laurent Vivier" <address@hidden
> <mailto:address@hidden>> је написао/ла:
>>
>> Le 04/09/2019 à 14:59, Aleksandar Markovic a écrit :
>> > From: Aleksandar Markovic <address@hidden
> <mailto:address@hidden>>
>> >
>> > FDSETEMSGTRESH, FDSETMAXERRS, and FDGETMAXERRS ioctls are commands
>> > for controlling error reporting of a floppy drive.
>> >
>> > Signed-off-by: Aleksandar Markovic <address@hidden
> <mailto:address@hidden>>
>> > ---
>> >  linux-user/ioctls.h        |  2 ++
>> >  linux-user/syscall_defs.h  | 19 +++++++++++++++++++
>> >  linux-user/syscall_types.h |  7 +++++++
>> >  3 files changed, 28 insertions(+)
>> >
>> > diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
>> > index 622874b..0c75d03 100644
>> > --- a/linux-user/ioctls.h
>> > +++ b/linux-user/ioctls.h
>> > @@ -118,6 +118,8 @@
>> >       IOCTL(FDFMTTRK, IOC_W, MK_PTR(MK_STRUCT(STRUCT_format_descr)))
>> >       IOCTL(FDFMTEND, 0, TYPE_NULL)
>> >       IOCTL(FDFLUSH, 0, TYPE_NULL)
>> > +     IOCTL(FDSETMAXERRS, IOC_W,
> MK_PTR(MK_STRUCT(STRUCT_floppy_max_errors)))
>> > +     IOCTL(FDGETMAXERRS, IOC_R,
> MK_PTR(MK_STRUCT(STRUCT_floppy_max_errors)))
>>
>> where is FDSETEMSGTRESH?
>>
>> >       IOCTL(FDRESET, 0, TYPE_NULL)
>> >       IOCTL(FDRAWCMD, 0, TYPE_NULL)
>> >       IOCTL(FDTWADDLE, 0, TYPE_NULL)
>> > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
>> > index 834a085..7c5b614 100644
>> > --- a/linux-user/syscall_defs.h
>> > +++ b/linux-user/syscall_defs.h
>> > @@ -890,12 +890,31 @@ struct target_pollfd {
>> > 
>> >  /* From <linux/fd.h> */
>> > 
>> > +struct target_floppy_max_errors {
>> > +    abi_uint        abort;
>> > +    abi_uint        read_track;
>> > +    abi_uint        reset;
>> > +    abi_uint        recal;
>> > +    abi_uint        reporting;
>> > +};
>>
>> You don't need this, you can use floppy_max_errors from <linux/fd.h>.
>>
>> But you can define it if you want because it is used to know the size of
>> the target structure (and if alignment or data types differ it can
>> mismatch. With "int" it's not the case).
>>
> 
> Laurent, thanks for the review, I'll correct this in the next version.
> 
> Just a follow-up question:
> 
> If the structure of related to a (not-yet-supported in QEMU) ioctl was:
> 
> struct hd_geometry {
>       unsigned char heads;
>       unsigned char sectors;
>       unsigned short cylinders;
>       unsigned long start;
> };
> 
> ... would "target_hd_geometry" be needed, or not?

In this case I think it is needed:
"unsigned long start" can be needed to be aligned on 64bit on some 64bit
architectures whereas on other it can be needed to be aligned on 32bit
or 16bit (m68k). So host and guest can behave differently.

Thanks,
Laurent