Hi Reinette,
> -Original Message-
> From: Reinette Chatre
> Sent: Thursday, May 21, 2020 9:12 AM
> To: Prakhya, Sai Praneeth ;
> sh...@kernel.org; sk...@linuxfoundation.org; linux-kselft...@vger.kernel.org
> Cc: t...@linutronix.de; mi...@redhat.com; b...@alien8.de; Luck
> >> Without patch:
> >> --
> >> [ 204.836425] mm/pgtable-generic.c:29: bad p4d
> >> 89eb4e92(80025f941467)
> >> [ 204.836544] BUG: Bad rss-counter state mm:f75895ea idx:0
> >> val:2 [ 204.836615] BUG: Bad rss-counter state mm:f75895ea
> >> idx:1 val:5 [
> > With patch:
> > ---
> > [ 69.815453] mm/pgtable-generic.c:29: bad p4d
> 84653642(80025ca37467)
> > [ 69.815872] BUG: Bad rss-counter state mm:014a6c03
> type:MM_FILEPAGES val:2
> > [ 69.815962] BUG: Bad rss-counter state mm:014a6c03
> type:MM_ANONPAGES
> >
> > Cc: Ingo Molnar
> > Cc: Peter Zijlstra
> > Cc: Andrew Morton
> > Suggested-by/Acked-by: Dave Hansen
>
> Though I am not sure, should the above be two separate lines instead ?
Sure! Will split them in V2.
>
> > Signed-off-by: Sai Praneeth Prakhya
> > ---
> > include/linux/mm_types_
> > > > +static const char * const resident_page_types[NR_MM_COUNTERS] = {
> > > > + "MM_FILEPAGES",
> > > > + "MM_ANONPAGES",
> > > > + "MM_SWAPENTS",
> > > > + "MM_SHMEMPAGES",
> > > > +};
> > >
> > > But please let's not put this in a header file. We're asking the
> > >
> > > efi_map_region() creates VA mappings for an given EFI region using
> > > any one of the two helper functions (namely __map_region() and
> old_map_region()).
> > > These helper functions *could* fail while creating mappings and
> > > presently their return value is not checked. Not checking fo
> > Acked-by: Ard Biesheuvel
> >
> > with the sidenote that I won't be able to test this myself until
> > monday at the earliest.
>
> OK, so I have tested both efi=old_map and mixed mode before and after
> applying this patch, using QEMU/KVM with 64-bit and 32-bit builds of OVMF
> [respectively]
>
> * Sai Praneeth Prakhya wrote:
>
> > Commit d5052a7130a6 ("x86/efi: Unmap EFI boot services code/data
> > regions from efi_pgd") forgets to take two EFI modes into
> > consideration namely EFI_OLD_MEMMAP and EFI_MIXED_MODE.
>
> So the commit sha1 ended up being this one in tip:efi/core:
>
>
> > > For the short term, could we simply make your changes dependent on
> > > efi != old_map? That gives us some time to fix the old_map case properly.
> >
> > Yes, I think we could and it should work but I hesitated to propose it
> > because (as you already noted) it's a short term fix and not th
> > > > Hi Thomas and Ingo,
> > > >
> > > > I recently noticed that the below commits [1] and [2] are broken
> > > > when kernel command line argument "efi=old_map" is passed. Sorry!
> > > > I missed to test this condition prior to sending these patches to
> > > > mailing list.
> > > > I am workin
> > Hi Thomas and Ingo,
> >
> > I recently noticed that the below commits [1] and [2] are broken when
> > kernel command line argument "efi=old_map" is passed. Sorry! I missed
> > to test this condition prior to sending these patches to mailing list.
> > I am working on a fix and will send it to ma
> Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80
> Gitweb:
> https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80
> Author: Sai Praneeth Prakhya
> AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100
> Committer: Ingo Molnar
> CommitDate: Fri, 30 Nov 2018 09:10:30 +0100
>
> x86/
> > +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
> > For (Intel) Memory Bandwidth features like MBA and MBM, perf iMC
> > (Integrated Memory Controller) is used for validation. As a part of
> > test, we run a benchmark (eg: fill_buf) and get memory bandwidth
> > values from MBM and iMC and verify if the difference between both the
> reported valu
> > No, the selftest in this patch set will not replace intel-cmt-cat or
> > vice versa.
> >
> > The selftest in this patch set has a different purpose from intel-cmt-cat:
> > the selftest is a test tool which validates resctrl functionalities
> > while intel-cmt-cat is mainly a utility that provid
> On Mon, Sep 03, 2018 at 05:03:56AM +0000, Prakhya, Sai Praneeth wrote:
> > Hmm.. thought that __efi_init might be confusing with the normal
> > __init attribute
>
> How would that be confusing? It has "__efi" prepended?!
>
> All I'm saying is, i
> > Yes, that makes sense.
> > But on the machine, I see IBRS bit set on all cores. As you said,
> > someone else might be writing the MSR. I will try to find that out and will
> update the patch accordingly.
> >
> > I initially suspected it to be __ssb_select_mitigation() as I have
> > "spec_store
> The feature strings are automatically generated from the define. The comment
> can be used to supress them by an empty "" string or to modify them by a
> "override" string at the beginning of the comment.
I overlooked "override" part. Sorry! about that.
It's clear now. Thanks for the explanation
> There is no reason not to use indentation and quotation marks in a changelog.
> Squeezing it into square brackets does not really improve readability.
>
> From the specification [1]:
>
> "With enhanced IBRS, the predicted targets of indirect branches executed
>cannot be controlled by sof
> > >> From: Sai Praneeth Some future
> > >> Intel processors may support "Enhanced IBRS" which is an "always
> > >> on" mode i.e. IBRS bit in SPEC_CTRL MSR is enabled once and never
> > >> disabled. According to specification[1], this should simplify
> > >> software enabling and improve performan
> > diff --git a/arch/x86/platform/efi/quirks.c
> > b/arch/x86/platform/efi/quirks.c index 36c1f8b9f7e0..6af39dc40325
> > 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -105,12 +105,11 @@ early_param("efi_no_storage_paranoia",
> > setup_storage_paran
> > Another follow on question is, does every firmware support both
> > blocking and non-blocking variants (specially legacy EFI firmware)? I
> > am worried about this because, presently efi_delete_dummy_variable()
> > uses set_variable() and
> > query_variable_info() but if I change efi_delete_dum
> > One more question again, if we are sure that non-blocking variants
> > will _always_ be called in atomic context, then, we got it covered.
> > Because, in
> > set_variable() and query_variable_info() (both blocking and
> > non-blocking) we check for in_atomic() and if so, we don't use efi_rts_w
> > Assume some user requested to execute some non-blocking variant of
> > efi_rts and the kernel hasn't called efi_call_virt() yet, but was
> > scheduled out. IOW, even though user requests for non-blocking efi call, we
> might still block. Am I right?
> >
>
> No, that is the whole point. These f
> > Changes from V3 to V4:
> > --
> > 1. As suggested by Peter, use completions instead of flush_work() as the
> > former is cheaper
> > 2. Call efi_delete_dummy_variable() from efisubsys_init(). Sorry! Ard,
> > wasn't able to find a better alternative to keep this change lo
> On Mon, May 21, 2018 at 08:13:03PM -0700, Sai Praneeth Prakhya wrote:
> > + /* \
> > +* queue_work() returns 0 if work was already on queue, \
> > +* _ideally_ this should never happen. \
> > +
> > That's true! AFAIK, we don't have any issues handling NMI while in efi_pgd.
> > We might have issues only when, we are already in efi_pgd, NMI comes
> > along
>
> Can you trigger this? Or is it something hypothetical?
>
AFAIK, it's hypothetical. I did try to trigger the issue, but failed [1]
> > diff --git a/include/linux/efi.h b/include/linux/efi.h index
> > f5083aa72eae..f1b7d68ac460 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -966,6 +966,8 @@ extern struct efi {
> > unsigned long flags;
> > } efi;
> >
> > +extern struct mm_struct efi_mm;
> > +
> >
> > Another warning by checkpatch is "use of in_atomic() in drivers code"
>
> I'm assuming it warns because you're touching files in drivers/ but the efi
> fun is
> not really a driver...
True! That makes sense :)
>
> But looking at patch 3, that thing looks like a real mess. Some of the thing
/*
> >> > +* Since we process only one efi_runtime_service() at a time, an
> >> > +* ordered workqueue (which creates only one execution context)
> >> > +* should suffice all our needs.
> >> > +*/
> >> > + efi_rts_wq = alloc_ordered_workqueue("efi_rts_workqueue
> > void __init efi_enter_virtual_mode(void) diff --git
> > a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index
> > cd42f66a7c85..838b8efe639c 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -328,6 +328,12 @@ static int __init efisubsys_init(void
+Cc Miguel Ojeda
> > > +({
> > > \
> > > + struct efi_runtime_work efi_rts_work; \
> > > + \
> > > + INIT_WORK_ONSTACK(&efi_rts_work.work
> > +({ \
> > + struct efi_runtime_work efi_rts_work; \
> > + \
> > + INIT_WORK_ONSTACK(&efi_rts_work.work, efi_call_rts);\
> >
> >> > pstore writes could potentially be invoked in interrupt context and
> >> > it uses set_variable<>() and query_variable_info<>() to store logs.
> >> > If we invoke efi_runtime_services() through efi_rts_wq while in
> >> > atomic() kernel issues a warning ("scheduling wile in atomic") and
> >>
> > +struct workqueue_struct *efi_rts_wq;
> > +
> > static bool disable_runtime;
> > static int __init setup_noefi(char *arg) { @@ -329,6 +331,19 @@
> > static int __init efisubsys_init(void)
> > return 0;
> >
> > /*
> > +* Since we process only one efi_runtime_se
05, 2018 at 03:23:10PM -0800, Sai Praneeth Prakhya wrote:
> > From: Sai Praneeth
> >
> > Presently, efi_runtime_services() are executed by firmware in process
> > context. To execute efi_runtime_service(), kernel switches the page
> > directory from swapper_pgd to efi_pgd. However, efi_pgd doesn't
-0800, Sai Praneeth Prakhya wrote:
> > @@ -329,6 +331,19 @@ static int __init efisubsys_init(void)
> > return 0;
> >
> > /*
> > +* Since we process only one efi_runtime_service() at a time, an
> > +* ordered workqueue (which creates only one execution context)
> > +* sho
> > Presently, efi_runtime_services() are executed by firmware in process
> > context. To execute efi_runtime_service(), kernel switches the page
> > directory from swapper_pgd to efi_pgd. However, efi_pgd doesn't have
> > any user space mappings. A potential issue could be, for instance, an
> > NM
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index ac5db5f8dbbf..4714b305ca90 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -76,6 +76,8 @@ static unsigned long *efi_tables[] = {
> > &efi.mem_attr_table,
> > };
> >
>
> > Changes in V3:
> > 1. When CPUMASK_OFFSTACK is enabled, switch_mm_irqs_off() sets cpumask
> > by calling cpumask_set_cpu(). This panics kernel as efi_mm is not
> > initialized, therefore initialize efi_mm in efi_alloc_page_tables().
>
> Thanks for the v3.
>
> I confirmed that the issue I saw
> So, in summary it seems that the primary kernel boot _fails_ with your
> v2 patchset on the real hardware for me irrespective of whether I use Matt's
> tree or Linus's tree:
>
> a) I would suggest that you perform some more checks on real hardware as
> qemu boot tests sometimes do not expose the
> -Original Message-
> From: Sai Praneeth Prakhya [mailto:sai.praneeth.prak...@intel.com]
> Sent: Tuesday, September 5, 2017 7:43 PM
> To: Bhupesh Sharma
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Matt Fleming
> ; Ard Biesheuvel ;
> j...@suse.com; Borislav Petkov ; Lu
> >
> > Thanks for this v2.
> > Introducing the 'efi_switch_mm() ' helper instead of manually
> > twiddling with %cr3 seems more cleaner.
> >
> > I have tested this patchset on a x86_64 machine with the following
> > configurations:
> >
> > 1. Primary kernel boot with efi=old_map 2. Primary kernel
>>On Thu, Dec 10, 2015 at 10:27:01AM -0800, Sai Praneeth Prakhya wrote:
>> From: Sai Praneeth
>>
>> Starting with this commit 35eb8b81edd4 ("x86/efi: Build our own page
>> table structures") efi regions have a separate page directory called
>> "efi_pgd". In order to access any efi region we ha
44 matches
Mail list logo
