Re: [PATCH v4] PCI: vmd: Disable MSI remap only for low MSI count

[Bjorn Helgaas <helgaas@kernel.org>]

I think this refers specifically to MSI-X, so use "MSI-X" in the
subject.

On Thu, Apr 18, 2024 at 11:31:21AM -0400, Nirmal Patel wrote:
> VMD MSI remapping is disabled by default for all the CPUs with 28c0 VMD
> deviceID. We used to disable remapping because drives supported more
> vectors than the VMD so the performance was better without remapping.
> Now with CPUs that support more than 64 (128 VMD MSIx vectors for gen5)
> we no longer need to disable this feature.

"because drives supported more vectors" ... I guess you are referring
to typical devices that might be behind a VMD?  But I assume there's
no actual requirement that those devices be "drives", right?

"CPUs that support more than 64 ... 128 VMD vectors"  Are we talking
about *CPUs* that support more vectors, or *VMDs* that support more
vectors?

I guess you probably think of CPUs here because VMD is integrated into
the same package, right?  That would explain the "CPUs with 28c0 VMD"
comment.  But the vmd driver doesn't care about that; it just claims a
PCI device.

s/MSI remapping/MSI-X remapping/ (I think?)
s/MSIx/MSI-X/ to match spec usage.

A reference to ee81ee84f873 ("PCI: vmd: Disable MSI-X remapping when
possible"), which added VMD_FEAT_CAN_BYPASS_MSI_REMAP, might be
useful because it has nice context.

IIUC this will keep MSI-X remapping enabled in more cases, e.g., on
new devices that support more vectors.  What is the benefit of keeping
it enabled?

The ee81ee84f873 commit log suggests two issues:

  - Number of vectors available to child domain is limited by size of
    VMD MSI-X table.

  - Remapping means child interrupts have to go through the VMD domain
    interrupt handler instead of going straight to the device handler.

But this commit log suggests that with more vectors, you can enable
remapping even without a performance penalty?  Maybe the VMD domain
interrupt handler was only needed because of vector sharing?

I'm just a little confused because this commit log doesn't say what
the actual benefit is, other than "keeping remapping enabled", and I
don't know enough to know why that's good.

> Note, pci_msix_vec_count() failure is translated to ENODEV per typical
> expectations that drivers may return ENODEV when some driver-known
> fundamental detail of the device is missing.
> 
> Signed-off-by: Nirmal Patel <nirmal.patel@linux.intel.com>
> ---
> v1->v2: Updating commit message.
> v2->v3: Use VMD MSI count instead of cpu count.
> v3->v4: Updating commit message.
> ---
>  drivers/pci/controller/vmd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 769eedeb8802..ba63af70bb63 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -34,6 +34,8 @@
>  #define MB2_SHADOW_OFFSET	0x2000
>  #define MB2_SHADOW_SIZE		16
>  
> +#define VMD_MIN_MSI_VECTOR_COUNT 64
> +
>  enum vmd_features {
>  	/*
>  	 * Device may contain registers which hint the physical location of the
> @@ -807,13 +809,20 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  
>  	sd->node = pcibus_to_node(vmd->dev->bus);
>  
> +	vmd->msix_count = pci_msix_vec_count(vmd->dev);
> +	if (vmd->msix_count < 0)
> +		return -ENODEV;
> +
>  	/*
>  	 * Currently MSI remapping must be enabled in guest passthrough mode
>  	 * due to some missing interrupt remapping plumbing. This is probably
>  	 * acceptable because the guest is usually CPU-limited and MSI
>  	 * remapping doesn't become a performance bottleneck.

This part of the comment might need some updating.  I don't see the
connection with guest passthrough mode in the code.

> +	 * Disable MSI remapping only if supported by VMD hardware and when
> +	 * VMD MSI count is less than or equal to minimum MSI count.

Add blank line between paragraphs or rewrap into a single paragraph.

>  	 */
>  	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> +	    vmd->msix_count > VMD_MIN_MSI_VECTOR_COUNT ||
>  	    offset[0] || offset[1]) {

I think this conditional might be easier to read if it were inverted:

  if (features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) && ...) {
    vmd_set_msi_remapping(vmd, false);
  } else {
    ret = vmd_alloc_irqs(vmd);
    ...
  }

Maybe a separate patch though.

>  		ret = vmd_alloc_irqs(vmd);
>  		if (ret)
> -- 
> 2.31.1
>