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

["Derrick, Jonathan" <jonathan.derrick@intel.com>]

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