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 = 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@xxxxxxxxx>
>
Thanks!
> Krzysztof
