Message-ID:
 <SN6PR02MB41573C486E03FA07162F7CA7D4CC2@SN6PR02MB4157.namprd02.prod.outlook.com>
Date: Sun, 16 Jun 2024 04:04:11 +0000
From: Michael Kelley <mhklinux@...look.com>
To: Haiyang Zhang <haiyangz@...rosoft.com>, "linux-hyperv@...r.kernel.org"
	<linux-hyperv@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "decui@...rosoft.com" <decui@...rosoft.com>, "stephen@...workplumber.org"
	<stephen@...workplumber.org>, "kys@...rosoft.com" <kys@...rosoft.com>,
	"paulros@...rosoft.com" <paulros@...rosoft.com>, "olaf@...fle.de"
	<olaf@...fle.de>, "vkuznets@...hat.com" <vkuznets@...hat.com>,
	"davem@...emloft.net" <davem@...emloft.net>, "wei.liu@...nel.org"
	<wei.liu@...nel.org>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"leon@...nel.org" <leon@...nel.org>, "longli@...rosoft.com"
	<longli@...rosoft.com>, "ssengar@...ux.microsoft.com"
	<ssengar@...ux.microsoft.com>, "linux-rdma@...r.kernel.org"
	<linux-rdma@...r.kernel.org>, "daniel@...earbox.net" <daniel@...earbox.net>,
	"john.fastabend@...il.com" <john.fastabend@...il.com>, "bpf@...r.kernel.org"
	<bpf@...r.kernel.org>, "ast@...nel.org" <ast@...nel.org>, "hawk@...nel.org"
	<hawk@...nel.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"shradhagupta@...ux.microsoft.com" <shradhagupta@...ux.microsoft.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2, net-next] net: mana: Add support for page sizes other
 than 4KB on ARM64

