RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> > /* > > * The testcases use internal knowledge of the implementation that > > shouldn't > > * be exposed to the rest of the kernel. Include these directly here. > > diff --git a/arch/x86/platform/efi/quirks.c > > b/arch/x86/platform/efi/quirks.c index 669babcaf245..fb1c44b11235 > > 100644 > > --- a/arch/x86/platform/efi/quirks.c > > +++ b/arch/x86/platform/efi/quirks.c > > While you are at it, can you please split the EFI part out into a separate > patch? Sure! I will do it in V3. Regards, Sai
RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> > In general it's good to do on list because others can learn from the > > answers as well. > > So you somehow managed to keep everyone and the lists in CC nevertheless. > Did not pay attention to the CC list until I got my reply on the list :) Sorry! my bad.. yes, the conversation did happen on the list and it's by accident. > > > > > > > + retval = __change_page_attr_set_clr(&cpa, 0); [] > > > So some questions I have are, > > > 1. What did Peter Z mean here? "How is that not a TLB invalidation bug ?" > > > > __flush_tlb_all() flushes only the mapping on the current CPU. So in a > > SMP environment it's not sufficient. Thanks for the explanation. The issue makes sense to me now. [.] > > I'll have a second look at the whole thing and reply on list. > > So actually for the problem at hand __flush_tlb_all() works, because that is > called way before the secondary CPUs are up. > > Ditto for kernel_map_pages_in_pgd(). Yes, you are right. Both the functions are *always* called before SMP initialization and as you rightly pointed out they should be marked with __init. > But both functions want to be marked > __init and aside of that they both should contain a warning when they are > called > after the secondary CPUs have been brought up. Makes sense too.. I will include these changes in V3. Regards, Sai
RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai, On Mon, 29 Oct 2018, Thomas Gleixner wrote: > On Mon, 29 Oct 2018, Prakhya, Sai Praneeth wrote: > > > Hi Thomas and Peter, > > > > [off the mailing list because I wasn't sure if it's a good idea to spam > > others with my questions] > > In general it's good to do on list because others can learn from the > answers as well. So you somehow managed to keep everyone and the lists in CC nevertheless. Did not pay attention to the CC list until I got my reply on the list :) > > > > > + retval = __change_page_attr_set_clr(&cpa, 0); > > > > > + __flush_tlb_all(); > > > > > > > > So this looks like you copied it from kernel_map_pages_in_pgd() which > > > > has been discussed before to be not sufficient, but it can't be > > > > changed right now due to locking issues. > > > > > > Managed to confuse myself. The place which cannot be changed is a > > > different > > > one, but still for your call site __flush_tlb_all() might not be > > > sufficient. > > > > Since the mappings are changed, I thought a flush_tlb() might be needed. > > But, (to be honest), I don't completely understand the x86/mm code. So, > > could you > > please elaborate the issue more for my better understanding? > > > > So some questions I have are, > > 1. What did Peter Z mean here? "How is that not a TLB invalidation bug ?" > > __flush_tlb_all() flushes only the mapping on the current CPU. So in a SMP > environment it's not sufficient. > > > 2. I assumed kernel_map_pages_in_pgd() to be a good code but you said that > > it has some locking issues. So, could you please elaborate more on that. > > I corrected myself. It's the other function which cannot use the proper > cpa_flush.*() functions. > > > Or, could you please provide me some pointers in the source code that I > > can take a look at so that I could understand the issue much better. > > I'll have a second look at the whole thing and reply on list. So actually for the problem at hand __flush_tlb_all() works, because that is called way before the secondary CPUs are up. Ditto for kernel_map_pages_in_pgd(). But both functions want to be marked __init and aside of that they both should contain a warning when they are called after the secondary CPUs have been brought up. Thanks, tglx
RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai, On Mon, 29 Oct 2018, Prakhya, Sai Praneeth wrote: > Hi Thomas and Peter, > > [off the mailing list because I wasn't sure if it's a good idea to spam > others with my questions] In general it's good to do on list because others can learn from the answers as well. > > > > + retval = __change_page_attr_set_clr(&cpa, 0); > > > > + __flush_tlb_all(); > > > > > > So this looks like you copied it from kernel_map_pages_in_pgd() which > > > has been discussed before to be not sufficient, but it can't be > > > changed right now due to locking issues. > > > > Managed to confuse myself. The place which cannot be changed is a different > > one, but still for your call site __flush_tlb_all() might not be sufficient. > > Since the mappings are changed, I thought a flush_tlb() might be needed. > But, (to be honest), I don't completely understand the x86/mm code. So, could > you > please elaborate the issue more for my better understanding? > > So some questions I have are, > 1. What did Peter Z mean here? "How is that not a TLB invalidation bug ?" __flush_tlb_all() flushes only the mapping on the current CPU. So in a SMP environment it's not sufficient. > 2. I assumed kernel_map_pages_in_pgd() to be a good code but you said that > it has some locking issues. So, could you please elaborate more on that. I corrected myself. It's the other function which cannot use the proper cpa_flush.*() functions. > Or, could you please provide me some pointers in the source code that I > can take a look at so that I could understand the issue much better. I'll have a second look at the whole thing and reply on list. Thanks, tglx
RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Hi Thomas and Peter,
[off the mailing list because I wasn't sure if it's a good idea to spam others
with my questions]
> > > + /*
> > > + * The typical sequence for unmapping is to find a pte through
> > > + * lookup_address_in_pgd() (ideally, it should never return NULL
> because
> > > + * the address is already mapped) and change it's protections.
> > > + * As pfn is the *target* of a mapping, it's not useful while unmapping.
> > > + */
> > > + struct cpa_data cpa = {
> > > + .vaddr = &address,
> > > + .pgd= pgd,
> > > + .numpages = numpages,
> > > + .mask_set = __pgprot(0),
> > > + .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> > > + .flags = 0,
> > > + };
> > > +
> > > + retval = __change_page_attr_set_clr(&cpa, 0);
> > > + __flush_tlb_all();
> >
> > So this looks like you copied it from kernel_map_pages_in_pgd() which
> > has been discussed before to be not sufficient, but it can't be
> > changed right now due to locking issues.
>
> Managed to confuse myself. The place which cannot be changed is a different
> one, but still for your call site __flush_tlb_all() might not be sufficient.
Since the mappings are changed, I thought a flush_tlb() might be needed.
But, (to be honest), I don't completely understand the x86/mm code. So, could
you
please elaborate the issue more for my better understanding?
So some questions I have are,
1. What did Peter Z mean here? "How is that not a TLB invalidation bug ?"
2. I assumed kernel_map_pages_in_pgd() to be a good code but you said that
it has some locking issues. So, could you please elaborate more on that.
Or, could you please provide me some pointers in the source code that I can
take a look at so that I could understand the issue much better.
Regards,
Sai
Re: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai,
On Mon, 29 Oct 2018, Thomas Gleixner wrote:
> On Fri, 26 Oct 2018, Sai Praneeth Prakhya wrote:
> >
> > +int kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
> > + unsigned long numpages)
> > +{
> > + int retval;
> > +
> > + /*
> > +* The typical sequence for unmapping is to find a pte through
> > +* lookup_address_in_pgd() (ideally, it should never return NULL because
> > +* the address is already mapped) and change it's protections.
> > +* As pfn is the *target* of a mapping, it's not useful while unmapping.
> > +*/
> > + struct cpa_data cpa = {
> > + .vaddr = &address,
> > + .pgd= pgd,
> > + .numpages = numpages,
> > + .mask_set = __pgprot(0),
> > + .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> > + .flags = 0,
> > + };
> > +
> > + retval = __change_page_attr_set_clr(&cpa, 0);
> > + __flush_tlb_all();
>
> So this looks like you copied it from kernel_map_pages_in_pgd() which has
> been discussed before to be not sufficient, but it can't be changed right
> now due to locking issues.
Managed to confuse myself. The place which cannot be changed is a different
one, but still for your call site __flush_tlb_all() might not be sufficient.
Thanks,
tglx
Re: [PATCH V2 2/2] x86/efi: Move efi__boot_services() to arch/x86
On Fri, 26 Oct 2018, Sai Praneeth Prakhya wrote: > efi__boot_services() are x86 specific quirks and as such > should be in asm/efi.h, so move them from linux/efi.h. Also, call > efi_free_boot_services() from __efi_enter_virtual_mode() as it is x86 > specific call and ideally shouldn't be part of init/main.c > > Signed-off-by: Sai Praneeth Prakhya Acked-by: Thomas Gleixner
Re: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Sai,
On Fri, 26 Oct 2018, Sai Praneeth Prakhya wrote:
>
> +int kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
> + unsigned long numpages)
> +{
> + int retval;
> +
> + /*
> + * The typical sequence for unmapping is to find a pte through
> + * lookup_address_in_pgd() (ideally, it should never return NULL because
> + * the address is already mapped) and change it's protections.
> + * As pfn is the *target* of a mapping, it's not useful while unmapping.
> + */
> + struct cpa_data cpa = {
> + .vaddr = &address,
> + .pgd= pgd,
> + .numpages = numpages,
> + .mask_set = __pgprot(0),
> + .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> + .flags = 0,
> + };
> +
> + retval = __change_page_attr_set_clr(&cpa, 0);
> + __flush_tlb_all();
So this looks like you copied it from kernel_map_pages_in_pgd() which has
been discussed before to be not sufficient, but it can't be changed right
now due to locking issues.
For this particular use case, this should not be an issue at all, so please
use the proper cpa_flush_*() variant.
> +
> + return retval;
> +}
> +
> /*
> * The testcases use internal knowledge of the implementation that shouldn't
> * be exposed to the rest of the kernel. Include these directly here.
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 669babcaf245..fb1c44b11235 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
While you are at it, can you please split the EFI part out into a separate
patch?
Thanks,
tglx
Re: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Fri, Oct 26, 2018 at 02:38:44PM -0700, Sai Praneeth Prakhya wrote:
> +int kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
> + unsigned long numpages)
> +{
> + int retval;
> +
> + /*
> + * The typical sequence for unmapping is to find a pte through
> + * lookup_address_in_pgd() (ideally, it should never return NULL because
> + * the address is already mapped) and change it's protections.
> + * As pfn is the *target* of a mapping, it's not useful while unmapping.
> + */
> + struct cpa_data cpa = {
> + .vaddr = &address,
> + .pgd= pgd,
> + .numpages = numpages,
> + .mask_set = __pgprot(0),
> + .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> + .flags = 0,
> + };
> +
> + retval = __change_page_attr_set_clr(&cpa, 0);
> + __flush_tlb_all();
How is that not a TLB invalidation bug ?
> +
> + return retval;
> +}
Re: [PATCH 0/8] add generic builtin command line
On Mon, Oct 29, 2018 at 10:29:15AM +, Will Deacon wrote: > On Wed, Oct 24, 2018 at 10:07:32AM +0100, Russell King - ARM Linux wrote: > > On Wed, Oct 24, 2018 at 11:57:44AM +0300, Maksym Kokhan wrote: > > > Do you mean, that you haven't seen patch for ARM, which I sent on > > > September 27 along with cover and patch 1? It is strange, because > > > you was the one from recipients. If so, you can see this patch here: > > > https://lore.kernel.org/patchwork/patch/992779/ > > > > It seems that I have received patch 5, _but_ it's not threaded with > > the cover message and patch 1. With 50k messages in my inbox, and 3k > > messages since you sent the series, it's virtually impossible to find > > it (I only found it by looking at my mail server logs from September > > to find the subject, and then searching my mailbox for that subject.) > > > > This is unnecessarily difficult. > > This comes up surprisingly often, and I think part of the issue is that > different maintainers have different preferences. I also prefer to receive > the entire series and cover-letter, but I've seen people object to being > CC'd on the whole series as well (how they manage to review things in > isolation is another question...!) This series has the odd situation where patch 1 is threaded to the cover letter, but nothing else is - that makes it inconsistent. Where I've seen people disagree with threading is when sending follow-up series - whether that should be threaded to the previous series or not - some people want it others hate it. However, I haven't seen any disagreement is about having the patches threaded to the cover. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up According to speedtest.net: 11.9Mbps down 500kbps up
Re: [PATCH 0/8] add generic builtin command line
On Wed, Oct 24, 2018 at 10:07:32AM +0100, Russell King - ARM Linux wrote: > On Wed, Oct 24, 2018 at 11:57:44AM +0300, Maksym Kokhan wrote: > > Do you mean, that you haven't seen patch for ARM, which I sent on > > September 27 along with cover and patch 1? It is strange, because > > you was the one from recipients. If so, you can see this patch here: > > https://lore.kernel.org/patchwork/patch/992779/ > > It seems that I have received patch 5, _but_ it's not threaded with > the cover message and patch 1. With 50k messages in my inbox, and 3k > messages since you sent the series, it's virtually impossible to find > it (I only found it by looking at my mail server logs from September > to find the subject, and then searching my mailbox for that subject.) > > This is unnecessarily difficult. This comes up surprisingly often, and I think part of the issue is that different maintainers have different preferences. I also prefer to receive the entire series and cover-letter, but I've seen people object to being CC'd on the whole series as well (how they manage to review things in isolation is another question...!) I wonder if we could have an entry in MAINTAINERS for this sort of preference? Maksym: in the short term, please just stick me and Russell on CC for the entire thing. Will
[PATCH v2 0/2] efi/fdt: get_fdt cleanups
Hi, Here are just some minor cleanups of get_fdt function. Changes since v1: - Add a set of brace for the for loop - Reorganize code as suggested by Joe [1] https://lkml.org/lkml/2018/10/1/465 Thanks, Julien --> Julien Thierry (2): efi/fdt: Indentation fix efi/fdt: Simplify get_fdt flow drivers/firmware/efi/libstub/fdt.c | 30 -- 1 file changed, 16 insertions(+), 14 deletions(-) -- 1.9.1
[PATCH v2 2/2] efi/fdt: Simplify get_fdt flow
Reorganize get_fdt lookup loop, clearly showing that:
- Nothing is done for table entries that do not have fdt_guid
- Once an entry with fdt_guid is found, break out of the loop
No functional changes.
Signed-off-by: Julien Thierry
Suggested-by: Joe Perches
Cc: Ard Biesheuvel
---
drivers/firmware/efi/libstub/fdt.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/efi/libstub/fdt.c
b/drivers/firmware/efi/libstub/fdt.c
index 8b6696e..5b7e2b3 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -366,23 +366,24 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned
long *fdt_size)
{
efi_guid_t fdt_guid = DEVICE_TREE_GUID;
efi_config_table_t *tables;
- void *fdt;
int i;
- tables = (efi_config_table_t *) sys_table->tables;
- fdt = NULL;
+ tables = (efi_config_table_t *)sys_table->tables;
for (i = 0; i < sys_table->nr_tables; i++) {
- if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
- fdt = (void *) tables[i].table;
- if (fdt_check_header(fdt) != 0) {
- pr_efi_err(sys_table, "Invalid header detected
on UEFI supplied FDT, ignoring ...\n");
- return NULL;
- }
- *fdt_size = fdt_totalsize(fdt);
- break;
+ void *fdt;
+
+ if (efi_guidcmp(tables[i].guid, fdt_guid) != 0)
+ continue;
+
+ fdt = (void *) tables[i].table;
+ if (fdt_check_header(fdt) != 0) {
+ pr_efi_err(sys_table, "Invalid header detected on UEFI
supplied FDT, ignoring ...\n");
+ return NULL;
}
+ *fdt_size = fdt_totalsize(fdt);
+ return fdt;
}
- return fdt;
+ return NULL;
}
--
1.9.1
[PATCH v2 1/2] efi/fdt: Indentation fix
Closing bracket seems to end a for statement when it is actually ending
the contained if. Add some brackets to have clear delimitation of each
scope.
No functional change/fix, just fix the indentation.
Signed-off-by: Julien Thierry
Cc: Ard Biesheuvel
---
drivers/firmware/efi/libstub/fdt.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/fdt.c
b/drivers/firmware/efi/libstub/fdt.c
index 8830fa6..8b6696e 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -372,7 +372,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long
*fdt_size)
tables = (efi_config_table_t *) sys_table->tables;
fdt = NULL;
- for (i = 0; i < sys_table->nr_tables; i++)
+ for (i = 0; i < sys_table->nr_tables; i++) {
if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
fdt = (void *) tables[i].table;
if (fdt_check_header(fdt) != 0) {
@@ -381,7 +381,8 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long
*fdt_size)
}
*fdt_size = fdt_totalsize(fdt);
break;
-}
+ }
+ }
return fdt;
}
--
1.9.1
Re: [PATCH] efi/fdt: Indentation fix
Hi Joe,
On 02/10/18 17:05, Joe Perches wrote:
On Mon, 2018-10-01 at 11:29 +0100, Julien Thierry wrote:
Closing bracket seems to end a for statement when it is actually ending
the contained if.
No functional change/fix, just fix the indentation.
[]
diff --git a/drivers/firmware/efi/libstub/fdt.c
b/drivers/firmware/efi/libstub/fdt.c
[]
@@ -381,7 +381,7 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned long
*fdt_size)
}
*fdt_size = fdt_totalsize(fdt);
break;
-}
+ }
return fdt;
}
Hello Julian.
[trivial style bikeshedding follows]
I think it better to add braces to the for loop so
this stands out better.
Also, I would generally write this so the if test is
reversed and uses a continue so the match block saves
an indent level.
This is the only use efi_guidcmp with a == test in
drivers/firmware, so perhaps remove that and use ! as
the other uses have no == or != test.
Lastly, a direct return might be better and the return
fdt could be removed and converted to return NULL.
The suggestions make sense, I'll respin this taking your remarks into
account.
Thanks,
Something like:
---
drivers/firmware/efi/libstub/fdt.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/firmware/efi/libstub/fdt.c
b/drivers/firmware/efi/libstub/fdt.c
index 8830fa601e45..2d0b895e2f67 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -366,22 +366,23 @@ void *get_fdt(efi_system_table_t *sys_table, unsigned
long *fdt_size)
{
efi_guid_t fdt_guid = DEVICE_TREE_GUID;
efi_config_table_t *tables;
- void *fdt;
int i;
- tables = (efi_config_table_t *) sys_table->tables;
- fdt = NULL;
+ tables = (efi_config_table_t *)sys_table->tables;
- for (i = 0; i < sys_table->nr_tables; i++)
- if (efi_guidcmp(tables[i].guid, fdt_guid) == 0) {
- fdt = (void *) tables[i].table;
- if (fdt_check_header(fdt) != 0) {
- pr_efi_err(sys_table, "Invalid header detected on
UEFI supplied FDT, ignoring ...\n");
- return NULL;
- }
- *fdt_size = fdt_totalsize(fdt);
- break;
-}
+ for (i = 0; i < sys_table->nr_tables; i++) {
+ void *fdt;
+
+ if (efi_guidcmp(tables[i].guid, fdt_guid))
+ continue;
+ fdt = (void *)tables[i].table;
+ if (fdt_check_header(fdt) != 0) {
+ pr_efi_err(sys_table, "Invalid header detected on UEFI
supplied FDT, ignoring ...\n");
+ return NULL;
+ }
+ *fdt_size = fdt_totalsize(fdt);
+ return fdt;
+ }
- return fdt;
+ return NULL;
}
--
Julien Thierry
RE: [PATCH V2 1/2] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> > +int kernel_unmap_pages_in_pgd(pgd_t *pgd, unsigned long address,
> > + unsigned long numpages)
> > +{
> > + int retval;
> > +
> > + /*
> > +* The typical sequence for unmapping is to find a pte through
> > +* lookup_address_in_pgd() (ideally, it should never return NULL
> because
> > +* the address is already mapped) and change it's protections.
> > +* As pfn is the *target* of a mapping, it's not useful while unmapping.
> > +*/
> > + struct cpa_data cpa = {
> > + .vaddr = &address,
> > + .pgd= pgd,
> > + .numpages = numpages,
> > + .mask_set = __pgprot(0),
> > + .mask_clr = __pgprot(_PAGE_PRESENT | _PAGE_RW),
> > + .flags = 0,
> > + };
> > +
> > + retval = __change_page_attr_set_clr(&cpa, 0);
> > + __flush_tlb_all();
>
> So, just to clarify, 'pfn' is kept at 0 here? Might make sense to write it out
> explicitly like 'flags', even if it's not used by
> __change_page_attr_set_clr().
Sure! Makes sense. I will add it.
Regards,
Sai
