On Tue, Mar 24, 2015 at 04:08:35PM -0600, Toshi Kani wrote:
> Document the return values of KVA mapping functions,
KVA?
Please write it out.
> pud_set_huge(), pmd_set_huge, pud_clear_huge() and
> pmd_clear_huge().
>
> Simplify the conditions to select HAVE_ARCH_HUGE_VMAP
> in the Kconfig, since X86_PAE depends on X86_32.
>
> There is no functional change in this patch.
>
> Signed-off-by: Toshi Kani <[email protected]>
> ---
> arch/x86/Kconfig | 2 +-
> arch/x86/mm/pgtable.c | 36 ++++++++++++++++++++++++++++--------
> 2 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index cb23206..2ea27da 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -99,7 +99,7 @@ config X86
> select IRQ_FORCED_THREADING
> select HAVE_BPF_JIT if X86_64
> select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> - select HAVE_ARCH_HUGE_VMAP if X86_64 || (X86_32 && X86_PAE)
> + select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE
> select ARCH_HAS_SG_CHAIN
> select CLKEVT_I8253
> select ARCH_HAVE_NMI_SAFE_CMPXCHG
This is an unrelated change, please carve it out in a separate patch.
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 0b97d2c..4891fa1 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -563,14 +563,19 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
> }
>
> #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> +/**
> + * pud_set_huge - setup kernel PUD mapping
> + *
> + * MTRR can override PAT memory types with 4KB granularity. Therefore,
> + * it does not set up a huge page when the range is covered by a non-WB
"it" is what exactly?
> + * type of MTRR. 0xFF indicates that MTRR are disabled.
So this shows that this patch shouldn't be the first one in the series.
IMO you want to start with cleaning up mtrr_type_lookup(), add the
defines for its retval and *then* document its users. This way you won't
have to touch the same place twice, the net-size of your patchset will
go down and it will be easier for reviewiers.
> + *
> + * Return 1 on success, and 0 when no PUD was set.
"Returns 1 on success and 0 on failure."
> + */
> int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> {
> u8 mtrr;
>
> - /*
> - * Do not use a huge page when the range is covered by non-WB type
> - * of MTRRs.
> - */
> mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
> if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
> return 0;
Ditto for the rest.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
On Tue, 2015-05-05 at 13:19 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:35PM -0600, Toshi Kani wrote:
> > Document the return values of KVA mapping functions,
>
> KVA?
> Please write it out.
Will expand it as Kernel Virtual Address.
> > pud_set_huge(), pmd_set_huge, pud_clear_huge() and
> > pmd_clear_huge().
> >
> > Simplify the conditions to select HAVE_ARCH_HUGE_VMAP
> > in the Kconfig, since X86_PAE depends on X86_32.
> >
> > There is no functional change in this patch.
> >
> > Signed-off-by: Toshi Kani <[email protected]>
> > ---
> > arch/x86/Kconfig | 2 +-
> > arch/x86/mm/pgtable.c | 36 ++++++++++++++++++++++++++++--------
> > 2 files changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index cb23206..2ea27da 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -99,7 +99,7 @@ config X86
> > select IRQ_FORCED_THREADING
> > select HAVE_BPF_JIT if X86_64
> > select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > - select HAVE_ARCH_HUGE_VMAP if X86_64 || (X86_32 && X86_PAE)
> > + select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE
> > select ARCH_HAS_SG_CHAIN
> > select CLKEVT_I8253
> > select ARCH_HAVE_NMI_SAFE_CMPXCHG
>
> This is an unrelated change, please carve it out in a separate patch.
Will do.
> > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> > index 0b97d2c..4891fa1 100644
> > --- a/arch/x86/mm/pgtable.c
> > +++ b/arch/x86/mm/pgtable.c
> > @@ -563,14 +563,19 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
> > }
> >
> > #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> > +/**
> > + * pud_set_huge - setup kernel PUD mapping
> > + *
> > + * MTRR can override PAT memory types with 4KB granularity. Therefore,
> > + * it does not set up a huge page when the range is covered by a non-WB
>
> "it" is what exactly?
Will change to "this function".
> > + * type of MTRR. 0xFF indicates that MTRR are disabled.
>
> So this shows that this patch shouldn't be the first one in the series.
>
> IMO you want to start with cleaning up mtrr_type_lookup(), add the
> defines for its retval and *then* document its users. This way you won't
> have to touch the same place twice, the net-size of your patchset will
> go down and it will be easier for reviewiers.
Agreed. This patch-set was originally a small set of patches, but was
extended later with additional patches, which ended up with touching the
same place again. I will reorganize the patch-set.
> > + *
> > + * Return 1 on success, and 0 when no PUD was set.
>
> "Returns 1 on success and 0 on failure."
Will do.
> > + */
> > int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> > {
> > u8 mtrr;
> >
> > - /*
> > - * Do not use a huge page when the range is covered by non-WB type
> > - * of MTRRs.
> > - */
> > mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
> > if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
> > return 0;
>
> Ditto for the rest.
Will do.
Thanks,
-Toshi
On Tue, 2015-05-05 at 16:19 +0200, Borislav Petkov wrote:
> On Tue, May 05, 2015 at 07:46:36AM -0600, Toshi Kani wrote:
> > Agreed. This patch-set was originally a small set of patches, but was
> > extended later with additional patches, which ended up with touching the
> > same place again. I will reorganize the patch-set.
>
> Ok, but please wait until I take a look at the rest.
Sure, I will wait for your review.
>
> Thanks.
>
> Btw, is there anything else MTRR-related pending for tip?
Not exactly MTRR-related, but I am planing to re-submit my WT patchset
after checking to see if Luis's patchset (which you are reviewing) has
any conflict with this.
https://lkml.org/lkml/2015/2/24/773
Thanks,
-Toshi
On Tue, May 05, 2015 at 07:46:36AM -0600, Toshi Kani wrote:
> Agreed. This patch-set was originally a small set of patches, but was
> extended later with additional patches, which ended up with touching the
> same place again. I will reorganize the patch-set.
Ok, but please wait until I take a look at the rest.
Thanks.
Btw, is there anything else MTRR-related pending for tip?
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--