Sven Schnelle wrote:
> On Tue, May 28, 2019 at 10:19:11AM +0200, Rolf Eike Beer wrote:
> > Sven Schnelle wrote:
> > > + p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
> > > +
> > > + while (len > 0) {
> > > + *p++ = *insn++;
> > > + addr += 4;
> > > + len -= sizeof(u32);
> >
> > I wonder if this can't just use memcpy inside the pages?
>
> I think using memcpy here makes things just more complicated and harder to
> read. We would need to extract the amount of bytes to copy, and call memcpy
> multiple times. As this code is not performance critical and usually only
> copies only a few bytes i doubt that it's worth the effort.
>
> > If not there should be a comment describing what's going on here.
>
> Is it that complicated? It's just a copy loop like in every C beginner book,
> the only things that makes things more complicated is the need to remap
> when crossing a page.
The copy loop not. But things like "why are you doing it backwards" come to
mind. Be careful when you change the length to unsigned, your loop will not
work this way anymore afterwards.
> > Another nitpick: the "+4" and "-sizeof(u32)" are just the same at the end,
> > but why do they use entirely different wording? What do we need "addr" for
> > anyway, one could just look at "p" which would cross a page boundary at
> > the
> > same time, no?
>
> You can't, because of the patch_map() call below which updates the fixed
> mapping. That call needs the real virtual address, while *p holds the
> virtual address of the fixed mapping for patching.
Can that remapping really place it at a non-zero offset regarding to the
underlying page? That it moves the page descriptor around is normal, but it
will keep the low order bits intact, so the page boundary crossing will be
still the same, no?
EikeAttachment:
signature.asc
Description: This is a digitally signed message part.
