[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] xen/ioapic: Don't use local_irq_restore() to disable irqs
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
- Date: Tue, 21 Feb 2023 18:14:01 +0000
- Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
- Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=ypegoioatRCyWXwJPAOfgkp/o360rWNmzPJTKGTFzL4=; b=ENcU9j7jO5knO1QC83FWOdZG0/Ql2D7i5p1bVbR1TY4tztS1x/d2s/7SejwF6TmnpNvrBkts23nus5+eBjV23sHbZ3TpVJUnfW4gucTPK9d5TcZa1TYBz3lm+FKJA5ZwoxhpHTfM3h4q1mm4q+KojJfIre9E08ALxyaN13hOe0vgakLhGI6bJqwq76hx0t8LQ1/Gs71+MRE7BJjCxL1a2bFSlPaaSrW1aYnrViRsDTEG4+QSn4qxeUf+haSnn7Rldmz49Q2phlQf9ZpM2zJJFbdCBeG/lLF5XxQnbHYnwJG3h7d471kxQNDi4N7t9sDxZM79tKFEKJxK3kdpy7QKCA==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kDiNZWVXrD/e3useeeejDi3CLwNlAI369KKQnoW/XtcfARTLY+NY8gHfj7NNwTWGp9rJxpBoxUx8reRP5/p5Fc2vaZHTPAJA518h+Yflh4Qq3ZVm3YGop9AWm9ohgO7zzVzftYKyDhTeh02Wc1KePMCocl7tAnDlVBljXjRRfdQUvIup2mBmQ7J0kpLKSXw6kH6ShzjQioXS6nxUjKjz1zscw2M5hj9x6hkJ6Awhi/+828ihvKj7Bzy4RDZWMrqV42J0A1CTz34GVwhcN6GceYNnIFtFlfOjqf6DO+9TaJMmCl/4o7mlxtTKfMwMrcZSiXDcUBPjSdzSctLYcFb1eA==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
- Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
- Delivery-date: Tue, 21 Feb 2023 18:14:24 +0000
- Ironport-data: A9a23:8NUBLa1pYXYvhTJbDfbD5fFwkn2cJEfYwER7XKvMYLTBsI5bpzMEy mAdCmrTMqnbajH8et8ibo3k8EIHsZPdxoNrQVZvpC1hF35El5HIVI+TRqvS04F+DeWYFR46s J9OAjXkBJppJpMJjk71atANlVEliefTAOK6ULWeUsxIbVcMYD87jh5+kPIOjIdtgNyoayuAo tq3qMDEULOf82cc3lk8tuTS93uDgNyo4GlD5gZkPKgQ1LPjvyJ94Kw3dPnZw0TQGuG4LsbiL 87fwbew+H/u/htFIrtJRZ6iLyXm6paLVeS/oiI+t5qK23CulQRrukoPD9IOaF8/ttm8t4sZJ OOhF3CHYVxB0qXkwIzxWvTDes10FfUuFLTveRBTvSEPpqFvnrSFL/hGVSkL0YMkFulfKEdkx KYSb2A3SBGNpMiqx5yVdMZVmZF2RCXrFNt3VnBI6xj8VKxjbbWdBqLA6JlfwSs6gd1IEbDGf c0FZDFzbRPGJRpSJlMQD5F4l+Ct7pX9W2QA9BTJ+uxqsi6KklwZPLvFabI5fvSjQ8lPk1nej WXB52njWTkRNcCFyCrD+XWp7gPKtXKkA9hCRO3onhJsqE/PnzYfMAIqaUKqruKepGCFWNkBJ 1NBr0LCqoB3riRHVOLVXRe1vXqFtR40QMdLHqsx7wTl4rXQyxaUAC4DVDEpQMwrsoo6SCIn0 neNnsj1Hnp/vbuNU3Wf+7yI6zSoNkAowXQqYCYFSU4J5oflqYRq1BbXFI89QOiyk8H/Hiz2z 3aSti8iir4PjMkNkaKm4VTAhDHqrZ/MJuIo2jjqsquexlsRTOaYi0aAszA3Md4owF6lc2S8
- Ironport-hdrordr: A9a23:tf9NSK7Dp+qhc8VnqwPXwM/XdLJyesId70hD6qkRc3xom6mj/P xG88506faZskdyZJhfo6HnBEDwexLhHPdOiOF8AV6MZmbbUQCTXeJfBOXZsljd82WXzIRgPe 0JScVD4JOaNykfsS4biDPIdOod/A==
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 21/02/2023 1:40 pm, Jan Beulich wrote:
> On 20.02.2023 20:47, Andrew Cooper wrote:
>> Despite its name, the irq{save,restore}() APIs are only intended to
>> conditionally disable and re-enable interrupts.
> Are they?
Yes, absolutely.
And as said before, the potentially dubious naming does not give us
permission or the right to interpret the behaviour differently.
> Maybe nowadays they indeed are, but I couldn't spot any wording
> to this effect in Linux'es Documentation/ (and I don't think we have
> anywhere such could be put). Yet I'd expect such a statement to be backed
> by something.
It is backed by the rude words the owners of this API had to say on
discovering this particular use.
Conditionally enabling with a construct like this is bogus everywhere.
It is only ever safe to enable irqs if you know exactly why they were
disabled previously, and that can never be the case with a construct
like this.
It only reason this one example doesn't explode is because its an init
path not nested within an irq/irqsave lock or other critical region.
>
>> IO-APIC's timer_irq_works() violates this intention. As it is init code,
>> switch to simple irq enable/disable().
> If all callers of the function obviously did have interrupts off, I might
> agree. But the last of them sits _after_ local_irq_restore(), and all of
> this is called from underneath smp_prepare_cpus()
Which do you mean "the last of them" ?
There is a large amount of genuinely dead logic here.
~Andrew
|
