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

["Patel, Nirmal" <nirmal.patel@linux.intel.com>]

On 3/30/2022 10:49 AM, Bjorn Helgaas wrote:
> 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

I will apply these changes.

Thank you
Nirmal