From: LKML haiyangz <lkmlhyz@...rosoft.com> On Behalf Of Haiyang Zhang Sent: Friday, June 14, 2024 11:36 AM
> 
> As defined by the MANA Hardware spec, the queue size for DMA is 4KB
> minimal, and power of 2. And, the HWC queue size has to be exactly
> 4KB.
> 
> To support page sizes other than 4KB on ARM64, define the minimal
> queue size as a macro separately from the PAGE_SIZE, which we always
> assumed it to be 4KB before supporting ARM64.
> 
> Also, add MANA specific macros and update code related to size
> alignment, DMA region calculations, etc.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@...rosoft.com>
> ---
> v2: Updated alignments, naming as suggested by Michael and Paul.
> 
> ---
>  drivers/net/ethernet/microsoft/Kconfig            |  2 +-
>  drivers/net/ethernet/microsoft/mana/gdma_main.c   | 10 +++++-----
>  drivers/net/ethernet/microsoft/mana/hw_channel.c  | 14 +++++++-------
>  drivers/net/ethernet/microsoft/mana/mana_en.c     |  8 ++++----
>  drivers/net/ethernet/microsoft/mana/shm_channel.c | 13 +++++++------
>  include/net/mana/gdma.h                           | 10 +++++++++-
>  include/net/mana/mana.h                           |  3 ++-
>  7 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/Kconfig
> b/drivers/net/ethernet/microsoft/Kconfig
> index 286f0d5697a1..901fbffbf718 100644
> --- a/drivers/net/ethernet/microsoft/Kconfig
> +++ b/drivers/net/ethernet/microsoft/Kconfig
> @@ -18,7 +18,7 @@ if NET_VENDOR_MICROSOFT
>  config MICROSOFT_MANA
>  	tristate "Microsoft Azure Network Adapter (MANA) support"
>  	depends on PCI_MSI
> -	depends on X86_64 || (ARM64 && !CPU_BIG_ENDIAN && ARM64_4K_PAGES)
> +	depends on X86_64 || (ARM64 && !CPU_BIG_ENDIAN)
>  	depends on PCI_HYPERV
>  	select AUXILIARY_BUS
>  	select PAGE_POOL
> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 1332db9a08eb..aa215e2e9606 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -182,7 +182,7 @@ int mana_gd_alloc_memory(struct gdma_context *gc, unsigned int length,
>  	dma_addr_t dma_handle;
>  	void *buf;
> 
> -	if (length < PAGE_SIZE || !is_power_of_2(length))
> +	if (length < MANA_MIN_QSIZE || !is_power_of_2(length))
>  		return -EINVAL;

Since mana_gd_alloc_memory() is a somewhat generic function that wraps
dma_alloc_coherent(), checking the length against MANA_MIN_QSIZE is
unexpected.  In looking at the call graph, I see that mana_gd_alloc_memory()
is used in creating queues, but all the callers already ensure that the minimum
size requirement is met.  For robustness, having a check here seems OK, but
I would have expected checking against MANA_PAGE_SIZE, since that's the
DMA-related concept.

> 
>  	gmi->dev = gc->dev;
> @@ -717,7 +717,7 @@ EXPORT_SYMBOL_NS(mana_gd_destroy_dma_region, NET_MANA);
>  static int mana_gd_create_dma_region(struct gdma_dev *gd,
>  				     struct gdma_mem_info *gmi)
>  {
> -	unsigned int num_page = gmi->length / PAGE_SIZE;
> +	unsigned int num_page = gmi->length / MANA_PAGE_SIZE;
>  	struct gdma_create_dma_region_req *req = NULL;
>  	struct gdma_create_dma_region_resp resp = {};
>  	struct gdma_context *gc = gd->gdma_context;
> @@ -727,10 +727,10 @@ static int mana_gd_create_dma_region(struct gdma_dev *gd,
>  	int err;
>  	int i;
> 
> -	if (length < PAGE_SIZE || !is_power_of_2(length))
> +	if (length < MANA_MIN_QSIZE || !is_power_of_2(length))

Same here.  Checking against MANA_MIN_QSIZE is unexpected in a function devoted
to DMA functionality.  Callers higher in the call graph already ensure that the
MANA_MIN_QSIZE requirement is met.  Again, for robustness, checking against
MANA_PAGE_SIZE would be OK.

>  		return -EINVAL;
> 
> -	if (offset_in_page(gmi->virt_addr) != 0)
> +	if (!MANA_PAGE_ALIGNED(gmi->virt_addr))
>  		return -EINVAL;
> 
>  	hwc = gc->hwc.driver_data;
> @@ -751,7 +751,7 @@ static int mana_gd_create_dma_region(struct gdma_dev *gd,
>  	req->page_addr_list_len = num_page;
> 
>  	for (i = 0; i < num_page; i++)
> -		req->page_addr_list[i] = gmi->dma_handle +  i * PAGE_SIZE;
> +		req->page_addr_list[i] = gmi->dma_handle +  i * MANA_PAGE_SIZE;
> 
>  	err = mana_gd_send_request(gc, req_msg_size, req, sizeof(resp), &resp);
>  	if (err)
> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index bbc4f9e16c98..cafded2f9382 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -362,12 +362,12 @@ static int mana_hwc_create_cq(struct hw_channel_context *hwc, u16 q_depth,
>  	int err;
> 
>  	eq_size = roundup_pow_of_two(GDMA_EQE_SIZE * q_depth);
> -	if (eq_size < MINIMUM_SUPPORTED_PAGE_SIZE)
> -		eq_size = MINIMUM_SUPPORTED_PAGE_SIZE;
> +	if (eq_size < MANA_MIN_QSIZE)
> +		eq_size = MANA_MIN_QSIZE;
> 
>  	cq_size = roundup_pow_of_two(GDMA_CQE_SIZE * q_depth);
> -	if (cq_size < MINIMUM_SUPPORTED_PAGE_SIZE)
> -		cq_size = MINIMUM_SUPPORTED_PAGE_SIZE;
> +	if (cq_size < MANA_MIN_QSIZE)
> +		cq_size = MANA_MIN_QSIZE;
> 
>  	hwc_cq = kzalloc(sizeof(*hwc_cq), GFP_KERNEL);
>  	if (!hwc_cq)
> @@ -429,7 +429,7 @@ static int mana_hwc_alloc_dma_buf(struct hw_channel_context *hwc, u16 q_depth,
> 
>  	dma_buf->num_reqs = q_depth;
> 
> -	buf_size = PAGE_ALIGN(q_depth * max_msg_size);
> +	buf_size = MANA_PAGE_ALIGN(q_depth * max_msg_size);
> 
>  	gmi = &dma_buf->mem_info;
>  	err = mana_gd_alloc_memory(gc, buf_size, gmi);
> @@ -497,8 +497,8 @@ static int mana_hwc_create_wq(struct hw_channel_context *hwc,
>  	else
>  		queue_size = roundup_pow_of_two(GDMA_MAX_SQE_SIZE * q_depth);
> 
> -	if (queue_size < MINIMUM_SUPPORTED_PAGE_SIZE)
> -		queue_size = MINIMUM_SUPPORTED_PAGE_SIZE;
> +	if (queue_size < MANA_MIN_QSIZE)
> +		queue_size = MANA_MIN_QSIZE;
> 
>  	hwc_wq = kzalloc(sizeof(*hwc_wq), GFP_KERNEL);
>  	if (!hwc_wq)
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index b89ad4afd66e..1381de866b2e 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -1904,10 +1904,10 @@ static int mana_create_txq(struct mana_port_context *apc,
>  	 *  to prevent overflow.
>  	 */
>  	txq_size = MAX_SEND_BUFFERS_PER_QUEUE * 32;
> -	BUILD_BUG_ON(!PAGE_ALIGNED(txq_size));
> +	BUILD_BUG_ON(!MANA_PAGE_ALIGNED(txq_size));
> 
>  	cq_size = MAX_SEND_BUFFERS_PER_QUEUE * COMP_ENTRY_SIZE;
> -	cq_size = PAGE_ALIGN(cq_size);
> +	cq_size = MANA_PAGE_ALIGN(cq_size);
> 
>  	gc = gd->gdma_context;
> 
> @@ -2204,8 +2204,8 @@ static struct mana_rxq *mana_create_rxq(struct mana_port_context *apc,
>  	if (err)
>  		goto out;
> 
> -	rq_size = PAGE_ALIGN(rq_size);
> -	cq_size = PAGE_ALIGN(cq_size);
> +	rq_size = MANA_PAGE_ALIGN(rq_size);
> +	cq_size = MANA_PAGE_ALIGN(cq_size);
> 
>  	/* Create RQ */
>  	memset(&spec, 0, sizeof(spec));
> diff --git a/drivers/net/ethernet/microsoft/mana/shm_channel.c
> b/drivers/net/ethernet/microsoft/mana/shm_channel.c
> index 5553af9c8085..0f1679ebad96 100644
> --- a/drivers/net/ethernet/microsoft/mana/shm_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/shm_channel.c
> @@ -6,6 +6,7 @@
>  #include <linux/io.h>
>  #include <linux/mm.h>
> 
> +#include <net/mana/gdma.h>
>  #include <net/mana/shm_channel.h>
> 
>  #define PAGE_FRAME_L48_WIDTH_BYTES 6
> @@ -155,8 +156,8 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
>  		return err;
>  	}
> 
> -	if (!PAGE_ALIGNED(eq_addr) || !PAGE_ALIGNED(cq_addr) ||
> -	    !PAGE_ALIGNED(rq_addr) || !PAGE_ALIGNED(sq_addr))
> +	if (!MANA_PAGE_ALIGNED(eq_addr) || !MANA_PAGE_ALIGNED(cq_addr) ||
> +	    !MANA_PAGE_ALIGNED(rq_addr) || !MANA_PAGE_ALIGNED(sq_addr))
>  		return -EINVAL;
> 
>  	if ((eq_msix_index & VECTOR_MASK) != eq_msix_index)
> @@ -183,7 +184,7 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
> 
>  	/* EQ addr: low 48 bits of frame address */
>  	shmem = (u64 *)ptr;
> -	frame_addr = PHYS_PFN(eq_addr);
> +	frame_addr = MANA_PFN(eq_addr);
>  	*shmem = frame_addr & PAGE_FRAME_L48_MASK;
>  	all_addr_h4bits |= (frame_addr >> PAGE_FRAME_L48_WIDTH_BITS) <<
>  		(frame_addr_seq++ * PAGE_FRAME_H4_WIDTH_BITS);
> @@ -191,7 +192,7 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
> 
>  	/* CQ addr: low 48 bits of frame address */
>  	shmem = (u64 *)ptr;
> -	frame_addr = PHYS_PFN(cq_addr);
> +	frame_addr = MANA_PFN(cq_addr);
>  	*shmem = frame_addr & PAGE_FRAME_L48_MASK;
>  	all_addr_h4bits |= (frame_addr >> PAGE_FRAME_L48_WIDTH_BITS) <<
>  		(frame_addr_seq++ * PAGE_FRAME_H4_WIDTH_BITS);
> @@ -199,7 +200,7 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
> 
>  	/* RQ addr: low 48 bits of frame address */
>  	shmem = (u64 *)ptr;
> -	frame_addr = PHYS_PFN(rq_addr);
> +	frame_addr = MANA_PFN(rq_addr);
>  	*shmem = frame_addr & PAGE_FRAME_L48_MASK;
>  	all_addr_h4bits |= (frame_addr >> PAGE_FRAME_L48_WIDTH_BITS) <<
>  		(frame_addr_seq++ * PAGE_FRAME_H4_WIDTH_BITS);
> @@ -207,7 +208,7 @@ int mana_smc_setup_hwc(struct shm_channel *sc, bool reset_vf, u64 eq_addr,
> 
>  	/* SQ addr: low 48 bits of frame address */
>  	shmem = (u64 *)ptr;
> -	frame_addr = PHYS_PFN(sq_addr);
> +	frame_addr = MANA_PFN(sq_addr);
>  	*shmem = frame_addr & PAGE_FRAME_L48_MASK;
>  	all_addr_h4bits |= (frame_addr >> PAGE_FRAME_L48_WIDTH_BITS) <<
>  		(frame_addr_seq++ * PAGE_FRAME_H4_WIDTH_BITS);
> diff --git a/include/net/mana/gdma.h b/include/net/mana/gdma.h
> index c547756c4284..83963d9e804d 100644
> --- a/include/net/mana/gdma.h
> +++ b/include/net/mana/gdma.h
> @@ -224,7 +224,15 @@ struct gdma_dev {
>  	struct auxiliary_device *adev;
>  };
> 
> -#define MINIMUM_SUPPORTED_PAGE_SIZE PAGE_SIZE
> +/* MANA_PAGE_SIZE is the DMA unit */
> +#define MANA_PAGE_SHIFT 12
> +#define MANA_PAGE_SIZE BIT(MANA_PAGE_SHIFT)
> +#define MANA_PAGE_ALIGN(x) ALIGN((x), MANA_PAGE_SIZE)
> +#define MANA_PAGE_ALIGNED(addr) IS_ALIGNED((unsigned long)(addr),
> MANA_PAGE_SIZE)
> +#define MANA_PFN(a) ((a) >> MANA_PAGE_SHIFT)
> +
> +/* Required by HW */
> +#define MANA_MIN_QSIZE MANA_PAGE_SIZE
> 
>  #define GDMA_CQE_SIZE 64
>  #define GDMA_EQE_SIZE 16
> diff --git a/include/net/mana/mana.h b/include/net/mana/mana.h
> index 59823901b74f..e39b8676fe54 100644
> --- a/include/net/mana/mana.h
> +++ b/include/net/mana/mana.h
> @@ -42,7 +42,8 @@ enum TRI_STATE {
> 
>  #define MAX_SEND_BUFFERS_PER_QUEUE 256
> 
> -#define EQ_SIZE (8 * PAGE_SIZE)
> +#define EQ_SIZE (8 * MANA_PAGE_SIZE)
> +
>  #define LOG2_EQ_THROTTLE 3
> 
>  #define MAX_PORTS_IN_MANA_DEV 256
> --
> 2.34.1
> 

The rest of the changes here look good to me.

Michael

Powered by blists - more mailing lists