On Fri, Mar 10, 2023 at 10:18:32AM +0100, Roger Pau Monné wrote: > On Fri, Mar 10, 2023 at 10:01:39AM +1100, Michael Ellerman wrote: > > Roger Pau Monné <roger....@citrix.com> writes: > > > On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote: > > >> On Fri, Dec 02, 2022 at 12:40:05PM +0100, Roger Pau Monné wrote: > > >> > On Wed, Nov 30, 2022 at 05:08:06PM -0800, Stefano Stabellini wrote: > > >> > > On Wed, 30 Nov 2022, Roger Pau Monne wrote: > > >> > > > The hvc machinery registers both a console and a tty device based > > >> > > > on > > >> > > > the hv ops provided by the specific implementation. Those two > > >> > > > interfaces however have different locks, and there's no single > > >> > > > locks > > >> > > > that's shared between the tty and the console implementations, > > >> > > > hence > > >> > > > the driver needs to protect itself against concurrent accesses. > > >> > > > Otherwise concurrent calls using the split interfaces are likely to > > >> > > > corrupt the ring indexes, leaving the console unusable. > > >> > > > > > >> > > > Introduce a lock to xencons_info to serialize accesses to the > > >> > > > shared > > >> > > > ring. This is only required when using the shared memory console, > > >> > > > concurrent accesses to the hypercall based console implementation > > >> > > > are > > >> > > > not an issue. > > >> > > > > > >> > > > Note the conditional logic in domU_read_console() is slightly > > >> > > > modified > > >> > > > so the notify_daemon() call can be done outside of the locked > > >> > > > region: > > >> > > > it's an hypercall and there's no need for it to be done with the > > >> > > > lock > > >> > > > held. > > >> > > > > >> > > For domU_read_console: I don't mean to block this patch but we need > > >> > > to > > >> > > be sure about the semantics of hv_ops.get_chars. Either it is > > >> > > expected > > >> > > to be already locked, then we definitely shouldn't add another lock > > >> > > to > > >> > > domU_read_console. Or it is not expected to be already locked, then > > >> > > we > > >> > > should add the lock. > > >> > > > > >> > > My impression is that it is expected to be already locked, but I > > >> > > think > > >> > > we need Greg or Jiri to confirm one way or the other. > > >> > > > >> > Let me move both to the 'To:' field then. > > >> > > > >> > My main concern is the usage of hv_ops.get_chars hook in > > >> > hvc_poll_get_char(), as it's not obvious to me that callers of > > >> > tty->poll_get_char hook as returned by tty_find_polling_driver() will > > >> > always do so with the tty lock held (in fact the only user right now > > >> > doesn't seem to hold the tty lock). > > >> > > > >> > > Aside from that the rest looks fine. > > > > > > I guess I could reluctantly remove the lock in the get_chars hook, > > > albeit I'm not convinced at all the lock is not needed there. > > > > I don't know the xen driver, but other HVC backends have a lock around > > their private state in their get_chars() implementations. > > > > See eg. hvterm_raw_get_chars(). > > Yes, that was one of the motivation for adding the lock also here, and > it has already been mentioned. The other is the usage of the hooks by > callers of hvc_poll_get_char().
Ping? Thanks, Roger.