Re: [PATCH v3 2/2] PCI: vmd: Disable MSI-X remapping when possible

From: "Derrick, Jonathan" <jonathan.derrick@intel.com>
To: "kw@linux.com" <kw@linux.com>
Cc: "joro@8bytes.org" <joro@8bytes.org>,
	"Patel, Nirmal" <nirmal.patel@intel.com>,
	"baolu.lu@linux.intel.com" <baolu.lu@linux.intel.com>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"jroedel@suse.de" <jroedel@suse.de>,
	"Karkra, Kapil" <kapil.karkra@intel.com>,
	"helgaas@kernel.org" <helgaas@kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] PCI: vmd: Disable MSI-X remapping when possible
Date: Mon, 8 Feb 2021 16:30:48 +0000	[thread overview]
Message-ID: <37b6f55d6fad77d7536890a540ecf5369f8de0a9.camel@intel.com> (raw)
In-Reply-To: <YCE4a4swLUTw6j9Y@rocinante>

Hi Krzysztof,

On Mon, 2021-02-08 at 14:11 +0100, Krzysztof Wilczyński wrote:
> Hi Jon,
> 
> Thank you for all the work here!
> 
> Just a number of suggestions, mainly nitpicks, so feel free to ignore
> these, of course.
> 
> [...]
> > +#define VMCFG_MSI_RMP_DIS	0x2
> [...]
> 
> What about calling this VMCONFIG_MSI_REMAP so that is more
> self-explanatory (it also shares some similarity with the
> PCI_REG_VMCONFIG defintition).
> 
> [...]
> > +	VMD_FEAT_BYPASS_MSI_REMAP		= (1 << 4),
> [...]
> 
> Following on the naming that included "HAS" to indicate a feature (or
> support for thereof), perhaps we could name this as, for example:
> 
> 	VMD_FEAT_CAN_BYPASS_MSI_REMAP
> 
> What do you think?
Sure

> 
> [...] 
> > +static void vmd_enable_msi_remapping(struct vmd_dev *vmd, bool enable)
> > +{
> > +	u16 reg;
> > +
> > +	pci_read_config_word(vmd->dev, PCI_REG_VMCONFIG, &reg);
> > +	reg = enable ? (reg & ~VMCFG_MSI_RMP_DIS) : (reg | VMCFG_MSI_RMP_DIS);
> > +	pci_write_config_word(vmd->dev, PCI_REG_VMCONFIG, reg);
> > +}
> 
> I wonder if calling this function vmd_set_msi_remapping() would be more
> aligned with what it does, since it turns the MSI remapping support on
> and off, so to speak, as needed.  Do you think this would be OK to do?
> 
Yes that makes sense

> [...]
> > +		/*
> > +		 * Override the irq domain bus token so the domain can be
> > +		 * distinguished from a regular PCI/MSI domain.
> > +		 */
> 
> It would be "IRQ" here.
> 
No problem!

> Reviewed-by: Krzysztof Wilczyński <kw@linux.com>
> 
Thanks!

> Krzysztof