IronPort-Data: A9a23:xDWGFq8iP4FeQdtIjC8IDrUDNX+TJUtcMsCJ2f8bNWPcYEJGY0x3m
 jEaUG+OOa2KM2T8Lo1yOo2xoR8Ou5Dcx4IwTQRvrXo8E34SpcT7XtnIdU2Y0wF+jCHgZBk+s
 5hBMImowOQcFCK0SsKFa+C5xZVE/fjUAOG6UKicYXoZqTZMEE8JkQhkl/MynrlmiN24BxLlk
 d7pqojUNUTNNwRcawr40Ire7kI/1BjOkGlA5AdmPqkU5Aa2e0Q9V/rzG4ngdxMUfaEMdgKKb
 76r5K20+Grf4yAsBruN+losWhRXKlJ6FVHmZkt+A8BOsDAbzsAB+v9T2M4nQVVWk120c+VZk
 72hg3ASpTABZcUgkMxFO/VR/roX0aduoNcrKlDn2SCfItGvn9IBDJyCAWlvVbD09NqbDklJ+
 cJfJy8KMyufvMGHxuKBSbdytpUKeZyD0IM34hmMzBn/JNN/G9XpZfWP4tVVmjAtmspJAPDSI
 dIDbiZiZwjBZBsJPUoLDJU5n6GjgXyXnz9w8QrJ4/ZopTWOilUvgNABM/KMEjCObd9SkUuC4
 HrP4kzyAw0ANczZwj2Amp6prraXxHqmCd9OSdVU8NYzhQeq4mdKWCEGUFiZj9WB1xeBavt2f
 hl8Fi0G6PJaGFaQZsLmQxSyrXqAvxgdc9ldCes37EeK0KW8ywSWHG8fVRZadccr8sQxQFQC0
 0eEt97tATF1tbSTD3ORsL6JxRu9IyUaLm8qYS4CUBsL5MTlrIgvjxXJCNF5H8adjNzvGCr0y
 jqbhCsznbMeiYgMzarT1VTGhS+845vEVAg44i3JUW+/qAB0foioY8qv81ezxeZNKsOVQ0eMu
 FAAmtOC96YeAJeVjiuPTe4RWraz6J6tNDzanE53B5Jk+zmz03qiZpxLpjZsIE5jKYADYzCBX
 aPIkQZY5ZsWNn36a6ZyOti1E55zk/imEsn5XPfJaNYIeoJ2aAKM4CBpYwiXwnzpl08v16o4P
 P93bPqRMJrTMow/pBLeegvX+eVDKvwWrY8Lea3G8g==
IronPort-HdrOrdr: A9a23:9C4lJK43UenTY4oI5wPXwHPXdLJyesId70hD6qm+c31om6uj5q
 aTdZUgpHjJYVMqMk3I9ursBEDtex/hHNtOkOos1VnLZnibhILqFvAe0WPaqweQZBEWj9Qtq5
 uIEZIfNDSANykfsS+g2njALz9I+rDum5xAx92urUuFKzsEV0gK1XYdNu/0KCNLrSB9dOsEPa
 vZyMpbhiaqPU8aZt68ARA+LpL+juyOupL6QAIMQyUq4gmWjT+u9dfBYmOl9yZbfTNT4KsotV
 PImQzh5qmlrrWSxxLG23XIhq4m6OfJ+59sBNGslsNQEDnqhwqyDb4RI4G/gA==
Date:   Fri, 17 Mar 2023 11:10:13 +0100
From:   Roger Pau =?utf-8?B?TW9ubsOp?= <roger.pau@citrix.com>
To:     Stefano Stabellini <sstabellini@kernel.org>,
        Juergen Gross <jgross@suse.com>
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org,
        linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2] hvc/xen: prevent concurrent accesses to the shared
 ring
