linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: afzal mohammed <afzal.mohd.ma@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	Arnd Bergmann <arnd@arndb.de>,
	Linus Walleij <linus.walleij@linaro.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-mm <linux-mm@kvack.org>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	Nicolas Pitre <nico@fluxnic.net>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [RFC 1/3] lib: copy_{from,to}_user using gup & kmap_atomic()
Date: Sat, 13 Jun 2020 18:59:30 +0530	[thread overview]
Message-ID: <20200613132930.GA4005@afzalpc> (raw)
In-Reply-To: <CAHp75VfRxDH-UE+O7_9W4zyBzPt2+3LeV-=C4iZq2PLwPEbhBw@mail.gmail.com>

Hi,

On Sat, Jun 13, 2020 at 02:08:11PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 12, 2020 at 1:20 PM afzal mohammed <afzal.mohd.ma@gmail.com> wrote:

> > +// Started from arch/um/kernel/skas/uaccess.c
> 
> Does it mean you will deduplicate it there?

What i meant was, that file was taken as a template & nothing more, at
same time i wanted to give credit to that file, i will explicitly
mention it next time.

It is not meant to deduplicate it. Functionality here is completely
different.

In the case here, there would be different virtual address mapping
that CPU will be see once in Kernel as compared to user mode.

Here a facility is provided to access the user page, when the
current virtual address mapping of the CPU excludes it. This
is for providing full 4G virtual address to both user & kernel on
32bit ARM to avoid using highmem or reduce the impact of highmem,
i.e. so that Kernel can address till 4GiB (almost) as lowmem.

Here assumption is that user mapping is not a subset of virtual
address mapped by CPU, but a separate one. Upon Kernel entry ttbr0
is changed to Kernel lowmem, while upon Kernel exit is changed back to
user pages (ttbrx in ARM, iiuc, equivalent to cr3 in x86)

Now realize that i am unable to put coherently the problem being
attempted to solve here to a person not familiar w/ the issue
w/o taking considerable time. If above explanation is not enough,
i will try to explain later in a better way.

> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/highmem.h>
> > +#include <linux/mm.h>
> 
> Perhaps ordered?

will take care

> > +static int do_op_one_page(unsigned long addr, int len,
> > +                int (*op)(unsigned long addr, int len, void *arg), void *arg,
> > +                struct page *page)
> 
> Maybe typedef for the func() ?

will take care

> > +{
> > +       int n;
> > +
> > +       addr = (unsigned long) kmap_atomic(page) + (addr & ~PAGE_MASK);
> 
> I don't remember about this one...

i am not following you here, for my case !CONFIG_64BIT case in that
file was required, hence only it was picked (or rather not deleted)

> > +       size = min(PAGE_ALIGN(addr) - addr, (unsigned long) len);
> 
> ...but here seems to me you can use helpers (offset_in_page() or how
> it's called).

i was not aware of it, will use it as required.

> 
> Also consider to use macros like PFN_DOWN(), PFN_UP(), etc in your code.

Okay

> 
> > +       remain = len;
> > +       if (size == 0)
> > +               goto page_boundary;
> > +
> > +       n = do_op_one_page(addr, size, op, arg, *pages);
> > +       if (n != 0) {
> 
> > +               remain = (n < 0 ? remain : 0);
> 
> Why duplicate three times (!) this line, if you can move it to under 'out'?

yes better to move there

> 
> > +               goto out;
> > +       }
> > +
> > +       pages++;
> > +       addr += size;
> > +       remain -= size;
> > +
> > +page_boundary:
> > +       if (remain == 0)
> > +               goto out;
> > +       while (addr < ((addr + remain) & PAGE_MASK)) {
> > +               n = do_op_one_page(addr, PAGE_SIZE, op, arg, *pages);
> > +               if (n != 0) {
> > +                       remain = (n < 0 ? remain : 0);
> > +                       goto out;
> > +               }
> > +
> > +               pages++;
> > +               addr += PAGE_SIZE;
> > +               remain -= PAGE_SIZE;
> > +       }
> 
> Sounds like this can be refactored to iterate over pages rather than addresses.

Okay, i will check

> > +static int copy_chunk_from_user(unsigned long from, int len, void *arg)
> > +{
> > +       unsigned long *to_ptr = arg, to = *to_ptr;
> > +
> > +       memcpy((void *) to, (void *) from, len);
> 
> What is the point in the casting to void *?

The reason it was there was because of copy-paste :), passing unsigned
long as 'void *' or 'const void *' requires casting right ?, or you
meant something else ?

now i checked removing the cast, compiler is abusing me :), says
'makes pointer from integer without a cast'

> > +       num_pages = DIV_ROUND_UP((unsigned long)from + n, PAGE_SIZE) -
> > +                                (unsigned long)from / PAGE_SIZE;
> 
> PFN_UP() ?

Okay

> I think you can clean up the code a bit after you will get the main
> functionality working.

Yes, surely, intention was to post proof-of-concept ASAP, perhaps
contents will change drastically in next version so that any
resemblence of arch/um/kernel/skas/uaccess.c might not be there.

Regards
afzal

  reply	other threads:[~2020-06-13 13:29 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-12 10:17 [RFC 0/3] ARM: copy_{from,to}_user() for vmsplit 4g/4g afzal mohammed
2020-06-12 10:17 ` [RFC 1/3] lib: copy_{from,to}_user using gup & kmap_atomic() afzal mohammed
2020-06-12 12:02   ` Arnd Bergmann
2020-06-12 13:55     ` afzal mohammed
2020-06-12 20:07       ` Arnd Bergmann
2020-06-13 12:04         ` afzal mohammed
2020-06-13 12:51           ` Al Viro
2020-06-13 12:56             ` Al Viro
2020-06-13 13:42               ` afzal mohammed
2020-06-13 15:31                 ` Al Viro
2020-06-13 15:41                   ` Al Viro
2020-06-13 16:00                     ` Al Viro
2020-06-13 18:55                       ` Arnd Bergmann
2020-06-15 11:22                   ` David Laight
2020-06-13 13:15           ` Russell King - ARM Linux admin
2020-06-14 13:06             ` afzal mohammed
2020-06-13 20:45           ` Arnd Bergmann
2020-06-13 22:16             ` Matthew Wilcox
2020-06-14 13:21             ` afzal mohammed
2020-06-14 14:55               ` afzal mohammed
2020-06-13 11:08   ` Andy Shevchenko
2020-06-13 13:29     ` afzal mohammed [this message]
2020-06-12 10:18 ` [RFC 2/3] ARM: uaccess: let UACCESS_GUP_KMAP_MEMCPY enabling afzal mohammed
2020-06-12 10:18 ` [RFC 3/3] ARM: provide CONFIG_VMSPLIT_4G_DEV for development afzal mohammed
2020-06-12 15:19 ` [RFC 0/3] ARM: copy_{from,to}_user() for vmsplit 4g/4g Nicolas Pitre
2020-06-12 16:01   ` afzal mohammed
2020-06-12 16:03     ` afzal mohammed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200613132930.GA4005@afzalpc \
    --to=afzal.mohd.ma@gmail.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=arnd@arndb.de \
    --cc=catalin.marinas@arm.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux@armlinux.org.uk \
    --cc=nico@fluxnic.net \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).