Re: [PATCH v2 2/2] Allow VMD to disable MSIX remapping with interrupt remapping enabled.

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Mar 29, 2022 at 03:48:21PM -0700, Patel, Nirmal wrote:
> On 3/16/2022 8:51 AM, Nirmal Patel wrote:
> > This patch removes a placeholder patch 2565e5b69c44 ("PCI: vmd: Do
> > not disable MSI-X remapping if interrupt remapping is enabled by IOMMU.")
> > This patch was added as a workaround to disable MSI remapping if iommu
> > enables interrupt remapping. VMD does not assign proper IRQ domain to
> > child devices when MSIX is disabled. There is no dependency between MSI
> > remapping by VMD and interrupt remapping by iommu. MSI remapping can be
> > enabled or disabled with and without interrupt remap.
> >
> > Signed-off-by: Nirmal Patel <nirmal.patel@intel.com>
> > ---
> >  drivers/pci/controller/vmd.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index 3a6570e5b765..91bc1b40d40c 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -6,7 +6,6 @@
> >  
> >  #include <linux/device.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/iommu.h>
> >  #include <linux/irq.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > @@ -813,8 +812,7 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
> >  	 * acceptable because the guest is usually CPU-limited and MSI
> >  	 * remapping doesn't become a performance bottleneck.
> >  	 */
> > -	if (iommu_capable(vmd->dev->dev.bus, IOMMU_CAP_INTR_REMAP) ||
> > -	    !(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> > +	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> >  	    offset[0] || offset[1]) {
> >  		ret = vmd_alloc_irqs(vmd);
> >  		if (ret)

If/when you repost this, please update the subject and commit log to
use "MSI-X" consistently instead of the current mix of "MSI-X" and
"MSIX".

Also s/iommu/IOMMU/.

In subject line, add "PCI: vmd: " prefix and drop trailing period.

Also rewrite commit log in imperative mood, e.g., "Revert 2565e5b69c44
..." instead of "This patch removes ..."  It's 100% clear that the
commit log refers to *this* patch, so it's pointless to include that.

It's further confusing that "This patch was added ..." refers to
*2565e5b69c44*, not this revert.

This reverts 2565e5b69c44 (but doesn't remove the #include
<linux/iommu.h>" added by 2565e5b69c44).

2565e5b69c44 fixed a problem.  If that fix is no longer necessary
because of some other change, the commit log should mention that
change.  Otherwise somebody will backport this fix too far and
reintroduce the problem solved by 2565e5b69c44.

Bjorn