Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl

[Aleksandar Markovic]

On Thu, Jan 16, 2020 at 1:00 PM Arnd Bergmann <address@hidden> wrote:
>
> On Thu, Jan 16, 2020 at 12:27 PM Aleksandar Markovic
> <address@hidden> wrote:
> > On Thursday, January 16, 2020, Aleksandar Markovic <address@hidden> wrote:
> >> On Wednesday, January 15, 2020, Laurent Vivier <address@hidden> wrote:
> >>> Le 15/01/2020 à 20:17, Filip Bozuta a écrit :
> >>> > On 15.1.20. 17:37, Arnd Bergmann wrote:
> >>> >> On Wed, Jan 15, 2020 at 5:32 PM Laurent Vivier <address@hidden> wrote:
> >>> >>> Le 15/01/2020 à 17:18, Arnd Bergmann a écrit :
> >>> >>>> On Wed, Jan 15, 2020 at 4:53 PM Filip Bozuta
> >>> >>>> <address@hidden> wrote:
> >>> >>>>> This patch implements functionality of following ioctl:
> >>> >>>>>
> >>> >>>>> SNDRV_TIMER_IOCTL_TREAD - Setting enhanced time read
> >>> >>>>>
> >>> >>>>>      Sets enhanced time read which is used for reading time with
> >>> >>>>> timestamps
> >>> >>>>>      and events. The third ioctl's argument is a pointer to an
> >>> >>>>> 'int'. Enhanced
> >>> >>>>>      reading is set if the third argument is different than 0,
> >>> >>>>> otherwise normal
> >>> >>>>>      time reading is set.
> >>> >>>>>
> >>> >>>>> Implementation notes:
> >>> >>>>>
> >>> >>>>>      Because the implemented ioctl has 'int' as its third argument,
> >>> >>>>> the
> >>> >>>>>      implementation was straightforward.
> >>> >>>>>
> >>> >>>>> Signed-off-by: Filip Bozuta <address@hidden>
> >>> >>>> I think this one is wrong when you go between 32-bit and 64-bit
> >>> >>> kernel uses an "int" and "int" is always 32bit.
> >>> >>> The problem is most likely with timespec I think.
> >>> >>>
> >>> >>>> targets, and it gets worse with the kernel patches that just got
> >>> >>>> merged for linux-5.5, which extends the behavior to deal with
> >>> >>>> 64-bit time_t on 32-bit architectures.
> >>> >>>>
> >>> >>>> Please have a look at
> >>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?h=80fe7430c70859
> >>> >>>>
> >>> >>> Yes, we already had the same kind of problem with SIOCGSTAMP and
> >>> >>> SIOCGSTAMPNS.
> >>> >>>
> >>> >>> Do the kernel patches add new ioctl numbers to differentiate 32bit and
> >>> >>> 64bit time_t?
> >>> >> Yes, though SNDRV_TIMER_IOCTL_TREAD is worse: the ioctl argument
> >>> >> is a pure 'int' that decides what format you get when you 'read' from 
> >>> >> the
> >>> >> same file descriptor.
> >>> >>
> >>> >> For emulating 64-bit on 32-bit kernels, you have to use the new
> >>> >> SNDRV_TIMER_IOCTL_TREAD64, and for emulating 32-bit on
> >>> >> 64-bit kernels, you probably have to return -ENOTTY to
> >>> >> SNDRV_TIMER_IOCTL_TREAD_OLD unless you also want to
> >>> >> emulate the read() behavior.
> >>> >> When a 32-bit process calls SNDRV_TIMER_IOCTL_TREAD64,
> >>> >> you can translate that into the 64-bit
> >>> >> SNDRV_TIMER_IOCTL_TREAD_OLD.
> >>> >
> >>> > Thank you for bringing this up to my attention. Unfortunately i have
> >>> > some duties of academic nature in next month so i won't have much time
> >>> > fix this bug. I will try to fix this as soon as possible.
> >>>
> >>> Could you at least to try to have a mergeable series before you have to
> >>> stop to work on this?
> >>>
> >>> You can only manage the case before the change reported by Arnd (I will
> >>> manage the new case myself later).
>
> >>
> >> Sorry for interjecting myself into this discussion, but I just want to let 
> >> you know about some related practicalities.
> >>
> >> Filip is a student that is from time to time (usually between two exam 
> >> seasons) an intern in our company, working 4 hours each work day. He spent 
> >> his internship in different teams in prevous years, and, from around 
> >> mid-October 2019, was appointed to QEMU team. After some introductory 
> >> tasks, he was assigned his main task: linux-user support for RTCs and ALSA 
> >> timers. This series is the result of his work, and, to my great pleasure, 
> >> is virtually entirely his independant work. I am positive he can complete 
> >> the series by himself, even in the light of additional complexities 
> >> mentioned in this thread.
> >>
> >> However, his exam season just started (Jan. 15th), and lasts till Feb. 
> >> 15th. Our policy, in general, is not to burden the students during exam 
> >> seasons, and that is why we can't expect prompt updates from him for the 
> >> time being.
> >>
> >> In view of this, Laurent, please take Filip's status into consideration. 
> >> As far as mergeability is concerned, my impression is that patches 1-6 and 
> >> 13 are ready for merging, while patches 7-12 would require some additional 
> >> (netlink-support-like) work, that would unfortunately be possible only 
> >> after Feb. 15th.
> >>
>
> > Laurent, hi again.
> >
> > I am not completely familiar with all details of Filip's work, since, as I 
> > said, he had
> > large degree of independance (which was intentional, and is a desired and 
> > good
> > thing IMHO), but taking a closer look, a question starting lingering: Do we 
> > need
> > special handling od read() even for RTC devices - not only ALSA timer 
> > devices?
>
> Adding Alexandre Belloni and the RTC list to Cc for more expertise. Alexandre,
> this question is about how qemu-user should emulate the rtc ioctl interface 
> when
> running a user binary for a foreign architecture. The ioctl emulation seems 
> fine
> to me, but read() from /dev/rtc is probably not.
>
> As I understand it, reading from /dev/rtc is one of the more obscure features.
> This would return either 32 bits or 64 bits of structured data from the 
> kernel,
> depending on how much data was requested and whether the kernel runs
> as 64 bit. A 32-bit process running on a 64-bit kernel will get the correct
> result when it asks for 4 bytes, but probably not when it asks for 8 bytes.
> (we could fix this with an explict check for in_compat_syscall() in the kernel
> function).
>
> A process running on qemu with the opposite endianess will always get the
> wrong result (unless the kernel returns 0), and emulating 64-bit task on
> a 32-bit kernel will result in only four bytes to be read, which also likely
> results in incorrect behavior.
>


Alexandre (and Arnd too, or any other person knowledgeable in the area),

I just need to clarify a couple of details with you, please.

Firstly, here is what man page rtc(4) says:

"The /dev/rtc (or /dev/rtc0, /dev/rtc1, etc.) device can be opened
only once (until it is closed) and it is read-only. On read(2) and
select(2) the calling process is blocked until the next interrupt from
that RTC is received. Following the interrupt, the process can read a
long integer, of which the least significant byte contains a bit mask
encoding the types of interrupt that occurred, while the remaining 3
bytes contain the number of interrupts since the last read(2)."

So, it looks read() will always return only 4 bytes of useful info
(regardless of host being 32-bit/64-bit).

My questions are:

- Is the description in man page genuinely accurate?

- To me (but I am really an outsider to using RTC in applications),
this feature (blocking read()/select()) even looks very nice and
convenient, in all fairness. But I would like to ask you: Is this
feature used rarely or frequently by other libraries/tools/etc.? In
other words, is the feature "obscure" or "crucial" part of RTC kernel
support? Or, something in between?

- Does MC146818 support this feature?

Thanks a lot in advance for any response!

Aleksandar

P.S. for Arnd: I sent this message only to you, by mistake, around 15
min ago. Now I include everybody.

>        Arnd