* [PATCH v4 17/21] powerpc/8xx: set PTE bit 22 off TLBmiss
@ 2014-09-19 8:36 Christophe Leroy
2014-11-07 3:37 ` [v4,17/21] " Scott Wood
0 siblings, 1 reply; 6+ messages in thread
From: Christophe Leroy @ 2014-09-19 8:36 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras
Cc: scottwood, linuxppc-dev, linux-kernel
No need to re-set this bit at each TLB miss. Let's set it in the PTE.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
Changes in v2:
- None
Changes in v3:
- Removed PPC405 related macro from PPC8xx specific code
- PTE_NONE_MASK doesn't need PAGE_ACCESSED in Linux 2.6
Changes in v4:
- None
arch/powerpc/include/asm/pgtable-ppc32.h | 20 ++++++++++++++++++++
arch/powerpc/include/asm/pte-8xx.h | 7 +++++--
arch/powerpc/kernel/head_8xx.S | 10 ++--------
3 files changed, 27 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h b/arch/powerpc/include/asm/pgtable-ppc32.h
index 47edde8..35a9b44 100644
--- a/arch/powerpc/include/asm/pgtable-ppc32.h
+++ b/arch/powerpc/include/asm/pgtable-ppc32.h
@@ -172,6 +172,25 @@ static inline unsigned long pte_update(pte_t *p,
#ifdef PTE_ATOMIC_UPDATES
unsigned long old, tmp;
+#ifdef CONFIG_PPC_8xx
+ unsigned long tmp2;
+
+ __asm__ __volatile__("\
+1: lwarx %0,0,%4\n\
+ andc %1,%0,%5\n\
+ or %1,%1,%6\n\
+ /* 0x200 == Extended encoding, bit 22 */ \
+ /* Bit 22 has to be 1 if neither _PAGE_USER nor _PAGE_RW are set */ \
+ rlwimi %1,%1,32-2,0x200\n /* get _PAGE_USER */ \
+ rlwinm %3,%1,32-1,0x200\n /* get _PAGE_RW */ \
+ or %1,%3,%1\n\
+ xori %1,%1,0x200\n"
+" stwcx. %1,0,%4\n\
+ bne- 1b"
+ : "=&r" (old), "=&r" (tmp), "=m" (*p), "=&r" (tmp2)
+ : "r" (p), "r" (clr), "r" (set), "m" (*p)
+ : "cc" );
+#else /* CONFIG_PPC_8xx */
__asm__ __volatile__("\
1: lwarx %0,0,%3\n\
andc %1,%0,%4\n\
@@ -182,6 +201,7 @@ static inline unsigned long pte_update(pte_t *p,
: "=&r" (old), "=&r" (tmp), "=m" (*p)
: "r" (p), "r" (clr), "r" (set), "m" (*p)
: "cc" );
+#endif /* CONFIG_PPC_8xx */
#else /* PTE_ATOMIC_UPDATES */
unsigned long old = pte_val(*p);
*p = __pte((old & ~clr) | set);
diff --git a/arch/powerpc/include/asm/pte-8xx.h b/arch/powerpc/include/asm/pte-8xx.h
index d44826e..daa4616 100644
--- a/arch/powerpc/include/asm/pte-8xx.h
+++ b/arch/powerpc/include/asm/pte-8xx.h
@@ -48,19 +48,22 @@
*/
#define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */
#define _PAGE_USER 0x0800 /* msb PP bits */
+/* set when neither _PAGE_USER nor _PAGE_RW are set */
+#define _PAGE_KNLRO 0x0200
#define _PMD_PRESENT 0x0001
#define _PMD_BAD 0x0ff0
#define _PMD_PAGE_MASK 0x000c
#define _PMD_PAGE_8M 0x000c
-#define _PTE_NONE_MASK _PAGE_ACCESSED
+#define _PTE_NONE_MASK _PAGE_KNLRO
/* Until my rework is finished, 8xx still needs atomic PTE updates */
#define PTE_ATOMIC_UPDATES 1
/* We need to add _PAGE_SHARED to kernel pages */
-#define _PAGE_KERNEL_RO (_PAGE_SHARED)
+#define _PAGE_KERNEL_RO (_PAGE_SHARED | _PAGE_KNLRO)
+#define _PAGE_KERNEL_ROX (_PAGE_EXEC | _PAGE_KNLRO)
#define _PAGE_KERNEL_RW (_PAGE_DIRTY | _PAGE_RW | _PAGE_HWWRITE)
#endif /* __KERNEL__ */
diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index a7af26e..48d3de8 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -445,14 +445,8 @@ DataStoreTLBMiss:
and r11, r11, r10
rlwimi r10, r11, 0, _PAGE_PRESENT
#endif
- /* Honour kernel RO, User NA */
- /* 0x200 == Extended encoding, bit 22 */
- rlwimi r10, r10, 32-2, 0x200 /* Copy USER to bit 22, 0x200 */
- /* r11 = (r10 & _PAGE_RW) >> 1 */
- rlwinm r11, r10, 32-1, 0x200
- or r10, r11, r10
- /* invert RW and 0x200 bits */
- xori r10, r10, _PAGE_RW | 0x200
+ /* invert RW */
+ xori r10, r10, _PAGE_RW
/* The Linux PTE won't go exactly into the MMU TLB.
* Software indicator bits 22 and 28 must be clear.
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [v4,17/21] powerpc/8xx: set PTE bit 22 off TLBmiss
2014-09-19 8:36 [PATCH v4 17/21] powerpc/8xx: set PTE bit 22 off TLBmiss Christophe Leroy
@ 2014-11-07 3:37 ` Scott Wood
2014-11-07 8:00 ` leroy christophe
0 siblings, 1 reply; 6+ messages in thread
From: Scott Wood @ 2014-11-07 3:37 UTC (permalink / raw)
To: LEROY Christophe; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
On Fri, Sep 19, 2014 at 10:36:09AM +0200, LEROY Christophe wrote:
> No need to re-set this bit at each TLB miss. Let's set it in the PTE.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> Changes in v2:
> - None
>
> Changes in v3:
> - Removed PPC405 related macro from PPC8xx specific code
> - PTE_NONE_MASK doesn't need PAGE_ACCESSED in Linux 2.6
>
> Changes in v4:
> - None
>
> arch/powerpc/include/asm/pgtable-ppc32.h | 20 ++++++++++++++++++++
> arch/powerpc/include/asm/pte-8xx.h | 7 +++++--
> arch/powerpc/kernel/head_8xx.S | 10 ++--------
> 3 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h b/arch/powerpc/include/asm/pgtable-ppc32.h
> index 47edde8..35a9b44 100644
> --- a/arch/powerpc/include/asm/pgtable-ppc32.h
> +++ b/arch/powerpc/include/asm/pgtable-ppc32.h
> @@ -172,6 +172,25 @@ static inline unsigned long pte_update(pte_t *p,
> #ifdef PTE_ATOMIC_UPDATES
> unsigned long old, tmp;
>
> +#ifdef CONFIG_PPC_8xx
> + unsigned long tmp2;
> +
> + __asm__ __volatile__("\
> +1: lwarx %0,0,%4\n\
> + andc %1,%0,%5\n\
> + or %1,%1,%6\n\
> + /* 0x200 == Extended encoding, bit 22 */ \
> + /* Bit 22 has to be 1 if neither _PAGE_USER nor _PAGE_RW are set */ \
> + rlwimi %1,%1,32-2,0x200\n /* get _PAGE_USER */ \
> + rlwinm %3,%1,32-1,0x200\n /* get _PAGE_RW */ \
> + or %1,%3,%1\n\
> + xori %1,%1,0x200\n"
> +" stwcx. %1,0,%4\n\
> + bne- 1b"
Why do you need this...
> diff --git a/arch/powerpc/include/asm/pte-8xx.h b/arch/powerpc/include/asm/pte-8xx.h
> index d44826e..daa4616 100644
> --- a/arch/powerpc/include/asm/pte-8xx.h
> +++ b/arch/powerpc/include/asm/pte-8xx.h
> @@ -48,19 +48,22 @@
> */
> #define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */
> #define _PAGE_USER 0x0800 /* msb PP bits */
> +/* set when neither _PAGE_USER nor _PAGE_RW are set */
> +#define _PAGE_KNLRO 0x0200
>
> #define _PMD_PRESENT 0x0001
> #define _PMD_BAD 0x0ff0
> #define _PMD_PAGE_MASK 0x000c
> #define _PMD_PAGE_8M 0x000c
>
> -#define _PTE_NONE_MASK _PAGE_ACCESSED
> +#define _PTE_NONE_MASK _PAGE_KNLRO
>
> /* Until my rework is finished, 8xx still needs atomic PTE updates */
> #define PTE_ATOMIC_UPDATES 1
>
> /* We need to add _PAGE_SHARED to kernel pages */
> -#define _PAGE_KERNEL_RO (_PAGE_SHARED)
> +#define _PAGE_KERNEL_RO (_PAGE_SHARED | _PAGE_KNLRO)
> +#define _PAGE_KERNEL_ROX (_PAGE_EXEC | _PAGE_KNLRO)
> #define _PAGE_KERNEL_RW (_PAGE_DIRTY | _PAGE_RW | _PAGE_HWWRITE)
...if 0x200 is already being set in the PTE here?
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4,17/21] powerpc/8xx: set PTE bit 22 off TLBmiss
2014-11-07 3:37 ` [v4,17/21] " Scott Wood
@ 2014-11-07 8:00 ` leroy christophe
2014-11-08 0:08 ` Scott Wood
0 siblings, 1 reply; 6+ messages in thread
From: leroy christophe @ 2014-11-07 8:00 UTC (permalink / raw)
To: Scott Wood; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
Le 07/11/2014 04:37, Scott Wood a écrit :
> On Fri, Sep 19, 2014 at 10:36:09AM +0200, LEROY Christophe wrote:
>> No need to re-set this bit at each TLB miss. Let's set it in the PTE.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> Changes in v2:
>> - None
>>
>> Changes in v3:
>> - Removed PPC405 related macro from PPC8xx specific code
>> - PTE_NONE_MASK doesn't need PAGE_ACCESSED in Linux 2.6
>>
>> Changes in v4:
>> - None
>>
>> arch/powerpc/include/asm/pgtable-ppc32.h | 20 ++++++++++++++++++++
>> arch/powerpc/include/asm/pte-8xx.h | 7 +++++--
>> arch/powerpc/kernel/head_8xx.S | 10 ++--------
>> 3 files changed, 27 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h b/arch/powerpc/include/asm/pgtable-ppc32.h
>> index 47edde8..35a9b44 100644
>> --- a/arch/powerpc/include/asm/pgtable-ppc32.h
>> +++ b/arch/powerpc/include/asm/pgtable-ppc32.h
>> @@ -172,6 +172,25 @@ static inline unsigned long pte_update(pte_t *p,
>> #ifdef PTE_ATOMIC_UPDATES
>> unsigned long old, tmp;
>>
>> +#ifdef CONFIG_PPC_8xx
>> + unsigned long tmp2;
>> +
>> + __asm__ __volatile__("\
>> +1: lwarx %0,0,%4\n\
>> + andc %1,%0,%5\n\
>> + or %1,%1,%6\n\
>> + /* 0x200 == Extended encoding, bit 22 */ \
>> + /* Bit 22 has to be 1 if neither _PAGE_USER nor _PAGE_RW are set */ \
>> + rlwimi %1,%1,32-2,0x200\n /* get _PAGE_USER */ \
>> + rlwinm %3,%1,32-1,0x200\n /* get _PAGE_RW */ \
>> + or %1,%3,%1\n\
>> + xori %1,%1,0x200\n"
>> +" stwcx. %1,0,%4\n\
>> + bne- 1b"
> Why do you need this...
>
>> diff --git a/arch/powerpc/include/asm/pte-8xx.h b/arch/powerpc/include/asm/pte-8xx.h
>> index d44826e..daa4616 100644
>> --- a/arch/powerpc/include/asm/pte-8xx.h
>> +++ b/arch/powerpc/include/asm/pte-8xx.h
>> @@ -48,19 +48,22 @@
>> */
>> #define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */
>> #define _PAGE_USER 0x0800 /* msb PP bits */
>> +/* set when neither _PAGE_USER nor _PAGE_RW are set */
>> +#define _PAGE_KNLRO 0x0200
>>
>> #define _PMD_PRESENT 0x0001
>> #define _PMD_BAD 0x0ff0
>> #define _PMD_PAGE_MASK 0x000c
>> #define _PMD_PAGE_8M 0x000c
>>
>> -#define _PTE_NONE_MASK _PAGE_ACCESSED
>> +#define _PTE_NONE_MASK _PAGE_KNLRO
>>
>> /* Until my rework is finished, 8xx still needs atomic PTE updates */
>> #define PTE_ATOMIC_UPDATES 1
>>
>> /* We need to add _PAGE_SHARED to kernel pages */
>> -#define _PAGE_KERNEL_RO (_PAGE_SHARED)
>> +#define _PAGE_KERNEL_RO (_PAGE_SHARED | _PAGE_KNLRO)
>> +#define _PAGE_KERNEL_ROX (_PAGE_EXEC | _PAGE_KNLRO)
>> #define _PAGE_KERNEL_RW (_PAGE_DIRTY | _PAGE_RW | _PAGE_HWWRITE)
> ...if 0x200 is already being set in the PTE here?
>
If I understand well the way it works, those defines are used for
setting the PTE the first time.
Then, pte_update() is used to modify the pte settings on an existing pte
0x200 must be set when and only when the page is a RO kernel page. If
later on pte_update() is called for instance to set the page to RW, the
0x200 has to be removed. Same, if pte_update() is called to remove
_PAGE_RW (ptep_set_wrprotect() does this), 0x200 must be set back.
Christophe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4,17/21] powerpc/8xx: set PTE bit 22 off TLBmiss
2014-11-07 8:00 ` leroy christophe
@ 2014-11-08 0:08 ` Scott Wood
2014-11-18 20:07 ` christophe leroy
0 siblings, 1 reply; 6+ messages in thread
From: Scott Wood @ 2014-11-08 0:08 UTC (permalink / raw)
To: leroy christophe; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
On Fri, 2014-11-07 at 09:00 +0100, leroy christophe wrote:
> Le 07/11/2014 04:37, Scott Wood a écrit :
> > On Fri, Sep 19, 2014 at 10:36:09AM +0200, LEROY Christophe wrote:
> >> No need to re-set this bit at each TLB miss. Let's set it in the PTE.
> >>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> >> ---
> >> Changes in v2:
> >> - None
> >>
> >> Changes in v3:
> >> - Removed PPC405 related macro from PPC8xx specific code
> >> - PTE_NONE_MASK doesn't need PAGE_ACCESSED in Linux 2.6
> >>
> >> Changes in v4:
> >> - None
> >>
> >> arch/powerpc/include/asm/pgtable-ppc32.h | 20 ++++++++++++++++++++
> >> arch/powerpc/include/asm/pte-8xx.h | 7 +++++--
> >> arch/powerpc/kernel/head_8xx.S | 10 ++--------
> >> 3 files changed, 27 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h b/arch/powerpc/include/asm/pgtable-ppc32.h
> >> index 47edde8..35a9b44 100644
> >> --- a/arch/powerpc/include/asm/pgtable-ppc32.h
> >> +++ b/arch/powerpc/include/asm/pgtable-ppc32.h
> >> @@ -172,6 +172,25 @@ static inline unsigned long pte_update(pte_t *p,
> >> #ifdef PTE_ATOMIC_UPDATES
> >> unsigned long old, tmp;
> >>
> >> +#ifdef CONFIG_PPC_8xx
> >> + unsigned long tmp2;
> >> +
> >> + __asm__ __volatile__("\
> >> +1: lwarx %0,0,%4\n\
> >> + andc %1,%0,%5\n\
> >> + or %1,%1,%6\n\
> >> + /* 0x200 == Extended encoding, bit 22 */ \
> >> + /* Bit 22 has to be 1 if neither _PAGE_USER nor _PAGE_RW are set */ \
> >> + rlwimi %1,%1,32-2,0x200\n /* get _PAGE_USER */ \
> >> + rlwinm %3,%1,32-1,0x200\n /* get _PAGE_RW */ \
> >> + or %1,%3,%1\n\
> >> + xori %1,%1,0x200\n"
> >> +" stwcx. %1,0,%4\n\
> >> + bne- 1b"
> > Why do you need this...
> >
> >> diff --git a/arch/powerpc/include/asm/pte-8xx.h b/arch/powerpc/include/asm/pte-8xx.h
> >> index d44826e..daa4616 100644
> >> --- a/arch/powerpc/include/asm/pte-8xx.h
> >> +++ b/arch/powerpc/include/asm/pte-8xx.h
> >> @@ -48,19 +48,22 @@
> >> */
> >> #define _PAGE_RW 0x0400 /* lsb PP bits, inverted in HW */
> >> #define _PAGE_USER 0x0800 /* msb PP bits */
> >> +/* set when neither _PAGE_USER nor _PAGE_RW are set */
> >> +#define _PAGE_KNLRO 0x0200
> >>
> >> #define _PMD_PRESENT 0x0001
> >> #define _PMD_BAD 0x0ff0
> >> #define _PMD_PAGE_MASK 0x000c
> >> #define _PMD_PAGE_8M 0x000c
> >>
> >> -#define _PTE_NONE_MASK _PAGE_ACCESSED
> >> +#define _PTE_NONE_MASK _PAGE_KNLRO
> >>
> >> /* Until my rework is finished, 8xx still needs atomic PTE updates */
> >> #define PTE_ATOMIC_UPDATES 1
> >>
> >> /* We need to add _PAGE_SHARED to kernel pages */
> >> -#define _PAGE_KERNEL_RO (_PAGE_SHARED)
> >> +#define _PAGE_KERNEL_RO (_PAGE_SHARED | _PAGE_KNLRO)
> >> +#define _PAGE_KERNEL_ROX (_PAGE_EXEC | _PAGE_KNLRO)
> >> #define _PAGE_KERNEL_RW (_PAGE_DIRTY | _PAGE_RW | _PAGE_HWWRITE)
> > ...if 0x200 is already being set in the PTE here?
> >
> If I understand well the way it works, those defines are used for
> setting the PTE the first time.
> Then, pte_update() is used to modify the pte settings on an existing pte
>
> 0x200 must be set when and only when the page is a RO kernel page. If
> later on pte_update() is called for instance to set the page to RW, the
> 0x200 has to be removed. Same, if pte_update() is called to remove
> _PAGE_RW (ptep_set_wrprotect() does this), 0x200 must be set back.
OK, so the _PAGE_KERNEL_RO(X) stuff is because initially setting the PTE
doesn't go through pte_update().
I'll apply this, though it'd be cleaner to just have 8xx versions of the
relevant PTE accessor functions to maintain the PTE the way the hardware
wants (this would also eliminate the _PAGE_RW inversion that's still in
the TLB miss handler).
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4,17/21] powerpc/8xx: set PTE bit 22 off TLBmiss
2014-11-08 0:08 ` Scott Wood
@ 2014-11-18 20:07 ` christophe leroy
2014-11-26 1:58 ` Scott Wood
0 siblings, 1 reply; 6+ messages in thread
From: christophe leroy @ 2014-11-18 20:07 UTC (permalink / raw)
To: Scott Wood; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
Le 08/11/2014 01:08, Scott Wood a écrit :
>
> OK, so the _PAGE_KERNEL_RO(X) stuff is because initially setting the PTE
> doesn't go through pte_update().
>
> I'll apply this, though it'd be cleaner to just have 8xx versions of the
> relevant PTE accessor functions to maintain the PTE the way the hardware
> wants (this would also eliminate the _PAGE_RW inversion that's still in
> the TLB miss handler).
>
>
Yes, I've been looking at a simple way to also eliminate the _PAGE_RW
inversion, but i've not been able to find an easy solution up to now.
It seems that we have two functions that set PTE: set_pte_at() and
pte_update()
I could perform the bit 22 (0x200) stuff and the _PAGE_RW invertion in
both functions, but then there are functions that read the PTE to make
decision based on PAGE_RW for instance. Most (but not all it seems) do
it through pte_val()
But pte_val() is defined in page.h and doesn't seem to be intended to be
family specific. Should I change this ? if so, what is it cleanest way
to do so ?
Should I also change __pgprot() and __pte() to do the bit 22 and PAGE_RW
inversion stuffs ? But not all functions use those accessors.
So for the time being I don't see the best way to progress on this. Any
suggestion ?
Christophe
---
Ce courrier électronique ne contient aucun virus ou logiciel malveillant parce que la protection avast! Antivirus est active.
http://www.avast.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [v4,17/21] powerpc/8xx: set PTE bit 22 off TLBmiss
2014-11-18 20:07 ` christophe leroy
@ 2014-11-26 1:58 ` Scott Wood
0 siblings, 0 replies; 6+ messages in thread
From: Scott Wood @ 2014-11-26 1:58 UTC (permalink / raw)
To: christophe leroy; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
On Tue, 2014-11-18 at 21:07 +0100, christophe leroy wrote:
> Le 08/11/2014 01:08, Scott Wood a écrit :
> >
> > OK, so the _PAGE_KERNEL_RO(X) stuff is because initially setting the PTE
> > doesn't go through pte_update().
> >
> > I'll apply this, though it'd be cleaner to just have 8xx versions of the
> > relevant PTE accessor functions to maintain the PTE the way the hardware
> > wants (this would also eliminate the _PAGE_RW inversion that's still in
> > the TLB miss handler).
> >
> >
>
> Yes, I've been looking at a simple way to also eliminate the _PAGE_RW
> inversion, but i've not been able to find an easy solution up to now.
>
> It seems that we have two functions that set PTE: set_pte_at() and
> pte_update()
> I could perform the bit 22 (0x200) stuff and the _PAGE_RW invertion in
> both functions, but then there are functions that read the PTE to make
> decision based on PAGE_RW for instance.
Where? The only relevant use of _PAGE_RW outside accessors that I see
is in gup_pte_range(), which shouldn't be too hard to fix up.
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-26 1:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-19 8:36 [PATCH v4 17/21] powerpc/8xx: set PTE bit 22 off TLBmiss Christophe Leroy
2014-11-07 3:37 ` [v4,17/21] " Scott Wood
2014-11-07 8:00 ` leroy christophe
2014-11-08 0:08 ` Scott Wood
2014-11-18 20:07 ` christophe leroy
2014-11-26 1:58 ` Scott Wood
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).