Re: [PATCH 1/6] parisc: add support for patching multiple words

[Sven Schnelle <svens@stackframe.org>]

Hi,

On Tue, May 28, 2019 at 10:19:11AM +0200, Rolf Eike Beer wrote:
> Sven Schnelle wrote:
> > add patch_text_multiple() which allows to patch multiple
> > text words in memory. This can be used to copy functions.
> > +void __patch_text(void *addr, u32 insn);
> > +void __patch_text_multiple(void *addr, u32 *insn, int len);
> 
> A signed length always looks suspicious to me.

Agreed. Will change.

> > +	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.

> 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.

> > +		if (len && !((unsigned long)addr & ~PAGE_MASK)) {
> > +			/*
> > +			 * We're crossing a page boundary, so
> > +			 * need to remap
> > +			 */
> > +			flush_kernel_vmap_range((void *)fixmap,
> > +						(p-fixmap) * sizeof(*p));
> > +			if (mapped)
> > +				patch_unmap(FIX_TEXT_POKE0);
> > +			p = fixmap = patch_map(addr, FIX_TEXT_POKE0, &mapped);
> > +		}
> > +	}
> > +

Regards
Sven