Message-ID: <ZBQ8hXiPvrbneKUF@Air-de-Roger>
References: <20221130150919.13935-1-roger.pau@citrix.com>
 <alpine.DEB.2.22.394.2211301657200.4039@ubuntu-linux-20-04-desktop>
 <Y4nkFZal7oy+aICa@Air-de-Roger>
 <Y5cgYLNwtPbmP1JL@Air-de-Roger>
 <ZAm8BDGTMaI0XmMI@Air-de-Roger>
 <87zg8lo7fw.fsf@mpe.ellerman.id.au>
 <ZAr16LM+qpGqa1h6@Air-de-Roger>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZAr16LM+qpGqa1h6@Air-de-Roger>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aHZHcGJ1aG1GQUlNRWlScUNTY2p5Qng2Z0JqdWNEK2Ftc2FuSUFCV09yVFFY?=
 =?utf-8?B?MVVkZU00M0lqMTIxNmwyczVDY0M2YWs1RXZXdzdWM0ZRdVI4eno1RVYxRDhB?=
 =?utf-8?B?VUJaMG03cFJidnNMcnNiMlhXY2FsRVZGNUhQdDgxMklDMXZNbHp2NWJ0Mkxj?=
 =?utf-8?B?K2huR3Z2c0ZPMUlqVFdqa3lLTVdsTFdIWHROQnRRL290VDBHeUlIWEhST2pL?=
 =?utf-8?B?dkhnL01keDFDd2QzczlLZUx1eS9MQnlLQ0c1b2o5ckh3OUJkQy9ZMXBGSVNy?=
 =?utf-8?B?OGE3RCtmUXRTeDhNOWFQbmQ3c3gwa0JkOTlUY1pQOGtSZkdpN3hldjlDTVBv?=
 =?utf-8?B?VFp6Uzl0Z3ZRT2RZYTYyOTA2UWdVZ2hHYjc2VjZWc1JLa2ZXWm04UGR3V3Bl?=
 =?utf-8?B?RkUveTNOZUlaZytrdDR4SVNOUnBjUkZ6bmtSZUR2V3ZmcTI2QXErcmJiS1VI?=
 =?utf-8?B?UDk3bWhZNmZnbll4U3V2MGtTSTNFRVJDVVAyOU5vbTF2QmdHMlNnam1LRHpU?=
 =?utf-8?B?Q2RydDJlQlBrR2k0bzFLeVNzQnpyWWlLQWROT3FBNlNpQ1Z4RERrQllRd2FS?=
 =?utf-8?B?VjJ4Z0tFdUgyVEM2bUh3ZElBaFIyZ2wxMlNnM3lXUm9JenFXc0NRYk1YS0lw?=
 =?utf-8?B?L0ZPcERXTXN1T3JlMUMvY3JuTUdXN1lVTmtIWE1hQjROaDJaQmdLbmhpeHVF?=
 =?utf-8?B?ck5wU2pVWjF4VFpQV2FITXNVRTk0eUY4R0lIUVI4ZUY0RGJTd0NYcVFyNFVa?=
 =?utf-8?B?Q0puNTJxZ2llUXh0bTV2aVJ3SEpWNStLRU82MkMvMGVhR0RFQ0poTTZTajlr?=
 =?utf-8?B?bDJwSmViVUxTUUI0cTJ0MXdLNEhaSmRQNjFHZG1jVlNHQ1lkaklvV3VaQ1J1?=
 =?utf-8?B?dGlnbkUyd2l1amd0eHZEL25EQ1JVbm9zS1ZzNWhYeUJ2MUlTZlNhajEzZmZN?=
 =?utf-8?B?aGl5R2J6NXlMaTltQnNwL09Nb0gyeG8rekZia3lxbHJPWHNSYnlWV2RVbnlh?=
 =?utf-8?B?Q2VzVU9PcVJxSU54ZlZ2cTlzL01DZG1nQ1BKaGZ2ekUrdjMvSUZTeWo1THBO?=
 =?utf-8?B?ZWhJQVZYOVR5S0xaVG92bG1JclVoQkowUEV0bENjU2VOd29rNnBmNzVuMEZ0?=
 =?utf-8?B?ODdtUkROZWNkVzRZQmFiaFpwbDBIMFBMaE1wbWpIYkdZbG82VGFOQUVvaExV?=
 =?utf-8?B?N3JjQ2U1WnJuZjNEQjRueW9OcXdsZ0xaZlF3RFBZbjVROGVJMEE3VUIwNGRP?=
 =?utf-8?B?akhsVng5MFlIUUxOSmt4NGdqc0F6eEdCL1MvUDJ5ZmhFYkk5ZWZIWkc5Mkxk?=
 =?utf-8?B?d1JZNGE2NXZzZTlnRDNtMGcrV0o0c2VWK2JpcHpGcUpodmdGeWYwYVh5WERo?=
 =?utf-8?B?L0xWTDNSaU8reHM5SEZ3TG9iVFg1KzVrbnBncUJsc3JtcVJUNFllalA0OFd5?=
 =?utf-8?B?MkZ4Vy9zNTY2M0dRN2JWcEtNRnZMMUVGUHBzYmo3dTJONkhOSGZvZHRYQzFv?=
 =?utf-8?B?bVpGK2NZOWM0RTBiVHZQYStJZnpuK2NvUnJBYVZEVWdiM0hhZzVaN2xKcGZs?=
 =?utf-8?B?NFBCR0I0RHg1eXphcUs4MnhoQk5FWTEzRnc2MUsrMkZZRWZHTllZblZhWFV0?=
 =?utf-8?B?L2RVWktiOWEzUDZHdFB6OWdESURjOHRNbEhNTWdscUw0MCtPWUp4SklWbGVV?=
 =?utf-8?B?V2g1TCtwVElGdXRMUXp4WXJnY0V1bzRYQ1BLSVdKNHk3djNGVytJUEVMNzBv?=
 =?utf-8?B?RnE1SHhPMXdVSkREVzIybTlMN0lsVURjVERxNjZnVHg4RC9SZjM3WUk1dVg4?=
 =?utf-8?B?VlQybDltQlJtMzhFUVpzemNtUnBUM2dKai9TUWtsVC9UY1RiNTJwOWQ5SzlW?=
 =?utf-8?B?QWVXVXdoRitaaWhPeWdjNHE1aWp0bjFIQjdFUzhhNlpvWUVQQlNTd29wc2ZL?=
 =?utf-8?B?bmpsMmdiYndBam1zT0QvbW1DQ1VUWG9zWDlHU0FSUVpGRkdBQW5pc0N0eElH?=
 =?utf-8?B?UkpBb0tsdDhHMGtCTWJaUkI4bFBjcXBxUEI5UlBwSURnRGVnNDNjK1VSM0gr?=
 =?utf-8?B?aFFuOUJVZkwwSDBhTTRHVVRFMkdSSTJ1anVrbnIzMUxWUTlIQ2tGVk9USGZV?=
 =?utf-8?B?QUNEV1RGVklucnU3d0xjRGhQbGh6UWpLTmc4UERZdGJhakplMFFNR0xUSW1L?=
 =?utf-8?B?R1E9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?eDltU2NFVGhLSjk3YWQ0UXNBRkR6U1AxRVNrUWZRRFYzNVBHQ1NiUUFmQVo4?=
 =?utf-8?B?Tkp0d0F0SFFwNUtFaU5lTDhNdXhHa0hqTDFDRWpjVE9yQloycCtYV285UDVx?=
 =?utf-8?B?TUp4L2hJVS9qblRCOTdrRjZTK1l4ejk2M2Z2a01CRDVGUEM1bFVqVlg0TXdw?=
 =?utf-8?B?emZFWm95MmJYbjlFME8vTmd0eDVCc0xrOTkrMEF6WGprMW5OcGxDTzBrczlV?=
 =?utf-8?B?VG5wVHRReGdPMUt4RUd1Q2tzdVQ2RjZ0cEZ5RXowV1k0dEliM1FYOXRyZHZT?=
 =?utf-8?B?dDliVjVTcmdRRkYyamVmOWoxeEVhSVFoTEpLRW9FdXdQa0xEUU5LSWFxQWty?=
 =?utf-8?B?OTlCMnM5eFJJUXZHalNvSHVmUk5uOThoTHp1VW0yeVB1RmdQbVB3WFcyVGNn?=
 =?utf-8?B?bEVjSVU3QnA2MDFWWVkwUG9HaTQzQ0duNFRkMnVJM0J5bHNEeG55K1ZvZWJ4?=
 =?utf-8?B?QUFsVzk4OEZIR2lkNGpFU0l4bUxoWmFwa09yOWVjU0xpcFpObnV3VkN3NWJv?=
 =?utf-8?B?RVVOejZSZ1JpN1E3VFVsV3ZhM2tobmtVQWlFUjF4Vm9zcU5JS2tUeVhFZ1NL?=
 =?utf-8?B?aHZEUC96eWlrTmxqWE51Sk1aVUVKUUN5L1NNdWFDSmlPa2gzeFZFNTQzQ0xD?=
 =?utf-8?B?UEkyWWtuYXRkTUZudk1EaUxZYytGTGRGZ2ptd2pSSUllY2ljMktzakY2VjVN?=
 =?utf-8?B?TUV1d1RTN3lMWTlTZ2d0TWxiV25KT3o0OFBDaHdLUnhXQWpZdGxRKy96K2l5?=
 =?utf-8?B?eE4wd0JJazZKTzkwRmhWV3RtY29BTWdKeHE2MkdNYUdhUkpoWXpmb1B2UkZt?=
 =?utf-8?B?VmpscUt6ejhOSmNxTHV5NkJTeTQ5bGQ0VHY5bTk2WVpEM3dxbGVRcXBpYllm?=
 =?utf-8?B?a2pSbUlXSDM1bElaK1BnQVRvOFBQTE9HYXg4RWp4YlhOY2lIQVE3SHZJenpo?=
 =?utf-8?B?S29GRm9aY3RWenY5dHlXQlp0V3U0TkVYanFPOGhSSHhKZ09tZlpwWXBzOENT?=
 =?utf-8?B?L3A4dFNKR0doWUNLUXE3SnQvdmkxVjlLSnNrN0dlN2dCd3RmTE04b0xXemY4?=
 =?utf-8?B?U2VFNDdzdTVnaDNlMXRWOXpicGllZTFyS09MT0o5TDVtUkV4OCtJMWpZY3BP?=
 =?utf-8?B?ZXJWeERhUU5sZUFBQjI0SStqdU5HVXoyV01kcTk3Zk1lRVJqQk4rSDZsQVNz?=
 =?utf-8?B?L2daRDJKRVRwRnJDNDNpK1VJeWhqbllKeVJnVFNmTEUrNHBJcURXclVMeU9l?=
 =?utf-8?B?YlM1d2tOMnJXbzNxU2luTWoyNVFsbWgrZDFseVR6MXAvbnNEZWhXcVREb1o2?=
 =?utf-8?Q?ID8caCL00S7ezh77IsxauvjkeexNc6bnsp?=
X-OriginatorOrg: citrix.com
X-MS-Exchange-CrossTenant-Network-Message-Id: efe421bf-e4d7-4033-d74a-08db26cfcf61
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR03MB6360.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 10:10:18.5094
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 335836de-42ef-43a2-b145-348c2ee9ca5b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gfKJ5YZw/t09MwHxE+0/sBPPzd3Q8NUDHaIcK87ua0l7Ubsyc9S7gCm0Uft0tGsjoGeAAdLh5C5YY+U3Pe3aCg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR03MB5085
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.pau@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.