Re: [Qemu-devel] [PULL 0/3] Linux user for 4.1 patches

[Laurent Vivier <laurent@vivier.eu>]

Le 18/07/2019 à 13:00, Peter Maydell a écrit :
> On Thu, 18 Jul 2019 at 11:40, Laurent Vivier <laurent@vivier.eu> wrote:
>> It comes from linux-user/syscall.c:
>>
>>  6328         /* automatic consistency check if same arch */
>>  6329 #if (defined(__i386__) && defined(TARGET_I386) && defined(TARGET_ABI32)) || \
>>  6330     (defined(__x86_64__) && defined(TARGET_X86_64))
>>  6331         if (unlikely(ie->target_cmd != ie->host_cmd)) {
>>  6332             fprintf(stderr, "ERROR: ioctl(%s): target=0x%x host=0x%x\n",
>>  6333                     ie->name, ie->target_cmd, ie->host_cmd);
>>  6334         }
>>  6335 #endif
>>
>> because of:
>>
>> +  { TARGET_SIOCGSTAMP_OLD, SIOCGSTAMP, "IOCGSTAMP_OLD", IOC_R, \
>> +    do_ioctl_SIOCGSTAMP },
>> +  { TARGET_SIOCGSTAMPNS_OLD, SIOCGSTAMPNS, "IOCGSTAMPNS_OLD", IOC_R, \
>> +    do_ioctl_SIOCGSTAMPNS },
>> +  { TARGET_SIOCGSTAMP_NEW, SIOCGSTAMP, "IOCGSTAMP_NEW", IOC_R, \
>> +    do_ioctl_SIOCGSTAMP },
>> +  { TARGET_SIOCGSTAMPNS_NEW, SIOCGSTAMPNS, "IOCGSTAMPNS_NEW", IOC_R, \
>> +    do_ioctl_SIOCGSTAMPNS },
>>
>> As the host_cmd is not used, the simplest way to fix that is
>>
>> +  { TARGET_SIOCGSTAMP_OLD, TARGET_SIOCGSTAMP_OLD, "IOCGSTAMP_OLD", IOC_R, \
>> +    do_ioctl_SIOCGSTAMP },
>> +  { TARGET_SIOCGSTAMPNS_OLD, TARGET_SIOCGSTAMPNS_OLD, "IOCGSTAMPNS_OLD", IOC_R, \
>> +    do_ioctl_SIOCGSTAMPNS },
>> +  { TARGET_SIOCGSTAMP_NEW, TARGET_SIOCGSTAMP_NEW, "IOCGSTAMP_NEW", IOC_R, \
>> +    do_ioctl_SIOCGSTAMP },
>> +  { TARGET_SIOCGSTAMPNS_NEW, TARGET_SIOCGSTAMPNS_NEW, "IOCGSTAMPNS_NEW", IOC_R, \
>> +    do_ioctl_SIOCGSTAMPNS },
>>
>> As SIOCGSTAMP_OLD and SIOCGSTAMP_NEW can be undefined on the host (and not needed
>> because we always use SIOCGSTAMP and SIOCGSTAMPNS)
> 
> So we don't use the host_cmd because we have a custom do_ioctl_foo
> function which doesn't look at that field?

Yes

> Sounds OK, but please include a comment explaining why.

OK

> PS: why didn't you use IOCTL_SPECIAL() rather than hand-written
> array entries? None of the other ioctls.h entries do that.

because IOCTL_SPECIAL() extracts the host command from the parameter and
so we would need the _NEW and _OLD definitions on the host, and they are
not available with kernel before 5.2

> Of course now we're trying to sidestep the consistency check
> we can't use the macro, but it wolud have been fine otherwise.
> It also would get the names of the ioctls in the string form
> right -- they are all missing the initial "S" here.

oh, yes. I fix that too.

> Perhaps for 4.2 it might be worth considering having a
> macro for "IOCTL_SPECIAL but skip the consistency check"
> to be a bit less hacky here.

Perhaps, rather than using TARGET_XXX in host_cmd I can use NULL and
check for NULL in consistency check?

Do you think we should defer the whole patch after 4.1 release?
But then the build of 4.1 will be broken with 5.2+ kernel

Thanks,
Laurent