Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > The printk removals do change the objects.
> > 
> > The value of that type of change is only for resource limited systems.
> 
> I imagine that such small code adjustments are also useful for other systems.

Your imagination and mine differ.
Where do you _think_ it matters?

For instance, nothing about

sizeof(type)
vs
sizeof(*ptr)

makes it easier for a human to read the code.

This class of change now require a syntactic parser
to find instances of the use of type where previously
a grep or equivalent tool worked well.

> > Markus' changelogs leave much to be desired.
> 
> Would you like to help more to improve the provided information
> for the shown change patterns?

I've done that for you far too many times already.

Your changelogs need to detail _why_ something is being
done, not describe any tool used to perform or find a
particular instance of any change.

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>> I imagine that such small code adjustments are also useful for other systems.
> 
> Your imagination and mine differ.

This can generally be.


> Where do you _think_ it matters?

It seems that this discussion branch referred still to my cover letter
for possible changes in the TPM software area.

The four update steps (in this patch series) demonstrate different
change possibilities which could be desired.
Would you like to distinguish them a bit more?


> For instance, nothing about
> 
>   sizeof(type)
> vs
>   sizeof(*ptr)
> 
> makes it easier for a human to read the code.

I could agree to this view (in the general short form).
But nine statements became shorter in the concrete update suggestion
so that such a reduction could help the trained eyes
of some software developers and code reviewers.


> This class of change now require a syntactic parser
> to find instances of the use of type where previously
> a grep or equivalent tool worked well.

Does the Linux coding style convention prefer safety over this
data processing concern?


>>> Markus' changelogs leave much to be desired.
>>
>> Would you like to help more to improve the provided information
>> for the shown change patterns?
> 
> I've done that for you far too many times already.

I got an other impression.
You gave constructive feedback (also for me) occasionally.

There were a few cases where a desired agreement was not achieved so far.


> Your changelogs need to detail _why_ something is being done,

I could improve descriptions if involved information sources
could also become clearer and really safe.


> not describe any tool used to perform or find a
> particular instance of any change.

This part refers to a bit of attribution.

Regards,
Markus

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

> The printk removals do change the objects.
> 
> The value of that type of change is only for resource limited systems.

I imagine that such small code adjustments are also useful for other systems.


> Markus' changelogs leave much to be desired.

Would you like to help more to improve the provided information
for the shown change patterns?

Regards,
Markus

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > The printk removals do change the objects.
> > >
> > > The value of that type of change is only for resource limited systems.
> >
> > I imagine that such small code adjustments are also useful for other
> systems.
> 
> Your imagination and mine differ.
> Where do you _think_ it matters?
> 
> For instance, nothing about
> 
>   sizeof(type)
> vs
>   sizeof(*ptr)
> 
> makes it easier for a human to read the code.

If it does not make it easier to read the code for you, then maybe you should 
consider that this might not be true for all humans. For me, it makes it much 
easier to see at a glance, that code like ptr=malloc(sizeof(*ptr)) is correct.

Alexander

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 2017-10-18 at 10:44 +, alexander.stef...@infineon.com wrote:
> > > For instance, nothing about
> > > > >   sizeof(type)
> > > > > vs
> > > > >   sizeof(*ptr)
> > > > > makes it easier for a human to read the code.
> > > >
> > > > If it does not make it easier to read the code for you, then maybe you
> > > > should consider that this might not be true for all humans. For me, it
> > > > makes it much easier to see at a glance, that code like
> > > > ptr=malloc(sizeof(*ptr)) is correct.
> > >
> > > I don't think there is a perfect solution.
> >
> > Maybe. But for the second variant the correctness is easier to check,
> 
> How often should
>   ptr = alloc(sizeof(*ptr))
> be
>   ptr = alloc(sizeof(**ptr))

Never? Because in that case it probably should be *ptr=alloc(sizeof(**ptr)), 
unless you are doing something horrible ;-)

Alexander

RE: char-TPM: Adjustments for ten function implementations

[Julia Lawall]

On Wed, 18 Oct 2017, alexander.stef...@infineon.com wrote:

> > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > The printk removals do change the objects.
> > > >
> > > > The value of that type of change is only for resource limited systems.
> > >
> > > I imagine that such small code adjustments are also useful for other
> > systems.
> >
> > Your imagination and mine differ.
> > Where do you _think_ it matters?
> >
> > For instance, nothing about
> >
> > sizeof(type)
> > vs
> > sizeof(*ptr)
> >
> > makes it easier for a human to read the code.
>
> If it does not make it easier to read the code for you, then maybe you
> should consider that this might not be true for all humans. For me, it
> makes it much easier to see at a glance, that code like
> ptr=malloc(sizeof(*ptr)) is correct.

I don't think there is a perfect solution.  The type argument to sizeof
could have the wrong type.  The expression argument to sizeof could be
missing the *.  Unpleasant consequences are possible in both cases.
Probably each maintainer has a style they prefer.  Perhaps it could be
useful to adjust the code to follow the dominant strategy, in cases where
there are a inconsistencies.  For example

if (...)
  x = foo1(sizeof(struct xtype));
else
  x = foo2(sizeof(*x));

might at least cause some unnecessary mental effort to process.

julia

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 12:00 +0200, Julia Lawall wrote:
> 
> On Wed, 18 Oct 2017, alexander.stef...@infineon.com wrote:
> 
> > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > > The printk removals do change the objects.
> > > > > 
> > > > > The value of that type of change is only for resource limited systems.
> > > > 
> > > > I imagine that such small code adjustments are also useful for other
> > > 
> > > systems.
> > > 
> > > Your imagination and mine differ.
> > > Where do you _think_ it matters?
> > > 
> > > For instance, nothing about
> > > 
> > >   sizeof(type)
> > > vs
> > >   sizeof(*ptr)
> > > 
> > > makes it easier for a human to read the code.
> > 
> > If it does not make it easier to read the code for you, then maybe you
> > should consider that this might not be true for all humans. For me, it
> > makes it much easier to see at a glance, that code like
> > ptr=malloc(sizeof(*ptr)) is correct.
> 
> I don't think there is a perfect solution.  The type argument to sizeof
> could have the wrong type.  The expression argument to sizeof could be
> missing the *.

Yup.

Today, even after all of Markus' patches for this style
conversion, there is still only ~2:1 preference for
ptr = k.alloc(sizeof(*ptr))
over
ptr = k.alloc(sizeof(struct foo))
in the kernel tree

Ugly grep follows:

$ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
  sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo = 
k.alloc(sizeof(*foo))/' \
 -e 
's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = 
k.alloc(sizeof(struct foo))/' | \
  sort | uniq -c | sort -rn | head -2
   6123 foo = k.alloc(sizeof(*foo)),
   3060 foo = k.alloc(sizeof(struct foo)),

> Unpleasant consequences are possible in both cases.

Yup.

> Probably each maintainer has a style they prefer.  Perhaps it could be
> useful to adjust the code to follow the dominant strategy, in cases where
> there are a inconsistencies.  For example
> 
> if (...)
>   x = foo1(sizeof(struct xtype));
> else
>   x = foo2(sizeof(*x));
> 
> might at least cause some unnecessary mental effort to process.

Sure, but perhaps _only_ when there are inconsistencies
in the same compilation unit.'

Re: [PATCH v2 2/2] vgaarb: Factor out EFI and fallback default device selection

[Daniel Vetter]

On Wed, Oct 18, 2017 at 11:24:43AM +1100, Daniel Axtens wrote:
> Hi Daniel,
> 
> >> Initially I wondered if this info printk could be moved into
> >> vga_arbiter_check_bridge_sharing(), but it's been separated out since
> >> 3448a19da479b ("vgaarb: use bridges to control VGA routing where
> >> possible."), and upon closer examination, it seems you can't be sure a
> >> device doesn't share a bridge until the end of the process, so this is
> >> indeed correct.
> >> 
> >> Everything else also looks good to me.
> >> 
> >> Reviewed-by: Daniel Axtens 
> >
> > R-b for both patches? And ok with everyone if I pull this into drm-misc
> > for 4.15 (deadline is end of this week for feature-y stuff)?
> 
> I had only intended it for patch 2, but I've now read over patch 1 to my
> satisfaction, so it too is:
> 
> Reviewed-by: Daniel Axtens 

Both applied for 4.15, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 2/2] vgaarb: Factor out EFI and fallback default device selection

[Daniel Axtens]

Daniel Vetter  writes:

> On Wed, Oct 18, 2017 at 11:24:43AM +1100, Daniel Axtens wrote:
>> Hi Daniel,
>> 
>> >> Initially I wondered if this info printk could be moved into
>> >> vga_arbiter_check_bridge_sharing(), but it's been separated out since
>> >> 3448a19da479b ("vgaarb: use bridges to control VGA routing where
>> >> possible."), and upon closer examination, it seems you can't be sure a
>> >> device doesn't share a bridge until the end of the process, so this is
>> >> indeed correct.
>> >> 
>> >> Everything else also looks good to me.
>> >> 
>> >> Reviewed-by: Daniel Axtens 
>> >
>> > R-b for both patches? And ok with everyone if I pull this into drm-misc
>> > for 4.15 (deadline is end of this week for feature-y stuff)?
>> 
>> I had only intended it for patch 2, but I've now read over patch 1 to my
>> satisfaction, so it too is:
>> 
>> Reviewed-by: Daniel Axtens 
>
> Both applied for 4.15, thanks.

Cool - thanks!

A special thanks to Bjorn for persisting with this and coming up with a
nice simple solution :)

Regards,
Daniel

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 18 Oct 2017, alexander.stef...@infineon.com wrote:
> 
> > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > > The printk removals do change the objects.
> > > > >
> > > > > The value of that type of change is only for resource limited systems.
> > > >
> > > > I imagine that such small code adjustments are also useful for other
> > > systems.
> > >
> > > Your imagination and mine differ.
> > > Where do you _think_ it matters?
> > >
> > > For instance, nothing about
> > >
> > >   sizeof(type)
> > > vs
> > >   sizeof(*ptr)
> > >
> > > makes it easier for a human to read the code.
> >
> > If it does not make it easier to read the code for you, then maybe you
> > should consider that this might not be true for all humans. For me, it
> > makes it much easier to see at a glance, that code like
> > ptr=malloc(sizeof(*ptr)) is correct.
> 
> I don't think there is a perfect solution.

Maybe. But for the second variant the correctness is easier to check, both 
mentally and programmatically, because there is no need for any context (the 
type of ptr does not matter).

> The type argument to sizeof
> could have the wrong type.  The expression argument to sizeof could be
> missing the *.  Unpleasant consequences are possible in both cases.
> Probably each maintainer has a style they prefer.  Perhaps it could be
> useful to adjust the code to follow the dominant strategy, in cases where
> there are a inconsistencies.

Certainly. At least within a file, there should be only one style.

> For example
> 
> if (...)
>   x = foo1(sizeof(struct xtype));
> else
>   x = foo2(sizeof(*x));
> 
> might at least cause some unnecessary mental effort to process.
> 
> julia

Alexander

Re: Adjusting further size determinations?

[SF Markus Elfring]

> Ugly grep follows:
> 
> $ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
>   sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo = 
> k.alloc(sizeof(*foo))/' \
>  -e 
> 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = 
> k.alloc(sizeof(struct foo))/' | \
>   sort | uniq -c | sort -rn | head -2
>6123 foo = k.alloc(sizeof(*foo)),
>3060 foo = k.alloc(sizeof(struct foo)),

Would you like to get this ratio changed in any ways?

Available development tools could help to improve the software situation
in a desired direction, couldn't they?


>> Unpleasant consequences are possible in both cases.

How much do you care to reduce the failure probability further?

Regards,
Markus

[PATCH] powerpc: ipic - fix status get and status clear

[Christophe Leroy]

IPIC Status is provided by register IPIC_SERSR and not by IPIC_SERMR
which is the mask register.

Signed-off-by: Christophe Leroy 
---
 arch/powerpc/sysdev/ipic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c
index 16f1edd78c40..535cf1f6941c 100644
--- a/arch/powerpc/sysdev/ipic.c
+++ b/arch/powerpc/sysdev/ipic.c
@@ -846,12 +846,12 @@ void ipic_disable_mcp(enum ipic_mcp_irq mcp_irq)
 
 u32 ipic_get_mcp_status(void)
 {
-   return ipic_read(primary_ipic->regs, IPIC_SERMR);
+   return ipic_read(primary_ipic->regs, IPIC_SERSR);
 }
 
 void ipic_clear_mcp_status(u32 mask)
 {
-   ipic_write(primary_ipic->regs, IPIC_SERMR, mask);
+   ipic_write(primary_ipic->regs, IPIC_SERSR, mask);
 }
 
 /* Return an interrupt vector or 0 if no interrupt is pending. */
-- 
2.13.3

Re: [PATCH] cxl: Rework the implementation of cxl_stop_trace_psl9()

[christophe lombard]

Le 11/10/2017 à 14:30, Vaibhav Jain a écrit :


Presently the PSL9 specific cxl_stop_trace_psl9() only stops the RX0
traces on the CXL adapter when a PSL error irq is triggered. The patch
updates the function to stop all the traces arrays and move them to
the FIN state. The implementation issues the mmio to TRACECFG register
to stop the trace array iff it already not in FIN state. This prevents
the issue of trace data being reset in case of multiple stop mmio
issued for a single trace array.

Also the patch does some refactoring of existing cxl_stop_trace_psl9()
and cxl_stop_trace_psl8() functions by moving them to 'pci.c' from
'debugfs.c' file and marking them as static.


agree.



Signed-off-by: Vaibhav Jain 
---
  drivers/misc/cxl/cxl.h | 14 --
  drivers/misc/cxl/debugfs.c | 22 --
  drivers/misc/cxl/pci.c | 38 ++
  3 files changed, 42 insertions(+), 32 deletions(-)


Thanks

Acked-by:  Christophe Lombard

RE: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Alexander.Steffen]

> On Tue, 17 Oct 2017, Mimi Zohar wrote:
> 
> > On Tue, 2017-10-17 at 14:58 +0200, Julia Lawall wrote:
> > >
> > > On Tue, 17 Oct 2017, Mimi Zohar wrote:
> > >
> > > > On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
> > > > wrote:
> > > > > > > Replace the specification of data structures by pointer
> dereferences
> > > > > > > as the parameter for the operator "sizeof" to make the
> corresponding
> > > > > > > size
> > > > > > > determination a bit safer according to the Linux coding style
> > > > > > > convention.
> > > > > >
> > > > > >
> > > > > > This patch does one style in favor of the other.
> > > > >
> > > > > I actually prefer that style, so I'd welcome this change :)
> > > >
> > > > Style changes should be reviewed and documented, like any other
> code
> > > > change, and added to Documentation/process/coding-style.rst or an
> > > > equivalent file.
> > >
> > > Actually, it has been there for many years:
> > >
> > > 14) Allocating memory
> > > -
> > > ...
> > > The preferred form for passing a size of a struct is the following:
> > >
> > > .. code-block:: c
> > >
> > >   p = kmalloc(sizeof(*p), ...);
> > >
> > > The alternative form where struct name is spelled out hurts readability
> and
> > > introduces an opportunity for a bug when the pointer variable type is
> changed
> > > but the corresponding sizeof that is passed to a memory allocator is not.
> >
> > True, thanks for the reminder.  Is this common in new code?  Is there
> > a script/ or some other automated way of catching this usage before
> > patches are upstreamed?
> >
> > Just as you're doing here, the patch description should reference this
> > in the patch description.
> 
> The comment in the documentation seems have been there since Linux
> 2.6.14,
> ie 2005.  The fact that a lot of code still doesn't use that style, 12
> years later, suggests that actually it is not preferred, or not preferred
> by everyone.  Perhaps the paragraph in coding style should just be
> dropped.

Or maybe everyone just copied existing code, which did not follow that pattern, 
because nobody bothered to fix old code ;-)

(This is true at least for tpm_tis_spi, where I was involved in its creation.)

Alexander

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 10:44 +, alexander.stef...@infineon.com wrote:
> > For instance, nothing about
> > > > sizeof(type)
> > > > vs
> > > > sizeof(*ptr)
> > > > makes it easier for a human to read the code.
> > > 
> > > If it does not make it easier to read the code for you, then maybe you
> > > should consider that this might not be true for all humans. For me, it
> > > makes it much easier to see at a glance, that code like
> > > ptr=malloc(sizeof(*ptr)) is correct.
> > 
> > I don't think there is a perfect solution.
> 
> Maybe. But for the second variant the correctness is easier to check,

How often should
ptr = alloc(sizeof(*ptr))
be
ptr = alloc(sizeof(**ptr))

>  both mentally and programmatically, because there is no need for any context 
> (the type of ptr does not matter).

Context matters.

Re: [PATCH] powerpc/watchdog: Convert timers to use timer_setup()

[Michael Ellerman]

Michael Ellerman  writes:
> Kees Cook  writes:
>> On Tue, Oct 17, 2017 at 5:29 AM, Michael Ellerman  
>> wrote:
>>> Nicholas Piggin  writes:
 On Mon, 16 Oct 2017 16:47:10 -0700
 Kees Cook  wrote:
> In preparation for unconditionally passing the struct timer_list pointer 
> to
> all timer callbacks, switch to using the new timer_setup() and 
> from_timer()
> to pass the timer pointer explicitly.
>
> Cc: Benjamin Herrenschmidt 
> Cc: Paul Mackerras 
> Cc: Michael Ellerman 
> Cc: Nicholas Piggin 
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Kees Cook 

 Looks fine to me. Is this intended to be merged via the powerpc tree
 in the next merge window?
>>>
>>> It relies on the new timer_setup(), which is in one of tglx's trees (I
>>> think). So I expect it to go via that tree.
>>
>> It's in -rc3, but the timer tree can carry it if you want. Which do
>> you prefer?
>
> Oh sorry, I assumed it was in only in linux-next.
>
> I'll take this. Thanks.

Ugh, I'm an .

My next is based on rc2, so I can't take this.

Can you please pick it up.

cheers

Re: [PATCH 3/7] powerpc: Free up four 64K PTE bits in 4K backed HPTE pages

[Michael Ellerman]

Ram Pai  writes:

> diff --git a/arch/powerpc/mm/hash64_64k.c b/arch/powerpc/mm/hash64_64k.c
> index 1a68cb1..c6c5559 100644
> --- a/arch/powerpc/mm/hash64_64k.c
> +++ b/arch/powerpc/mm/hash64_64k.c
> @@ -126,18 +113,13 @@ int __hash_page_4K(unsigned long ea, unsigned long 
> access, unsigned long vsid,
>   if (__rpte_sub_valid(rpte, subpg_index)) {
>   int ret;
>  
> - hash = hpt_hash(vpn, shift, ssize);
> - hidx = __rpte_to_hidx(rpte, subpg_index);
> - if (hidx & _PTEIDX_SECONDARY)
> - hash = ~hash;
> - slot = (hash & htab_hash_mask) * HPTES_PER_GROUP;
> - slot += hidx & _PTEIDX_GROUP_IX;
> + gslot = pte_get_hash_gslot(vpn, shift, ssize, rpte,
> + subpg_index);
> + ret = mmu_hash_ops.hpte_updatepp(gslot, rflags, vpn,
> + MMU_PAGE_4K, MMU_PAGE_4K, ssize, flags);

This was formatted correctly before:
  
> - ret = mmu_hash_ops.hpte_updatepp(slot, rflags, vpn,
> -  MMU_PAGE_4K, MMU_PAGE_4K,
> -  ssize, flags);
>   /*
> -  *if we failed because typically the HPTE wasn't really here
> +  * if we failed because typically the HPTE wasn't really here

If you're fixing it up please make it "If ...".

>* we try an insertion.
>*/
>   if (ret == -1)
> @@ -148,6 +130,15 @@ int __hash_page_4K(unsigned long ea, unsigned long 
> access, unsigned long vsid,
>   }
>  
>  htab_insert_hpte:
> +
> + /*
> +  * initialize all hidx entries to invalid value,
> +  * the first time the PTE is about to allocate
> +  * a 4K hpte
> +  */

Should be:
/*
 * Initialize all hidx entries to invalid value, the first time
 * the PTE is about to allocate a 4K HPTE.
 */

> + if (!(old_pte & H_PAGE_COMBO))
> + rpte.hidx = ~0x0UL;
> +

Paul had the idea that if we biased the slot number by 1, we could make
the "invalid" value be == 0.

That would avoid needing to that above, and also mean the value is
correctly invalid from the get-go, which would be good IMO.

I think now that you've added the slot accessors it would be pretty easy
to do.


>   /*
>* handle H_PAGE_4K_PFN case
>*/
> @@ -172,15 +163,41 @@ int __hash_page_4K(unsigned long ea, unsigned long 
> access, unsigned long vsid,
>* Primary is full, try the secondary
>*/
>   if (unlikely(slot == -1)) {
> + bool soft_invalid;
> +
>   hpte_group = ((~hash & htab_hash_mask) * HPTES_PER_GROUP) & 
> ~0x7UL;
>   slot = mmu_hash_ops.hpte_insert(hpte_group, vpn, pa,
>   rflags, HPTE_V_SECONDARY,
>   MMU_PAGE_4K, MMU_PAGE_4K,
>   ssize);
> - if (slot == -1) {
> - if (mftb() & 0x1)
> +
> + soft_invalid = hpte_soft_invalid(slot);
> + if (unlikely(soft_invalid)) {


> + /*
> +  * we got a valid slot from a hardware point of view.
> +  * but we cannot use it, because we use this special
> +  * value; as defined   byhpte_soft_invalid(),
> +  * to  trackinvalid  slots.  We  cannot  use  it.
> +  * So invalidate it.
> +  */
> + gslot = slot & _PTEIDX_GROUP_IX;
> + mmu_hash_ops.hpte_invalidate(hpte_group+gslot, vpn,
> + MMU_PAGE_4K, MMU_PAGE_4K,
> + ssize, 0);

Please:
mmu_hash_ops.hpte_invalidate(hpte_group+gslot, vpn,
 MMU_PAGE_4K, MMU_PAGE_4K,
 ssize, 0);

> + }
> +
> + if (unlikely(slot == -1 || soft_invalid)) {
> + /*
> +  * for soft invalid slot, lets   ensure that we

For .. let's


cheers

[PATCH] powerpc/64s: Replace CONFIG_PPC_STD_MMU_64 with CONFIG_PPC_BOOK3S_64

[Michael Ellerman]

CONFIG_PPC_STD_MMU_64 indicates support for the "standard" powerpc MMU
on 64-bit CPUs. The "standard" MMU refers to the hash page table MMU
found in "server" processors, from IBM mainly.

Currently CONFIG_PPC_STD_MMU_64 is == CONFIG_PPC_BOOK3S_64. While it's
annoying to have two symbols that always have the same value, it's not
quite annoying enough to bother removing one.

However with the arrival of Power9, we now have the situation where
CONFIG_PPC_STD_MMU_64 is enabled, but the kernel is running using the
Radix MMU - *not* the "standard" MMU. So it is now actively confusing
to use it, because it implies that code is disabled or inactive when
the Radix MMU is in use, however that is not necessarily true.

So s/CONFIG_PPC_STD_MMU_64/CONFIG_PPC_BOOK3S_64/, and do some minor
formatting updates of some of the affected lines.

This will be a pain for backports, but c'est la vie.

Signed-off-by: Michael Ellerman 
---
 arch/powerpc/Kconfig |  6 +++---
 arch/powerpc/include/asm/nohash/64/pgtable.h |  2 +-
 arch/powerpc/include/asm/paca.h  | 10 +-
 arch/powerpc/include/asm/page_64.h   |  6 +++---
 arch/powerpc/include/asm/pgtable-be-types.h  |  2 +-
 arch/powerpc/include/asm/pgtable-types.h |  4 ++--
 arch/powerpc/include/asm/tlbflush.h  |  2 +-
 arch/powerpc/kernel/asm-offsets.c|  4 ++--
 arch/powerpc/kernel/entry_64.S   |  4 ++--
 arch/powerpc/kernel/exceptions-64s.S |  8 
 arch/powerpc/kernel/machine_kexec_64.c   |  4 ++--
 arch/powerpc/kernel/mce_power.c  |  4 ++--
 arch/powerpc/kernel/paca.c   | 12 ++--
 arch/powerpc/kernel/pci_64.c |  4 ++--
 arch/powerpc/kernel/process.c| 12 ++--
 arch/powerpc/kernel/prom.c   |  2 +-
 arch/powerpc/kernel/setup-common.c   |  4 ++--
 arch/powerpc/mm/Makefile |  6 +++---
 arch/powerpc/mm/dump_hashpagetable.c |  2 +-
 arch/powerpc/mm/dump_linuxpagetables.c   | 10 +-
 arch/powerpc/mm/init_64.c|  8 
 arch/powerpc/mm/pgtable_64.c |  2 +-
 arch/powerpc/platforms/Kconfig.cputype   |  6 +-
 arch/powerpc/platforms/pseries/lpar.c|  8 
 arch/powerpc/platforms/pseries/lparcfg.c |  2 +-
 arch/powerpc/xmon/xmon.c |  6 +++---
 26 files changed, 68 insertions(+), 72 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 809c468edab1..b8b75b423316 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -334,7 +334,7 @@ config PPC_OF_PLATFORM_PCI
default n
 
 config ARCH_SUPPORTS_DEBUG_PAGEALLOC
-   depends on PPC32 || PPC_STD_MMU_64
+   depends on PPC32 || PPC_BOOK3S_64
def_bool y
 
 config ARCH_SUPPORTS_UPROBES
@@ -721,7 +721,7 @@ config PPC_16K_PAGES
 
 config PPC_64K_PAGES
bool "64k page size"
-   depends on !PPC_FSL_BOOK3E && (44x || PPC_STD_MMU_64 || PPC_BOOK3E_64)
+   depends on !PPC_FSL_BOOK3E && (44x || PPC_BOOK3S_64 || PPC_BOOK3E_64)
select HAVE_ARCH_SOFT_DIRTY if PPC_BOOK3S_64
 
 config PPC_256K_PAGES
@@ -780,7 +780,7 @@ config FORCE_MAX_ZONEORDER
 
 config PPC_SUBPAGE_PROT
bool "Support setting protections for 4k subpages"
-   depends on PPC_STD_MMU_64 && PPC_64K_PAGES
+   depends on PPC_BOOK3S_64 && PPC_64K_PAGES
help
  This option adds support for a system call to allow user programs
  to set access permissions (read/write, readonly, or no access)
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h 
b/arch/powerpc/include/asm/nohash/64/pgtable.h
index f0ff384d4ca5..b1c42ac54d96 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -203,7 +203,7 @@ static inline unsigned long pte_update(struct mm_struct *mm,
if (!huge)
assert_pte_locked(mm, addr);
 
-#ifdef CONFIG_PPC_STD_MMU_64
+#ifdef CONFIG_PPC_BOOK3S_64
if (old & _PAGE_HASHPTE)
hpte_need_flush(mm, addr, ptep, old, huge);
 #endif
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index cfc2a10b9064..d2804f143c99 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -91,14 +91,14 @@ struct paca_struct {
u8 cpu_start;   /* At startup, processor spins until */
/* this becomes non-zero. */
u8 kexec_state; /* set when kexec down has irqs off */
-#ifdef CONFIG_PPC_STD_MMU_64
+#ifdef CONFIG_PPC_BOOK3S_64
struct slb_shadow *slb_shadow_ptr;
struct dtl_entry *dispatch_log;
struct dtl_entry *dispatch_log_end;
-#endif /* CONFIG_PPC_STD_MMU_64 */
+#endif
u64 dscr_default;   /* per-CPU default DSCR */
 
-#ifdef CONFIG_PPC_STD_MMU_64
+#ifdef CONFIG_PPC_BOOK3S_64
/*
   

Re: [PATCH 21/25] powerpc: introduce get_pte_pkey() helper

[Balbir Singh]

On Fri,  8 Sep 2017 15:45:09 -0700
Ram Pai  wrote:

> get_pte_pkey() helper returns the pkey associated with
> a address corresponding to a given mm_struct.
>

This is really get_mm_addr_key() -- no?

Balbir Singh.

Re: [PATCH 10/25] powerpc: store and restore the pkey state across context switches

[Ram Pai]

On Thu, Oct 19, 2017 at 10:00:38AM +1100, Balbir Singh wrote:
> On Wed, 18 Oct 2017 13:47:05 -0700
> Ram Pai  wrote:
> 
> > On Wed, Oct 18, 2017 at 02:49:14PM +1100, Balbir Singh wrote:
> > > On Fri,  8 Sep 2017 15:44:58 -0700
> > > Ram Pai  wrote:
> > >   
> > > > Store and restore the AMR, IAMR and UAMOR register state of the task
> > > > before scheduling out and after scheduling in, respectively.
> > > > 
> > > > Signed-off-by: Ram Pai 
> > > > ---
> > > >  arch/powerpc/include/asm/pkeys.h |4 +++
> > > >  arch/powerpc/include/asm/processor.h |5 
> > > >  arch/powerpc/kernel/process.c|   10 
> > > >  arch/powerpc/mm/pkeys.c  |   39 
> > > > ++
> > > >  4 files changed, 58 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/arch/powerpc/include/asm/pkeys.h 
> > > > b/arch/powerpc/include/asm/pkeys.h
> > > > index 7fd48a4..78c5362 100644
> > > > --- a/arch/powerpc/include/asm/pkeys.h
> > > > +++ b/arch/powerpc/include/asm/pkeys.h
> > > > @@ -143,5 +143,9 @@ static inline void pkey_mm_init(struct mm_struct 
> > > > *mm)
> > > > mm_pkey_allocation_map(mm) = initial_allocation_mask;
> > > >  }
> > > >  
> > > > +extern void thread_pkey_regs_save(struct thread_struct *thread);
> > > > +extern void thread_pkey_regs_restore(struct thread_struct *new_thread,
> > > > +   struct thread_struct *old_thread);
> > > > +extern void thread_pkey_regs_init(struct thread_struct *thread);
> > > >  extern void pkey_initialize(void);
> > > >  #endif /*_ASM_PPC64_PKEYS_H */
> > > > diff --git a/arch/powerpc/include/asm/processor.h 
> > > > b/arch/powerpc/include/asm/processor.h
> > > > index fab7ff8..de9d9ba 100644
> > > > --- a/arch/powerpc/include/asm/processor.h
> > > > +++ b/arch/powerpc/include/asm/processor.h
> > > > @@ -309,6 +309,11 @@ struct thread_struct {
> > > > struct thread_vr_state ckvr_state; /* Checkpointed VR state */
> > > > unsigned long   ckvrsave; /* Checkpointed VRSAVE */
> > > >  #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> > > > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > > > +   unsigned long   amr;
> > > > +   unsigned long   iamr;
> > > > +   unsigned long   uamor;
> > > > +#endif
> > > >  #ifdef CONFIG_KVM_BOOK3S_32_HANDLER
> > > > void*   kvm_shadow_vcpu; /* KVM internal data */
> > > >  #endif /* CONFIG_KVM_BOOK3S_32_HANDLER */
> > > > diff --git a/arch/powerpc/kernel/process.c 
> > > > b/arch/powerpc/kernel/process.c
> > > > index a0c74bb..ba80002 100644
> > > > --- a/arch/powerpc/kernel/process.c
> > > > +++ b/arch/powerpc/kernel/process.c
> > > > @@ -42,6 +42,7 @@
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >  
> > > >  #include 
> > > >  #include 
> > > > @@ -1085,6 +1086,9 @@ static inline void save_sprs(struct thread_struct 
> > > > *t)
> > > > t->tar = mfspr(SPRN_TAR);
> > > > }
> > > >  #endif
> > > > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > > > +   thread_pkey_regs_save(t);
> > > > +#endif  
> > > 
> > > Just define two variants of thread_pkey_regs_save() based on
> > > CONFIG_PPC64_MEMORY_PROTECTION_KEYS and remove the #ifdefs from process.c
> > > Ditto for the lines below  
> > 
> > ok.
> > 
> > >   
> > > >  }
> > > >  
> > > >  static inline void restore_sprs(struct thread_struct *old_thread,
> > > > @@ -1120,6 +1124,9 @@ static inline void restore_sprs(struct 
> > > > thread_struct *old_thread,
> > > > mtspr(SPRN_TAR, new_thread->tar);
> > > > }
> > > >  #endif
> > > > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > > > +   thread_pkey_regs_restore(new_thread, old_thread);
> > > > +#endif  
> > 
> > ok.
> > 
> > > >  }
> > > >  
> > > >  #ifdef CONFIG_PPC_BOOK3S_64
> > > > @@ -1705,6 +1712,9 @@ void start_thread(struct pt_regs *regs, unsigned 
> > > > long start, unsigned long sp)
> > > > current->thread.tm_tfiar = 0;
> > > > current->thread.load_tm = 0;
> > > >  #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
> > > > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> > > > +   thread_pkey_regs_init(>thread);
> > > > +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> > > >  }
> > > >  EXPORT_SYMBOL(start_thread);
> > > >  
> > > > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> > > > index 2282864..7cd1be4 100644
> > > > --- a/arch/powerpc/mm/pkeys.c
> > > > +++ b/arch/powerpc/mm/pkeys.c
> > > > @@ -149,3 +149,42 @@ int __arch_set_user_pkey_access(struct task_struct 
> > > > *tsk, int pkey,
> > > > init_amr(pkey, new_amr_bits);
> > > > return 0;
> > > >  }
> > > > +
> > > > +void thread_pkey_regs_save(struct thread_struct *thread)
> > > > +{
> > > > +   if (!pkey_inited)
> > > > +   return;
> > > > +
> > > > +   /* @TODO skip saving any registers if the thread
> > > > +* has not used any 

Re: [PATCH 1/10] KVM: PPC: Book3S HV: Use ARRAY_SIZE macro

[Paul Mackerras]

On Sun, Sep 03, 2017 at 02:19:31PM +0200, Thomas Meyer wrote:
> Use ARRAY_SIZE macro, rather than explicitly coding some variant of it
> yourself.
> Found with: find -type f -name "*.c" -o -name "*.h" | xargs perl -p -i -e
> 's/\bsizeof\s*\(\s*(\w+)\s*\)\s*\ /\s*sizeof\s*\(\s*\1\s*\[\s*0\s*\]\s*\)
> /ARRAY_SIZE(\1)/g' and manual check/verification.
> 
> Signed-off-by: Thomas Meyer 

Thanks, applied to my kvm-ppc-next branch.

Paul.

Re: [PATCH 6/7] KVM: PPC: Cocci spatch "vma_pages"

[Paul Mackerras]

On Thu, Sep 21, 2017 at 12:29:36AM +0200, Thomas Meyer wrote:
> Use vma_pages function on vma object instead of explicit computation.
> Found by coccinelle spatch "api/vma_pages.cocci"
> 
> Signed-off-by: Thomas Meyer 

Thanks, applied to my kvm-ppc-next branch, with the headline "KVM:
PPC: BookE: Use vma_pages function".

Paul.

Re: [PATCH 01/25] powerpc: initial pkey plumbing

[Michael Ellerman]

Ram Pai  writes:

> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 9fc3c0b..a4cd210 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -864,6 +864,22 @@ config SECCOMP
>  
> If unsure, say Y. Only embedded should say N here.
>  
> +config PPC64_MEMORY_PROTECTION_KEYS

That's pretty wordy, can we make it CONFIG_PPC_MEM_KEYS ?

I think you're a sufficient vim wizard to search and replace all
usages at once, if not I can do it before I apply the series.

> + prompt "PowerPC Memory Protection Keys"
> + def_bool y
> + # Note: only available in 64-bit mode

We don't need the note, that's exactly what the next line says:
> + depends on PPC64

But shouldn't it be BOOK3S_64 ?

I don't think it works on BookE does it?

> + select ARCH_USES_HIGH_VMA_FLAGS
> + select ARCH_HAS_PKEYS
> + ---help---

I prefer just "help".

> diff --git a/arch/powerpc/include/asm/mmu_context.h 
> b/arch/powerpc/include/asm/mmu_context.h
> index 3095925..7badf29 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -141,5 +141,10 @@ static inline bool arch_vma_access_permitted(struct 
> vm_area_struct *vma,
>   /* by default, allow everything */
>   return true;
>  }
> +
> +#ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> +#define pkey_initialize()
> +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */

You don't need ifdefs around that. But you also don't need it (see below).

> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> new file mode 100644
> index 000..c02305a
> --- /dev/null
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -0,0 +1,45 @@
> +#ifndef _ASM_PPC64_PKEYS_H
> +#define _ASM_PPC64_PKEYS_H

_ASM_POWERPC_KEYS_H

Missing copyright header here.

> +
> +extern bool pkey_inited;
> +extern bool pkey_execute_disable_support;
> +#define ARCH_VM_PKEY_FLAGS 0
> +
> +static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
> +{
> + return (pkey == 0);

That means pkey 1 is not allocated and pkey 0 is?

Surely this should just return false for now?

> +}
> +
> +static inline int mm_pkey_alloc(struct mm_struct *mm)
> +{
> + return -1;
> +}
> +
> +static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> +{
> + return -EINVAL;
> +}
> +
> +/*
> + * Try to dedicate one of the protection keys to be used as an
> + * execute-only protection key.
> + */
> +static inline int execute_only_pkey(struct mm_struct *mm)
> +{
> + return 0;
> +}
> +
> +static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
> + int prot, int pkey)

static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
  int prot, int pkey)

> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index b89c6aa..3b67014 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -316,6 +317,9 @@ void __init early_setup(unsigned long dt_ptr)
>   /* Initialize the hash table or TLB handling */
>   early_init_mmu();
>  
> + /* initialize the key subsystem */
> + pkey_initialize();
> +

I'm not sure we need to initialise this that early, but if we do, it
should be done in early_init_mmu(), not here.

> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index 0dff57b..67f62b5 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -35,6 +35,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

This should go in a later patch when it's needed.

> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> new file mode 100644
> index 000..418a05b
> --- /dev/null
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -0,0 +1,33 @@
> +/*
> + * PowerPC Memory Protection Keys management
> + * Copyright (c) 2015, Intel Corporation.

Is any of it really copyright Intel?

> + * Copyright (c) 2017, IBM Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.

We're meant to use "or later" on new code.

> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.

But not this part.

> + */

Blank line.

> +#include /* PKEY_*   */

Comment is wrong and unnecessary.

> +bool pkey_inited;
> +bool pkey_execute_disable_support;
> +
> +void __init pkey_initialize(void)
> +{
> + /* disable the pkey system till everything
> +  * is in place. A patch further down the
> +  * line will enable it.
> +  */

/*
 * Disable the pkey system 

Re: [V14,1/4] powerpc/vphn: Update CPU topology when VPHN enabled

[Michael Ellerman]

On Fri, 2017-09-08 at 20:47:27 UTC, Michael Bringmann wrote:
> powerpc/vphn: On Power systems with shared configurations of CPUs
> and memory, there are some issues with the association of additional
> CPUs and memory to nodes when hot-adding resources.  This patch
> corrects the currently broken capability to set the topology for
> shared CPUs in LPARs.  At boot time for shared CPU lpars, the
> topology for each CPU was being set to node zero.  Now when
> numa_update_cpu_topology() is called appropriately, the Virtual
> Processor Home Node (VPHN) capabilities information provided by the
> pHyp allows the appropriate node in the shared configuration to be
> selected for the CPU.
> 
> Signed-off-by: Michael Bringmann 

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/17f444c0549f2ce037646871e748cf

cheers

Re: [PATCH 0/2] Optimize mmu_notifier->invalidate_range callback

[Balbir Singh]

On Mon, 16 Oct 2017 23:10:01 -0400
jgli...@redhat.com wrote:

> From: Jérôme Glisse 
> 
> (Andrew you already have v1 in your queue of patch 1, patch 2 is new,
>  i think you can drop it patch 1 v1 for v2, v2 is bit more conservative
>  and i fixed typos)
> 
> All this only affect user of invalidate_range callback (at this time
> CAPI arch/powerpc/platforms/powernv/npu-dma.c, IOMMU ATS/PASID in
> drivers/iommu/amd_iommu_v2.c|intel-svm.c)
> 
> This patchset remove useless double call to mmu_notifier->invalidate_range
> callback wherever it is safe to do so. The first patch just remove useless
> call

As in an extra call? Where does that come from?

> and add documentation explaining why it is safe to do so. The second
> patch go further by introducing mmu_notifier_invalidate_range_only_end()
> which skip callback to invalidate_range this can be done when clearing a
> pte, pmd or pud with notification which call invalidate_range right after
> clearing under the page table lock.
>

Balbir Singh.

Re: [PATCH] KVM: PPC: Book3S HV: Delete an error message for a failed memory allocation in kvmppc_allocate_hpt()

[Paul Mackerras]

On Thu, Oct 05, 2017 at 01:23:30PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Thu, 5 Oct 2017 13:16:51 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 

Thanks, applied to my kvm-ppc-next branch.

Paul.

[PATCH] powerpc/64: Free up CPU_FTR_ICSWX

[Michael Ellerman]

The last user of CPU_FTR_ICSWX was removed in commit
6ff4d3e96652 ("powerpc: Remove old unused icswx based coprocessor
support"), so free the bit up for future use.

Signed-off-by: Michael Ellerman 
---
 arch/powerpc/include/asm/cputable.h | 6 +++---
 arch/powerpc/kernel/dt_cpu_ftrs.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/cputable.h 
b/arch/powerpc/include/asm/cputable.h
index a9bf921f4efc..e188dd57bf51 100644
--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h
@@ -206,7 +206,7 @@ enum {
 #define CPU_FTR_STCX_CHECKS_ADDRESSLONG_ASM_CONST(0x0004)
 #define CPU_FTR_POPCNTB
LONG_ASM_CONST(0x0008)
 #define CPU_FTR_POPCNTD
LONG_ASM_CONST(0x0010)
-#define CPU_FTR_ICSWX  LONG_ASM_CONST(0x0020)
+/* Free
LONG_ASM_CONST(0x0020) */
 #define CPU_FTR_VMX_COPY   LONG_ASM_CONST(0x0040)
 #define CPU_FTR_TM LONG_ASM_CONST(0x0080)
 #define CPU_FTR_CFAR   LONG_ASM_CONST(0x0100)
@@ -451,7 +451,7 @@ enum {
CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
CPU_FTR_DSCR | CPU_FTR_SAO  | CPU_FTR_ASYM_SMT | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
-   CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | \
+   CPU_FTR_CFAR | CPU_FTR_HVMODE | \
CPU_FTR_VMX_COPY | CPU_FTR_HAS_PPR | CPU_FTR_DABRX)
 #define CPU_FTRS_POWER8 (CPU_FTR_USE_TB | CPU_FTR_LWSYNC | \
CPU_FTR_PPCAS_ARCH_V2 | CPU_FTR_CTRL | CPU_FTR_ARCH_206 |\
@@ -460,7 +460,7 @@ enum {
CPU_FTR_PURR | CPU_FTR_SPURR | CPU_FTR_REAL_LE | \
CPU_FTR_DSCR | CPU_FTR_SAO  | \
CPU_FTR_STCX_CHECKS_ADDRESS | CPU_FTR_POPCNTB | CPU_FTR_POPCNTD | \
-   CPU_FTR_ICSWX | CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
+   CPU_FTR_CFAR | CPU_FTR_HVMODE | CPU_FTR_VMX_COPY | \
CPU_FTR_DBELL | CPU_FTR_HAS_PPR | CPU_FTR_DAWR | \
CPU_FTR_ARCH_207S | CPU_FTR_TM_COMP)
 #define CPU_FTRS_POWER8E (CPU_FTRS_POWER8 | CPU_FTR_PMAO_BUG)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c 
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 7275fed271af..36afefa68720 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -634,7 +634,7 @@ static struct dt_cpu_feature_match __initdata
{"no-execute", feat_enable, 0},
{"strong-access-ordering", feat_enable, CPU_FTR_SAO},
{"cache-inhibited-large-page", feat_enable_large_ci, 0},
-   {"coprocessor-icswx", feat_enable, CPU_FTR_ICSWX},
+   {"coprocessor-icswx", feat_enable, 0},
{"hypervisor-virtualization-interrupt", feat_enable_hvi, 0},
{"program-priority-register", feat_enable, CPU_FTR_HAS_PPR},
{"wait", feat_enable, 0},
-- 
2.7.4

Re: powerpc/modules: Use WARN_ON() in stub_for_addr()

[Michael Ellerman]

On Tue, 2017-10-10 at 14:47:32 UTC, Kamalesh Babulal wrote:
> Use WARN_ON(), while running out of stubs in stub_for_addr()
> and abort loading of the module instead of BUG_ON().
> 
> Signed-off-by: Kamalesh Babulal 

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/1c0437af9fca8de6e4ba179d18cf13

cheers

Re: [v2] cxl: Dump PSL_FIR register on PSL9 error irq

[Michael Ellerman]

On Wed, 2017-10-11 at 06:14:41 UTC, Vaibhav Jain wrote:
> For PSL9 currently we aren't dumping the PSL FIR register when a
> PSL error interrupt is triggered. Contents of this register are useful
> in debugging AFU issues.
> 
> This patch fixes issue by adding a new service_layer_ops callback
> cxl_native_err_irq_dump_regs_psl9() to dump the PSL_FIR registers on a
> PSL error interrupt thereby bringing the behavior in line with PSL on
> POWER-8. Also the existing service_layer_ops callback
> for PSL8 has been renamed to cxl_native_err_irq_dump_regs_psl8().
> 
> Signed-off-by: Vaibhav Jain 
> Acked-by: Frederic Barrat 
> Acked-by: Andrew Donnellan 

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/990f19ae6feefb4a6e718355719cde

cheers

Re: [PATCH 1/7] powerpc: introduce pte_set_hash_slot() helper

[Michael Ellerman]

Ram Pai  writes:
> diff --git a/arch/powerpc/include/asm/book3s/64/hash-64k.h 
> b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> index 9732837..6652669 100644
> --- a/arch/powerpc/include/asm/book3s/64/hash-64k.h
> +++ b/arch/powerpc/include/asm/book3s/64/hash-64k.h
> @@ -74,6 +74,31 @@ static inline unsigned long __rpte_to_hidx(real_pte_t 
> rpte, unsigned long index)
>   return (pte_val(rpte.pte) >> H_PAGE_F_GIX_SHIFT) & 0xf;
>  }
>  
> +/*
> + * Commit the hash slot and return pte bits that needs to be modified.
> + * The caller is expected to modify the pte bits accordingly and
> + * commit the pte to memory.
> + */
> +static inline unsigned long pte_set_hash_slot(pte_t *ptep, real_pte_t rpte,
> + unsigned int subpg_index, unsigned long slot)
> +{
> + unsigned long *hidxp = (unsigned long *)(ptep + PTRS_PER_PTE);
> +
> + rpte.hidx &= ~(0xfUL << (subpg_index << 2));
> + *hidxp = rpte.hidx  | (slot << (subpg_index << 2));
   ^
   stray space here
> + /*
> +  * Commit the hidx bits to memory before returning.

I'd prefer we didn't use "commit", it implies the bits are actually
written to memory by the barrier, which is not true. The barrier is just
a barrier or fence which prevents some reorderings of the things before
it and the things after it.

> +  * Anyone reading  pte  must  ensure hidx bits are
> +  * read  only  after  reading the pte by using the
> +  * read-side  barrier  smp_rmb().

That seems OK. Though I'm reminded that I dislike your justified
comments, the odd spacing is jarring to read.

>  __real_pte() can
> +  * help ensure that.

It doesn't help, it *does* do that.

cheers

Re: [PATCH] powerpc/xmon: Always enable xmon sysrq trigger

[Michael Ellerman]

"Guilherme G. Piccoli"  writes:

> Distros vary the way they enable SysRq by default - mostly they seem
> to enable some mask and then majority of the SysRq functions are
> disabled. For instance, xmon does not even have a mask, and unsless
> SysRq are completely enabled ( == 1), xmon trigger keeps disabled.
>
> Countless times while investigating hangs we needed xmon and it was
> disabled - machine just got hung and while in serial console, we just
> couldn't drop to xmon, forcing to a new attempt to reproduce the issue
> with SysRq fully enabled.
>
> This patch "fixes" this by having xmon enabled in all possible masks
> of SysRq. In other words, xmon trigger will only be disabled if SysRq
> is 0 (completely disabled). So, while debugging a hung, when one tries
> to drop to xmon this patch prevents the frustrating message:
> "This sysrq operation is disabled".

I know it's annoying when you get stuck with a box like this, but I
can't merge this patch.

You're *removing* the system administrators ability to control access to
xmon (other than disabling sysrq entirely). That's a regression.

What we should do is get a bit allocated for xmon, so it can have a
non-zero single-bit enable mask.

cheers


> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 4679aeb84767..780d708472a2 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -3514,6 +3514,7 @@ static struct sysrq_key_op sysrq_xmon_op = {
>   .handler =  sysrq_handle_xmon,
>   .help_msg = "xmon(x)",
>   .action_msg =   "Entering xmon",
> + .enable_mask =  0x,
>  };
>  
>  static int __init setup_xmon_sysrq(void)
> -- 
> 2.14.2

Re: [PATCH 0/2] Optimize mmu_notifier->invalidate_range callback

[Jerome Glisse]

On Thu, Oct 19, 2017 at 01:43:19PM +1100, Balbir Singh wrote:
> On Mon, 16 Oct 2017 23:10:01 -0400
> jgli...@redhat.com wrote:
> 
> > From: Jérôme Glisse 
> > 
> > (Andrew you already have v1 in your queue of patch 1, patch 2 is new,
> >  i think you can drop it patch 1 v1 for v2, v2 is bit more conservative
> >  and i fixed typos)
> > 
> > All this only affect user of invalidate_range callback (at this time
> > CAPI arch/powerpc/platforms/powernv/npu-dma.c, IOMMU ATS/PASID in
> > drivers/iommu/amd_iommu_v2.c|intel-svm.c)
> > 
> > This patchset remove useless double call to mmu_notifier->invalidate_range
> > callback wherever it is safe to do so. The first patch just remove useless
> > call
> 
> As in an extra call? Where does that come from?

Before this patch you had the following pattern:
  mmu_notifier_invalidate_range_start();
  take_page_table_lock()
  ...
  update_page_table()
  mmu_notifier_invalidate_range()
  ...
  drop_page_table_lock()
  mmu_notifier_invalidate_range_end();

It happens that mmu_notifier_invalidate_range_end() also make an
unconditional call to mmu_notifier_invalidate_range() so in the
above scenario you had 2 calls to mmu_notifier_invalidate_range()

Obviously one of the 2 call is useless. In some case you can drop
the first call (under the page table lock) this is what patch 1
does.

In other cases you can drop the second call that happen inside
mmu_notifier_invalidate_range_end() that is what patch 2 does.

Hence why i am referring to useless double call. I have added
more documentation to explain all this in the code and also under
Documentation/vm/mmu_notifier.txt


> 
> > and add documentation explaining why it is safe to do so. The second
> > patch go further by introducing mmu_notifier_invalidate_range_only_end()
> > which skip callback to invalidate_range this can be done when clearing a
> > pte, pmd or pud with notification which call invalidate_range right after
> > clearing under the page table lock.
> >
> 
> Balbir Singh.
> 

Re: [PATCH kernel] KVM: PPC: Enable in-kernel TCE handlers for PR KVM

[Paul Mackerras]

On Wed, Oct 11, 2017 at 04:01:08PM +1100, Alexey Kardashevskiy wrote:
> The handlers support PR KVM from the day one; however the PR KVM's
> enable/disable hcalls handler missed these ones.
> 
> Signed-off-by: Alexey Kardashevskiy 

Thanks, applied to my kvm-ppc-next branch.

Paul.

Re: selftests/powerpc: context_switch: Fix pthread errors

[Michael Ellerman]

On Tue, 2017-07-04 at 01:21:15 UTC, Cyril Bur wrote:
> Turns out pthreads returns an errno and doesn't set errno. This doesn't
> play well with perror().
> 
> Signed-off-by: Cyril Bur 

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/89aca4753eb451a48f65a12b026403

cheers

Re: [PATCH] powerpc: ipic - fix status get and status clear

[Michael Ellerman]

Christophe Leroy  writes:

> IPIC Status is provided by register IPIC_SERSR and not by IPIC_SERMR
> which is the mask register.

This seems like it would be a bad bug. But I guess it hasn't mattered
for some reason?

cheers

> diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c
> index 16f1edd78c40..535cf1f6941c 100644
> --- a/arch/powerpc/sysdev/ipic.c
> +++ b/arch/powerpc/sysdev/ipic.c
> @@ -846,12 +846,12 @@ void ipic_disable_mcp(enum ipic_mcp_irq mcp_irq)
>  
>  u32 ipic_get_mcp_status(void)
>  {
> - return ipic_read(primary_ipic->regs, IPIC_SERMR);
> + return ipic_read(primary_ipic->regs, IPIC_SERSR);
>  }
>  
>  void ipic_clear_mcp_status(u32 mask)
>  {
> - ipic_write(primary_ipic->regs, IPIC_SERMR, mask);
> + ipic_write(primary_ipic->regs, IPIC_SERSR, mask);
>  }
>  
>  /* Return an interrupt vector or 0 if no interrupt is pending. */
> -- 
> 2.13.3

Re: [v4, 1/5] powerpc/mce.c: Remove unused function get_mce_fault_addr()

[Michael Ellerman]

On Fri, 2017-09-29 at 04:26:51 UTC, Balbir Singh wrote:
> There are no users of get_mce_fault_addr()
> 
> Fixes: b63a0ff ("powerpc/powernv: Machine check exception handling.")
> 
> Signed-off-by: Balbir Singh 
> Reviewed-by: Nicholas Piggin 

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/73e341eb6bea01fde706d10d7edba9

cheers

Re: cxl: Rename register PSL9_FIR2 to PSL9_FIR_MASK

[Michael Ellerman]

On Mon, 2017-10-09 at 17:56:27 UTC, Vaibhav Jain wrote:
> PSL9 doesn't have a FIR2 register as was the case with PSL8. However
> currently the register definitions in 'cxl.h' have a definition for
> PSL9_FIR2 that actually points to PSL9_FIR_MASK register in the P1
> area at offset 0x308.
> 
> So this patch renames the def PSL9_FIR2 to PSL9_FIR_MASK and updates
> the references in the code to point to the new identifier. It also
> removes the code to dump contents of FIR2 (FIR_MASK actually) in
> cxl_native_irq_dump_regs_psl9().
> 
> Fixes: f24be42aab37("cxl: Add psl9 specific code")
> Reported-by: Frederic Barrat 
> Signed-off-by: Vaibhav Jain 
> Acked-by: Frederic Barrat 

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/8f6a90421c7637984fb352da079fb1

cheers

Re: [RFC v7 16/25] powerpc: helper to validate key-access permissions of a pte

[Michael Ellerman]

Ram Pai  writes:
> On Wed, Oct 18, 2017 at 06:08:34PM +0200, Laurent Dufour wrote:
>> On 31/07/2017 02:12, Ram Pai wrote:
>> > diff --git a/arch/powerpc/include/asm/pkeys.h 
>> > b/arch/powerpc/include/asm/pkeys.h
>> > index 4b7e3f4..ba7bff6 100644
>> > --- a/arch/powerpc/include/asm/pkeys.h
>> > +++ b/arch/powerpc/include/asm/pkeys.h
>> > @@ -85,6 +85,18 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>> >((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
>> >  }
>> > 
>> > +static inline u16 pte_to_pkey_bits(u64 pteflags)
>> > +{
>> > +  if (!pkey_inited)
>> > +  return 0x0UL;
>> 
>> Is it really needed to make such a check in this low level function ?
>> The only caller is already checking for pkey_inited before making the call.
>
> There are two callers to this function. get_pte_pkey() is one among
> them and it calls this function ignorant of the status of the
> pkey-subsystem.

But if none of the bits are set it will return 0 anyway right?

cheers

Re: [PATCH 1/2] mm/mmu_notifier: avoid double notification when it is useless v2

[Balbir Singh]

On Mon, 16 Oct 2017 23:10:02 -0400
jgli...@redhat.com wrote:

> From: Jérôme Glisse 
> 
> + /*
> +  * No need to call mmu_notifier_invalidate_range() as we are
> +  * downgrading page table protection not changing it to point
> +  * to a new page.
> +  *
> +  * See Documentation/vm/mmu_notifier.txt
> +  */
>   if (pmdp) {
>  #ifdef CONFIG_FS_DAX_PMD
>   pmd_t pmd;
> @@ -628,7 +635,6 @@ static void dax_mapping_entry_mkclean(struct 
> address_space *mapping,
>   pmd = pmd_wrprotect(pmd);
>   pmd = pmd_mkclean(pmd);
>   set_pmd_at(vma->vm_mm, address, pmdp, pmd);
> - mmu_notifier_invalidate_range(vma->vm_mm, start, end);

Could the secondary TLB still see the mapping as dirty and propagate the dirty 
bit back?

>  unlock_pmd:
>   spin_unlock(ptl);
>  #endif
> @@ -643,7 +649,6 @@ static void dax_mapping_entry_mkclean(struct 
> address_space *mapping,
>   pte = pte_wrprotect(pte);
>   pte = pte_mkclean(pte);
>   set_pte_at(vma->vm_mm, address, ptep, pte);
> - mmu_notifier_invalidate_range(vma->vm_mm, start, end);

Ditto

>  unlock_pte:
>   pte_unmap_unlock(ptep, ptl);
>   }
> diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
> index 6866e8126982..49c925c96b8a 100644
> --- a/include/linux/mmu_notifier.h
> +++ b/include/linux/mmu_notifier.h
> @@ -155,7 +155,8 @@ struct mmu_notifier_ops {
>* shared page-tables, it not necessary to implement the
>* invalidate_range_start()/end() notifiers, as
>* invalidate_range() alread catches the points in time when an
> -  * external TLB range needs to be flushed.
> +  * external TLB range needs to be flushed. For more in depth
> +  * discussion on this see Documentation/vm/mmu_notifier.txt
>*
>* The invalidate_range() function is called under the ptl
>* spin-lock and not allowed to sleep.
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index c037d3d34950..ff5bc647b51d 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1186,8 +1186,15 @@ static int do_huge_pmd_wp_page_fallback(struct 
> vm_fault *vmf, pmd_t orig_pmd,
>   goto out_free_pages;
>   VM_BUG_ON_PAGE(!PageHead(page), page);
>  
> + /*
> +  * Leave pmd empty until pte is filled note we must notify here as
> +  * concurrent CPU thread might write to new page before the call to
> +  * mmu_notifier_invalidate_range_end() happens which can lead to a
> +  * device seeing memory write in different order than CPU.
> +  *
> +  * See Documentation/vm/mmu_notifier.txt
> +  */
>   pmdp_huge_clear_flush_notify(vma, haddr, vmf->pmd);
> - /* leave pmd empty until pte is filled */
>  
>   pgtable = pgtable_trans_huge_withdraw(vma->vm_mm, vmf->pmd);
>   pmd_populate(vma->vm_mm, &_pmd, pgtable);
> @@ -2026,8 +2033,15 @@ static void __split_huge_zero_page_pmd(struct 
> vm_area_struct *vma,
>   pmd_t _pmd;
>   int i;
>  
> - /* leave pmd empty until pte is filled */
> - pmdp_huge_clear_flush_notify(vma, haddr, pmd);
> + /*
> +  * Leave pmd empty until pte is filled note that it is fine to delay
> +  * notification until mmu_notifier_invalidate_range_end() as we are
> +  * replacing a zero pmd write protected page with a zero pte write
> +  * protected page.
> +  *
> +  * See Documentation/vm/mmu_notifier.txt
> +  */
> + pmdp_huge_clear_flush(vma, haddr, pmd);

Shouldn't the secondary TLB know if the page size changed?

>  
>   pgtable = pgtable_trans_huge_withdraw(mm, pmd);
>   pmd_populate(mm, &_pmd, pgtable);
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 1768efa4c501..63a63f1b536c 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -3254,9 +3254,14 @@ int copy_hugetlb_page_range(struct mm_struct *dst, 
> struct mm_struct *src,
>   set_huge_swap_pte_at(dst, addr, dst_pte, entry, sz);
>   } else {
>   if (cow) {
> + /*
> +  * No need to notify as we are downgrading page
> +  * table protection not changing it to point
> +  * to a new page.
> +  *
> +  * See Documentation/vm/mmu_notifier.txt
> +  */
>   huge_ptep_set_wrprotect(src, addr, src_pte);

OK.. so we could get write faults on write accesses from the device.

> - mmu_notifier_invalidate_range(src, mmun_start,
> -mmun_end);
>   }
>  

Re: char/tpm: Improve a size determination in nine functions

[Jerry Snitselaar]

On Wed Oct 18 17, SF Markus Elfring wrote:

For 1/4 and 2/4: explain why the message can be omitted.


Why did you not reply directly with this request for the update steps
with the subject “Delete an error message for a failed memory allocation
in tpm_…()”?

https://patchwork.kernel.org/patch/10009405/
https://patchwork.kernel.org/patch/10009415/

I find that there can be difficulty to show an appropriate information
source for the reasonable explanation of this change pattern.



Shouldn't this information source for the explanation be the
submitter? I'd hope they understand what it is they are submitting.




Remove sentence about Coccinelle.


I got the impression that there is a bit of value in such
a kind of attribution.



That's all.


I assume that there might be also some communication challenges involved.



3/4: definitive NAK, too much noise compared to value.


I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.



4/4: this a good commit message.


Why did you not reply directly with this feedback for the update step
“[PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error 
detection”?

https://patchwork.kernel.org/patch/10009429/
https://lkml.kernel.org/r/<09a2c3a1-1b10-507d-a866-258b570f6...@users.sourceforge.net>



Requires a Tested-by before can be accepted, which I'm not able to give.


I am curious on how this detail will evolve.

Regards,
Markus

Re: [PATCH 1/2] mm/mmu_notifier: avoid double notification when it is useless v2

[Jerome Glisse]

On Thu, Oct 19, 2017 at 02:04:26PM +1100, Balbir Singh wrote:
> On Mon, 16 Oct 2017 23:10:02 -0400
> jgli...@redhat.com wrote:
> 
> > From: Jérôme Glisse 
> > 
> > +   /*
> > +* No need to call mmu_notifier_invalidate_range() as we are
> > +* downgrading page table protection not changing it to point
> > +* to a new page.
> > +*
> > +* See Documentation/vm/mmu_notifier.txt
> > +*/
> > if (pmdp) {
> >  #ifdef CONFIG_FS_DAX_PMD
> > pmd_t pmd;
> > @@ -628,7 +635,6 @@ static void dax_mapping_entry_mkclean(struct 
> > address_space *mapping,
> > pmd = pmd_wrprotect(pmd);
> > pmd = pmd_mkclean(pmd);
> > set_pmd_at(vma->vm_mm, address, pmdp, pmd);
> > -   mmu_notifier_invalidate_range(vma->vm_mm, start, end);
> 
> Could the secondary TLB still see the mapping as dirty and propagate the 
> dirty bit back?

I am assuming hardware does sane thing of setting the dirty bit only
when walking the CPU page table when device does a write fault ie
once the device get a write TLB entry the dirty is set by the IOMMU
when walking the page table before returning the lookup result to the
device and that it won't be set again latter (ie propagated back
latter).

I should probably have spell that out and maybe some of the ATS/PASID
implementer did not do that.

> 
> >  unlock_pmd:
> > spin_unlock(ptl);
> >  #endif
> > @@ -643,7 +649,6 @@ static void dax_mapping_entry_mkclean(struct 
> > address_space *mapping,
> > pte = pte_wrprotect(pte);
> > pte = pte_mkclean(pte);
> > set_pte_at(vma->vm_mm, address, ptep, pte);
> > -   mmu_notifier_invalidate_range(vma->vm_mm, start, end);
> 
> Ditto
> 
> >  unlock_pte:
> > pte_unmap_unlock(ptep, ptl);
> > }
> > diff --git a/include/linux/mmu_notifier.h b/include/linux/mmu_notifier.h
> > index 6866e8126982..49c925c96b8a 100644
> > --- a/include/linux/mmu_notifier.h
> > +++ b/include/linux/mmu_notifier.h
> > @@ -155,7 +155,8 @@ struct mmu_notifier_ops {
> >  * shared page-tables, it not necessary to implement the
> >  * invalidate_range_start()/end() notifiers, as
> >  * invalidate_range() alread catches the points in time when an
> > -* external TLB range needs to be flushed.
> > +* external TLB range needs to be flushed. For more in depth
> > +* discussion on this see Documentation/vm/mmu_notifier.txt
> >  *
> >  * The invalidate_range() function is called under the ptl
> >  * spin-lock and not allowed to sleep.
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index c037d3d34950..ff5bc647b51d 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -1186,8 +1186,15 @@ static int do_huge_pmd_wp_page_fallback(struct 
> > vm_fault *vmf, pmd_t orig_pmd,
> > goto out_free_pages;
> > VM_BUG_ON_PAGE(!PageHead(page), page);
> >  
> > +   /*
> > +* Leave pmd empty until pte is filled note we must notify here as
> > +* concurrent CPU thread might write to new page before the call to
> > +* mmu_notifier_invalidate_range_end() happens which can lead to a
> > +* device seeing memory write in different order than CPU.
> > +*
> > +* See Documentation/vm/mmu_notifier.txt
> > +*/
> > pmdp_huge_clear_flush_notify(vma, haddr, vmf->pmd);
> > -   /* leave pmd empty until pte is filled */
> >  
> > pgtable = pgtable_trans_huge_withdraw(vma->vm_mm, vmf->pmd);
> > pmd_populate(vma->vm_mm, &_pmd, pgtable);
> > @@ -2026,8 +2033,15 @@ static void __split_huge_zero_page_pmd(struct 
> > vm_area_struct *vma,
> > pmd_t _pmd;
> > int i;
> >  
> > -   /* leave pmd empty until pte is filled */
> > -   pmdp_huge_clear_flush_notify(vma, haddr, pmd);
> > +   /*
> > +* Leave pmd empty until pte is filled note that it is fine to delay
> > +* notification until mmu_notifier_invalidate_range_end() as we are
> > +* replacing a zero pmd write protected page with a zero pte write
> > +* protected page.
> > +*
> > +* See Documentation/vm/mmu_notifier.txt
> > +*/
> > +   pmdp_huge_clear_flush(vma, haddr, pmd);
> 
> Shouldn't the secondary TLB know if the page size changed?

It should not matter, we are talking virtual to physical on behalf
of a device against a process address space. So the hardware should
not care about the page size.

Moreover if any of the new 512 (assuming 2MB huge and 4K pages) zero
4K pages is replace by something new then a device TLB shootdown will
happen before the new page is set.

Only issue i can think of is if the IOMMU TLB (if there is one) or
the device TLB (you do expect that there is one) does not invalidate
TLB entry if the TLB shootdown is smaller than the TLB entry. That
would be idiotic but yes i know hardware bug.


> 
> >  

Re: [RFC v7 15/25] powerpc: Program HPTE key protection bits

[Michael Ellerman]

Ram Pai  writes:
> On Wed, Oct 18, 2017 at 06:15:40PM +0200, Laurent Dufour wrote:
>> On 31/07/2017 02:12, Ram Pai wrote:
>> > diff --git a/arch/powerpc/include/asm/pkeys.h 
>> > b/arch/powerpc/include/asm/pkeys.h
>> > index 1ded6df..4b7e3f4 100644
>> > --- a/arch/powerpc/include/asm/pkeys.h
>> > +++ b/arch/powerpc/include/asm/pkeys.h
>> > @@ -72,6 +72,19 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>> >  #define AMR_RD_BIT 0x1UL
>> >  #define AMR_WR_BIT 0x2UL
>> >  #define IAMR_EX_BIT 0x1UL
>> > +
>> > +static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>> > +{
>> > +  if (!pkey_inited)
>> > +  return 0x0UL;
>> 
>> Why making such a check here, is it to avoid the following check during the
>> boot process only ?
>> IIUC, there is no way to get H_PAGE_PKEY_BIT* set when pkey_inited is false.
>
> I know its a little paronia. Trying to avoid a case where the
> uninitialized pkey bits in the pte are erroneously interpreted as valid
> by this function. Remember that the caller of this function will use the
> return value to program the hpte. Nothing really bad should happen
> since none of the keys are enabled. But ... just playing it safe.

I think that's probably over-paranoid. It's not like it adds much
overhead, but it is a hot path, so no need to make it slower than it
needs to be.

cheers

Re: [PATCH] powerpc: ipic - fix status get and status clear

[Christophe LEROY]

Le 19/10/2017 à 07:06, Michael Ellerman a écrit :

Christophe Leroy  writes:


IPIC Status is provided by register IPIC_SERSR and not by IPIC_SERMR
which is the mask register.


This seems like it would be a bad bug. But I guess it hasn't mattered
for some reason?


As far as I can see, this function has been added in kernel 2.6.12 but 
has never been used in-tree.


I have discovered this error while implementing NMI watchdog on a 832x 
board, ie this function is needed to know when a machine check exception 
is generated by the watchdog timer.


Christophe



cheers


diff --git a/arch/powerpc/sysdev/ipic.c b/arch/powerpc/sysdev/ipic.c
index 16f1edd78c40..535cf1f6941c 100644
--- a/arch/powerpc/sysdev/ipic.c
+++ b/arch/powerpc/sysdev/ipic.c
@@ -846,12 +846,12 @@ void ipic_disable_mcp(enum ipic_mcp_irq mcp_irq)
  
  u32 ipic_get_mcp_status(void)

  {
-   return ipic_read(primary_ipic->regs, IPIC_SERMR);
+   return ipic_read(primary_ipic->regs, IPIC_SERSR);
  }
  
  void ipic_clear_mcp_status(u32 mask)

  {
-   ipic_write(primary_ipic->regs, IPIC_SERMR, mask);
+   ipic_write(primary_ipic->regs, IPIC_SERSR, mask);
  }
  
  /* Return an interrupt vector or 0 if no interrupt is pending. */

--
2.13.3

Re: Adjusting further size determinations?

[SF Markus Elfring]

 Unpleasant consequences are possible in both cases.
>> How much do you care to reduce the failure probability further?
> 
> Zero.

I am interested to improve the software situation a bit more here.

Regards,
Markus

Re: [PATCH] powerpc/watchdog: Convert timers to use timer_setup()

[Michael Ellerman]

Kees Cook  writes:

> On Tue, Oct 17, 2017 at 5:29 AM, Michael Ellerman  wrote:
>> Nicholas Piggin  writes:
>>
>>> On Mon, 16 Oct 2017 16:47:10 -0700
>>> Kees Cook  wrote:
>>>
 In preparation for unconditionally passing the struct timer_list pointer to
 all timer callbacks, switch to using the new timer_setup() and from_timer()
 to pass the timer pointer explicitly.

 Cc: Benjamin Herrenschmidt 
 Cc: Paul Mackerras 
 Cc: Michael Ellerman 
 Cc: Nicholas Piggin 
 Cc: linuxppc-dev@lists.ozlabs.org
 Signed-off-by: Kees Cook 
>>>
>>> Looks fine to me. Is this intended to be merged via the powerpc tree
>>> in the next merge window?
>>
>> It relies on the new timer_setup(), which is in one of tglx's trees (I
>> think). So I expect it to go via that tree.
>
> It's in -rc3, but the timer tree can carry it if you want. Which do
> you prefer?

Oh sorry, I assumed it was in only in linux-next.

I'll take this. Thanks.

cheers

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 02:03:02PM +0300, Andy Shevchenko wrote:
> On Mon, 2017-10-16 at 19:33 +0200, SF Markus Elfring wrote:
> > From: Markus Elfring 
> > Date: Mon, 16 Oct 2017 18:28:17 +0200
> > 
> > Replace the specification of data structures by pointer dereferences
> > as the parameter for the operator "sizeof" to make the corresponding
> > size
> > determination a bit safer according to the Linux coding style
> > convention.
> 
> 
> This patch does one style in favor of the other.
> 
> At the end it's Jarkko's call, though I would NAK this as I think some
> one already told this to you for some other similar patch(es).
> 
> 
> I even would suggest to stop doing this noisy stuff, which keeps people
> busy for nothing.

I favor using "sizeof(*foo)" for pointers but as a part of a commit where
something useful is done to the corresponding line of code.

So, I would say it's a NAK.

/Jarkko

[PATCH] powerpc/xmon: Always enable xmon sysrq trigger

[Guilherme G. Piccoli]

Distros vary the way they enable SysRq by default - mostly they seem
to enable some mask and then majority of the SysRq functions are
disabled. For instance, xmon does not even have a mask, and unsless
SysRq are completely enabled ( == 1), xmon trigger keeps disabled.

Countless times while investigating hangs we needed xmon and it was
disabled - machine just got hung and while in serial console, we just
couldn't drop to xmon, forcing to a new attempt to reproduce the issue
with SysRq fully enabled.

This patch "fixes" this by having xmon enabled in all possible masks
of SysRq. In other words, xmon trigger will only be disabled if SysRq
is 0 (completely disabled). So, while debugging a hung, when one tries
to drop to xmon this patch prevents the frustrating message:
"This sysrq operation is disabled".

Signed-off-by: Guilherme G. Piccoli 
---
Patch built and tested against powerpc/next.

 arch/powerpc/xmon/xmon.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 4679aeb84767..780d708472a2 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3514,6 +3514,7 @@ static struct sysrq_key_op sysrq_xmon_op = {
.handler =  sysrq_handle_xmon,
.help_msg = "xmon(x)",
.action_msg =   "Entering xmon",
+   .enable_mask =  0x,
 };
 
 static int __init setup_xmon_sysrq(void)
-- 
2.14.2

Re: [PATCH] powerpc/xmon: check before calling xive functions

[Michael Ellerman]

Breno Leitao  writes:

> Currently xmon could call XIVE functions from OPAL even if the XIVE is
> disabled or does not exist in the system, as in POWER8 machines.  This
> causes the following exception:
>
>  1:mon> dx
>  cpu 0x1: Vector: 700 (Program Check) at [c00423c93450]
>  pc: c009cfa4: opal_xive_dump+0x50/0x68
>  lr: c00997b8: opal_return+0x0/0x50
>
> This patch simply checks if XIVE is enabled before calling XIVE
> functions.

Thanks. I'll merge this.

But we should also fix it in skiboot.

cheers

> Suggested-by: Guilherme G. Piccoli 
> Signed-off-by: Breno Leitao 
> ---
>  arch/powerpc/xmon/xmon.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 4679aeb84767..b34976c4a6ba 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -2508,6 +2508,12 @@ static void dump_xives(void)
>   unsigned long num;
>   int c;
>  
> + if (!xive_enabled()) {
> + printf("Xive disabled on this system\n");
> +
> + return;
> + }
> +
>   c = inchar();
>   if (c == 'a') {
>   dump_all_xives();
> -- 
> 2.14.2

RE: Adjusting further size determinations?

[Julia Lawall]

On Wed, 18 Oct 2017, David Laight wrote:

> From: SF Markus Elfring
> >  Unpleasant consequences are possible in both cases.
> > >> How much do you care to reduce the failure probability further?
> > >
> > > Zero.
> >
> > I am interested to improve the software situation a bit more here.
>
> There are probably better places to spend your time!
>
> If you want 'security' for kmalloc() then:
>
> #define KMALLOC_TYPE(flags) (type *)kmalloc(sizeof (type), flags)
> #define KMALLOC(ptr, flags) *(ptr) = KMALLOC_TYPE(typeof *(ptr), flags)
>
> and change:
>   ptr = kmalloc(sizeof *ptr, flags);
> to:
>   KMALLOC(, flags);
>
> But it is all churn for churn's sake.

Please don't.  Coccinelle won't find real problems with kmalloc any more
if this is done.

julia

Re: [PATCH v3 2/2] pseries/eeh: Add Pseries pcibios_bus_add_device

[Juan Alvarez]

On 10/17/17 8:36 PM, Alexey Kardashevskiy wrote:
> PowerNV KVM guest is a pseries machine so this code will execute there.
>
The configure sriov path will fail and not enable sriov if resources are
not met. I.e. the IOV Bar is not set in PF IOV Resources, which in this
case gets assigned by firmware.

We have separated the calls to put PowerNV and PSeries as machine dependent
calls. Furthermore, we are adding device node properties in the device tree to 
identify if
this is an SR-IOV slot on Phyp Pseries platform. Verification will be in place 
to
distinguish between platforms that support SR-IOV in PSeries.
>> Which is the reason for
>> the separation of calls to the machine dependent stuff.
>>> We also use the pseries platform when running under KVM.
>>>
>>> cheers
>>>
>> If anyone plans to enable SR-IOV on Pseries platform, firmware must provide 
>> the
>> resources to enable the VFs and map them with system resources.
> This is what the PowerNV platform does.
Again, we have separated the machine dependent calls to different platforms. In 
our
case we don't use opal and our dependent on phyp to associate resources.
>
>> A new version
>> of the PAPR Document will be added to document these system resources.
> The guest simply gets yet another PCI device, how is IOV different here?
>
> In regard of EEH, the API does not change afaik, it is up to the hypervisor
> (KVM+QEMU) to handle IOV case correctly.

I don't understand your question entirely, can you rephrase?

Re: Adjusting further size determinations?

[SF Markus Elfring]

>> If you want 'security' for kmalloc() then:
>>
>> #define KMALLOC_TYPE(flags) (type *)kmalloc(sizeof (type), flags)
>> #define KMALLOC(ptr, flags) *(ptr) = KMALLOC_TYPE(typeof *(ptr), flags)

Such an approach might help.


>> and change:
>>  ptr = kmalloc(sizeof *ptr, flags);
>> to:
>>  KMALLOC(, flags);
>>
>> But it is all churn for churn's sake.
> 
> Please don't.

Interesting …


> Coccinelle won't find real problems with kmalloc any more if this is done.

The corresponding source code analysis will become different
(or more challenging) then. Are you still looking for related solutions?

Regards,
Markus

Re: Adjusting further size determinations?

[Joe Perches]

On Wed, 2017-10-18 at 13:00 +0200, SF Markus Elfring wrote:
> > Ugly grep follows:
> > 
> > $ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
> >   sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo 
> > = k.alloc(sizeof(*foo))/' \
> >  -e 
> > 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = 
> > k.alloc(sizeof(struct foo))/' | \
> >   sort | uniq -c | sort -rn | head -2
> >6123 foo = k.alloc(sizeof(*foo)),
> >3060 foo = k.alloc(sizeof(struct foo)),
> 
> Would you like to get this ratio changed in any ways?

No.

> Available development tools could help to improve the software situation
> in a desired direction, couldn't they?
> > > Unpleasant consequences are possible in both cases.
> How much do you care to reduce the failure probability further?

Zero.

The alloc style is trivially useful for new code.
Existing code doesn't need change.

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 08:18:58PM +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 18, 2017 at 06:43:10PM +0200, SF Markus Elfring wrote:
> > > Commit message should just describe in plain text what you are doing
> > 
> > Did other contributors find the wording “Replace …”
> > 
> > 
> > > and why.
> > 
> > and “… a bit safer according to the Linux coding style convention.”
> > sufficient often enough already?
> > 
> > Which description would you find more appropriate for this change pattern?
> > 
> > Regards,
> > Markus
> 
> For 1/4 and 2/4: explain why the message can be omitted. Remove sentence
> about Coccinelle. That's all.
> 3/4: definitive NAK, too much noise compared to value.
> 4/4: this a good commit message. Requires a Tested-by before can be
> accepted, which I'm not able to give.
> 
> Hope this helps.
> 
> /Jarkko

One more word of advice: send the three as separate patches. My guess is
that it takes a factor longer time to apply 4/4 than other patches
because there's more limited crowd who can test it.

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> One more word of advice: send the three as separate patches.

I do not see a need for an immediate resend at the moment.


> My guess is that it takes a factor longer time to apply 4/4
> than other patches because there's more limited crowd who can test it.

This is fine for me if somebody would like to integrate
this update suggestion at all.


How do you think about to separate replies better between affected
update steps?

Regards,
Markus

Re: char/tpm: Improve a size determination in nine functions

[Andy Shevchenko]

On Wed, 2017-10-18 at 19:48 +0200, SF Markus Elfring wrote:
> > For 1/4 and 2/4: explain why the message can be omitted.

> > That's all.
> 
> I assume that there might be also some communication challenges
> involved.
> 
> 
> > 3/4: definitive NAK, too much noise compared to value.
> 
> I tried to reduce deviations from the Linux coding style again.
> You do not like such an attempt for this software area so far.

The problem here in a time line or what comes first. Definitely, you are
trying to fix the code which _is_ upstream vs. the code which _might be_
upstream (exception is drivers/staging).

Why didn't you listen to what people are telling you?

Why are you spending too much time on little sense crap instead of doing
real fixes?

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 09:09:48AM -0700, James Bottomley wrote:
> On Wed, 2017-10-18 at 18:10 +0300, Jarkko Sakkinen wrote:
> > On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
> > > 
> > > On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > Fixes is only for bug fixes.  These don't fix any bugs.
> > > > 
> > > > How do you distinguish these in questionable source code
> > > > from other error categories or software weaknesses?
> > > 
> > > A style change is one that doesn't change the effect of the
> > > execution.
> > >  These don't actually even change the assembly, so there's
> > > programmatic
> > > proof they're not fixing anything.
> > > 
> > > Bug means potentially user visible fault.  In any bug fix commit
> > > you
> > > should document the fault and its effects on users so those
> > > backporting
> > > can decide if they care or not.
> > > 
> > > James
> > 
> > OK, I'll adjust my definition of a bug :-)
> 
> Subsystems are free to define bugs in any reasonable way.  However,
> there are two things to note here:
> 
>1. The style guide is just that, a guide; it's not hard and fast rules.
>    That means that violations aren't bugs in the usual sense.
>    However, new code should mostly follow it and if it doesn't, there
>   should be a good reason to go against the guide which should be
>   explained in the change log.
>2. The coding style evolves, so older drivers usually don't conform.
>    Classifying coding style issues as bugs leads to tons of patches
>   "fixing" older drivers, some of which actually end up breaking the
>   drivers in subtle ways which take ages to be found (at least that's
>   what we've seen in SCSI).
> 
> James

Makes sense. Thanks for verbose explanation.

/Jarkko

Re: [PATCH v12 07/11] x86/kasan: add and use kasan_map_populate()

[Pavel Tatashin]

Thank you Andrey, I will test this patch. Should it go on top or replace 
the existing patch in mm-tree? ARM and x86 should be done the same 
either both as follow-ups or both replace.


Pavel

Re: [PATCH v12 07/11] x86/kasan: add and use kasan_map_populate()

[Andrey Ryabinin]

On 10/18/2017 08:14 PM, Pavel Tatashin wrote:
> Thank you Andrey, I will test this patch. Should it go on top or replace the 
> existing patch in mm-tree? ARM and x86 should be done the same either both as 
> follow-ups or both replace.
> 

 It's a replacement of your patch.


> Pavel
> 
> -- 
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 06:43:10PM +0200, SF Markus Elfring wrote:
> > Commit message should just describe in plain text what you are doing
> 
> Did other contributors find the wording “Replace …”
> 
> 
> > and why.
> 
> and “… a bit safer according to the Linux coding style convention.”
> sufficient often enough already?
> 
> Which description would you find more appropriate for this change pattern?
> 
> Regards,
> Markus

For 1/4 and 2/4: explain why the message can be omitted. Remove sentence
about Coccinelle. That's all.
3/4: definitive NAK, too much noise compared to value.
4/4: this a good commit message. Requires a Tested-by before can be
accepted, which I'm not able to give.

Hope this helps.

/Jarkko

Re: [PATCH v12 08/11] arm64/kasan: add and use kasan_map_populate()

[Pavel Tatashin]

Hi Andrew and Michal,

There are a few changes I need to do to my series:

1. Replace these two patches:

arm64/kasan: add and use kasan_map_populate()
x86/kasan: add and use kasan_map_populate()

With:

x86/mm/kasan: don't use vmemmap_populate() to initialize
 shadow
arm64/mm/kasan: don't use vmemmap_populate() to initialize
 shadow

2. Fix a kbuild warning about section mismatch in
mm: deferred_init_memmap improvements

How should I proceed to get these replaced in mm-tree? Send three new 
patches, or send a new series?


Thank you,
Pavel

On 10/18/2017 01:18 PM, Andrey Ryabinin wrote:

On 10/18/2017 08:08 PM, Pavel Tatashin wrote:


As I said, I'm fine either way, I just didn't want to cause extra work
or rebasing:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/535703.html


Makes sense. I am also fine either way, I can submit a new patch merging 
together the two if needed.



Please, do this. Single patch makes more sense



Pavel

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> For 1/4 and 2/4: explain why the message can be omitted.

Why did you not reply directly with this request for the update steps
with the subject “Delete an error message for a failed memory allocation
in tpm_…()”?

https://patchwork.kernel.org/patch/10009405/
https://patchwork.kernel.org/patch/10009415/

I find that there can be difficulty to show an appropriate information
source for the reasonable explanation of this change pattern.


> Remove sentence about Coccinelle.

I got the impression that there is a bit of value in such
a kind of attribution.


> That's all.

I assume that there might be also some communication challenges involved.


> 3/4: definitive NAK, too much noise compared to value.

I tried to reduce deviations from the Linux coding style again.
You do not like such an attempt for this software area so far.


> 4/4: this a good commit message.

Why did you not reply directly with this feedback for the update step
“[PATCH 4/4] char/tpm: Less checks in tpm_ibmvtpm_probe() after error 
detection”?

https://patchwork.kernel.org/patch/10009429/
https://lkml.kernel.org/r/<09a2c3a1-1b10-507d-a866-258b570f6...@users.sourceforge.net>


> Requires a Tested-by before can be accepted, which I'm not able to give.

I am curious on how this detail will evolve.

Regards,
Markus

RE: Adjusting further size determinations?

[David Laight]

From: SF Markus Elfring
>  Unpleasant consequences are possible in both cases.
> >> How much do you care to reduce the failure probability further?
> >
> > Zero.
> 
> I am interested to improve the software situation a bit more here.

There are probably better places to spend your time!

If you want 'security' for kmalloc() then:

#define KMALLOC_TYPE(flags) (type *)kmalloc(sizeof (type), flags)
#define KMALLOC(ptr, flags) *(ptr) = KMALLOC_TYPE(typeof *(ptr), flags)

and change:
ptr = kmalloc(sizeof *ptr, flags);
to:
KMALLOC(, flags);

But it is all churn for churn's sake.

David

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

> Commit message should just describe in plain text what you are doing

Did other contributors find the wording “Replace …”


> and why.

and “… a bit safer according to the Linux coding style convention.”
sufficient often enough already?

Which description would you find more appropriate for this change pattern?

Regards,
Markus

Re: [PATCH v12 08/11] arm64/kasan: add and use kasan_map_populate()

[Will Deacon]

On Wed, Oct 18, 2017 at 01:03:10PM -0400, Pavel Tatashin wrote:
> I asked Will, about it, and he preferred to have this patched added to the
> end of my series instead of replacing "arm64/kasan: add and use
> kasan_map_populate()".

As I said, I'm fine either way, I just didn't want to cause extra work
or rebasing:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/535703.html

> In addition, Will's patch stops using large pages for kasan memory, and thus
> might add some regression in which case it is easier to revert just that
> patch instead of the whole series. It is unlikely that regression is going
> to be detectable, because kasan by itself makes system quiet slow already.

If it causes problems, I'll just fix them. No need to revert.

Will

Re: [PATCH v12 08/11] arm64/kasan: add and use kasan_map_populate()

[Pavel Tatashin]

Hi Andrey,

I asked Will, about it, and he preferred to have this patched added to 
the end of my series instead of replacing "arm64/kasan: add and use 
kasan_map_populate()".


In addition, Will's patch stops using large pages for kasan memory, and 
thus might add some regression in which case it is easier to revert just 
that patch instead of the whole series. It is unlikely that regression 
is going to be detectable, because kasan by itself makes system quiet 
slow already.


Pasha

Re: [PATCH v12 08/11] arm64/kasan: add and use kasan_map_populate()

[Pavel Tatashin]

As I said, I'm fine either way, I just didn't want to cause extra work
or rebasing:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-October/535703.html


Makes sense. I am also fine either way, I can submit a new patch merging 
together the two if needed.


Pavel

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
> On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > 
> > > Fixes is only for bug fixes.  These don't fix any bugs.
> > 
> > How do you distinguish these in questionable source code
> > from other error categories or software weaknesses?
> 
> A style change is one that doesn't change the effect of the execution.
>  These don't actually even change the assembly, so there's programmatic
> proof they're not fixing anything.
> 
> Bug means potentially user visible fault.  In any bug fix commit you
> should document the fault and its effects on users so those backporting
> can decide if they care or not.
> 
> James

OK, I'll adjust my definition of a bug :-)

/Jarkko

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Wed, Oct 18, 2017 at 05:22:19PM +0200, SF Markus Elfring wrote:
> >> Do you find my wording “This issue was detected by using the
> >> Coccinelle software.” insufficient?
> > 
> > This is fine for cover letter, not for the commits.
> 
> I guess that there are more opinions available by other contributors
> for this aspect.
> 
> 
> > After your analysis software finds an issue you should manually analyze
> > what is wrong
> 
> This view is generally fine.
> 
> 
> > and document that to the commit message.
> 
> I tried it in a single paragraph so far (besides the reference
> for the tool).
> 
> 
> > This applies to sparse, coccinelle or any other tool.
> 
> I find that further possibilities can be considered.
> 
> 
> > Tool-based commit messages are bad for commit history
> 
> I disagree to this view.
> 
> 
> > where as clean description gives idea what was done
> > (if you have to maintain a GIT tree).
> 
> How do you think about to offer any wording for an alternative
> which you would find better?
> 
> 
> > In my opinion tool is doing all the work but the part
> > that you should do is absent.
> 
> Really?
> 
> Regards,
> Markus

Commit message should just describe in plain text what you are doing
and why.

/Jarkko

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 11:50:05AM +, alexander.stef...@infineon.com wrote:
> > > Replace the specification of data structures by pointer dereferences
> > > as the parameter for the operator "sizeof" to make the corresponding
> > > size
> > > determination a bit safer according to the Linux coding style
> > > convention.
> > 
> > 
> > This patch does one style in favor of the other.
> 
> I actually prefer that style, so I'd welcome this change :)
> 
> > At the end it's Jarkko's call, though I would NAK this as I think some
> > one already told this to you for some other similar patch(es).
> > 
> > 
> > I even would suggest to stop doing this noisy stuff, which keeps people
> > busy for nothing.
> 
> Cleaning up old code is also worth something, even if does not change
> one bit in the assembly output in the end...
> 
> Alexander

Quite insignificant clean up it is that does more harm that gives any
benefit as any new change adds debt to backporting.

Anyway, this has been a useful patch set for me in the sense that I have
clearer picture now on discarding/accepting commits. One line minor
clean up will be from now on automatic NAK unless it causes a compiler
warning or some other visible side-effect.

/Jarkko

Re: [PATCH 3/4] char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 04:02:05PM +0300, Andy Shevchenko wrote:
> On Tue, 2017-10-17 at 08:52 -0400, Mimi Zohar wrote:
> > On Tue, 2017-10-17 at 11:50 +, alexander.stef...@infineon.com
> > wrote:
> > > > > Replace the specification of data structures by pointer
> > > > > dereferences
> > > > > as the parameter for the operator "sizeof" to make the
> > > > > corresponding
> > > > > size
> > > > > determination a bit safer according to the Linux coding style
> > > > > convention.
> > > > 
> > > > 
> > > > This patch does one style in favor of the other.
> > > 
> > > I actually prefer that style, so I'd welcome this change :)
> > 
> > Style changes should be reviewed and documented, like any other code
> > change, and added to Documentation/process/coding-style.rst or an
> > equivalent file.
> 
> +1.
> 
> > > > At the end it's Jarkko's call, though I would NAK this as I think
> > > > some
> > > > one already told this to you for some other similar patch(es).
> > > > 
> > > > 
> > > > I even would suggest to stop doing this noisy stuff, which keeps
> > > > people
> > > > busy for nothing.
> > > 
> > > Cleaning up old code is also worth something, even if does not
> > > change one bit in the assembly output in the end...
> > 
> > Wow, you're opening the door really wide for all sorts of trivial
> > changes!  Hope you have the time and inclination to review and comment
> > on all of them.  I certainly don't.
> 
> Moreover and not so obvious is an open door for making back port of
> *real* fixes much harder!

Yes. This is really the key observation:

  A commit must have value above the cost of fixing a merge conflict.

/Jarkko

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Alan Tull]

On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand  wrote:
> On 10/17/17 14:46, Rob Herring wrote:
>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull  wrote:
>>> On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:
>>>
>>> Hi Rob,
>>>
 With dependencies on a statically allocated full path name converted to
 use %pOF format specifier, we can store just the basename of node, and
 the unflattening of the FDT can be simplified.

 This commit will affect the remaining users of full_name. After
 analyzing these users, the remaining cases should only change some print
 messages. The main users of full_name are providing a name for struct
 resource. The resource names shouldn't be important other than providing
 /proc/iomem names.

 We no longer distinguish between pre and post 0x10 dtb formats as either
 a full path or basename will work. However, less than 0x10 formats have
 been broken since the conversion to use libfdt (and no one has cared).
 The conversion of the unflattening code to be non-recursive also broke
 pre 0x10 formats as the populate_node function would return 0 in that
 case.

 Signed-off-by: Rob Herring 
 ---
 v2:
 - rebase to linux-next

  drivers/of/fdt.c | 69 
 +---
  1 file changed, 11 insertions(+), 58 deletions(-)
>>>
>>> I've just updated to the latest next branch and am finding problems
>>> applying overlays.   Reverting this commit alleviates things.  The
>>> errors I get are:
>>>
>>> [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
>>> [   88.513447] OF: overlay: apply failed '/__symbols__'
>>> [   88.518423] create_overlay: Failed to create overlay (err=-12)
>>
>> Frank's series with overlay updates should fix this.
>
> Yes, it does:
>
>   [PATCH v3 11/12] of: overlay: remove a dependency on device node full_name

Thanks for the fast response.  I fetched the dt/next branch to test
this but there are sufficient changes that Pantelis' "OF: DT-Overlay
configfs interface (v7)" is broken now.  I've been adding that
downstream since 4.4.  We're using it as an interface for applying
overlays to program FPGAs.  If we fix it again, is there any chance
that can go upstream now?

Alan

Re: char/tpm: Improve a size determination in nine functions

[SF Markus Elfring]

>> Do you find my wording “This issue was detected by using the
>> Coccinelle software.” insufficient?
> 
> This is fine for cover letter, not for the commits.

I guess that there are more opinions available by other contributors
for this aspect.


> After your analysis software finds an issue you should manually analyze
> what is wrong

This view is generally fine.


> and document that to the commit message.

I tried it in a single paragraph so far (besides the reference
for the tool).


> This applies to sparse, coccinelle or any other tool.

I find that further possibilities can be considered.


> Tool-based commit messages are bad for commit history

I disagree to this view.


> where as clean description gives idea what was done
> (if you have to maintain a GIT tree).

How do you think about to offer any wording for an alternative
which you would find better?


> In my opinion tool is doing all the work but the part
> that you should do is absent.

Really?

Regards,
Markus

Re: [RFC v7 12/25] powerpc: implementation for arch_override_mprotect_pkey()

[Laurent Dufour]

Hi Ram,

On 31/07/2017 02:12, Ram Pai wrote:
> arch independent code calls arch_override_mprotect_pkey()
> to return a pkey that best matches the requested protection.
> 
> This patch provides the implementation.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/mmu_context.h |5 +++
>  arch/powerpc/include/asm/pkeys.h   |   17 ++-
>  arch/powerpc/mm/pkeys.c|   47 
> 
>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/mmu_context.h 
> b/arch/powerpc/include/asm/mmu_context.h
> index 4705dab..7232484 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -185,6 +185,11 @@ static inline bool arch_vma_access_permitted(struct 
> vm_area_struct *vma,
>  #ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
>  #define pkey_initialize()
>  #define pkey_mm_init(mm)
> +
> +static inline int vma_pkey(struct vm_area_struct *vma)
> +{
> + return 0;
> +}
>  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> 
>  #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index a715a08..03f7ea2 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -46,6 +46,16 @@ static inline u64 pkey_to_vmflag_bits(u16 pkey)
>   ((pkey & 0x10UL) ? VM_PKEY_BIT4 : 0x0UL));
>  }
> 
> +#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
> + VM_PKEY_BIT3 | VM_PKEY_BIT4)
> +
> +static inline int vma_pkey(struct vm_area_struct *vma)
> +{
> + if (!pkey_inited)
> + return 0;
> + return (vma->vm_flags & ARCH_VM_PKEY_FLAGS) >> VM_PKEY_SHIFT;
> +}
> +
>  #define arch_max_pkey()  pkeys_total
>  #define AMR_RD_BIT 0x1UL
>  #define AMR_WR_BIT 0x2UL
> @@ -146,11 +156,14 @@ static inline int execute_only_pkey(struct mm_struct 
> *mm)
>   return __execute_only_pkey(mm);
>  }
> 
> -
> +extern int __arch_override_mprotect_pkey(struct vm_area_struct *vma,
> + int prot, int pkey);
>  static inline int arch_override_mprotect_pkey(struct vm_area_struct *vma,
>   int prot, int pkey)
>  {
> - return 0;
> + if (!pkey_inited)
> + return 0;
> + return __arch_override_mprotect_pkey(vma, prot, pkey);
>  }
> 
>  extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 25749bf..edcbf48 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -154,3 +154,50 @@ int __execute_only_pkey(struct mm_struct *mm)
>   mm->context.execute_only_pkey = execute_only_pkey;
>   return execute_only_pkey;
>  }
> +
> +static inline bool vma_is_pkey_exec_only(struct vm_area_struct *vma)
> +{
> + /* Do this check first since the vm_flags should be hot */
> + if ((vma->vm_flags & (VM_READ | VM_WRITE | VM_EXEC)) != VM_EXEC)
> + return false;
> +
> + return (vma_pkey(vma) == vma->vm_mm->context.execute_only_pkey);
> +}
> +
> +/*
> + * This should only be called for *plain* mprotect calls.
> + */
> +int __arch_override_mprotect_pkey(struct vm_area_struct *vma, int prot,
> + int pkey)
> +{
> + /*
> +  * Is this an mprotect_pkey() call?  If so, never
> +  * override the value that came from the user.
> +  */
> + if (pkey != -1)
> + return pkey;

This check should be moved in arch_override_mprotect_pkey() in
arch/powerpc/include/asm/pkeys.h

> +
> + /*
> +  * If the currently associated pkey is execute-only,
> +  * but the requested protection requires read or write,
> +  * move it back to the default pkey.
> +  */
> + if (vma_is_pkey_exec_only(vma) &&
> + (prot & (PROT_READ|PROT_WRITE)))
> + return 0;
> +
> + /*
> +  * the requested protection is execute-only. Hence
> +  * lets use a execute-only pkey.
> +  */
> + if (prot == PROT_EXEC) {
> + pkey = execute_only_pkey(vma->vm_mm);
> + if (pkey > 0)
> + return pkey;
> + }
> +
> + /*
> +  * nothing to override.
> +  */
> + return vma_pkey(vma);
> +}
> 

Re: [RFC v7 02/25] powerpc: track allocation status of all pkeys

[Laurent Dufour]

Hi Ram,

On 31/07/2017 02:12, Ram Pai wrote:
> Total 32 keys are available on power7 and above. However
> pkey 0,1 are reserved. So effectively we  have  30 pkeys.
> 
> On 4K kernels, we do not  have  5  bits  in  the  PTE to
> represent  all the keys; we only have 3bits.Two of those
> keys are reserved; pkey 0 and pkey 1. So effectively  we
> have 6 pkeys.

IIUC, the pkey 0 and 1 are reserved by the hardware, and the kernel PTE has
only 5 bits to keep track of the pkey. Why hw pkey 0 and 1 has to be
represented in the kernel PTE ?

> This patch keeps track of reserved keys, allocated  keys
> and keys that are currently free.
> 
> Also it  adds  skeletal  functions  and macros, that the
> architecture-independent code expects to be available.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/mmu.h |9 +++
>  arch/powerpc/include/asm/mmu_context.h   |1 +
>  arch/powerpc/include/asm/pkeys.h |   98 -
>  arch/powerpc/mm/mmu_context_book3s64.c   |2 +
>  arch/powerpc/mm/pkeys.c  |2 +
>  5 files changed, 108 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h 
> b/arch/powerpc/include/asm/book3s/64/mmu.h
> index 77529a3..104ad72 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
> @@ -108,6 +108,15 @@ struct patb_entry {
>  #ifdef CONFIG_SPAPR_TCE_IOMMU
>   struct list_head iommu_group_mem_list;
>  #endif
> +
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> + /*
> +  * Each bit represents one protection key.
> +  * bit set   -> key allocated
> +  * bit unset -> key available for allocation
> +  */
> + u32 pkey_allocation_map;
> +#endif
>  } mm_context_t;
> 
>  /*
> diff --git a/arch/powerpc/include/asm/mmu_context.h 
> b/arch/powerpc/include/asm/mmu_context.h
> index 4b93547..4705dab 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -184,6 +184,7 @@ static inline bool arch_vma_access_permitted(struct 
> vm_area_struct *vma,
> 
>  #ifndef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
>  #define pkey_initialize()
> +#define pkey_mm_init(mm)
>  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> 
>  #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 4ccb8f5..def385f 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -2,6 +2,8 @@
>  #define _ASM_PPC64_PKEYS_H
> 
>  extern bool pkey_inited;
> +extern int pkeys_total; /* total pkeys as per device tree */
> +extern u32 initial_allocation_mask;/* bits set for reserved keys */
> 
>  /*
>   * powerpc needs an additional vma bit to support 32 keys.
> @@ -20,21 +22,76 @@
>  #define VM_PKEY_BIT4 VM_HIGH_ARCH_4
>  #endif
> 
> -#define ARCH_VM_PKEY_FLAGS 0
> +#define arch_max_pkey()  pkeys_total
> +#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
> + VM_PKEY_BIT3 | VM_PKEY_BIT4)
> +
> +#define pkey_alloc_mask(pkey) (0x1 << pkey)
> +
> +#define mm_pkey_allocation_map(mm)   (mm->context.pkey_allocation_map)
> +
> +#define mm_set_pkey_allocated(mm, pkey) {\
> + mm_pkey_allocation_map(mm) |= pkey_alloc_mask(pkey); \
> +}
> +
> +#define mm_set_pkey_free(mm, pkey) { \
> + mm_pkey_allocation_map(mm) &= ~pkey_alloc_mask(pkey);   \
> +}
> +
> +#define mm_set_pkey_is_allocated(mm, pkey)   \
> + (mm_pkey_allocation_map(mm) & pkey_alloc_mask(pkey))
> +
> +#define mm_set_pkey_is_reserved(mm, pkey) (initial_allocation_mask & \
> + pkey_alloc_mask(pkey))

This macro doesn't need a 'mm' argument.

>  static inline bool mm_pkey_is_allocated(struct mm_struct *mm, int pkey)
>  {
> - return (pkey == 0);
> + /* a reserved key is never considered as 'explicitly allocated' */
> + return ((pkey < arch_max_pkey()) &&
> + !mm_set_pkey_is_reserved(mm, pkey) &&
> + mm_set_pkey_is_allocated(mm, pkey));
>  }
> 
> +/*
> + * Returns a positive, 5-bit key on success, or -1 on failure.

I guess you rely on the mmap_sem to protect against concurrency in
mm_pkey_alloc() and mm_pkey_free().
As this is not explicit in the code, it should at least be mentioned in the
comment describing the function.

> + */
>  static inline int mm_pkey_alloc(struct mm_struct *mm)
>  {
> - return -1;
> + /*
> +  * Note: this is the one and only place we make sure
> +  * that the pkey is valid as far as the hardware is
> +  * concerned.  The rest of the kernel trusts that
> +  * only good, valid pkeys come out of here.
> +  */
> + u32 all_pkeys_mask = (u32)(~(0x0));
> + int ret;
> +
> + if (!pkey_inited)
> + return -1;
> + /*
> +  * Are we out of pkeys?  We must handle this specially
> +  * because ffz() behavior is undefined if there are no
> +  * 

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 12:44:34PM +0300, Dan Carpenter wrote:
> On Tue, Oct 17, 2017 at 10:56:42AM +0200, Julia Lawall wrote:
> > 
> > 
> > On Tue, 17 Oct 2017, Dan Carpenter wrote:
> > 
> > > On Mon, Oct 16, 2017 at 09:35:12PM +0300, Jarkko Sakkinen wrote:
> > > >
> > > > A minor complaint: all commits are missing "Fixes:" tag.
> > > >
> > >
> > > Fixes is only for bug fixes.  These don't fix any bugs.
> > 
> > 0-day seems to put Fixes for everything.  Should they be removed when the
> > old code is undesirable but doesn't actually cause a crash, eg out of date
> > API.
> 
> Yeah, I feel like Fixes tags don't belong for API updates and cleanups.
> 
> regards,
> dan carpenter

So breaking a rule documented in the style guide is not a bug? :-)

/Jarkko

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>>> A minor complaint: all commits are missing "Fixes:" tag.
>>
>> * Do you require it to be added to the commit messages?
> 
> I don't require it. It's part of the development process:
> 
> https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html

Yes. - But other contributors pointed the detail out again
that not every change is qualified for using this tag.


>> * Would you like to get a finer patch granularity then?
> 
> I don't understand what you are asking.

If you would insist on the addition of this tag to all my commits
for the discussed patch series, I imagine that I would need to split
the update step “Improve a size determination in nine functions”
into smaller parts.


>> * Do you find any more information missing?
> 
> I think I already answered to this in my earlier responses
> (commit messages).

Partly.


> I probably won't take "sizeof(*foo)" type of change even if it
> is a recommended style if that is the only useful thing that the
> commit does.

How much do you care for the section “14) Allocating memory”
in the document “coding-style.rst” then?

Regards,
Markus

Re: [RFC v7 16/25] powerpc: helper to validate key-access permissions of a pte

[Laurent Dufour]

On 31/07/2017 02:12, Ram Pai wrote:
> helper function that checks if the read/write/execute is allowed
> on the pte.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h |4 +++
>  arch/powerpc/include/asm/pkeys.h |   12 +++
>  arch/powerpc/mm/pkeys.c  |   28 
> ++
>  3 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h 
> b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 060a1b2..2bec0f6 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -477,6 +477,10 @@ static inline void write_uamor(u64 value)
>   mtspr(SPRN_UAMOR, value);
>  }
> 
> +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS
> +extern bool arch_pte_access_permitted(u64 pte, bool write, bool execute);
> +#endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> +
>  #define __HAVE_ARCH_PTEP_GET_AND_CLEAR
>  static inline pte_t ptep_get_and_clear(struct mm_struct *mm,
>  unsigned long addr, pte_t *ptep)
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 4b7e3f4..ba7bff6 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -85,6 +85,18 @@ static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
>   ((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
>  }
> 
> +static inline u16 pte_to_pkey_bits(u64 pteflags)
> +{
> + if (!pkey_inited)
> + return 0x0UL;

Is it really needed to make such a check in this low level function ?
The only caller is already checking for pkey_inited before making the call.

> +
> + return (((pteflags & H_PAGE_PKEY_BIT0) ? 0x10 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT1) ? 0x8 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT2) ? 0x4 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT3) ? 0x2 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT4) ? 0x1 : 0x0UL));
> +}
> +
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>   VM_PKEY_BIT3 | VM_PKEY_BIT4)
>  #define AMR_BITS_PER_PKEY 2
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index edcbf48..8d756ef 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -201,3 +201,31 @@ int __arch_override_mprotect_pkey(struct vm_area_struct 
> *vma, int prot,
>*/
>   return vma_pkey(vma);
>  }
> +
> +static bool pkey_access_permitted(int pkey, bool write, bool execute)
> +{
> + int pkey_shift;
> + u64 amr;
> +
> + if (!pkey)
> + return true;
> +
> + pkey_shift = pkeyshift(pkey);
> + if (!(read_uamor() & (0x3UL << pkey_shift)))
> + return true;
> +
> + if (execute && !(read_iamr() & (IAMR_EX_BIT << pkey_shift)))
> + return true;
> +
> + amr = read_amr(); /* delay reading amr uptil absolutely needed*/
> + return ((!write && !(amr & (AMR_RD_BIT << pkey_shift))) ||
> + (write &&  !(amr & (AMR_WR_BIT << pkey_shift;
> +}
> +
> +bool arch_pte_access_permitted(u64 pte, bool write, bool execute)
> +{
> + if (!pkey_inited)
> + return true;
> + return pkey_access_permitted(pte_to_pkey_bits(pte),
> + write, execute);
> +}
> 

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[James Bottomley]

On Wed, 2017-10-18 at 18:10 +0300, Jarkko Sakkinen wrote:
> On Tue, Oct 17, 2017 at 08:57:13AM -0700, James Bottomley wrote:
> > 
> > On Tue, 2017-10-17 at 11:25 +0200, SF Markus Elfring wrote:
> > > 
> > > > 
> > > > 
> > > > Fixes is only for bug fixes.  These don't fix any bugs.
> > > 
> > > How do you distinguish these in questionable source code
> > > from other error categories or software weaknesses?
> > 
> > A style change is one that doesn't change the effect of the
> > execution.
> >  These don't actually even change the assembly, so there's
> > programmatic
> > proof they're not fixing anything.
> > 
> > Bug means potentially user visible fault.  In any bug fix commit
> > you
> > should document the fault and its effects on users so those
> > backporting
> > can decide if they care or not.
> > 
> > James
> 
> OK, I'll adjust my definition of a bug :-)

Subsystems are free to define bugs in any reasonable way.  However,
there are two things to note here:

   1. The style guide is just that, a guide; it's not hard and fast rules.
   That means that violations aren't bugs in the usual sense.
   However, new code should mostly follow it and if it doesn't, there
  should be a good reason to go against the guide which should be
  explained in the change log.
   2. The coding style evolves, so older drivers usually don't conform.
   Classifying coding style issues as bugs leads to tons of patches
  "fixing" older drivers, some of which actually end up breaking the
  drivers in subtle ways which take ages to be found (at least that's
  what we've seen in SCSI).

James

Re: [RFC v7 15/25] powerpc: Program HPTE key protection bits

[Laurent Dufour]

On 31/07/2017 02:12, Ram Pai wrote:
> Map the PTE protection key bits to the HPTE key protection bits,
> while creating HPTE  entries.
> 
> Signed-off-by: Ram Pai 
> ---
>  arch/powerpc/include/asm/book3s/64/mmu-hash.h |5 +
>  arch/powerpc/include/asm/mmu_context.h|6 ++
>  arch/powerpc/include/asm/pkeys.h  |   13 +
>  arch/powerpc/mm/hash_utils_64.c   |1 +
>  4 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h 
> b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> index 6981a52..f7a6ed3 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> @@ -90,6 +90,8 @@
>  #define HPTE_R_PP0   ASM_CONST(0x8000)
>  #define HPTE_R_TSASM_CONST(0x4000)
>  #define HPTE_R_KEY_HIASM_CONST(0x3000)
> +#define HPTE_R_KEY_BIT0  ASM_CONST(0x2000)
> +#define HPTE_R_KEY_BIT1  ASM_CONST(0x1000)
>  #define HPTE_R_RPN_SHIFT 12
>  #define HPTE_R_RPN   ASM_CONST(0x0000)
>  #define HPTE_R_RPN_3_0   ASM_CONST(0x01fff000)
> @@ -104,6 +106,9 @@
>  #define HPTE_R_C ASM_CONST(0x0080)
>  #define HPTE_R_R ASM_CONST(0x0100)
>  #define HPTE_R_KEY_LOASM_CONST(0x0e00)
> +#define HPTE_R_KEY_BIT2  ASM_CONST(0x0800)
> +#define HPTE_R_KEY_BIT3  ASM_CONST(0x0400)
> +#define HPTE_R_KEY_BIT4  ASM_CONST(0x0200)
> 
>  #define HPTE_V_1TB_SEG   ASM_CONST(0x4000)
>  #define HPTE_V_VRMA_MASK ASM_CONST(0x4001ff00)
> diff --git a/arch/powerpc/include/asm/mmu_context.h 
> b/arch/powerpc/include/asm/mmu_context.h
> index 7232484..2eb7f80 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -190,6 +190,12 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>  {
>   return 0;
>  }
> +
> +static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
> +{
> + return 0x0UL;
> +}
> +
>  #endif /* CONFIG_PPC64_MEMORY_PROTECTION_KEYS */
> 
>  #endif /* __KERNEL__ */
> diff --git a/arch/powerpc/include/asm/pkeys.h 
> b/arch/powerpc/include/asm/pkeys.h
> index 1ded6df..4b7e3f4 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -72,6 +72,19 @@ static inline int vma_pkey(struct vm_area_struct *vma)
>  #define AMR_RD_BIT 0x1UL
>  #define AMR_WR_BIT 0x2UL
>  #define IAMR_EX_BIT 0x1UL
> +
> +static inline u64 pte_to_hpte_pkey_bits(u64 pteflags)
> +{
> + if (!pkey_inited)
> + return 0x0UL;

Why making such a check here, is it to avoid the following check during the
boot process only ?
IIUC, there is no way to get H_PAGE_PKEY_BIT* set when pkey_inited is false.

> +
> + return (((pteflags & H_PAGE_PKEY_BIT0) ? HPTE_R_KEY_BIT0 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT1) ? HPTE_R_KEY_BIT1 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT2) ? HPTE_R_KEY_BIT2 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT3) ? HPTE_R_KEY_BIT3 : 0x0UL) |
> + ((pteflags & H_PAGE_PKEY_BIT4) ? HPTE_R_KEY_BIT4 : 0x0UL));
> +}
> +
>  #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
>   VM_PKEY_BIT3 | VM_PKEY_BIT4)
>  #define AMR_BITS_PER_PKEY 2
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index f88423b..545f291 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -231,6 +231,7 @@ unsigned long htab_convert_pte_flags(unsigned long 
> pteflags)
>*/
>   rflags |= HPTE_R_M;
> 
> + rflags |= pte_to_hpte_pkey_bits(pteflags);
>   return rflags;
>  }
> 

Re: [PATCH 0/4] char-TPM: Adjustments for ten function implementations

[Jarkko Sakkinen]

On Mon, Oct 16, 2017 at 10:44:18PM +0200, SF Markus Elfring wrote:
> > A minor complaint: all commits are missing "Fixes:" tag.
> 
> * Do you require it to be added to the commit messages?

I don't require it. It's part of the development process:

https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html

> * Would you like to get a finer patch granularity then?

I don't understand what you are asking.

> * Do you find any more information missing?
> 
> Regards,
> Markus

I think I already answered to this in my earlier responses (commit
messages).

I probably won't take "sizeof(*foo)" type of change even if it
is a recommended style if that is the only useful thing that the
commit does.

/Jarkko

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Pantelis Antoniou]

On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote:
> On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull  wrote:
> > On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand  
> > wrote:
> >> On 10/17/17 14:46, Rob Herring wrote:
> >>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull  wrote:
>  On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:
> 
>  Hi Rob,
> 
> > With dependencies on a statically allocated full path name converted to
> > use %pOF format specifier, we can store just the basename of node, and
> > the unflattening of the FDT can be simplified.
> >
> > This commit will affect the remaining users of full_name. After
> > analyzing these users, the remaining cases should only change some print
> > messages. The main users of full_name are providing a name for struct
> > resource. The resource names shouldn't be important other than providing
> > /proc/iomem names.
> >
> > We no longer distinguish between pre and post 0x10 dtb formats as either
> > a full path or basename will work. However, less than 0x10 formats have
> > been broken since the conversion to use libfdt (and no one has cared).
> > The conversion of the unflattening code to be non-recursive also broke
> > pre 0x10 formats as the populate_node function would return 0 in that
> > case.
> >
> > Signed-off-by: Rob Herring 
> > ---
> > v2:
> > - rebase to linux-next
> >
> >  drivers/of/fdt.c | 69 
> > +---
> >  1 file changed, 11 insertions(+), 58 deletions(-)
> 
>  I've just updated to the latest next branch and am finding problems
>  applying overlays.   Reverting this commit alleviates things.  The
>  errors I get are:
> 
>  [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
>  [   88.513447] OF: overlay: apply failed '/__symbols__'
>  [   88.518423] create_overlay: Failed to create overlay (err=-12)
> >>>
> >>> Frank's series with overlay updates should fix this.
> >>
> >> Yes, it does:
> >>
> >>   [PATCH v3 11/12] of: overlay: remove a dependency on device node 
> >> full_name
> >
> > Thanks for the fast response.  I fetched the dt/next branch to test
> > this but there are sufficient changes that Pantelis' "OF: DT-Overlay
> > configfs interface (v7)" is broken now.  I've been adding that
> > downstream since 4.4.  We're using it as an interface for applying
> > overlays to program FPGAs.  If we fix it again, is there any chance
> > that can go upstream now?
> 
> With a drive-by posting once every few years, no.
> 

I take offense to that. There's nothing changed in the patch for years.
Reposting the same patch without changes would achieve nothing.

> The issue remains that the kernel is not really setup to deal with any
> random property or node to be changed at any point in run-time. I
> think there needs to be some restrictions around what the overlays can
> touch. We can't have it be wide open and then lock things down later
> and break users. One example of what you could do is you can only add
> sub-trees to whitelisted nodes. That's probably acceptable for your
> usecase.
> 

Defining what can and what cannot be changed is not as trivial as a
list of white-listed nodes.

In some cases there is a whole node hierarchy being inserted (like in
a FPGA). In others, it's merely changing a status property to "okay" and
a few device parameters.

The real issue is that the kernel has no way to verify that a given
device tree, either at boot time or at overlay application time, is
correct.

When the tree is wrong at boot-time you'll hang (if you're lucky).
If the tree is wrong at run-time you'll get some into some unidentified
funky state.

Finally what is, and what is not 'correct' is not for the kernel to
decide arbitrarily, it's a matter of policy, different for each
use-case. 

> Rob

Regards

-- Pantelis

Re: char/tpm: Improve a size determination in nine functions

[Jarkko Sakkinen]

On Tue, Oct 17, 2017 at 08:41:04PM +0200, SF Markus Elfring wrote:
> Do you find my wording “This issue was detected by using the
> Coccinelle software.” insufficient?

This is fine for cover letter, not for the commits.

After your analysis software finds an issue you should manually analyze
what is wrong and document that to the commit message. This applies to
sparse, coccinelle or any other tool.

Tool-based commit messages are bad for commit history where as clean
description gives idea what was done (if you have to maintain a GIT
tree).

In my opinion tool is doing all the work but the part that you should do
is absent.

> Regards,
> Markus

/Jarkko

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Rob Herring]

On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull  wrote:
> On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand  wrote:
>> On 10/17/17 14:46, Rob Herring wrote:
>>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull  wrote:
 On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:

 Hi Rob,

> With dependencies on a statically allocated full path name converted to
> use %pOF format specifier, we can store just the basename of node, and
> the unflattening of the FDT can be simplified.
>
> This commit will affect the remaining users of full_name. After
> analyzing these users, the remaining cases should only change some print
> messages. The main users of full_name are providing a name for struct
> resource. The resource names shouldn't be important other than providing
> /proc/iomem names.
>
> We no longer distinguish between pre and post 0x10 dtb formats as either
> a full path or basename will work. However, less than 0x10 formats have
> been broken since the conversion to use libfdt (and no one has cared).
> The conversion of the unflattening code to be non-recursive also broke
> pre 0x10 formats as the populate_node function would return 0 in that
> case.
>
> Signed-off-by: Rob Herring 
> ---
> v2:
> - rebase to linux-next
>
>  drivers/of/fdt.c | 69 
> +---
>  1 file changed, 11 insertions(+), 58 deletions(-)

 I've just updated to the latest next branch and am finding problems
 applying overlays.   Reverting this commit alleviates things.  The
 errors I get are:

 [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
 [   88.513447] OF: overlay: apply failed '/__symbols__'
 [   88.518423] create_overlay: Failed to create overlay (err=-12)
>>>
>>> Frank's series with overlay updates should fix this.
>>
>> Yes, it does:
>>
>>   [PATCH v3 11/12] of: overlay: remove a dependency on device node full_name
>
> Thanks for the fast response.  I fetched the dt/next branch to test
> this but there are sufficient changes that Pantelis' "OF: DT-Overlay
> configfs interface (v7)" is broken now.  I've been adding that
> downstream since 4.4.  We're using it as an interface for applying
> overlays to program FPGAs.  If we fix it again, is there any chance
> that can go upstream now?

With a drive-by posting once every few years, no.

The issue remains that the kernel is not really setup to deal with any
random property or node to be changed at any point in run-time. I
think there needs to be some restrictions around what the overlays can
touch. We can't have it be wide open and then lock things down later
and break users. One example of what you could do is you can only add
sub-trees to whitelisted nodes. That's probably acceptable for your
usecase.

Rob

Re: char/tpm: Delete an error message for a failed memory allocation in tpm_…()

[SF Markus Elfring]

>> Why did you not reply directly with this request for the update steps
>> with the subject “Delete an error message for a failed memory allocation
>> in tpm_…()”?
>>
>> https://patchwork.kernel.org/patch/10009405/
>> https://patchwork.kernel.org/patch/10009415/
>>
>> I find that there can be difficulty to show an appropriate information
>> source for the reasonable explanation of this change pattern.
>>
> 
> Shouldn't this information source for the explanation be the submitter?

I offered a bit of information. I agree that it could become better eventually.


> I'd hope they understand what it is they are submitting.

I do this to some degree.   ;-)

But I would appreciate if I could refer to a single Linux document
for this change pattern around questionable error messages.
Would a corresponding link for an accepted explanation in the documentation
be nice in this case?

Regards,
Markus

Re: char-TPM: Adjustments for ten function implementations

[Michal Suchánek]

On Wed, 18 Oct 2017 02:18:46 -0700
Joe Perches  wrote:

> On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > The printk removals do change the objects.
> > > 
> > > The value of that type of change is only for resource limited
> > > systems.  
> > 
> > I imagine that such small code adjustments are also useful for
> > other systems.  
> 
> Your imagination and mine differ.
> Where do you _think_ it matters?
> 
> For instance, nothing about
> 
>   sizeof(type)
> vs
>   sizeof(*ptr)
> 
> makes it easier for a human to read the code.
> 

However, it makes it less error-prone to modify the code.

If you do ptr = malloc(sizeof(*ptr)) and later you change the type of
the pointer the code is still correct whereas ptr = malloc(sizeof(some
type) no longer is.

That is the reason the source analysis tool warns about this usage and
you do not really need any more explanation for *this* change.

The others are not so clear.

Thanks

Michal

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Rob Herring]

On Wed, Oct 18, 2017 at 10:53 AM, Pantelis Antoniou
 wrote:
> On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote:
>> On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull  wrote:
>> > On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand  
>> > wrote:
>> >> On 10/17/17 14:46, Rob Herring wrote:
>> >>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull  wrote:
>>  On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:
>> 
>>  Hi Rob,
>> 
>> > With dependencies on a statically allocated full path name converted to
>> > use %pOF format specifier, we can store just the basename of node, and
>> > the unflattening of the FDT can be simplified.
>> >
>> > This commit will affect the remaining users of full_name. After
>> > analyzing these users, the remaining cases should only change some 
>> > print
>> > messages. The main users of full_name are providing a name for struct
>> > resource. The resource names shouldn't be important other than 
>> > providing
>> > /proc/iomem names.
>> >
>> > We no longer distinguish between pre and post 0x10 dtb formats as 
>> > either
>> > a full path or basename will work. However, less than 0x10 formats have
>> > been broken since the conversion to use libfdt (and no one has cared).
>> > The conversion of the unflattening code to be non-recursive also broke
>> > pre 0x10 formats as the populate_node function would return 0 in that
>> > case.
>> >
>> > Signed-off-by: Rob Herring 
>> > ---
>> > v2:
>> > - rebase to linux-next
>> >
>> >  drivers/of/fdt.c | 69 
>> > +---
>> >  1 file changed, 11 insertions(+), 58 deletions(-)
>> 
>>  I've just updated to the latest next branch and am finding problems
>>  applying overlays.   Reverting this commit alleviates things.  The
>>  errors I get are:
>> 
>>  [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
>>  [   88.513447] OF: overlay: apply failed '/__symbols__'
>>  [   88.518423] create_overlay: Failed to create overlay (err=-12)
>> >>>
>> >>> Frank's series with overlay updates should fix this.
>> >>
>> >> Yes, it does:
>> >>
>> >>   [PATCH v3 11/12] of: overlay: remove a dependency on device node 
>> >> full_name
>> >
>> > Thanks for the fast response.  I fetched the dt/next branch to test
>> > this but there are sufficient changes that Pantelis' "OF: DT-Overlay
>> > configfs interface (v7)" is broken now.  I've been adding that
>> > downstream since 4.4.  We're using it as an interface for applying
>> > overlays to program FPGAs.  If we fix it again, is there any chance
>> > that can go upstream now?
>>
>> With a drive-by posting once every few years, no.
>>
>
> I take offense to that. There's nothing changed in the patch for years.
> Reposting the same patch without changes would achieve nothing.

Are you still expecting review comments on it or something?
Furthermore, If something is posted infrequently, then I'm not
inclined to comment or care if the next posting is going to be after I
forget what I previously said (which is not very long).

I'm just saying, don't expect to forward port, post and it will be
accepted. Below is minimally one of the issues that needs to be
addressed.

>> The issue remains that the kernel is not really setup to deal with any
>> random property or node to be changed at any point in run-time. I
>> think there needs to be some restrictions around what the overlays can
>> touch. We can't have it be wide open and then lock things down later
>> and break users. One example of what you could do is you can only add
>> sub-trees to whitelisted nodes. That's probably acceptable for your
>> usecase.
>>
>
> Defining what can and what cannot be changed is not as trivial as a
> list of white-listed nodes.

No, but we have to start somewhere and we are not starting with any
change allowed anywhere at anytime. If that is what people want, then
they are going to get to maintain that out of tree.

> In some cases there is a whole node hierarchy being inserted (like in
> a FPGA).

Yes, so you'd have a target fpga region. That sounds fine to me. Maybe
its not a static whitelist, but drivers have to register target
nodes/paths.

> In others, it's merely changing a status property to "okay" and
> a few device parameters.

That seems fine too. Disabled nodes could be allowed. But what if you
add/change properties on a node that is not disabled? Once a node is
enabled, who is responsible for registering the device?

What about changing a node from enabled to disabled? The kernel would
need to handle that or not allow it.

> The real issue is that the kernel has no way to verify that a given
> device tree, either at boot time or at overlay application time, is
> correct.
>
> When the tree is wrong at 

Re: [PATCH v2 5/5] of/fdt: only store the device node basename in full_name

[Alan Tull]

On Wed, Oct 18, 2017 at 10:53 AM, Pantelis Antoniou
 wrote:
> On Wed, 2017-10-18 at 10:44 -0500, Rob Herring wrote:
>> On Wed, Oct 18, 2017 at 10:12 AM, Alan Tull  wrote:
>> > On Tue, Oct 17, 2017 at 6:51 PM, Frank Rowand  
>> > wrote:
>> >> On 10/17/17 14:46, Rob Herring wrote:
>> >>> On Tue, Oct 17, 2017 at 4:32 PM, Alan Tull  wrote:
>>  On Mon, Aug 21, 2017 at 10:16 AM, Rob Herring  wrote:
>> 
>>  Hi Rob,
>> 
>> > With dependencies on a statically allocated full path name converted to
>> > use %pOF format specifier, we can store just the basename of node, and
>> > the unflattening of the FDT can be simplified.
>> >
>> > This commit will affect the remaining users of full_name. After
>> > analyzing these users, the remaining cases should only change some 
>> > print
>> > messages. The main users of full_name are providing a name for struct
>> > resource. The resource names shouldn't be important other than 
>> > providing
>> > /proc/iomem names.
>> >
>> > We no longer distinguish between pre and post 0x10 dtb formats as 
>> > either
>> > a full path or basename will work. However, less than 0x10 formats have
>> > been broken since the conversion to use libfdt (and no one has cared).
>> > The conversion of the unflattening code to be non-recursive also broke
>> > pre 0x10 formats as the populate_node function would return 0 in that
>> > case.
>> >
>> > Signed-off-by: Rob Herring 
>> > ---
>> > v2:
>> > - rebase to linux-next
>> >
>> >  drivers/of/fdt.c | 69 
>> > +---
>> >  1 file changed, 11 insertions(+), 58 deletions(-)
>> 
>>  I've just updated to the latest next branch and am finding problems
>>  applying overlays.   Reverting this commit alleviates things.  The
>>  errors I get are:
>> 
>>  [   88.498704] OF: overlay: Failed to apply prop @/__symbols__/clk_0
>>  [   88.513447] OF: overlay: apply failed '/__symbols__'
>>  [   88.518423] create_overlay: Failed to create overlay (err=-12)
>> >>>
>> >>> Frank's series with overlay updates should fix this.
>> >>
>> >> Yes, it does:
>> >>
>> >>   [PATCH v3 11/12] of: overlay: remove a dependency on device node 
>> >> full_name
>> >
>> > Thanks for the fast response.  I fetched the dt/next branch to test
>> > this but there are sufficient changes that Pantelis' "OF: DT-Overlay
>> > configfs interface (v7)" is broken now.  I've been adding that
>> > downstream since 4.4.  We're using it as an interface for applying
>> > overlays to program FPGAs.  If we fix it again, is there any chance
>> > that can go upstream now?
>>
>> With a drive-by posting once every few years, no.
>>
> I take offense to that. There's nothing changed in the patch for years.
> Reposting the same patch without changes would achieve nothing.
>
>> The issue remains that the kernel is not really setup to deal with any
>> random property or node to be changed at any point in run-time.

Yeah, I'm not super surprised :)  I have some whitelist ideas below.

>> I
>> think there needs to be some restrictions around what the overlays can
>> touch. We can't have it be wide open and then lock things down later
>> and break users. One example of what you could do is you can only add
>> sub-trees to whitelisted nodes. That's probably acceptable for your
>> usecase.

I can take a look at making OF_OVERLAY_PRE_APPLY and
OF_OVERLAY_PRE_REMOVE notifiers mandatory if that's interesting.  The
behavior would be: If an overlay is applied, there's got to be some
handler in the kernel that verifies that it is acceptable.   In my
case, the handler for FPGA regions would look at the overlay and see
it only added stuff under a FPGA region.

And we would change the default to be: if there is no handler, reject
the overlay.

>>
>
> Defining what can and what cannot be changed is not as trivial as a
> list of white-listed nodes.
>
> In some cases there is a whole node hierarchy being inserted (like in
> a FPGA).

For FPGA, the insertion points are always FPGA regions.

> In others, it's merely changing a status property to "okay" and
> a few device parameters.
>
> The real issue is that the kernel has no way to verify that a given
> device tree, either at boot time or at overlay application time, is
> correct.
>
> When the tree is wrong at boot-time you'll hang (if you're lucky).
> If the tree is wrong at run-time you'll get some into some unidentified
> funky state.
>
> Finally what is, and what is not 'correct' is not for the kernel to
> decide arbitrarily, it's a matter of policy, different for each
> use-case.
>
>> Rob
>
> Regards
>
> -- Pantelis

Alan Tull

>
>

[PATCH 0/5] PowerPC-pSeries: Adjustments for seven function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 18 Oct 2017 21:11:23 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Delete five error messages for a failed memory allocation
  Improve nine size determinations
  Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()
  Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()
  Less function calls in iommu_pseries_alloc_group() after error detection

 arch/powerpc/platforms/pseries/dlpar.c |  5 ++--
 arch/powerpc/platforms/pseries/dtl.c   |  5 +---
 arch/powerpc/platforms/pseries/hvcserver.c |  7 ++
 arch/powerpc/platforms/pseries/iommu.c | 40 --
 arch/powerpc/platforms/pseries/vio.c   |  9 +++
 5 files changed, 24 insertions(+), 42 deletions(-)

-- 
2.14.2

[PATCH 1/5] powerpc-pseries: Delete five error messages for a failed memory allocation

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 18 Oct 2017 16:39:01 +0200

Omit extra messages for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/pseries/dtl.c   |  5 +
 arch/powerpc/platforms/pseries/hvcserver.c |  2 --
 arch/powerpc/platforms/pseries/iommu.c | 11 +++
 arch/powerpc/platforms/pseries/vio.c   |  4 +---
 4 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dtl.c 
b/arch/powerpc/platforms/pseries/dtl.c
index 18014cdeb590..467b9f578a7d 100644
--- a/arch/powerpc/platforms/pseries/dtl.c
+++ b/arch/powerpc/platforms/pseries/dtl.c
@@ -203,11 +203,8 @@ static int dtl_enable(struct dtl *dtl)
 
n_entries = dtl_buf_entries;
buf = kmem_cache_alloc_node(dtl_cache, GFP_KERNEL, 
cpu_to_node(dtl->cpu));
-   if (!buf) {
-   printk(KERN_WARNING "%s: buffer alloc failed for cpu %d\n",
-   __func__, dtl->cpu);
+   if (!buf)
return -ENOMEM;
-   }
 
spin_lock(>lock);
rc = -EBUSY;
diff --git a/arch/powerpc/platforms/pseries/hvcserver.c 
b/arch/powerpc/platforms/pseries/hvcserver.c
index 94a6e5612b0d..db2c20e65a58 100644
--- a/arch/powerpc/platforms/pseries/hvcserver.c
+++ b/arch/powerpc/platforms/pseries/hvcserver.c
@@ -177,8 +177,6 @@ int hvcs_get_partner_info(uint32_t unit_address, struct 
list_head *head,
GFP_ATOMIC);
 
if (!next_partner_info) {
-   printk(KERN_WARNING "HVCONSOLE: kmalloc() failed to"
-   " allocate partner info struct.\n");
hvcs_free_partner_info(head);
return -ENOMEM;
}
diff --git a/arch/powerpc/platforms/pseries/iommu.c 
b/arch/powerpc/platforms/pseries/iommu.c
index 7c181467d0ad..c92822fdf269 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -1063,19 +1063,14 @@ static u64 enable_ddw(struct pci_dev *dev, struct 
device_node *pdn)
}
len = order_base_2(max_addr);
win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
-   if (!win64) {
-   dev_info(>dev,
-   "couldn't allocate property for 64bit dma window\n");
+   if (!win64)
goto out_failed;
-   }
+
win64->name = kstrdup(DIRECT64_PROPNAME, GFP_KERNEL);
win64->value = ddwprop = kmalloc(sizeof(*ddwprop), GFP_KERNEL);
win64->length = sizeof(*ddwprop);
-   if (!win64->name || !win64->value) {
-   dev_info(>dev,
-   "couldn't allocate property name and value\n");
+   if (!win64->name || !win64->value)
goto out_free_prop;
-   }
 
ret = create_ddw(dev, ddw_avail, , page_shift, len);
if (ret != 0)
diff --git a/arch/powerpc/platforms/pseries/vio.c 
b/arch/powerpc/platforms/pseries/vio.c
index 12277bc9fd9e..74b919040e68 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -1386,10 +1386,8 @@ struct vio_dev *vio_register_device_node(struct 
device_node *of_node)
 
/* allocate a vio_dev for this node */
viodev = kzalloc(sizeof(struct vio_dev), GFP_KERNEL);
-   if (viodev == NULL) {
-   pr_warn("%s: allocation failure for VIO device.\n", __func__);
+   if (!viodev)
return NULL;
-   }
 
/* we need the 'device_type' property, in order to match with drivers */
viodev->family = family;
-- 
2.14.2

Re: [PATCH v2] kernel/module_64.c: Add REL24 relocation support of livepatch symbols

[Kamalesh Babulal]

On Tuesday 17 October 2017 08:17 PM, Torsten Duwe wrote:
> On Fri, Oct 06, 2017 at 11:27:42AM +0530, Kamalesh Babulal wrote:
>>
>> Consider the livepatch sequence[1]. Where function A calls B, B is the
>> function which has been livepatched and the call to function B is
>> redirected to patched version P. P calls the function C in M2, whereas
>> C was local to the function B and have became SHN_UNDEF in function P.
>> Local call becoming global.
>>
>>   ++++++  ++
>>   ||   +++--->||  +-->||
>>   |  A |   ||  B ||  F |  |   |  P |
>>   ||   ||||+--+   ||
>>   |+---+||||<-+   ||
>>   ||<--+   ++   C|||  |   ||
>>   ||   |   | +->||||  |   ||<---+
>>   | K / M1 |   |   | |  | K / M2 |  +-+ Kernel |  +---+ Mod3   +--+ |
>>   ++   |   | |  ++  | ++  ++  | |
>>|   | |  | | |
>>+---+-+--+ | |
>>| || |
>>| ++ |
>>++
>>
>>
>> Handling such call with regular stub, triggers another error:
>>
>> module_64: kpatch_meminfo: Expect noop after relocate, got 3d22
>>
>> Every branch to SHN_UNDEF is followed by a nop instruction, that gets
>> overwritten by an instruction to restore TOC with r2 value that get
>> stored onto the stack, before calling the function via global entry
>> point.
>>
>> Given that C was local to function B, it does not store/restore TOC as
>> they are not expected to be clobbered for functions called via local
>> entry point.
> 
> Can you please provide example source code of Mod3 and C? If P calls C, this
> is a regular global call, the TOC is saved by the stub and restored after the
> call instruction. Why do you think this is not the case? 
> 

Consider a trivial patch, supplied to kpatch tool for generating a
livepatch module:

--- a/fs/proc/meminfo.c
+++ b/fs/proc/meminfo.c
@@ -132,7 +132,7 @@ static int meminfo_proc_show(struct seq_file *m, void *v)
seq_printf(m, "VmallocTotal:   %8lu kB\n",
   (unsigned long)VMALLOC_TOTAL >> 10);
show_val_kb(m, "VmallocUsed:", 0ul);
-   show_val_kb(m, "VmallocChunk:   ", 0ul);
+   show_val_kb(m, "VMALLOCCHUNK:   ", 0ul);

 #ifdef CONFIG_MEMORY_FAILURE
seq_printf(m, "HardwareCorrupted: %5lu kB\n",

# readelf -s -W ./fs/proc/meminfo.o
Symbol table '.symtab' contains 54 entries:
Num: Value Size Type Bind  Vis Ndx Name
...
23:  0x50  224  FUNC LOCAL DEFAULT [: 8] 1 show_val_kb
...

# objdump -dr ./fs/proc/meminfo.o
0140 :
 204:   01 00 00 48 bl  204 
204: R_PPC64_REL24  si_mem_available
 208:   00 00 00 60 nop
 ...
 220:   01 00 00 48 bl  220 
220: R_PPC64_REL24  show_val_kb
 224:   88 00 a1 e8 ld  r5,136(r1)
 228:   00 00 82 3c addis   r4,r2,0

show_val_kb() is called by meminfo_proc_show() frequently to print memory
statistics, is also defined in meminfo.o. Which means both the functions
share the same TOC base and is accessed via local entry point by
calculating the offset with respect to current TOC.

A nop instruction is only excepted after every branch to a global call.
That gets overwritten by an instruction to restore TOC with r2 value of
callee. Given function show_val_kb() is local to meminfo_proc_show(),
any call to show_val_kb() doesn't requires setting up/restoring TOC as
they are not expected to be clobbered for local function call (via local
entry point).

kpatch identifies the patched function to be meminfo_proc_show() and copies
it into livepatch module, along with required symbols and livepatch hooks
but doesn't copies show_val_kb(). The reason being, it can be called like
any other global function and is marked with SHN_LIVEPATCH symbol section
index. show_val_kb() which is local to meminfo_proc_show(), is global to
patched version of meminfo_proc_show().

Symbol table '.symtab' contains 91 entries:
Num: Value Size Type Bind  Vis Ndx Name
...
82:  0x0   0FUNC LOCAL DEFAULT OS [0xff20] .klp.sym.vmlinux.show_val_kb,1
...

apply_relocate_add() should be modified to handle show_val_kb() via global
entry point (through stub) like SHN_UNDEF. Branch to a global function, is
excepted to be followed by a nop instruction. Whereas patched version of
meminfo_proc_show() code is not modified to add a nop after every branch to
show_val_kb(). Nop instruction is required for setting up r2 with the TOC
of livepatch module, which is 

[PATCH 2/5] powerpc-pseries: Improve nine size determinations

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 18 Oct 2017 18:18:11 +0200

Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/pseries/dlpar.c |  5 ++---
 arch/powerpc/platforms/pseries/hvcserver.c |  5 ++---
 arch/powerpc/platforms/pseries/iommu.c | 10 --
 arch/powerpc/platforms/pseries/vio.c   |  5 ++---
 4 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c 
b/arch/powerpc/platforms/pseries/dlpar.c
index 6e35780c5962..dca88997cb4b 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -389,11 +389,10 @@ void queue_hotplug_event(struct pseries_hp_errorlog 
*hp_errlog,
struct pseries_hp_work *work;
struct pseries_hp_errorlog *hp_errlog_copy;
 
-   hp_errlog_copy = kmalloc(sizeof(struct pseries_hp_errorlog),
-GFP_KERNEL);
+   hp_errlog_copy = kmalloc(sizeof(*hp_errlog_copy), GFP_KERNEL);
memcpy(hp_errlog_copy, hp_errlog, sizeof(struct pseries_hp_errorlog));
 
-   work = kmalloc(sizeof(struct pseries_hp_work), GFP_KERNEL);
+   work = kmalloc(sizeof(*work), GFP_KERNEL);
if (work) {
INIT_WORK((struct work_struct *)work, pseries_hp_work_fn);
work->errlog = hp_errlog_copy;
diff --git a/arch/powerpc/platforms/pseries/hvcserver.c 
b/arch/powerpc/platforms/pseries/hvcserver.c
index db2c20e65a58..b85cca04dd7d 100644
--- a/arch/powerpc/platforms/pseries/hvcserver.c
+++ b/arch/powerpc/platforms/pseries/hvcserver.c
@@ -173,9 +173,8 @@ int hvcs_get_partner_info(uint32_t unit_address, struct 
list_head *head,
 
/* This is a very small struct and will be freed soon in
 * hvcs_free_partner_info(). */
-   next_partner_info = kmalloc(sizeof(struct hvcs_partner_info),
-   GFP_ATOMIC);
-
+   next_partner_info = kmalloc(sizeof(*next_partner_info),
+   GFP_ATOMIC);
if (!next_partner_info) {
hvcs_free_partner_info(head);
return -ENOMEM;
diff --git a/arch/powerpc/platforms/pseries/iommu.c 
b/arch/powerpc/platforms/pseries/iommu.c
index c92822fdf269..b37d4fb20d1c 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -59,17 +59,15 @@ static struct iommu_table_group 
*iommu_pseries_alloc_group(int node)
struct iommu_table *tbl = NULL;
struct iommu_table_group_link *tgl = NULL;
 
-   table_group = kzalloc_node(sizeof(struct iommu_table_group), GFP_KERNEL,
-  node);
+   table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
if (!table_group)
goto fail_exit;
 
-   tbl = kzalloc_node(sizeof(struct iommu_table), GFP_KERNEL, node);
+   tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
if (!tbl)
goto fail_exit;
 
-   tgl = kzalloc_node(sizeof(struct iommu_table_group_link), GFP_KERNEL,
-   node);
+   tgl = kzalloc_node(sizeof(*tgl), GFP_KERNEL, node);
if (!tgl)
goto fail_exit;
 
@@ -1062,7 +1060,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct 
device_node *pdn)
goto out_failed;
}
len = order_base_2(max_addr);
-   win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
+   win64 = kzalloc(sizeof(*win64), GFP_KERNEL);
if (!win64)
goto out_failed;
 
diff --git a/arch/powerpc/platforms/pseries/vio.c 
b/arch/powerpc/platforms/pseries/vio.c
index 74b919040e68..cf0939eb3aee 100644
--- a/arch/powerpc/platforms/pseries/vio.c
+++ b/arch/powerpc/platforms/pseries/vio.c
@@ -754,8 +754,7 @@ static int vio_cmo_bus_probe(struct vio_dev *viodev)
viodev->cmo.desired = VIO_CMO_MIN_ENT;
size = VIO_CMO_MIN_ENT;
 
-   dev_ent = kmalloc(sizeof(struct vio_cmo_dev_entry),
- GFP_KERNEL);
+   dev_ent = kmalloc(sizeof(*dev_ent), GFP_KERNEL);
if (!dev_ent)
return -ENOMEM;
 
@@ -1385,7 +1384,7 @@ struct vio_dev *vio_register_device_node(struct 
device_node *of_node)
}
 
/* allocate a vio_dev for this node */
-   viodev = kzalloc(sizeof(struct vio_dev), GFP_KERNEL);
+   viodev = kzalloc(sizeof(*viodev), GFP_KERNEL);
if (!viodev)
return NULL;
 
-- 
2.14.2

[PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 18 Oct 2017 19:14:39 +0200

The variable "table_group" will be set to an appropriate pointer.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/pseries/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c 
b/arch/powerpc/platforms/pseries/iommu.c
index b37d4fb20d1c..b6c12b8e3ace 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -55,7 +55,7 @@
 
 static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 {
-   struct iommu_table_group *table_group = NULL;
+   struct iommu_table_group *table_group;
struct iommu_table *tbl = NULL;
struct iommu_table_group_link *tgl = NULL;
 
-- 
2.14.2

[PATCH 4/5] powerpc-pseries: Return directly after a failed kzalloc_node() in iommu_pseries_alloc_group()

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 18 Oct 2017 20:15:32 +0200

Return directly after a call of the function "kzalloc_node" failed
at the beginning.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/pseries/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c 
b/arch/powerpc/platforms/pseries/iommu.c
index b6c12b8e3ace..207ff8351af1 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -61,7 +61,7 @@ static struct iommu_table_group 
*iommu_pseries_alloc_group(int node)
 
table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
if (!table_group)
-   goto fail_exit;
+   return NULL;
 
tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
if (!tbl)
-- 
2.14.2

[PATCH 5/5] powerpc-pseries: Less function calls in iommu_pseries_alloc_group() after error detection

[SF Markus Elfring]

From: Markus Elfring 
Date: Wed, 18 Oct 2017 20:48:52 +0200

The kfree() function was called in up to two cases by the
iommu_pseries_alloc_group() function during error handling
even if the passed variable contained a null pointer.

* Adjust jump targets according to the Linux coding style convention.

* Delete initialisations for the variables "tbl" and "tgl"
  which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 arch/powerpc/platforms/pseries/iommu.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c 
b/arch/powerpc/platforms/pseries/iommu.c
index 207ff8351af1..13b424f34039 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -56,8 +56,8 @@
 static struct iommu_table_group *iommu_pseries_alloc_group(int node)
 {
struct iommu_table_group *table_group;
-   struct iommu_table *tbl = NULL;
-   struct iommu_table_group_link *tgl = NULL;
+   struct iommu_table *tbl;
+   struct iommu_table_group_link *tgl;
 
table_group = kzalloc_node(sizeof(*table_group), GFP_KERNEL, node);
if (!table_group)
@@ -65,11 +65,11 @@ static struct iommu_table_group 
*iommu_pseries_alloc_group(int node)
 
tbl = kzalloc_node(sizeof(*tbl), GFP_KERNEL, node);
if (!tbl)
-   goto fail_exit;
+   goto free_group;
 
tgl = kzalloc_node(sizeof(*tgl), GFP_KERNEL, node);
if (!tgl)
-   goto fail_exit;
+   goto free_table;
 
INIT_LIST_HEAD_RCU(>it_group_list);
kref_init(>it_kref);
@@ -80,11 +80,10 @@ static struct iommu_table_group 
*iommu_pseries_alloc_group(int node)
 
return table_group;
 
-fail_exit:
-   kfree(tgl);
-   kfree(table_group);
+free_table:
kfree(tbl);
-
+free_group:
+   kfree(table_group);
return NULL;
 }
 
-- 
2.14.2

Re: [PATCH 0/2] PowerPC-PS3: Adjustments for three function implementations

[Geoff Levand]

On 10/17/2017 11:54 AM, SF Markus Elfring wrote:
> Markus Elfring (2):
>   Delete an error message for a failed memory allocation in update_flash_db()
>   Improve a size determination in two functions

For consistency, please use 'powerpc/ps3' not 'powerpc-ps3' as
the commit log subject prefix.  Also, please try to keep the
commit log subject to 50 characters or less.

-Geoff

Re: [PATCH 18/25] powerpc: check key protection for user page access

[Balbir Singh]

On Fri,  8 Sep 2017 15:45:06 -0700
Ram Pai  wrote:

> Make sure that the kernel does not access user pages without
> checking their key-protection.
>

Why? This makes the routines AMR/thread specific? Looks like
x86 does this as well, but these routines are used by GUP from
the kernel.

Balbir Singh.

[PATCH V2 0/3] pseries/nodes: Fix issues with memoryless nodes

[Michael Bringmann]

pseries/nodes: Ensure enough nodes avail for operations

pseries/findnodes: Find nodes with memory when booting memoryless nodes

pseries/initnodes: Ensure nodes initialized for hotplug

Signed-off-by: Michael Bringmann 

Michael Bringmann (3):
  pseries/nodes: Ensure enough nodes avail for operations
  pseries/findnodes: Find nodes with memory when booting memoryless nodes
  pseries/initnodes: Ensure nodes initialized for hotplug

[PATCH V2 1/3] pseries/nodes: Ensure enough nodes avail for operations

[Michael Bringmann]

pseries/nodes: On pseries systems which allow 'hot-add' of CPU or
memory resources, it may occur that the new resources are to be
inserted into nodes that were not used for these resources at bootup.
In the kernel, any node that is used must be defined and initialized.
This patch ensures that sufficient nodes are defined to support
configuration requirements after boot, as well as at boot.

This patch extracts the value of the lowest domain level (number
of allocable resources) from the device tree property
"ibm,max-associativity-domains" to use as the maximum number of nodes
to setup as possibly available in the system.  This new setting will
override the instruction,

nodes_and(node_possible_map, node_possible_map, node_online_map);

presently seen in the function arch/powerpc/mm/numa.c:initmem_init().

If the property is not present at boot, no operation will be performed
to define or enable additional nodes.

Signed-off-by: Michael Bringmann 
---
 arch/powerpc/mm/numa.c |   39 ---
 1 file changed, 36 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index ec098b3..f885ab7 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -892,6 +892,36 @@ static void __init setup_node_data(int nid, u64 start_pfn, 
u64 end_pfn)
NODE_DATA(nid)->node_spanned_pages = spanned_pages;
 }
 
+static void __init find_possible_nodes(void)
+{
+   struct device_node *rtas;
+   u32 numnodes, i;
+
+   if (min_common_depth <= 0)
+   return;
+
+   rtas = of_find_node_by_path("/rtas");
+   if (!rtas)
+   return;
+
+   if (of_property_read_u32_index(rtas,
+   "ibm,max-associativity-domains",
+   min_common_depth, ))
+   goto out;
+
+   pr_info("numa: Nodes = %d (mcd = %d)\n", numnodes,
+   min_common_depth);
+
+   for (i = 0; i < numnodes; i++) {
+   if (!node_possible(i))
+   node_set(i, node_possible_map);
+   }
+
+out:
+   if (rtas)
+   of_node_put(rtas);
+}
+
 void __init initmem_init(void)
 {
int nid, cpu;
@@ -905,12 +935,15 @@ void __init initmem_init(void)
memblock_dump_all();
 
/*
-* Reduce the possible NUMA nodes to the online NUMA nodes,
-* since we do not support node hotplug. This ensures that  we
-* lower the maximum NUMA node ID to what is actually present.
+* Modify the set of possible NUMA nodes to reflect information
+* available about the set of online nodes, and the set of nodes
+* that we expect to make use of for this platform's affinity
+* calculations.
 */
nodes_and(node_possible_map, node_possible_map, node_online_map);
 
+   find_possible_nodes();
+
for_each_online_node(nid) {
unsigned long start_pfn, end_pfn;
 

[PATCH V2 2/3] pseries/findnodes: Find nodes with memory for memoryless nodes

[Michael Bringmann]

pseries/findnodes: On pseries systems which allow 'hot-add' of
resources, we may boot configurations that have CPUs, but no memory
associated to a node by the affinity calculations.  Previously, the
software took a shortcut to collapse initialization and references
to such memoryless nodes with other nodes that did have memory
associated with them at boot.  This patch is based on fixes that
allow the proper initialization and distinguishment of memoryless
and memory-plus nodes after NUMA initialization.  It extends the
use of the 'node_to_mem_node()' API from 'topology.h' to modules
that are allocating node-specific memory at boot, and allows such
references to find available memory in another node.

Signed-off-by: Michael Bringmann 
---
 block/blk-mq-cpumap.c |3 ++-
 mm/page_alloc.c   |1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-cpumap.c b/block/blk-mq-cpumap.c
index 9f8cffc..a27a31f 100644
--- a/block/blk-mq-cpumap.c
+++ b/block/blk-mq-cpumap.c
@@ -73,7 +73,8 @@ int blk_mq_hw_queue_to_node(unsigned int *mq_map, unsigned 
int index)
 
for_each_possible_cpu(i) {
if (index == mq_map[i])
-   return local_memory_node(cpu_to_node(i));
+   return local_memory_node(
+   node_to_mem_node(cpu_to_node(i)));
}
 
return NUMA_NO_NODE;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 77e4d3c..e7aaa2a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4188,6 +4188,7 @@ struct page *
 
gfp_mask &= gfp_allowed_mask;
alloc_mask = gfp_mask;
+   preferred_nid = node_to_mem_node(preferred_nid);
if (!prepare_alloc_pages(gfp_mask, order, preferred_nid, nodemask, , 
_mask, _flags))
return NULL;
 

[PATCH V2 3/3] pseries/initnodes: Ensure nodes initialized for hotplug

[Michael Bringmann]

pseries/nodes: On pseries systems which allow 'hot-add' of CPU,
it may occur that the new resources are to be inserted into nodes
that were not used for memory resources at bootup.  Many different
configurations of PowerPC resources may need to be supported depending
upon the environment.  This patch fixes some problems encountered at
runtime with configurations that support memory-less nodes, or that
hot-add resources during system execution after boot.

Signed-off-by: Michael Bringmann 
---
 arch/powerpc/mm/numa.c |   27 ---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index f885ab7..2be6363 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -551,7 +551,7 @@ static int numa_setup_cpu(unsigned long lcpu)
nid = of_node_to_nid_single(cpu);
 
 out_present:
-   if (nid < 0 || !node_online(nid))
+   if (nid < 0 || !node_possible(nid))
nid = first_online_node;
 
map_cpu_to_node(lcpu, nid);
@@ -1311,6 +1311,25 @@ static long vphn_get_associativity(unsigned long cpu,
return rc;
 }
 
+static int verify_node_preparation(int nid)
+{
+   /*
+* Need to allocate/initialize NODE_DATA from a node with
+* memory (see memblock_alloc_try_nid).  Code executed after
+* boot (like local_memory_node) often does not know enough
+* to recover fully for memoryless nodes. 
+*/
+   if (NODE_DATA(nid) == NULL) 
+   setup_node_data(nid, 0, 0);
+
+   if (NODE_DATA(nid)->node_spanned_pages == 0) {
+   if (try_online_node(nid))
+   return first_online_node;
+   }
+
+   return nid;
+}
+
 /*
  * Update the CPU maps and sysfs entries for a single CPU when its NUMA
  * characteristics change. This function doesn't perform any locking and is
@@ -1334,7 +1353,7 @@ static int update_cpu_topology(void *data)
unmap_cpu_from_node(cpu);
map_cpu_to_node(cpu, new_nid);
set_cpu_numa_node(cpu, new_nid);
-   set_cpu_numa_mem(cpu, local_memory_node(new_nid));
+   set_cpu_numa_mem(cpu, 
local_memory_node(node_to_mem_node(new_nid)));
vdso_getcpu_init();
}
 
@@ -1419,9 +1438,11 @@ int numa_update_cpu_topology(bool cpus_locked)
/* Use associativity from first thread for all siblings */
vphn_get_associativity(cpu, associativity);
new_nid = associativity_to_nid(associativity);
-   if (new_nid < 0 || !node_online(new_nid))
+   if (new_nid < 0 || !node_possible(new_nid))
new_nid = first_online_node;
 
+   new_nid = verify_node_preparation(new_nid);
+
if (new_nid == numa_cpu_lookup_table[cpu]) {
cpumask_andnot(_associativity_changes_mask,
_associativity_changes_mask,