> -----Original Message-----
> From: Michael Kelley <mhklinux@xxxxxxxxxxx>
> Sent: Tuesday, June 11, 2024 10:42 PM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; linux-hyperv@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; Paul Rosswurm <paulros@xxxxxxxxxxxxx>
> Cc: Dexuan Cui <decui@xxxxxxxxxxxxx>; stephen@xxxxxxxxxxxxxxxxxx; KY
> Srinivasan <kys@xxxxxxxxxxxxx>; olaf@xxxxxxxxx; vkuznets
> <vkuznets@xxxxxxxxxx>; davem@xxxxxxxxxxxxx; wei.liu@xxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; leon@xxxxxxxxxx;
> Long Li <longli@xxxxxxxxxxxxx>; ssengar@xxxxxxxxxxxxxxxxxxx; linux-
> rdma@xxxxxxxxxxxxxxx; daniel@xxxxxxxxxxxxx; john.fastabend@xxxxxxxxx;
> bpf@xxxxxxxxxxxxxxx; ast@xxxxxxxxxx; hawk@xxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> shradhagupta@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH net-next] net: mana: Add support for variable page
> sizes of ARM64
> > > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > index 1332db9a08eb..c9df942d0d02 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;
> > > >
> > > > 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_MIN_QSIZE;
> > >
> > > This calculation seems a bit weird when using MANA_MIN_QSIZE. The
> > > number of pages, and the construction of the page_addr_list array
> > > a few lines later, seem unrelated to the concept of a minimum queue
> > > size. Is the right concept really a "mapping chunk", and num_page
> > > would conceptually be "num_chunks", or something like that? Then
> > > a queue must be at least one chunk in size, but that's derived from
> the
> > > chunk size, and is not the core concept.
> >
> > I think calling it "num_chunks" is fine.
> > May I use "num_chunks" in next version?
> >
>
> I think first is the decision on what to use for MANA_MIN_QSIZE. I'm
> admittedly not familiar with mana and gdma, but the function
> mana_gd_create_dma_region() seems fairly typical in defining a
> logical region that spans multiple 4K chunks that may or may not
> be physically contiguous. So you set up an array of physical
> addresses that identify the physical memory location of each chunk.
> It seems very similar to a Hyper-V GPADL. I typically *do* see such
> chunks called "pages", but a "mapping chunk" or "mapping unit"
> is probably OK too. So mana_gd_create_dma_region() would use
> MANA_CHUNK_SIZE instead of MANA_MIN_QSIZE. Then you could
> also define MANA_MIN_QSIZE to be MANA_CHUNK_SIZE, for use
> specifically when checking the size of a queue.
>
> Then if you are using MANA_CHUNK_SIZE, the local variable
> would be "num_chunks". The use of "page_count", "page_addr_list",
> and "offset_in_page" field names in struct
> gdma_create_dma_region_req should then be changed as well.
I'm fine with these names. I will check with Paul too.
I'd define just one macro, with a code comment like this. It
will be used for chunk calculation and q len checking:
/* Chunk size of MANA DMA, which is also the min queue size */
#define MANA_CHUNK_SIZE 4096
> Looking further at the function mana_gd_create_dma_region(),
> there's also the use of offset_in_page(), which is based on the
> guest PAGE_SIZE. Wouldn't this be problematic if PAGE_SIZE
> is 64K?
As in my other email - I confirmed with Hostnet team that the
alignment requirement is just 4k.
So I will relax the following check to be 4k alignment too:
if (offset_in_page(gmi->virt_addr) != 0)
return -EINVAL;
Thanks,
- Haiyang
