[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/3] x86/msi: clear initial MSI-X state on boot
- To: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
- From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
- Date: Tue, 28 Mar 2023 13:37:14 +0200
- 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=lzEw7p6+PZaKQFwkoCVGL3EP/xw22qVnPpTX2XO8PSQ=; b=VCp4TQIQUVvhFCNJb6QYnsnjjT6NHeBHmEwAKOq3TVzxG1utfkmUdxpJ4lOmMUZxOtlaWKJ2NcL9epMggoOKkTiN8G4U1/MhDplayN+y+XnxE9rdLe1mEfkcMroAHmFS/nQXKz7WJbF8nQozl+fm//Ea4MyI4zEsvn4lGv2KLW5hKtGVrcIYQ2YkE2Bn6nZawroZcz1UOcffA9ZjjxmEfInHpo3oBBdBCmkxhqvOv0hLUhpdwxdLEhVP8ftbHEJgVxHhhgu7VwHXiisWjNzA4vj2zqlkxnuPs6h8uyyMOMlZG0BZTt0Yi0wfbEodyB5l9ouuhsrfz6PVeci7SlmgDw==
- Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=QgWgF8oPuVmsjpTeiHYthlyqOoitngwkXjs6xgGULjCzRHmPNDSHMsjYeZjPxj+UOrikKe8HKOiUV1ny8FpVgxLtb6JDMU3+ZnTbXW5cRNzz47C/992Lzce6oHbHZIA/2iui4i43y3ccrupk5A7ZoR1pYslwjptbNtGl9vj/+V61TbxH/Rdor826XK077KV6d2IVC0aEonqKwekvWb1iVJLHTcVdxpHByhIm/cDAbJ7GvV+ltQPqmNtu4QV0xtV2sUgA6wVX02tPRqjOqkSp3O2vhIOBP1SEN3dFVFnOs9fSNc115+ugzLZEsLtt7GrBQnjGE7rDncATLWV2iAJjMg==
- Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
- Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx, Jason Andryuk <jandryuk@xxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Paul Durrant <paul@xxxxxxx>
- Delivery-date: Tue, 28 Mar 2023 11:37:34 +0000
- Ironport-data: A9a23:kA1AFa2HYFu8Q8W6l/bD5Q9wkn2cJEfYwER7XKvMYLTBsI5bpzMFz zMeDW2BPKnYNDP8eYskPYi2o0xUu5/Tz9dmS1Q/pC1hF35El5HIVI+TRqvS04F+DeWYFR46s J9OAjXkBJppJpMJjk71atANlVEliefTAOK6ULWeUsxIbVcMYD87jh5+kPIOjIdtgNyoayuAo tq3qMDEULOf82cc3lk8tuTS+HuDgNyo4GlD5gdkNagR1LPjvyJ94Kw3dPnZw0TQGuG4LsbiL 87fwbew+H/u/htFIrtJRZ6iLyXm6paLVeS/oiI+t5qK23CulQRrukoPD9IOaF8/ttm8t4sZJ OOhF3CHYVxB0qXkwIzxWvTDes10FfUuFLTveRBTvSEPpqFvnrSFL/hGVSkL0YMkFulfESZCq uQYKhE0bgHYjMev7+iQdNFxmZF2RCXrFNt3VnBI6xj8VaxjeraaBqLA6JlfwSs6gd1IEbDGf c0FZDFzbRPGJRpSJlMQD5F4l+Ct7pX9W2QA9BTJ+uxqvC6PnWSd05C0WDbRUsaNSshP2F6Ru 0rN/njjAwFcP9uaodaA2iv027CUxnqkBur+EpWy7eBz2nmR2VYMDRAoZ2SG4sDit3eXDoc3x 0s8v3BGQbIJ3EmiVNz0RRC7iH+CoB8HWtBUHvE66QeC0a7d6UCSAW1sZi5MbpkqudE7QRQu1 0SVhJX5CDp3qrqXRHmBsLCOoluaOyETIXUTeCwsQg4M4t2lq4Y25jrNRNt+FK++jvXuBCr9h TuNqUAWn7wOieYb2qP9+krI6w9AvbDMRw8xox7RB2Ss61sjYJb/P9D5r1/G8fxHMYCVCEGbu 2QJkNSf6+ZICoyRkCuKQ6MGG7TBC+u5DQAwSGVHR/EJnwlBMVb5FWyMyFmS/HtUD/s=
- Ironport-hdrordr: A9a23:RGUe6qiv/YMfbk0VvYaMmx21wnBQXh0ji2hC6mlwRA09TySZ// re+sjztCWVtN91YhodcL+7WZVoLUmskKKdgrNhRItKPjOWwFdARbsKheSN/9SJIVyEygc379 YFT0ERMqyWMXFKyevBzU2fNf1I+rW6GaaT79v2/jNWYTsvQYdGwCdWNj2yL21RY019KadRLu v+2uN34zWhfHgMbte2HBA+MtTrrcHQiZTjbQUnKnccmWuzsQ8=
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On Sat, Mar 25, 2023 at 03:49:24AM +0100, Marek Marczykowski-Górecki wrote:
> Some firmware/devices are found to not reset MSI-X properly, leaving
> MASKALL set. Xen relies on initial state being both disabled.
The 'both' in this sentence seems out of context, as you just mention
MASKALL in the previous sentence.
> Especially, pci_reset_msix_state() assumes if MASKALL is set, it was Xen
> setting it due to msix->host_maskall or msix->guest_maskall. Clearing
> just MASKALL might be unsafe if ENABLE is set, so clear them both.
>
> Reported-by: Jason Andryuk <jandryuk@xxxxxxxxx>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> v2:
> - new patch
> ---
> xen/drivers/passthrough/msi.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/xen/drivers/passthrough/msi.c b/xen/drivers/passthrough/msi.c
> index ce1a450f6f4a..60adad47e379 100644
> --- a/xen/drivers/passthrough/msi.c
> +++ b/xen/drivers/passthrough/msi.c
> @@ -48,6 +48,13 @@ int pdev_msi_init(struct pci_dev *pdev)
> ctrl = pci_conf_read16(pdev->sbdf, msix_control_reg(pos));
> msix->nr_entries = msix_table_size(ctrl);
>
> + /*
> + * Clear both ENABLE and MASKALL, pci_reset_msix_state() relies on
> this
> + * initial state.
> + */
> + ctrl &= ~(PCI_MSIX_FLAGS_ENABLE|PCI_MSIX_FLAGS_MASKALL);
> + pci_conf_write16(pdev->sbdf, msix_control_reg(pos), ctrl);
Should you make this conditional to them being set in the first place:
if ( ctrl & (PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL) )
{
...
We should avoid the extra access if possible (specially if it's to
write the same value that has been read).
I would even move this before the msix_table_size() call, so the
handling of ctrl is closer together.
Would it also be worth to print a debug message that the initial
device state seems inconsistent?
Thanks, Roger.
|
