Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <df8309e3-5a56-ccf0-fc3d-16fa52f9fa7e@suse.com>
Date:   Fri, 18 Nov 2022 13:27:49 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH v2] xen-pciback: Consider INTx disabled when MSI/MSI-X is
 enabled
Content-Language: en-US
To:     =?UTF-8?Q?Marek_Marczykowski-G=c3=b3recki?= 
        <marmarek@invisiblethingslab.com>
Cc:     Juergen Gross <jgross@suse.com>,
        Stefano Stabellini <sstabellini@kernel.org>,
        Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
        "moderated list:XEN HYPERVISOR INTERFACE" 
        <xen-devel@lists.xenproject.org>, linux-kernel@vger.kernel.org
References: <20221118023535.1903459-1-marmarek@invisiblethingslab.com>
 <93b6385c-63c8-1b5a-13c0-838f7c03ccce@suse.com> <Y3d1UeCuDIMxmzTA@mail-itl>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <Y3d1UeCuDIMxmzTA@mail-itl>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WEIwSXNlNFBlaFVPbHpEZTBZVjRnWmRiNVVlcnlrdmxYcktDWXNTU1BRRW5E?=
 =?utf-8?B?T0xGTFlVQ2psMjBkQTJPM2VFZWpGeTBFd25mTW43ejhnU250dEN4VkpmTzB6?=
 =?utf-8?B?UVp3YVlEaHBxcGh0NW9YakZDZzFxMmxRenJ2RFJIUzBNMGlVZVR6S3FYME9C?=
 =?utf-8?B?V2VBb0xWMFp5Tk5TZXBUK0tlVHJ0THB5Y0ZpREZGMnJsNy9LdmY0bU95NHN6?=
 =?utf-8?B?Zm51emlwVGlIWjlValdCd1R4RXVEOVFHZEV3eS9pdVdYaVN5dUlsczhPSHdY?=
 =?utf-8?B?bHN0MUhOY01OdHI5WXVWYnBHM25UOGxySHRvSHdYd3JZMHB1TWNUQk1MakF4?=
 =?utf-8?B?MGFCdVVYdHdCZnVrNHhBbU11TlFnZENpSDBLVGdpdTZ4UzlFeUxNc1QzeGl6?=
 =?utf-8?B?U3FXTlJLTEdFcGZwQnA2eHVROEc5QzRrVUhvYWVxSVJsY3dnMXZoV1gvOFgx?=
 =?utf-8?B?MG1SYUViT1BHVkExbjhPZHFmQ2F1aVJpdWpCSUxodnAzRmlMcDBUUVNhWG9V?=
 =?utf-8?B?VG5rZUtsN3pwV0JRaCtHUTVNN3JCQlRNdUZrS3NXUGRFUjBGR3RzWGpqQ213?=
 =?utf-8?B?TG51VzRIU1Q3Zk1vdDFlbERTOCsrakgzV0tBRlowSStGQmthMkh2WWdZT1ZZ?=
 =?utf-8?B?TWxOZUlGSUhpL29zelk0eWUrcFBRelN2eEw2cXlDK2RmQ3pQVWlPdGExVi91?=
 =?utf-8?B?OEUvL2h6bVluZUh6c0laNngreHJ4bzF3S3MzdnVhOXdtVWVDV3BnVkhVaTVE?=
 =?utf-8?B?S2hwclVhbGJUMFJsVDVZRUJoNXZ6enFRbCt4RWM3ZUQ0L2h2YUlJbXhxTnEv?=
 =?utf-8?B?SW45WGRvRG9VSHVCb09OUmN4bk9PbHRSRGJ1bEloUitQOVE1aWdDcE1rZ0Iv?=
 =?utf-8?B?THFiTzI1Ykp1ZlFvK3dzek1NNzNVVk9JYUR1WDRLdnBBZUpFNW9tTkhYS09w?=
 =?utf-8?B?WEFUeW5GRS84cDY2dVlMRXJtcFBBeG5nTTRvVjkrSlpTNGdwdU1XK2Y5MFBj?=
 =?utf-8?B?UitDdWx4aHEya2FITmNKd0Rpb2ZuTG95S0oyRDE2Tzc1NXNJeWhRTXBqOFVT?=
 =?utf-8?B?R2hhdVo2dmlPVzlKdjlLSjFxd1VqUXdiSkpFM1V5dGpHb2lTTEVkUCsxeldy?=
 =?utf-8?B?TFZNd1NjWlN1dXlUNGxDWkg5RW0rUG1UY0pzM3JGdWV2YWlPOUl6TjJJOGFy?=
 =?utf-8?B?VmNPQU0vcFJVOERHVWg0Vm94Vi9YNXZRQjQvZ3NkaVQ5UVFoeDZhcmFwUFZU?=
 =?utf-8?B?OHM3a2ZFRXBsV01UeDRqZ3ZzWStETkxaaksvd3NIVzlhVWd5T3hGUy9uZDJD?=
 =?utf-8?B?eXNqdjk0MUttM3RxTFJ1QjJYcFBhOU1IVE5INkl4VldCVUY4R3MvZUoxSWhK?=
 =?utf-8?B?QnlxZWM1TkxSQjA2Yk95djQ0V1o5WlM5bVlBS0ZZVGo2d2NqVFZ4emJwT1FH?=
 =?utf-8?B?dURlUmhHQ1E0UGxmdi9UdTlNSjdsTDJXMTFqajdoUDhKOEdIYXc5YjVFelJT?=
 =?utf-8?B?UC90R1lwek1vUGJNSmtZaEE5TE5TcGhYT01sZml1SlJaeGJuYXY2WVdHank0?=
 =?utf-8?B?OG13c3lacTRTRWgwaFhtOG9yRFFTMTk5RFlCbzdLSi9yMFhsUThqNzczNXRm?=
 =?utf-8?B?NnpDcXlUU3krekdjYm1uUjFIYVVpK0hCZ0M0NnZWM3AvTHMwcXFMbVUrbU1t?=
 =?utf-8?B?SHdUU2t2YVFmamN5VDVlenhEWStraXdMR3I5RjFWV3htYnZrdVZndnluWjBN?=
 =?utf-8?B?ajM2eUl3NldsbnZvL253NUgwZjUrcHRHRU5ORWhiczFITTlDTUJTUnJ6bHNa?=
 =?utf-8?B?WmRxZ2N6RXdFVnFRNVYzbWVHVkQxMXorY2o5M0VQQklNT2NaWnJLVU5TcXdu?=
 =?utf-8?B?d1B1MWVxS3g2V2lDQnhxRUkzQ2I2eGJkejdLT1VJaVJuam1UbVcvbnFiRzZi?=
 =?utf-8?B?eXc4QTJRSlRtcVdiNy9lcEh4YlhxcUVxT3l2RVFHSWtQMVpvRGJxTVM3Q2hp?=
 =?utf-8?B?ZDNSY1pIUW5PSnlteW1iTGRLcyt1ZjJTc0wyOWlXMnc0WFgzL0VkNFBnUktR?=
 =?utf-8?B?cHJrWWhIcmg2UWVKdStHSEhxRU8vWXVtZE9sdUNMWXRUZ2V5cDlEdlVoaWtQ?=
 =?utf-8?Q?/EQB3ifp2fElO+6NtLNK/KjqN?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0d0089fa-893f-4081-5f54-08dac9604f14
