Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1732080 > unrolled thread
| Started by | Baoquan He <bhe@redhat.com> |
|---|---|
| First post | 2017-09-14 09:40 +0200 |
| Last post | 2017-09-15 03:00 +0200 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to check UV system at earlier stage Baoquan He <bhe@redhat.com> - 2017-09-14 09:40 +0200
Re: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to check UV system at earlier stage Dave Young <dyoung@redhat.com> - 2017-09-14 09:50 +0200
Re: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to check UV system at earlier stage Baoquan He <bhe@redhat.com> - 2017-09-14 10:10 +0200
Re: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to check UV system at earlier stage Dave Young <dyoung@redhat.com> - 2017-09-15 02:50 +0200
Re: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to check UV system at earlier stage Dave Young <dyoung@redhat.com> - 2017-09-15 03:00 +0200
| From | Baoquan He <bhe@redhat.com> |
|---|---|
| Date | 2017-09-14 09:40 +0200 |
| Subject | Re: [PATCH v2 RESEND 1/2] x86/UV: Introduce a helper function to check UV system at earlier stage |
| Message-ID | <upwUG-25C-23@gated-at.bofh.it> |
Add Dave to the CC list, he may have concerns about the code change.
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);
> +}
>
> #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
>
[toc] | [next] | [standalone]
| From | Dave Young <dyoung@redhat.com> |
|---|---|
| Date | 2017-09-14 09:50 +0200 |
| Message-ID | <upx4n-293-9@gated-at.bofh.it> |
| In reply to | #1732080 |
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
[toc] | [prev] | [next] | [standalone]
| From | Baoquan He <bhe@redhat.com> |
|---|---|
| Date | 2017-09-14 10:10 +0200 |
| Message-ID | <upxnH-2uK-1@gated-at.bofh.it> |
| In reply to | #1732084 |
On 09/14/17 at 03:49pm, Dave Young wrote:
> > > 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);
> > > +}
>
Thanks for looking into this, Dave!
>
> 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?
Hmm, in uv_bios_init() it also checks the !efi.uv_systab case. And
EFI_INVALID_TABLE_ADDR checking is earlier, it won't affect the result
if it's EFI_INVALID_TABLE_ADDR. And !efi.uv_systab can make it safer
since it doesn't work either if efi.uv_systab is 0. Mainly it's not
harmful.
Mike, what's your thought? Should I only check the (efi.uv_systab ==
EFI_INVALID_TABLE_ADDR) case?
>
> 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.
At the beginning I put it in efi.c, later Mike suggested putting it in
asm/uv/uv.h. You can also find the discussion in below link.
https://patchwork.kernel.org/patch/9732787/
Thanks
Baoquan
>
> 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
[toc] | [prev] | [next] | [standalone]
| From | Dave Young <dyoung@redhat.com> |
|---|---|
| Date | 2017-09-15 02:50 +0200 |
| Message-ID | <upMZr-3Or-3@gated-at.bofh.it> |
| In reply to | #1732096 |
Cc linux-efi list
On 09/14/17 at 04:08pm, Baoquan He wrote:
> On 09/14/17 at 03:49pm, Dave Young wrote:
> > > > 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);
> > > > +}
> >
>
> Thanks for looking into this, Dave!
>
> >
> > 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?
>
> Hmm, in uv_bios_init() it also checks the !efi.uv_systab case. And
> EFI_INVALID_TABLE_ADDR checking is earlier, it won't affect the result
> if it's EFI_INVALID_TABLE_ADDR. And !efi.uv_systab can make it safer
> since it doesn't work either if efi.uv_systab is 0. Mainly it's not
> harmful.
>
> Mike, what's your thought? Should I only check the (efi.uv_systab ==
> EFI_INVALID_TABLE_ADDR) case?
>
> >
> > 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.
>
> At the beginning I put it in efi.c, later Mike suggested putting it in
> asm/uv/uv.h. You can also find the discussion in below link.
> https://patchwork.kernel.org/patch/9732787/
>
> Thanks
> Baoquan
>
> >
> > 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
[toc] | [prev] | [next] | [standalone]
| From | Dave Young <dyoung@redhat.com> |
|---|---|
| Date | 2017-09-15 03:00 +0200 |
| Message-ID | <upN97-3RG-1@gated-at.bofh.it> |
| In reply to | #1732096 |
Missed a comma in cc list in last reply, readd linux-efi list in cc.
On 09/14/17 at 04:08pm, Baoquan He wrote:
> On 09/14/17 at 03:49pm, Dave Young wrote:
> > > > 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);
> > > > +}
> >
>
> Thanks for looking into this, Dave!
>
> >
> > 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?
>
> Hmm, in uv_bios_init() it also checks the !efi.uv_systab case. And
> EFI_INVALID_TABLE_ADDR checking is earlier, it won't affect the result
> if it's EFI_INVALID_TABLE_ADDR. And !efi.uv_systab can make it safer
> since it doesn't work either if efi.uv_systab is 0. Mainly it's not
> harmful.
>
> Mike, what's your thought? Should I only check the (efi.uv_systab ==
> EFI_INVALID_TABLE_ADDR) case?
>
> >
> > 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.
>
> At the beginning I put it in efi.c, later Mike suggested putting it in
> asm/uv/uv.h. You can also find the discussion in below link.
> https://patchwork.kernel.org/patch/9732787/
>
> Thanks
> Baoquan
>
> >
> > 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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web