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: <93b6385c-63c8-1b5a-13c0-838f7c03ccce@suse.com>
Date:   Fri, 18 Nov 2022 08:36:14 +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>
From:   Jan Beulich <jbeulich@suse.com>
In-Reply-To: <20221118023535.1903459-1-marmarek@invisiblethingslab.com>
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?elgybnhhUGtzeGF1aURNemxZTWlNWXVKVXZDcTg4RVI3aFY5S2tzZ0g4Mllu?=
 =?utf-8?B?VVJIbFFVWXI0Vks1WndIbFB5dEd6MFRsYjF3K0haSTRiWDFuem5ZTFVFNXlZ?=
 =?utf-8?B?M2JNNzhzWGpTaXZGSHZhRFRRS0Z6NVBTd2JiMERKUUlQT0dWcStQS012VWVa?=
 =?utf-8?B?NzBlUmMvVjlvRW45TytjSFZHWm0rbGZadHlrZHpoeStZWUsycUhMeG4wR3U4?=
 =?utf-8?B?WkpwUVI0aXZiRk51UmlmM3B4NEYrRitTMVVkeVhrM0ZMalZSdGhzSCtTL3pB?=
 =?utf-8?B?ei9MYTErTm16cGhHRTlIZmIrSXRwVy94N0VwZFJTMlRCWVd6dml4VlhlbUpa?=
 =?utf-8?B?b3RJK2lScWhvS2tvRC9qZ1dMbmc0dTF2UWt6MWRpZkFjVUFsYjdrdU4yZWtw?=
 =?utf-8?B?WlF3ckVhcHArKytHTW1Ec0FGaHFFNUFxNGFQc2FCMjlPVDRwS0Jra093Snpn?=
 =?utf-8?B?R0FYMk4vMjkrQ1lIMElEYnVhSm5KZjVqdTlRNWRDQVNUTkNYSFlOeUJuYXZR?=
 =?utf-8?B?QTk1MDcxT08zOEhQRFI4RldZbEw0WFdkWjlPc0pWNGxWVVkwcVBqaHlrenIw?=
 =?utf-8?B?OVZrc0JKOEM0RHZ5TS9QZUlENGRuaVErd0c0b1BKNDhhOGNSSmdnNmljUXMw?=
 =?utf-8?B?ZytJNkdYRTNkNzN4cWFlTkRLTzBKRGtuTWRxQjJBMUFDemYxR2dTcVg0WHRN?=
 =?utf-8?B?blpwWTVpbkRrUEJXVHlhektDN1VvL2M4eGJ6VWVoU2NsMkFsN1o4a25vS3Bp?=
 =?utf-8?B?MGFFUVdCd1YvQ08vQVc3RHVJV2R3bTJINVJOVUs0N00wd3FlcjNOQnhXa0lp?=
 =?utf-8?B?ajA4TFFLNjBwWUNUOG0zL0REYUsxZ2UwYzhPYXBIbFJFdmJ3RFVVZHk3MUJI?=
 =?utf-8?B?TiszbnNUWDFRQVhqKzlUZ0ZaL1l3R20vdWQzNnZnQk0vZlZObzVndjFmaDBM?=
 =?utf-8?B?aGNHMlBXSWlzT0RoLzlseEdVQ1dVYmsvVnRLN2UxVVYwSzBIWEkrUkJCNDRP?=
 =?utf-8?B?VEc2OTJGY0htaWZHckpwUDFTeTFvcFZpRWQvNkZMOExHMHNhR1VJTkFKN1g3?=
 =?utf-8?B?R3hsc3p1d05mYjhIS0hjUm9DR1A5UlBiVWY3M2dXT3NQZ0xVbVIxZjgzNGpU?=
 =?utf-8?B?bWh5emFtUnFMSnYzTEFrY3RqMytjMjlEQUV0SVJQYng0RWdPNkNobkJyVDRF?=
 =?utf-8?B?Ym5yV0drck5VYkdiYldZV1BrUUtzcGs1OHhDcVlIYmtvVG4vUTZVU1NmdDYx?=
 =?utf-8?B?QkxhQzdkUnFkMGs1cCtSSncxVEhaTVFpdys4WDBiY1NMZ1A2ZEdvQ3dTZG5u?=
 =?utf-8?B?UURmZVkxM09DSWo5R3RFV0gyaUlGRjFEZ3V0d2s0MTdtK2hjWjNiSVl6MHlk?=
 =?utf-8?B?cmlBQ3Z2V09yQVJPZXRoeHNJTnlqSDYzTk9BZDBXQzNRNFYrcHhONTJPZktv?=
 =?utf-8?B?UWJwTlk4d3dod25WeFRTQlVWbkpKaERxdkhwNHRSOXhLWVRWdU83UXBlS291?=
 =?utf-8?B?NkphUzMxZlZkZHUwSTlDR0haMFRKZDY1NndHeldrMHBnK0lDNFJzNm5HUWE4?=
 =?utf-8?B?bHVqR2dtNW8vZ2JrM1VEVnh6RUs2LzN3eDJTOWJxL2Zkd2c4eXBjeGRaTzJU?=
 =?utf-8?B?NmNwVzFUeElNMXRjR0s3ZCswRzYwQlBlbHFOVXhPMEkrUWdwVXJZbW1DTm9E?=
 =?utf-8?B?RUt6TFFNVUF3NUFTUGJZVW1ab3B6R3JpNU52Wmo3NTJFdXBsK2tUVnRCdnBX?=
 =?utf-8?B?d2lJN2NSYzBPM3dsUU1HcGVPRERJWmJKTG94eVpvNTFMZ0tCdG0vNzNVM2l2?=
 =?utf-8?B?QzNWZm9oUm5LbTlPNWkwNThZazBpTUFWb0pjeW1HWmhVbGNUbERhaWZkNmpu?=
 =?utf-8?B?WVRjb2FZRkFsNzNzTTRXMTdLN1RYS2xNNU1vclBBVlYzWndpWjFHYklDOUFa?=
 =?utf-8?B?NGtpZHV4OCttZnFYMldlcVFZWTVSandROWk3RlBYdzV2QSt5T2ZPUVBlV1hq?=
 =?utf-8?B?V3NJZDNhUTVVcDJLM1lWMVpYRXhtKzlXKzlEYkpoMUhrUkdaZ2JZVnl4NnFU?=
 =?utf-8?B?ZGhxUDVyWStnanptSE81eEFZL1BMUWFSOWYreE5vK1lrWDVyb003RlJkUjZ6?=
 =?utf-8?Q?RMYhinBjZV524DQ8JWIPLDB7S?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d6756e5-a830-456c-81b7-08dac937939c
X-MS-Exchange-CrossTenant-AuthSource: VE1PR04MB6560.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Nov 2022 07:36:16.4332
 (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: PxLuRRb99Rc75IQpwOHLu1jYS9ct5eRBzYk3H4PnpCo2oDtwiOFtLm6QHR704UFsCnI3WWF7xX7ROtMmADVhNg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXPR04MB9399
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 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.

Furthermore it looks to me as if you're making msi_msix_flags_write()
inconsistent with command_write() - you'd now want to also permit
clearing "INTx disable" when MSI or MSI-X are enabled. Which, I think,
would simply mean allowing the domain unconditional control of the bit
(as long as allow_interrupt_control is set of course).

Especially with these further changes I'm afraid at least for now I
view this as moving in the wrong direction. My view might change in
particular if the description made more clear what was wrong with the
original change (476878e4b2be ["xen-pciback: optionally allow interrupt
enable flag writes"]), or perhaps the discussion having led to the form
which was committed in the end.

Jan