Hello,
It's been 3 months and no reply.
On Mon, Dec 12, 2022 at 01:36:48PM +0100, Roger Pau Monné wrote:
> Hello,
>
> Gentle ping regarding the locking question below.
>
> Thanks, Roger.
>
> 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.
Roger.
Roger Pau Monné <[email protected]> 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().
cheers
On Fri, Mar 10, 2023 at 10:01:39AM +1100, Michael Ellerman wrote:
> Roger Pau Monné <[email protected]> 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().
Thanks, Roger.
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é <[email protected]> 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.