DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 997AE356D4
Date: Thu, 14 Sep 2017 15:49:01 +0800
From: Dave Young <dyoung@redhat.com>
To: Baoquan He <bhe@redhat.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, mingo@redhat.com,
        tglx@linutronix.de, hpa@zytor.com, thgarnie@google.com,
        keescook@chromium.org, akpm@linux-foundation.org,
        yamada.masahiro@socionext.com, rja@hpe.com, frank.ramsay@hpe.com
Subject: Re: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to
 check UV system at earlier stage
Message-ID: <20170914074901.GA5182@dhcp-128-65.nay.redhat.com>
References: <1504770150-25456-1-git-send-email-bhe@redhat.com>
 <1504770150-25456-2-git-send-email-bhe@redhat.com>
 <20170914072952.GN12824@x1>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170914072952.GN12824@x1>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2574
Lines: 72

On 09/14/17 at 03:29pm, Baoquan He wrote:
> Add Dave to the CC list, he may have concerns about the code change.

Baoquan, thanks for cc me

> 
> On 09/07/17 at 03:42pm, Baoquan He wrote:
> > The BIOS on SGI UV system will report a UV system table which describes
> > specific firmware capabilities available to the Linux kernel at runtime.
> > This UV system table only exists on SGI UV system. And it's detected
> > in efi_init() which is at very early stage.
> > 
> > So introduce a new helper function is_early_uv_system() to identify if
> > a system is UV system. Later we will use it to check if the running
> > system is UV system in mm KASLR code.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > Acked-by: Mike Travis <travis@sgi.com>
> > ---
> >  arch/x86/include/asm/uv/uv.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/uv/uv.h b/arch/x86/include/asm/uv/uv.h
> > index b5a32231abd8..93d7ad8763ba 100644
> > --- a/arch/x86/include/asm/uv/uv.h
> > +++ b/arch/x86/include/asm/uv/uv.h
> > @@ -18,6 +18,11 @@ extern void uv_nmi_init(void);
> >  extern void uv_system_init(void);
> >  extern const struct cpumask *uv_flush_tlb_others(const struct cpumask *cpumask,
> >  						 const struct flush_tlb_info *info);
> > +#include <linux/efi.h>
> > +static inline int is_early_uv_system(void)
> > +{
> > +	return !((efi.uv_systab == EFI_INVALID_TABLE_ADDR) || !efi.uv_systab);
> > +}


Sorry for jumping in late, I have two questions about the patch:

1) For efi tables, the only invalid value is EFI_INVALID_TABLE_ADDR, and
efi struct is initialized as EFI_INVALID_TABLE_ADDR by default so no
need to check "|| !efi.uv_systab". Do we have any UV firmware specific
assumption that "0" is also possible to be assigned?

2) It seems adding this function in uv.h for separating this for uv
system only purpose. But I feel it is better to put it in efi.h instead.

uv_systab is already a member of struct efi, it is in efi.h so it is
natural to check the table exist or not. Then just include efi.h in
kaslr.c and use the function.

something like drivers/firmware/efi/esrt.c: esrt_table_exists()

Anyway I have no strong opinon, it looks more natural to me though.

> >  
> >  #else	/* X86_UV */
> >  
> > @@ -30,6 +35,7 @@ static inline const struct cpumask *
> >  uv_flush_tlb_others(const struct cpumask *cpumask,
> >  		    const struct flush_tlb_info *info)
> >  { return cpumask; }
> > +static inline int is_early_uv_system(void)	{ return 0; }
> >  
> >  #endif	/* X86_UV */
> >  
> > -- 
> > 2.5.5
> > 

Thanks
Dave