[PATCH 1/1] PCI: hv: Fix ring buffer size calculation

[PATCH 1/1] PCI: hv: Fix ring buffer size calculation

[mhkelley58]

From: Michael Kelley <mhklinux@outlook.com>

For a physical PCI device that is passed through to a Hyper-V guest VM,
current code specifies the VMBus ring buffer size as 4 pages.  But this
is an inappropriate dependency, since the amount of ring buffer space
needed is unrelated to PAGE_SIZE. For example, on x86 the ring buffer
size ends up as 16 Kbytes, while on ARM64 with 64 Kbyte pages, the ring
size bloats to 256 Kbytes. The ring buffer for PCI pass-thru devices
is used for only a few messages during device setup and removal, so any
space above a few Kbytes is wasted.

Fix this by declaring the ring buffer size to be a fixed 16 Kbytes.
Furthermore, use the VMBUS_RING_SIZE() macro so that the ring buffer
header is properly accounted for, and so the size is rounded up to a
page boundary, using the page size for which the kernel is built. While
w/64 Kbyte pages this results in a 64 Kbyte ring buffer header plus a
64 Kbyte ring buffer, that's the smallest possible with that page size.
It's still 128 Kbytes better than the current code.

Cc: <stable@vger.kernel.org> # 5.15.x
Signed-off-by: Michael Kelley <mhklinux@outlook.com>
---
 drivers/pci/controller/pci-hyperv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 1eaffff40b8d..5f22ad38bb98 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -465,7 +465,7 @@ struct pci_eject_response {
 	u32 status;
 } __packed;
 
-static int pci_ring_size = (4 * PAGE_SIZE);
+static int pci_ring_size = VMBUS_RING_SIZE(16 * 1024);
 
 /*
  * Driver specific state.
-- 
2.25.1

Re: [PATCH 1/1] PCI: hv: Fix ring buffer size calculation

[Kuppuswamy Sathyanarayanan]

On 2/12/24 10:19 PM, mhkelley58@gmail.com wrote:
> From: Michael Kelley <mhklinux@outlook.com>
>
> For a physical PCI device that is passed through to a Hyper-V guest VM,
> current code specifies the VMBus ring buffer size as 4 pages.  But this
> is an inappropriate dependency, since the amount of ring buffer space
> needed is unrelated to PAGE_SIZE. For example, on x86 the ring buffer
> size ends up as 16 Kbytes, while on ARM64 with 64 Kbyte pages, the ring
> size bloats to 256 Kbytes. The ring buffer for PCI pass-thru devices
> is used for only a few messages during device setup and removal, so any
> space above a few Kbytes is wasted.
>
> Fix this by declaring the ring buffer size to be a fixed 16 Kbytes.
> Furthermore, use the VMBUS_RING_SIZE() macro so that the ring buffer
> header is properly accounted for, and so the size is rounded up to a
> page boundary, using the page size for which the kernel is built. While
> w/64 Kbyte pages this results in a 64 Kbyte ring buffer header plus a
> 64 Kbyte ring buffer, that's the smallest possible with that page size.
> It's still 128 Kbytes better than the current code.
>
> Cc: <stable@vger.kernel.org> # 5.15.x
> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> ---
Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>  drivers/pci/controller/pci-hyperv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 1eaffff40b8d..5f22ad38bb98 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -465,7 +465,7 @@ struct pci_eject_response {
>  	u32 status;
>  } __packed;
>  
> -static int pci_ring_size = (4 * PAGE_SIZE);
> +static int pci_ring_size = VMBUS_RING_SIZE(16 * 1024);
>  
Nit: I think you can use SZ_16K to make it more readable.
>  /*
>   * Driver specific state.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/1] PCI: hv: Fix ring buffer size calculation

[Ilpo Järvinen]

On Mon, 12 Feb 2024, mhkelley58@gmail.com wrote:

> From: Michael Kelley <mhklinux@outlook.com>
> 
> For a physical PCI device that is passed through to a Hyper-V guest VM,
> current code specifies the VMBus ring buffer size as 4 pages.  But this
> is an inappropriate dependency, since the amount of ring buffer space
> needed is unrelated to PAGE_SIZE. For example, on x86 the ring buffer
> size ends up as 16 Kbytes, while on ARM64 with 64 Kbyte pages, the ring
> size bloats to 256 Kbytes. The ring buffer for PCI pass-thru devices
> is used for only a few messages during device setup and removal, so any
> space above a few Kbytes is wasted.
> 
> Fix this by declaring the ring buffer size to be a fixed 16 Kbytes.
> Furthermore, use the VMBUS_RING_SIZE() macro so that the ring buffer
> header is properly accounted for, and so the size is rounded up to a
> page boundary, using the page size for which the kernel is built. While
> w/64 Kbyte pages this results in a 64 Kbyte ring buffer header plus a
> 64 Kbyte ring buffer, that's the smallest possible with that page size.
> It's still 128 Kbytes better than the current code.
> 
> Cc: <stable@vger.kernel.org> # 5.15.x
> Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> ---
>  drivers/pci/controller/pci-hyperv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 1eaffff40b8d..5f22ad38bb98 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -465,7 +465,7 @@ struct pci_eject_response {
>  	u32 status;
>  } __packed;
>  
> -static int pci_ring_size = (4 * PAGE_SIZE);
> +static int pci_ring_size = VMBUS_RING_SIZE(16 * 1024);

SZ_16K (and add the relevant #include if needed).

-- 
 i.

RE: [PATCH 1/1] PCI: hv: Fix ring buffer size calculation

[Michael Kelley]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> On 2/12/24 10:19 PM, mhkelley58@gmail.com wrote:
> > From: Michael Kelley <mhklinux@outlook.com>
> >
> > For a physical PCI device that is passed through to a Hyper-V guest VM,
> > current code specifies the VMBus ring buffer size as 4 pages.  But this
> > is an inappropriate dependency, since the amount of ring buffer space
> > needed is unrelated to PAGE_SIZE. For example, on x86 the ring buffer
> > size ends up as 16 Kbytes, while on ARM64 with 64 Kbyte pages, the ring
> > size bloats to 256 Kbytes. The ring buffer for PCI pass-thru devices
> > is used for only a few messages during device setup and removal, so any
> > space above a few Kbytes is wasted.
> >
> > Fix this by declaring the ring buffer size to be a fixed 16 Kbytes.
> > Furthermore, use the VMBUS_RING_SIZE() macro so that the ring buffer
> > header is properly accounted for, and so the size is rounded up to a
> > page boundary, using the page size for which the kernel is built. While
> > w/64 Kbyte pages this results in a 64 Kbyte ring buffer header plus a
> > 64 Kbyte ring buffer, that's the smallest possible with that page size.
> > It's still 128 Kbytes better than the current code.
> >
> > Cc: <stable@vger.kernel.org> # 5.15.x
> > Signed-off-by: Michael Kelley <mhklinux@outlook.com>
> > ---
> Looks good to me.
> 
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >  drivers/pci/controller/pci-hyperv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index 1eaffff40b8d..5f22ad38bb98 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -465,7 +465,7 @@ struct pci_eject_response {
> >  	u32 status;
> >  } __packed;
> >
> > -static int pci_ring_size = (4 * PAGE_SIZE);
> > +static int pci_ring_size = VMBUS_RING_SIZE(16 * 1024);
> >
> Nit: I think you can use SZ_16K to make it more readable.

Thanks for the review.  I'll send a v2 that uses SZ_16K per your
and Ilpo Järvinen's comment.

Michael