X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Nov 2022 12:27:50.8375
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wfZKIA9MKuFRHwhiLh3oIQ+senolfnhNkiFywootz90IEdT+K/0k78KoEcypVZC71A/LWHSU81TMKwH7eTRP3A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB7800
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 18.11.2022 13:06, Marek Marczykowski-Górecki wrote:
> On Fri, Nov 18, 2022 at 08:36:14AM +0100, Jan Beulich wrote:
>> On 18.11.2022 03:35, Marek Marczykowski-Górecki wrote:
>>> Linux enables MSI-X before disabling INTx, but keeps MSI-X masked until
>>> the table is filled. Then it disables INTx just before clearing MASKALL
>>> bit. Currently this approach is rejected by xen-pciback.
>>> According to the PCIe spec, device cannot use INTx when MSI/MSI-X is
>>> enabled.
>>
>> Similarly the spec doesn't allow using MSI and MSI-X at the same time.
>> Before your change xen_pcibk_get_interrupt_type() is consistent for all
>> three forms of interrupt delivery; imo it also wants to be consistent
>> after your change. This effectively would mean setting only one bit at
>> a time (or using an enum right away), but then the question is what
>> order you do the checks in. IOW I think the change to the function is
>> wrong.
> 
> IIUC the difference is that enabling MSI or MSI-X implicitly disables
> INTx, while enabling both MSI and MSI-X is UB. This means that MSI
> active and PCI_COMMAND_INTX_DISABLE bit not set means "only MSI is
> active" - which the function now properly reports.

Hmm, yes, this is perhaps a good way to look at it.

> Both MSI and MSI-X active at the same time means a bug somewhere else
> and the current code allows only to disable one of them in such case. I
> could replace this with BUG_ON, or simply assume such bug doesn't exist
> and ignore this case, if you prefer.

BUG_ON() would imply this state cannot be reached no matter what is
requested from outside of the kernel. I'm not sure that's true here,
so keeping that aspect unchanged is probably fine (and with the above
I then take back my "wrong" from the earlier reply.

Jan