| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1386076906.839.32.camel@oc7383187364.ibm.com>
Date: Tue, 03 Dec 2013 14:21:46 +0100
From: Frank Haverkamp <haver@...ux.vnet.ibm.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, arnd@...db.de,
cody@...ux.vnet.ibm.com, schwidefsky@...ibm.com,
utz.bacher@...ibm.com, mmarek@...e.cz, rmallon@...il.com,
jsvogt@...ibm.com, MIJUNG@...ibm.com, cascardo@...ux.vnet.ibm.com,
michael@...ra.de
Subject: Re: [PATCH 3/6] GenWQE Utility functions
Hi Greg,
Am Mittwoch, den 27.11.2013, 11:26 -0800 schrieb Greg KH:
> On Wed, Nov 06, 2013 at 01:45:40PM +0100, Frank Haverkamp wrote:
> > +/**
> > + * init_crc32() - Prepare a lookup table for fast crc32 calculations
> > + *
> > + * Existing kernel functions seem to use a different polynom,
> > + * therefore we could not use them here.
> > + *
> > + * Genwqe's Polynomial = 0x20044009
> > + */
> > +#define CRC32_POLYNOMIAL 0x20044009
> > +static u32 crc32_tab[256]; /* crc32 lookup table */
> > +
> > +void init_crc32(void)
>
> That's a _very_ generic function name to now be global in the whole
> kernel.
>
> And why not just add new polynomial functionality to the existing crc32
> in-kernel code instead of rolling your own here?
>
The name is indeed to generic. I fixed that. It is for sure a candidate
to be covered by the in-kernel crc support. I had a look and found that
static void crc32init_be(void)
is unfortunately defined static and that the polynomial is missing.
Maybe:
void crc32init_be_generic(const uint32_t polynomial,
uint32_t (*tab)[256])
inline u32 __pure crc32_be_generic(u32 crc, unsigned char const *p,
size_t len, const u32 (*tab)[256],
u32 polynomial)
would be good to have. Adding my own "crc32table_genwqe_be[]" to the
generic code might be waste memory. None except of us are using that
particular polynomial, as far as I saw. So enhancing the table
generation function and exporting that and the crc32_be_generic might be
a good idea.
In my latest version of the patchset I did not include such a proposal
(yet).
> > +static void genwqe_dump_sgl(struct genwqe_dev *cd, struct sg_entry *sgl,
> > + size_t sgl_size)
> > +{
> > + unsigned int i, j;
> > + struct pci_dev *pci_dev = cd->pci_dev;
> > +
> > + for (j = 0, i = 0; i < sgl_size/sizeof(struct sg_entry); i++, j++) {
> > + if (j == 8) {
> > + dev_info(&pci_dev->dev, " --\n");
> > + j = 0;
> > + }
> > + dev_info(&pci_dev->dev, " %016llx %08x %08x %s\n",
> > + be64_to_cpu(sgl[i].target_addr),
> > + be32_to_cpu(sgl[i].len),
> > + be32_to_cpu(sgl[i].flags),
> > + (be32_to_cpu(sgl[i].len) > PAGE_SIZE) ? "C" : "");
> > +
> > + if (be32_to_cpu(sgl[i].flags) == SG_END_LIST)
> > + break;
> > + }
> > +}
>
> That's a lot of kernel log mess, why?
I removed it. It was for debugging the scatter gather lists our hardware
uses. Since it works now, I can get rid of the printing.
> > +/**
> > + * free_user_pages() - Give pinned pages back
> > + *
> > + * Documentation of get_user_pages is in mm/memory.c:
> > + *
> > + * If the page is written to, set_page_dirty (or set_page_dirty_lock,
> > + * as appropriate) must be called after the page is finished with, and
> > + * before put_page is called.
> > + */
> > +static int free_user_pages(struct page **page_list, unsigned int nr_pages,
> > + int dirty)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < nr_pages; i++) {
> > + if (page_list[i] != NULL) {
> > + if (dirty)
> > + set_page_dirty_lock(page_list[i]);
> > + put_page(page_list[i]);
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * user_vmap() - Map user-space memory to virtual kernel memory
> > + * @cd: pointer to genwqe device
> > + * @m: mapping params
> > + * @uaddr: user virtual address
> > + * @size: size of memory to be mapped
> > + *
> > + * We need to think about how we could speed this up. Of course it is
> > + * not a good idea to do this over and over again, like we are
> > + * currently doing it. Nevertheless, I am curious where on the path
> > + * the performance is spend. Most probably within the memory
> > + * allocation functions, but maybe also in the DMA mapping code.
> > + *
> > + * Restrictions: The maximum size of the possible mapping currently depends
> > + * on the amount of memory we can get using kzalloc() for the
> > + * page_list and pci_alloc_coherent for the sg_list.
> > + * The sg_list is currently itself not scattered, which could
> > + * be fixed with some effort. The page_list must be split into
> > + * PAGE_SIZE chunks too. All that will make the complicated
> > + * code more complicated.
> > + *
> > + * Return: 0 if success
> > + */
> > +int user_vmap(struct genwqe_dev *cd, struct dma_mapping *m, void *uaddr,
> > + unsigned long size, struct ddcb_requ *req)
>
> Again, _very_ generic global symbol name. Please audit all of these.
I prefixed those and found some others too. I hope that I have found all
similar cases in our code.
> And shouldn't this just be a core mm function? Why is this in a driver?
The current implementation is at the moment not generic enough to be
useful for others. But you are right, something alike might be useful if
there is a 2nd or 3rd user with same/similar needs.
> > +/**
> > + * user_vunmap() - Undo mapping of user-space memory to virtual kernel memory
> > + * @cd: pointer to genwqe device
> > + * @m: mapping params
> > + */
> > +int user_vunmap(struct genwqe_dev *cd, struct dma_mapping *m,
> > + struct ddcb_requ *req)
>
> Same as above.
>
> thanks,
>
> greg k-h
>
Regards
Frank
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists