* [PATCH 1/2] crypto: vmx - Remove overly verbose printk from AES init routines
From: Michael Ellerman @ 2018-05-03 12:29 UTC (permalink / raw)
To: herbert; +Cc: linux-crypto, linuxppc-dev, linux-kernel, mhcerri, pfsmorigo
In the vmx AES init routines we do a printk(KERN_INFO ...) to report
the fallback implementation we're using.
However with a slow console this can significantly affect the speed of
crypto operations. Using 'cryptsetup benchmark' the removal of the
printk() leads to a ~5x speedup for aes-cbc decryption.
So remove them.
Fixes: 8676590a1593 ("crypto: vmx - Adding AES routines for VMX module")
Fixes: 8c755ace357c ("crypto: vmx - Adding CBC routines for VMX module")
Fixes: 4f7f60d312b3 ("crypto: vmx - Adding CTR routines for VMX module")
Fixes: cc333cd68dfa ("crypto: vmx - Adding GHASH routines for VMX module")
Cc: stable@vger.kernel.org # v4.1+
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
drivers/crypto/vmx/aes.c | 2 --
drivers/crypto/vmx/aes_cbc.c | 3 ---
drivers/crypto/vmx/aes_ctr.c | 2 --
drivers/crypto/vmx/ghash.c | 2 --
4 files changed, 9 deletions(-)
If this is the wrong fix please let me know, I'm not a crypto expert.
What we see is 'cryptsetup benchmark' causing thousands of these printks() to
happen. The call trace is something like:
[c000001e47867a60] [c0000000009cf6b4] p8_aes_cbc_init+0x74/0xf0
[c000001e47867ae0] [c000000000551a80] __crypto_alloc_tfm+0x1d0/0x2c0
[c000001e47867b20] [c00000000055aea4] crypto_skcipher_init_tfm+0x124/0x280
[c000001e47867b60] [c00000000055138c] crypto_create_tfm+0x9c/0x1a0
[c000001e47867ba0] [c000000000552220] crypto_alloc_tfm+0xa0/0x140
[c000001e47867c00] [c00000000055b168] crypto_alloc_skcipher+0x48/0x70
[c000001e47867c40] [c00000000057af28] skcipher_bind+0x38/0x50
[c000001e47867c80] [c00000000057a07c] alg_bind+0xbc/0x220
[c000001e47867d10] [c000000000a016a0] __sys_bind+0x90/0x100
[c000001e47867df0] [c000000000a01750] sys_bind+0x40/0x60
[c000001e47867e30] [c00000000000b320] system_call+0x58/0x6c
Is it normal for init to be called on every system call?
cheers
diff --git a/drivers/crypto/vmx/aes.c b/drivers/crypto/vmx/aes.c
index 96072b9b55c4..d7316f7a3a69 100644
--- a/drivers/crypto/vmx/aes.c
+++ b/drivers/crypto/vmx/aes.c
@@ -48,8 +48,6 @@ static int p8_aes_init(struct crypto_tfm *tfm)
alg, PTR_ERR(fallback));
return PTR_ERR(fallback);
}
- printk(KERN_INFO "Using '%s' as fallback implementation.\n",
- crypto_tfm_alg_driver_name((struct crypto_tfm *) fallback));
crypto_cipher_set_flags(fallback,
crypto_cipher_get_flags((struct
diff --git a/drivers/crypto/vmx/aes_cbc.c b/drivers/crypto/vmx/aes_cbc.c
index 7394d35d5936..5285ece4f33a 100644
--- a/drivers/crypto/vmx/aes_cbc.c
+++ b/drivers/crypto/vmx/aes_cbc.c
@@ -52,9 +52,6 @@ static int p8_aes_cbc_init(struct crypto_tfm *tfm)
alg, PTR_ERR(fallback));
return PTR_ERR(fallback);
}
- printk(KERN_INFO "Using '%s' as fallback implementation.\n",
- crypto_skcipher_driver_name(fallback));
-
crypto_skcipher_set_flags(
fallback,
diff --git a/drivers/crypto/vmx/aes_ctr.c b/drivers/crypto/vmx/aes_ctr.c
index fc60d00a2e84..cd777c75291d 100644
--- a/drivers/crypto/vmx/aes_ctr.c
+++ b/drivers/crypto/vmx/aes_ctr.c
@@ -50,8 +50,6 @@ static int p8_aes_ctr_init(struct crypto_tfm *tfm)
alg, PTR_ERR(fallback));
return PTR_ERR(fallback);
}
- printk(KERN_INFO "Using '%s' as fallback implementation.\n",
- crypto_skcipher_driver_name(fallback));
crypto_skcipher_set_flags(
fallback,
diff --git a/drivers/crypto/vmx/ghash.c b/drivers/crypto/vmx/ghash.c
index 27a94a119009..1c4b5b889fba 100644
--- a/drivers/crypto/vmx/ghash.c
+++ b/drivers/crypto/vmx/ghash.c
@@ -64,8 +64,6 @@ static int p8_ghash_init_tfm(struct crypto_tfm *tfm)
alg, PTR_ERR(fallback));
return PTR_ERR(fallback);
}
- printk(KERN_INFO "Using '%s' as fallback implementation.\n",
- crypto_tfm_alg_driver_name(crypto_shash_tfm(fallback)));
crypto_shash_set_flags(fallback,
crypto_shash_get_flags((struct crypto_shash
--
2.14.1
^ permalink raw reply related
* Re: [PATCH v10 12/25] mm: cache some VMA fields in the vm_fault structure
From: Laurent Dufour @ 2018-05-03 12:25 UTC (permalink / raw)
To: Minchan Kim
Cc: akpm, mhocko, peterz, kirill, ak, dave, jack, Matthew Wilcox,
benh, mpe, paulus, Thomas Gleixner, Ingo Molnar, hpa, Will Deacon,
Sergey Senozhatsky, Andrea Arcangeli, Alexei Starovoitov,
kemi.wang, sergey.senozhatsky.work, Daniel Jordan, David Rientjes,
Jerome Glisse, Ganesh Mahendran, linux-kernel, linux-mm, haren,
khandual, npiggin, bsingharora, paulmck, Tim Chen, linuxppc-dev,
x86
In-Reply-To: <20180423074221.GE114098@rodete-desktop-imager.corp.google.com>
On 23/04/2018 09:42, Minchan Kim wrote:
> On Tue, Apr 17, 2018 at 04:33:18PM +0200, Laurent Dufour wrote:
>> When handling speculative page fault, the vma->vm_flags and
>> vma->vm_page_prot fields are read once the page table lock is released. So
>> there is no more guarantee that these fields would not change in our back.
>> They will be saved in the vm_fault structure before the VMA is checked for
>> changes.
>
> Sorry. I cannot understand.
> If it is changed under us, what happens? If it's critical, why cannot we
> check with seqcounter?
> Clearly, I'm not understanding the logic here. However, it's a global
> change without CONFIG_SPF so I want to be more careful.
> It would be better to describe why we need to sanpshot those values
> into vm_fault rather than preventing the race.
The idea is to go forward processing the page fault using the VMA's fields
values saved in the vm_fault structure. Then once the pte are locked, the
vma->sequence_counter is checked again and if something has changed in our back
the speculative page fault processing is aborted.
Thanks,
Laurent.
>
> Thanks.
>
>>
>> This patch also set the fields in hugetlb_no_page() and
>> __collapse_huge_page_swapin even if it is not need for the callee.
>>
>> Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
>> ---
>> include/linux/mm.h | 10 ++++++++--
>> mm/huge_memory.c | 6 +++---
>> mm/hugetlb.c | 2 ++
>> mm/khugepaged.c | 2 ++
>> mm/memory.c | 50 ++++++++++++++++++++++++++------------------------
>> mm/migrate.c | 2 +-
>> 6 files changed, 42 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index f6edd15563bc..c65205c8c558 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -367,6 +367,12 @@ struct vm_fault {
>> * page table to avoid allocation from
>> * atomic context.
>> */
>> + /*
>> + * These entries are required when handling speculative page fault.
>> + * This way the page handling is done using consistent field values.
>> + */
>> + unsigned long vma_flags;
>> + pgprot_t vma_page_prot;
>> };
>>
>> /* page entry size for vm->huge_fault() */
>> @@ -687,9 +693,9 @@ void free_compound_page(struct page *page);
>> * pte_mkwrite. But get_user_pages can cause write faults for mappings
>> * that do not have writing enabled, when used by access_process_vm.
>> */
>> -static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
>> +static inline pte_t maybe_mkwrite(pte_t pte, unsigned long vma_flags)
>> {
>> - if (likely(vma->vm_flags & VM_WRITE))
>> + if (likely(vma_flags & VM_WRITE))
>> pte = pte_mkwrite(pte);
>> return pte;
>> }
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index a3a1815f8e11..da2afda67e68 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1194,8 +1194,8 @@ static int do_huge_pmd_wp_page_fallback(struct vm_fault *vmf, pmd_t orig_pmd,
>>
>> for (i = 0; i < HPAGE_PMD_NR; i++, haddr += PAGE_SIZE) {
>> pte_t entry;
>> - entry = mk_pte(pages[i], vma->vm_page_prot);
>> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>> + entry = mk_pte(pages[i], vmf->vma_page_prot);
>> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
>> memcg = (void *)page_private(pages[i]);
>> set_page_private(pages[i], 0);
>> page_add_new_anon_rmap(pages[i], vmf->vma, haddr, false);
>> @@ -2168,7 +2168,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
>> entry = pte_swp_mksoft_dirty(entry);
>> } else {
>> entry = mk_pte(page + i, READ_ONCE(vma->vm_page_prot));
>> - entry = maybe_mkwrite(entry, vma);
>> + entry = maybe_mkwrite(entry, vma->vm_flags);
>> if (!write)
>> entry = pte_wrprotect(entry);
>> if (!young)
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 218679138255..774864153407 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -3718,6 +3718,8 @@ static int hugetlb_no_page(struct mm_struct *mm, struct vm_area_struct *vma,
>> .vma = vma,
>> .address = address,
>> .flags = flags,
>> + .vma_flags = vma->vm_flags,
>> + .vma_page_prot = vma->vm_page_prot,
>> /*
>> * Hard to debug if it ends up being
>> * used by a callee that assumes
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 0b28af4b950d..2b02a9f9589e 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -887,6 +887,8 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
>> .flags = FAULT_FLAG_ALLOW_RETRY,
>> .pmd = pmd,
>> .pgoff = linear_page_index(vma, address),
>> + .vma_flags = vma->vm_flags,
>> + .vma_page_prot = vma->vm_page_prot,
>> };
>>
>> /* we only decide to swapin, if there is enough young ptes */
>> diff --git a/mm/memory.c b/mm/memory.c
>> index f76f5027d251..2fb9920e06a5 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1826,7 +1826,7 @@ static int insert_pfn(struct vm_area_struct *vma, unsigned long addr,
>> out_mkwrite:
>> if (mkwrite) {
>> entry = pte_mkyoung(entry);
>> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>> + entry = maybe_mkwrite(pte_mkdirty(entry), vma->vm_flags);
>> }
>>
>> set_pte_at(mm, addr, pte, entry);
>> @@ -2472,7 +2472,7 @@ static inline void wp_page_reuse(struct vm_fault *vmf)
>>
>> flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte));
>> entry = pte_mkyoung(vmf->orig_pte);
>> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
>> if (ptep_set_access_flags(vma, vmf->address, vmf->pte, entry, 1))
>> update_mmu_cache(vma, vmf->address, vmf->pte);
>> pte_unmap_unlock(vmf->pte, vmf->ptl);
>> @@ -2548,8 +2548,8 @@ static int wp_page_copy(struct vm_fault *vmf)
>> inc_mm_counter_fast(mm, MM_ANONPAGES);
>> }
>> flush_cache_page(vma, vmf->address, pte_pfn(vmf->orig_pte));
>> - entry = mk_pte(new_page, vma->vm_page_prot);
>> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>> + entry = mk_pte(new_page, vmf->vma_page_prot);
>> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
>> /*
>> * Clear the pte entry and flush it first, before updating the
>> * pte with the new entry. This will avoid a race condition
>> @@ -2614,7 +2614,7 @@ static int wp_page_copy(struct vm_fault *vmf)
>> * Don't let another task, with possibly unlocked vma,
>> * keep the mlocked page.
>> */
>> - if (page_copied && (vma->vm_flags & VM_LOCKED)) {
>> + if (page_copied && (vmf->vma_flags & VM_LOCKED)) {
>> lock_page(old_page); /* LRU manipulation */
>> if (PageMlocked(old_page))
>> munlock_vma_page(old_page);
>> @@ -2650,7 +2650,7 @@ static int wp_page_copy(struct vm_fault *vmf)
>> */
>> int finish_mkwrite_fault(struct vm_fault *vmf)
>> {
>> - WARN_ON_ONCE(!(vmf->vma->vm_flags & VM_SHARED));
>> + WARN_ON_ONCE(!(vmf->vma_flags & VM_SHARED));
>> if (!pte_map_lock(vmf))
>> return VM_FAULT_RETRY;
>> /*
>> @@ -2752,7 +2752,7 @@ static int do_wp_page(struct vm_fault *vmf)
>> * We should not cow pages in a shared writeable mapping.
>> * Just mark the pages writable and/or call ops->pfn_mkwrite.
>> */
>> - if ((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
>> + if ((vmf->vma_flags & (VM_WRITE|VM_SHARED)) ==
>> (VM_WRITE|VM_SHARED))
>> return wp_pfn_shared(vmf);
>>
>> @@ -2799,7 +2799,7 @@ static int do_wp_page(struct vm_fault *vmf)
>> return VM_FAULT_WRITE;
>> }
>> unlock_page(vmf->page);
>> - } else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
>> + } else if (unlikely((vmf->vma_flags & (VM_WRITE|VM_SHARED)) ==
>> (VM_WRITE|VM_SHARED))) {
>> return wp_page_shared(vmf);
>> }
>> @@ -3078,9 +3078,9 @@ int do_swap_page(struct vm_fault *vmf)
>>
>> inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
>> dec_mm_counter_fast(vma->vm_mm, MM_SWAPENTS);
>> - pte = mk_pte(page, vma->vm_page_prot);
>> + pte = mk_pte(page, vmf->vma_page_prot);
>> if ((vmf->flags & FAULT_FLAG_WRITE) && reuse_swap_page(page, NULL)) {
>> - pte = maybe_mkwrite(pte_mkdirty(pte), vma);
>> + pte = maybe_mkwrite(pte_mkdirty(pte), vmf->vma_flags);
>> vmf->flags &= ~FAULT_FLAG_WRITE;
>> ret |= VM_FAULT_WRITE;
>> exclusive = RMAP_EXCLUSIVE;
>> @@ -3105,7 +3105,7 @@ int do_swap_page(struct vm_fault *vmf)
>>
>> swap_free(entry);
>> if (mem_cgroup_swap_full(page) ||
>> - (vma->vm_flags & VM_LOCKED) || PageMlocked(page))
>> + (vmf->vma_flags & VM_LOCKED) || PageMlocked(page))
>> try_to_free_swap(page);
>> unlock_page(page);
>> if (page != swapcache && swapcache) {
>> @@ -3163,7 +3163,7 @@ static int do_anonymous_page(struct vm_fault *vmf)
>> pte_t entry;
>>
>> /* File mapping without ->vm_ops ? */
>> - if (vma->vm_flags & VM_SHARED)
>> + if (vmf->vma_flags & VM_SHARED)
>> return VM_FAULT_SIGBUS;
>>
>> /*
>> @@ -3187,7 +3187,7 @@ static int do_anonymous_page(struct vm_fault *vmf)
>> if (!(vmf->flags & FAULT_FLAG_WRITE) &&
>> !mm_forbids_zeropage(vma->vm_mm)) {
>> entry = pte_mkspecial(pfn_pte(my_zero_pfn(vmf->address),
>> - vma->vm_page_prot));
>> + vmf->vma_page_prot));
>> if (!pte_map_lock(vmf))
>> return VM_FAULT_RETRY;
>> if (!pte_none(*vmf->pte))
>> @@ -3220,8 +3220,8 @@ static int do_anonymous_page(struct vm_fault *vmf)
>> */
>> __SetPageUptodate(page);
>>
>> - entry = mk_pte(page, vma->vm_page_prot);
>> - if (vma->vm_flags & VM_WRITE)
>> + entry = mk_pte(page, vmf->vma_page_prot);
>> + if (vmf->vma_flags & VM_WRITE)
>> entry = pte_mkwrite(pte_mkdirty(entry));
>>
>> if (!pte_map_lock(vmf)) {
>> @@ -3418,7 +3418,7 @@ static int do_set_pmd(struct vm_fault *vmf, struct page *page)
>> for (i = 0; i < HPAGE_PMD_NR; i++)
>> flush_icache_page(vma, page + i);
>>
>> - entry = mk_huge_pmd(page, vma->vm_page_prot);
>> + entry = mk_huge_pmd(page, vmf->vma_page_prot);
>> if (write)
>> entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
>>
>> @@ -3492,11 +3492,11 @@ int alloc_set_pte(struct vm_fault *vmf, struct mem_cgroup *memcg,
>> return VM_FAULT_NOPAGE;
>>
>> flush_icache_page(vma, page);
>> - entry = mk_pte(page, vma->vm_page_prot);
>> + entry = mk_pte(page, vmf->vma_page_prot);
>> if (write)
>> - entry = maybe_mkwrite(pte_mkdirty(entry), vma);
>> + entry = maybe_mkwrite(pte_mkdirty(entry), vmf->vma_flags);
>> /* copy-on-write page */
>> - if (write && !(vma->vm_flags & VM_SHARED)) {
>> + if (write && !(vmf->vma_flags & VM_SHARED)) {
>> inc_mm_counter_fast(vma->vm_mm, MM_ANONPAGES);
>> page_add_new_anon_rmap(page, vma, vmf->address, false);
>> mem_cgroup_commit_charge(page, memcg, false, false);
>> @@ -3535,7 +3535,7 @@ int finish_fault(struct vm_fault *vmf)
>>
>> /* Did we COW the page? */
>> if ((vmf->flags & FAULT_FLAG_WRITE) &&
>> - !(vmf->vma->vm_flags & VM_SHARED))
>> + !(vmf->vma_flags & VM_SHARED))
>> page = vmf->cow_page;
>> else
>> page = vmf->page;
>> @@ -3789,7 +3789,7 @@ static int do_fault(struct vm_fault *vmf)
>> ret = VM_FAULT_SIGBUS;
>> else if (!(vmf->flags & FAULT_FLAG_WRITE))
>> ret = do_read_fault(vmf);
>> - else if (!(vma->vm_flags & VM_SHARED))
>> + else if (!(vmf->vma_flags & VM_SHARED))
>> ret = do_cow_fault(vmf);
>> else
>> ret = do_shared_fault(vmf);
>> @@ -3846,7 +3846,7 @@ static int do_numa_page(struct vm_fault *vmf)
>> * accessible ptes, some can allow access by kernel mode.
>> */
>> pte = ptep_modify_prot_start(vma->vm_mm, vmf->address, vmf->pte);
>> - pte = pte_modify(pte, vma->vm_page_prot);
>> + pte = pte_modify(pte, vmf->vma_page_prot);
>> pte = pte_mkyoung(pte);
>> if (was_writable)
>> pte = pte_mkwrite(pte);
>> @@ -3880,7 +3880,7 @@ static int do_numa_page(struct vm_fault *vmf)
>> * Flag if the page is shared between multiple address spaces. This
>> * is later used when determining whether to group tasks together
>> */
>> - if (page_mapcount(page) > 1 && (vma->vm_flags & VM_SHARED))
>> + if (page_mapcount(page) > 1 && (vmf->vma_flags & VM_SHARED))
>> flags |= TNF_SHARED;
>>
>> last_cpupid = page_cpupid_last(page);
>> @@ -3925,7 +3925,7 @@ static inline int wp_huge_pmd(struct vm_fault *vmf, pmd_t orig_pmd)
>> return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PMD);
>>
>> /* COW handled on pte level: split pmd */
>> - VM_BUG_ON_VMA(vmf->vma->vm_flags & VM_SHARED, vmf->vma);
>> + VM_BUG_ON_VMA(vmf->vma_flags & VM_SHARED, vmf->vma);
>> __split_huge_pmd(vmf->vma, vmf->pmd, vmf->address, false, NULL);
>>
>> return VM_FAULT_FALLBACK;
>> @@ -4072,6 +4072,8 @@ static int __handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
>> .flags = flags,
>> .pgoff = linear_page_index(vma, address),
>> .gfp_mask = __get_fault_gfp_mask(vma),
>> + .vma_flags = vma->vm_flags,
>> + .vma_page_prot = vma->vm_page_prot,
>> };
>> unsigned int dirty = flags & FAULT_FLAG_WRITE;
>> struct mm_struct *mm = vma->vm_mm;
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index bb6367d70a3e..44d7007cfc1c 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -240,7 +240,7 @@ static bool remove_migration_pte(struct page *page, struct vm_area_struct *vma,
>> */
>> entry = pte_to_swp_entry(*pvmw.pte);
>> if (is_write_migration_entry(entry))
>> - pte = maybe_mkwrite(pte, vma);
>> + pte = maybe_mkwrite(pte, vma->vm_flags);
>>
>> if (unlikely(is_zone_device_page(new))) {
>> if (is_device_private_page(new)) {
>> --
>> 2.7.4
>>
>
^ permalink raw reply
* Re: [Skiboot] [PATCH 1/2] SLW: Remove stop1_lite and stop0 stop states
From: Nicholas Piggin @ 2018-05-03 10:15 UTC (permalink / raw)
To: Stewart Smith; +Cc: Akshay Adiga, linuxppc-dev, skiboot
In-Reply-To: <87lgd13vjo.fsf@linux.vnet.ibm.com>
On Thu, 03 May 2018 20:03:55 +1000
Stewart Smith <stewart@linux.vnet.ibm.com> wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> > On Thu, 3 May 2018 14:36:47 +0530
> > Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
> >
> >> On Tue, May 01, 2018 at 01:47:23PM +1000, Nicholas Piggin wrote:
> >> > On Mon, 30 Apr 2018 14:42:08 +0530
> >> > Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
> >> >
> >> > > Powersaving for stop0_lite and stop1_lite is observed to be quite similar
> >> > > and both states resume without state loss. Using context_switch test [1]
> >> > > we observe that stop0_lite has slightly lower latency, hence removing
> >> > > stop1_lite.
> >> > >
> >> > > [1] linux/tools/testing/selftests/powerpc/benchmarks/context_switch.c
> >> > >
> >> > > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
> >> >
> >> > I'm okay for removing stop1_lite and stop2_lite -- SMT switching
> >> > is very latency critical. If we decide to actually start saving
> >> > real power then SMT should already have been switched.
> >> >
> >> > So I would put stop1_lite and stop2_lite removal in the same patch.
> >>
> >> I can do this.
> >>
> >> >
> >> > Then what do we have? stop0_lite, stop0, stop1 for our fast idle
> >> > states.
> >>
> >> Currently we were looking at stop0_lite , stop1 as the fast idle states
> >> because stop0 and stop1 have similar latency and powersaving.
> >> Having so many low latency states does not make sense.
> >>
> >> >
> >> > I would be against removing stop0 if that is our fastest way to
> >> > release SMT resources, even if there is only a small advantage. Why
> >> > not remove stop1 instead?
> >> >
> >> SMT-folding comes into picture only when we have at least one thread
> >> running in the core. stop0 and stop1 has exactly same power-saving and
> >> both will release SMT resources if at least one thread in the core is
> >> running.
> >
> > Right, but you don't know that other threads are running or will remain
> > running when you enter stop. If not, then latency is higher for stop1,
> > no? So we need to be using stop0.
> >
> >>
> >> As soon as all threads are idle core enters stop0/stop1, where stop1
> >> does a bit more powersaving than stop0.
> >>
> >> > We also need to better evaluate stop0_lite. How much advantage does
> >> > that have over snooze?
> >>
> >> I evaluated snooze and stop0_lite, there is an additional ipi latency of
> >> a few microseconds in case of stop0_lite. So snooze cannot still be
> >> replaced by stop0_lite.
> >
> > I meant the other way around. Replace stop0_lite with snooze.
> >
> > So we would have snooze, stop0, stop2, and stop4 and/or 5.
>
> Slightly stupid question: should we be disabling these here or should
> Linux be better and deciding what states to use?
Yeah not a bad question, I don't have a good answer. I don't know how
smart Linux is at deciding what to use and when.
I am pretty sure the way we set our _lite states wrong -- we don't
want to go into stop2_lite as a deeper sleep state than stop0 for
example, because that then prevents SMT folding.
>
> I'm inclined to say this is a Linux problem as it should make the
> decision of what hardware feature to used based on the ones OPAL says
> *can* be used.
>
> I'm also open to be being convinced otherwise though...
>
I would say we should manually decide what states we want, and then
work backwards and try to make the dt metadata reach that result
without fudging it too much. If we can't do that, then we should try
to improve the kernel so it can be made to work.
At some we may decide to trim the states by hand in skiboot just
so existing kernels work without so much fuss, and aim to do a bit
better with later devices.
Thanks,
Nick
^ permalink raw reply
* Re: [Skiboot] [PATCH 1/2] SLW: Remove stop1_lite and stop0 stop states
From: Stewart Smith @ 2018-05-03 10:03 UTC (permalink / raw)
To: Nicholas Piggin, Akshay Adiga; +Cc: linuxppc-dev, skiboot
In-Reply-To: <20180503192852.13a42712@roar.ozlabs.ibm.com>
Nicholas Piggin <npiggin@gmail.com> writes:
> On Thu, 3 May 2018 14:36:47 +0530
> Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
>
>> On Tue, May 01, 2018 at 01:47:23PM +1000, Nicholas Piggin wrote:
>> > On Mon, 30 Apr 2018 14:42:08 +0530
>> > Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
>> >
>> > > Powersaving for stop0_lite and stop1_lite is observed to be quite similar
>> > > and both states resume without state loss. Using context_switch test [1]
>> > > we observe that stop0_lite has slightly lower latency, hence removing
>> > > stop1_lite.
>> > >
>> > > [1] linux/tools/testing/selftests/powerpc/benchmarks/context_switch.c
>> > >
>> > > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
>> >
>> > I'm okay for removing stop1_lite and stop2_lite -- SMT switching
>> > is very latency critical. If we decide to actually start saving
>> > real power then SMT should already have been switched.
>> >
>> > So I would put stop1_lite and stop2_lite removal in the same patch.
>>
>> I can do this.
>>
>> >
>> > Then what do we have? stop0_lite, stop0, stop1 for our fast idle
>> > states.
>>
>> Currently we were looking at stop0_lite , stop1 as the fast idle states
>> because stop0 and stop1 have similar latency and powersaving.
>> Having so many low latency states does not make sense.
>>
>> >
>> > I would be against removing stop0 if that is our fastest way to
>> > release SMT resources, even if there is only a small advantage. Why
>> > not remove stop1 instead?
>> >
>> SMT-folding comes into picture only when we have at least one thread
>> running in the core. stop0 and stop1 has exactly same power-saving and
>> both will release SMT resources if at least one thread in the core is
>> running.
>
> Right, but you don't know that other threads are running or will remain
> running when you enter stop. If not, then latency is higher for stop1,
> no? So we need to be using stop0.
>
>>
>> As soon as all threads are idle core enters stop0/stop1, where stop1
>> does a bit more powersaving than stop0.
>>
>> > We also need to better evaluate stop0_lite. How much advantage does
>> > that have over snooze?
>>
>> I evaluated snooze and stop0_lite, there is an additional ipi latency of
>> a few microseconds in case of stop0_lite. So snooze cannot still be
>> replaced by stop0_lite.
>
> I meant the other way around. Replace stop0_lite with snooze.
>
> So we would have snooze, stop0, stop2, and stop4 and/or 5.
Slightly stupid question: should we be disabling these here or should
Linux be better and deciding what states to use?
I'm inclined to say this is a Linux problem as it should make the
decision of what hardware feature to used based on the ones OPAL says
*can* be used.
I'm also open to be being convinced otherwise though...
--
Stewart Smith
OPAL Architect, IBM.
^ permalink raw reply
* Re: [PATCH 11/11] KVM: PPC: reconstruct LOAD_VSX/STORE_VSX instruction mmio emulation with analyse_intr() input
From: Simon Guo @ 2018-05-03 9:46 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503062612.GL6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 04:26:12PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:44PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > This patch reconstructs LOAD_VSX/STORE_VSX instruction MMIO emulation with
> > analyse_intr() input. It utilizes VSX_FPCONV/VSX_SPLAT/SIGNEXT exported
> > by analyse_instr() and handle accordingly.
> >
> > When emulating VSX store, the VSX reg will need to be flushed so that
> > the right reg val can be retrieved before writing to IO MEM.
> >
> > Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
>
> Looks good, except that you shouldn't need the special case for
> stxsiwx. With size=4 and element_size=8, kvmppc_handle_vsx_store
> should just do the right thing, as far as I can see.
Yes. Let me test after update.
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 10/11] KVM: PPC: reconstruct LOAD_VMX/STORE_VMX instruction mmio emulation with analyse_intr() input
From: Simon Guo @ 2018-05-03 9:43 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503061715.GK6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 04:17:15PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:43PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > This patch reconstructs LOAD_VMX/STORE_VMX instruction MMIO emulation with
> > analyse_intr() input. When emulating the store, the VMX reg will need to
> > be flushed so that the right reg val can be retrieved before writing to
> > IO MEM.
> >
> > Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
>
> This looks fine for lvx and stvx, but now we are also doing something
> for the vector element loads and stores (lvebx, stvebx, lvehx, stvehx,
> etc.) without having the logic to insert or extract the correct
> element in the vector register image. We need either to generate an
> error for the element load/store instructions, or handle them
> correctly.
Yes. I will consider that.
>
> > diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> > index 2dbdf9a..0bfee2f 100644
> > --- a/arch/powerpc/kvm/emulate_loadstore.c
> > +++ b/arch/powerpc/kvm/emulate_loadstore.c
> > @@ -160,6 +160,27 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > KVM_MMIO_REG_FPR|op.reg, size, 1);
> > break;
> > #endif
> > +#ifdef CONFIG_ALTIVEC
> > + case LOAD_VMX:
> > + if (kvmppc_check_altivec_disabled(vcpu))
> > + return EMULATE_DONE;
> > +
> > + /* VMX access will need to be size aligned */
>
> This comment isn't quite right; it isn't that the address needs to be
> size-aligned, it's that the hardware forcibly aligns it. So I would
> say something like /* Hardware enforces alignment of VMX accesses */.
>
I will update that.
> > + vcpu->arch.vaddr_accessed &= ~((unsigned long)size - 1);
> > + vcpu->arch.paddr_accessed &= ~((unsigned long)size - 1);
> > +
> > + if (size == 16) {
> > + vcpu->arch.mmio_vmx_copy_nums = 2;
> > + emulated = kvmppc_handle_load128_by2x64(run,
> > + vcpu, KVM_MMIO_REG_VMX|op.reg,
> > + 1);
> > + } else if (size <= 8)
> > + emulated = kvmppc_handle_load(run, vcpu,
> > + KVM_MMIO_REG_VMX|op.reg,
> > + size, 1);
> > +
> > + break;
> > +#endif
> > case STORE:
> > if (op.type & UPDATE) {
> > vcpu->arch.mmio_ra = op.update_reg;
> > @@ -197,6 +218,36 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > VCPU_FPR(vcpu, op.reg), size, 1);
> > break;
> > #endif
> > +#ifdef CONFIG_ALTIVEC
> > + case STORE_VMX:
> > + if (kvmppc_check_altivec_disabled(vcpu))
> > + return EMULATE_DONE;
> > +
> > + /* VMX access will need to be size aligned */
> > + vcpu->arch.vaddr_accessed &= ~((unsigned long)size - 1);
> > + vcpu->arch.paddr_accessed &= ~((unsigned long)size - 1);
> > +
> > + /* if it is PR KVM, the FP/VEC/VSX registers need to
> > + * be flushed so that kvmppc_handle_store() can read
> > + * actual VMX vals from vcpu->arch.
> > + */
> > + if (!is_kvmppc_hv_enabled(vcpu->kvm))
>
> As before, I suggest just testing that the function pointer isn't
> NULL.
Got it.
Thanks,
- Simon
^ permalink raw reply
* Re: [Skiboot] [PATCH 1/2] SLW: Remove stop1_lite and stop0 stop states
From: Nicholas Piggin @ 2018-05-03 9:28 UTC (permalink / raw)
To: Akshay Adiga; +Cc: linuxppc-dev, stewart, skiboot
In-Reply-To: <20180503090647.xsfw3p7mq2pwd2rw@aksadiga.ibm>
On Thu, 3 May 2018 14:36:47 +0530
Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
> On Tue, May 01, 2018 at 01:47:23PM +1000, Nicholas Piggin wrote:
> > On Mon, 30 Apr 2018 14:42:08 +0530
> > Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
> >
> > > Powersaving for stop0_lite and stop1_lite is observed to be quite similar
> > > and both states resume without state loss. Using context_switch test [1]
> > > we observe that stop0_lite has slightly lower latency, hence removing
> > > stop1_lite.
> > >
> > > [1] linux/tools/testing/selftests/powerpc/benchmarks/context_switch.c
> > >
> > > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
> >
> > I'm okay for removing stop1_lite and stop2_lite -- SMT switching
> > is very latency critical. If we decide to actually start saving
> > real power then SMT should already have been switched.
> >
> > So I would put stop1_lite and stop2_lite removal in the same patch.
>
> I can do this.
>
> >
> > Then what do we have? stop0_lite, stop0, stop1 for our fast idle
> > states.
>
> Currently we were looking at stop0_lite , stop1 as the fast idle states
> because stop0 and stop1 have similar latency and powersaving.
> Having so many low latency states does not make sense.
>
> >
> > I would be against removing stop0 if that is our fastest way to
> > release SMT resources, even if there is only a small advantage. Why
> > not remove stop1 instead?
> >
> SMT-folding comes into picture only when we have at least one thread
> running in the core. stop0 and stop1 has exactly same power-saving and
> both will release SMT resources if at least one thread in the core is
> running.
Right, but you don't know that other threads are running or will remain
running when you enter stop. If not, then latency is higher for stop1,
no? So we need to be using stop0.
>
> As soon as all threads are idle core enters stop0/stop1, where stop1
> does a bit more powersaving than stop0.
>
> > We also need to better evaluate stop0_lite. How much advantage does
> > that have over snooze?
>
> I evaluated snooze and stop0_lite, there is an additional ipi latency of
> a few microseconds in case of stop0_lite. So snooze cannot still be
> replaced by stop0_lite.
I meant the other way around. Replace stop0_lite with snooze.
So we would have snooze, stop0, stop2, and stop4 and/or 5.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 09/11] KVM: PPC: reconstruct LOAD_FP/STORE_FP instruction mmio emulation with analyse_intr() input
From: Simon Guo @ 2018-05-03 9:25 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503061049.GJ6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 04:10:49PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:42PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > This patch reconstructs LOAD_FP/STORE_FP instruction MMIO emulation with
> > analyse_intr() input. It utilizes the FPCONV/UPDATE properties exported by
> > analyse_instr() and invokes kvmppc_handle_load(s)/kvmppc_handle_store()
> > accordingly.
> >
> > The FP regs need to be flushed so that the right FP reg vals can be read
> > from vcpu->arch.fpr.
>
> This only applies for store instructions; it would be clearer if you
> said that explicitly.
I will correct this message.
>
> >
> > Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
>
> Same comment about updating RA as for the other patches. Otherwise
> this looks fine.
>
> Paul.
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 08/11] KVM: PPC: add giveup_ext() hook for PPC KVM ops
From: Simon Guo @ 2018-05-03 9:21 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503060817.GI6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 04:08:17PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:41PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > Currently HV will save math regs(FP/VEC/VSX) when trap into host. But
> > PR KVM will only save math regs when qemu task switch out of CPU.
> >
> > To emulate FP/VEC/VSX load, PR KVM need to flush math regs firstly and
> > then be able to update saved VCPU FPR/VEC/VSX area reasonably.
> >
> > This patch adds the giveup_ext() to KVM ops (an empty one for HV KVM)
> > and kvmppc_complete_mmio_load() can invoke that hook to flush math
> > regs accordingly.
> >
> > Math regs flush is also necessary for STORE, which will be covered
> > in later patch within this patch series.
> >
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
>
> I don't see where you have provided a function for Book E.
>
> I would suggest you only set the function pointer to non-NULL when the
> function is actually needed, i.e. for PR KVM.
Got it.
>
> It seems to me that this means that emulation of FP/VMX/VSX loads is
> currently broken for PR KVM for the case where kvm_io_bus_read() is
> able to supply the data, and the emulation of FP/VMX/VSX stores is
> broken for PR KVM for all cases. Do you agree?
>
Yes. I think so.
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index 5b875ba..7eb5507 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -2084,6 +2084,10 @@ static int kvmhv_set_smt_mode(struct kvm *kvm, unsigned long smt_mode,
> > return err;
> > }
> >
> > +static void kvmhv_giveup_ext(struct kvm_vcpu *vcpu, ulong msr)
> > +{
> > +}
> > +
> > static void unpin_vpa(struct kvm *kvm, struct kvmppc_vpa *vpa)
> > {
> > if (vpa->pinned_addr)
> > @@ -4398,6 +4402,7 @@ static int kvmhv_configure_mmu(struct kvm *kvm, struct kvm_ppc_mmuv3_cfg *cfg)
> > .configure_mmu = kvmhv_configure_mmu,
> > .get_rmmu_info = kvmhv_get_rmmu_info,
> > .set_smt_mode = kvmhv_set_smt_mode,
> > + .giveup_ext = kvmhv_giveup_ext,
> > };
> >
> > static int kvm_init_subcore_bitmap(void)
>
> I think HV KVM could leave this pointer as NULL, and then...
ok.
>
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 17f0315..e724601 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -1061,6 +1061,9 @@ static void kvmppc_complete_mmio_load(struct kvm_vcpu *vcpu,
> > kvmppc_set_gpr(vcpu, vcpu->arch.io_gpr, gpr);
> > break;
> > case KVM_MMIO_REG_FPR:
> > + if (!is_kvmppc_hv_enabled(vcpu->kvm))
> > + vcpu->kvm->arch.kvm_ops->giveup_ext(vcpu, MSR_FP);
> > +
>
> This could become
> if (vcpu->kvm->arch.kvm_ops->giveup_ext)
> vcpu->kvm->arch.kvm_ops->giveup_ext(vcpu, MSR_FP);
>
> and you wouldn't need to fix Book E explicitly.
Yes
>
> Paul.
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 07/11] KVM: PPC: reconstruct non-SIMD LOAD/STORE instruction mmio emulation with analyse_intr() input
From: Simon Guo @ 2018-05-03 9:07 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503060346.GH6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 04:03:46PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:40PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > This patch reconstructs non-SIMD LOAD/STORE instruction MMIO emulation
> > with analyse_intr() input. It utilizes the BYTEREV/UPDATE/SIGNEXT
> > properties exported by analyse_instr() and invokes
> > kvmppc_handle_load(s)/kvmppc_handle_store() accordingly.
> >
> > It also move CACHEOP type handling into the skeleton.
> >
> > instruction_type within sstep.h is renamed to avoid conflict with
> > kvm_ppc.h.
>
> I'd prefer to change the one in kvm_ppc.h, especially since that one
> isn't exactly about the type of instruction, but more about the type
> of interrupt led to us trying to fetch the instruction.
>
Agreed.
> > Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> > ---
> > arch/powerpc/include/asm/sstep.h | 2 +-
> > arch/powerpc/kvm/emulate_loadstore.c | 282 +++++++----------------------------
> > 2 files changed, 51 insertions(+), 233 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> > index ab9d849..0a1a312 100644
> > --- a/arch/powerpc/include/asm/sstep.h
> > +++ b/arch/powerpc/include/asm/sstep.h
> > @@ -23,7 +23,7 @@
> > #define IS_RFID(instr) (((instr) & 0xfc0007fe) == 0x4c000024)
> > #define IS_RFI(instr) (((instr) & 0xfc0007fe) == 0x4c000064)
> >
> > -enum instruction_type {
> > +enum analyse_instruction_type {
> > COMPUTE, /* arith/logical/CR op, etc. */
> > LOAD, /* load and store types need to be contiguous */
> > LOAD_MULTI,
> > diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> > index 90b9692..aaaf872 100644
> > --- a/arch/powerpc/kvm/emulate_loadstore.c
> > +++ b/arch/powerpc/kvm/emulate_loadstore.c
> > @@ -31,9 +31,12 @@
> > #include <asm/kvm_ppc.h>
> > #include <asm/disassemble.h>
> > #include <asm/ppc-opcode.h>
> > +#include <asm/sstep.h>
> > #include "timing.h"
> > #include "trace.h"
> >
> > +int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > + unsigned int instr);
>
> You shouldn't need this prototype here, since there's one in sstep.h.
>
Yes.
> > #ifdef CONFIG_PPC_FPU
> > static bool kvmppc_check_fp_disabled(struct kvm_vcpu *vcpu)
> > {
> > @@ -84,8 +87,9 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > struct kvm_run *run = vcpu->run;
> > u32 inst;
> > int ra, rs, rt;
> > - enum emulation_result emulated;
> > + enum emulation_result emulated = EMULATE_FAIL;
> > int advance = 1;
> > + struct instruction_op op;
> >
> > /* this default type might be overwritten by subcategories */
> > kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
> > @@ -114,144 +118,64 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> > vcpu->arch.mmio_update_ra = 0;
> > vcpu->arch.mmio_host_swabbed = 0;
> >
> > - switch (get_op(inst)) {
> > - case 31:
> > - switch (get_xop(inst)) {
> > - case OP_31_XOP_LWZX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> > - break;
> > -
> > - case OP_31_XOP_LWZUX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > -
> > - case OP_31_XOP_LBZX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> > - break;
> > + emulated = EMULATE_FAIL;
> > + vcpu->arch.regs.msr = vcpu->arch.shared->msr;
> > + vcpu->arch.regs.ccr = vcpu->arch.cr;
> > + if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
> > + int type = op.type & INSTR_TYPE_MASK;
> > + int size = GETSIZE(op.type);
> >
> > - case OP_31_XOP_LBZUX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > + switch (type) {
> > + case LOAD: {
> > + int instr_byte_swap = op.type & BYTEREV;
> >
> > - case OP_31_XOP_STDX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 8, 1);
> > - break;
> > + if (op.type & UPDATE) {
> > + vcpu->arch.mmio_ra = op.update_reg;
> > + vcpu->arch.mmio_update_ra = 1;
> > + }
>
> Any reason we can't just update RA here immediately?
>
> >
> > - case OP_31_XOP_STDUX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 8, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > -
> > - case OP_31_XOP_STWX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 4, 1);
> > - break;
> > -
> > - case OP_31_XOP_STWUX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 4, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > -
> > - case OP_31_XOP_STBX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 1, 1);
> > - break;
> > -
> > - case OP_31_XOP_STBUX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 1, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > -
> > - case OP_31_XOP_LHAX:
> > - emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
> > - break;
> > -
> > - case OP_31_XOP_LHAUX:
> > - emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > + if (op.type & SIGNEXT)
> > + emulated = kvmppc_handle_loads(run, vcpu,
> > + op.reg, size, !instr_byte_swap);
> > + else
> > + emulated = kvmppc_handle_load(run, vcpu,
> > + op.reg, size, !instr_byte_swap);
> >
> > - case OP_31_XOP_LHZX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
> > break;
> > -
> > - case OP_31_XOP_LHZUX:
> > - emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > -
> > - case OP_31_XOP_STHX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 2, 1);
> > - break;
> > -
> > - case OP_31_XOP_STHUX:
> > - emulated = kvmppc_handle_store(run, vcpu,
> > - kvmppc_get_gpr(vcpu, rs), 2, 1);
> > - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> > - break;
> > -
> > - case OP_31_XOP_DCBST:
> > - case OP_31_XOP_DCBF:
> > - case OP_31_XOP_DCBI:
> > + }
> > + case STORE:
> > + if (op.type & UPDATE) {
> > + vcpu->arch.mmio_ra = op.update_reg;
> > + vcpu->arch.mmio_update_ra = 1;
> > + }
>
> Same comment again about updating RA.
>
> > +
> > + /* if need byte reverse, op.val has been reverted by
>
> "reversed" rather than "reverted". "Reverted" means put back to a
> former state.
I will correct that.
>
> Paul.
Thanks,
- Simon
^ permalink raw reply
* Re: [Skiboot] [PATCH 1/2] SLW: Remove stop1_lite and stop0 stop states
From: Akshay Adiga @ 2018-05-03 9:06 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, stewart, skiboot
In-Reply-To: <20180501134723.5d00ddf0@roar.ozlabs.ibm.com>
On Tue, May 01, 2018 at 01:47:23PM +1000, Nicholas Piggin wrote:
> On Mon, 30 Apr 2018 14:42:08 +0530
> Akshay Adiga <akshay.adiga@linux.vnet.ibm.com> wrote:
>
> > Powersaving for stop0_lite and stop1_lite is observed to be quite similar
> > and both states resume without state loss. Using context_switch test [1]
> > we observe that stop0_lite has slightly lower latency, hence removing
> > stop1_lite.
> >
> > [1] linux/tools/testing/selftests/powerpc/benchmarks/context_switch.c
> >
> > Signed-off-by: Akshay Adiga <akshay.adiga@linux.vnet.ibm.com>
>
> I'm okay for removing stop1_lite and stop2_lite -- SMT switching
> is very latency critical. If we decide to actually start saving
> real power then SMT should already have been switched.
>
> So I would put stop1_lite and stop2_lite removal in the same patch.
I can do this.
>
> Then what do we have? stop0_lite, stop0, stop1 for our fast idle
> states.
Currently we were looking at stop0_lite , stop1 as the fast idle states
because stop0 and stop1 have similar latency and powersaving.
Having so many low latency states does not make sense.
>
> I would be against removing stop0 if that is our fastest way to
> release SMT resources, even if there is only a small advantage. Why
> not remove stop1 instead?
>
SMT-folding comes into picture only when we have at least one thread
running in the core. stop0 and stop1 has exactly same power-saving and
both will release SMT resources if at least one thread in the core is
running.
As soon as all threads are idle core enters stop0/stop1, where stop1
does a bit more powersaving than stop0.
> We also need to better evaluate stop0_lite. How much advantage does
> that have over snooze?
I evaluated snooze and stop0_lite, there is an additional ipi latency of
a few microseconds in case of stop0_lite. So snooze cannot still be
replaced by stop0_lite.
>
> Thanks,
> Nick
>
>
> > ---
> > hw/slw.c | 30 ------------------------------
> > 1 file changed, 30 deletions(-)
> >
> > diff --git a/hw/slw.c b/hw/slw.c
> > index 3f9abaa..edfc783 100644
> > --- a/hw/slw.c
> > +++ b/hw/slw.c
> > @@ -521,36 +521,6 @@ static struct cpu_idle_states power9_cpu_idle_states[] = {
> > | OPAL_PM_PSSCR_TR(3),
> > .pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> > {
> > - .name = "stop0",
> > - .latency_ns = 2000,
> > - .residency_ns = 20000,
> > - .flags = 0*OPAL_PM_DEC_STOP \
> > - | 0*OPAL_PM_TIMEBASE_STOP \
> > - | 1*OPAL_PM_LOSE_USER_CONTEXT \
> > - | 0*OPAL_PM_LOSE_HYP_CONTEXT \
> > - | 0*OPAL_PM_LOSE_FULL_CONTEXT \
> > - | 1*OPAL_PM_STOP_INST_FAST,
> > - .pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(0) \
> > - | OPAL_PM_PSSCR_MTL(3) \
> > - | OPAL_PM_PSSCR_TR(3) \
> > - | OPAL_PM_PSSCR_ESL \
> > - | OPAL_PM_PSSCR_EC,
> > - .pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> > - {
> > - .name = "stop1_lite", /* Enter stop1 with no state loss */
> > - .latency_ns = 4900,
> > - .residency_ns = 49000,
> > - .flags = 0*OPAL_PM_DEC_STOP \
> > - | 0*OPAL_PM_TIMEBASE_STOP \
> > - | 0*OPAL_PM_LOSE_USER_CONTEXT \
> > - | 0*OPAL_PM_LOSE_HYP_CONTEXT \
> > - | 0*OPAL_PM_LOSE_FULL_CONTEXT \
> > - | 1*OPAL_PM_STOP_INST_FAST,
> > - .pm_ctrl_reg_val = OPAL_PM_PSSCR_RL(1) \
> > - | OPAL_PM_PSSCR_MTL(3) \
> > - | OPAL_PM_PSSCR_TR(3),
> > - .pm_ctrl_reg_mask = OPAL_PM_PSSCR_MASK },
> > - {
> > .name = "stop1",
> > .latency_ns = 5000,
> > .residency_ns = 50000,
>
^ permalink raw reply
* Re: [PATCH 04/11] KVM: PPC: fix incorrect element_size for stxsiwx in analyse_instr
From: Simon Guo @ 2018-05-03 9:05 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503055047.GE6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 03:50:47PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:37PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > stwsiwx will place contents of word element 1 of VSR into word
> > storage of EA. So the element size of stwsiwx should be 4.
> >
> > This patch correct the size from 8 to 4.
> >
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> > ---
> > arch/powerpc/lib/sstep.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index 34d68f1..151d484 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -2178,7 +2178,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> > case 140: /* stxsiwx */
> > op->reg = rd | ((instr & 1) << 5);
> > op->type = MKOP(STORE_VSX, 0, 4);
> > - op->element_size = 8;
> > + op->element_size = 4;
>
> I made the element_size be 8 deliberately because this way, with
> size=4 but element_size=8, the code will naturally choose the correct
> word (the least-significant word of the left half) of the register to
> store into memory. With this change you then need the special case in
> a later patch for stxsiwx, which you shouldn't need if you don't make
> this change.
>
> Paul.
Thanks for point out. I will update accordingly.
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 05/11] KVM: PPC: add GPR RA update skeleton for MMIO emulation
From: Simon Guo @ 2018-05-03 8:37 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503055814.GF6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 03:58:14PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:38PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > To optimize kvm emulation code with analyse_instr, adds new
> > mmio_update_ra flag to aid with GPR RA update.
> >
> > This patch arms RA update at load/store emulation path for both
> > qemu mmio emulation or coalesced mmio emulation.
>
> It's not clear to me why you need to do this. The existing code
> writes RA at the point where the instruction is decoded. In later
> patches, you change that so the RA update occurs after the MMIO
> operation is performed. Is there a particular reason why you made
> that change?
>
> Paul.
I wanted to avoid the case that GPR RA was updated even when EMULATE_FAIL. But
if it is not mandatory, I can update RA in kvmppc_emulate_loadstore()
instead..
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 03/11] KVM: PPC: Fix a mmio_host_swabbed uninitialized usage issue when VMX store
From: Simon Guo @ 2018-05-03 7:52 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503054826.GD6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 03:48:26PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:36PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > When KVM emulates VMX store, it will invoke kvmppc_get_vmx_data() to
> > retrieve VMX reg val. kvmppc_get_vmx_data() will check mmio_host_swabbed
> > to decide which double word of vr[] to be used. But the
> > mmio_host_swabbed can be uninitiazed during VMX store procedure:
> >
> > kvmppc_emulate_loadstore
> > \- kvmppc_handle_store128_by2x64
> > \- kvmppc_get_vmx_data
> >
> > This patch corrects this by using kvmppc_need_byteswap() to choose
> > double word of vr[] and initialized mmio_host_swabbed to avoid invisble
> > trouble.
> >
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
>
> The patch is correct, but I think the patch description needs to say
> that vcpu->arch.mmio_host_swabbed is not meant to be used at all for
> emulation of store instructions, and this patch makes that true for
> VMX stores.
I will revise the commit message accordingly.
>
> Paul.
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 02/11] KVM: PPC: mov nip/ctr/lr/xer registers to pt_regs in kvm_vcpu_arch
From: Simon Guo @ 2018-05-03 7:51 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503054600.GC6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 03:46:01PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:35PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > This patch moves nip/ctr/lr/xer registers from scattered places in
> > kvm_vcpu_arch to pt_regs structure.
> >
> > cr register is "unsigned long" in pt_regs and u32 in vcpu->arch.
> > It will need more consideration and may move in later patches.
> >
> > Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
>
> Mostly looks fine; some nits below.
>
> > diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> > index e8a78a5..731f7d4 100644
> > --- a/arch/powerpc/kernel/asm-offsets.c
> > +++ b/arch/powerpc/kernel/asm-offsets.c
> > @@ -431,14 +431,14 @@ int main(void)
> > #ifdef CONFIG_ALTIVEC
> > OFFSET(VCPU_VRS, kvm_vcpu, arch.vr.vr);
> > #endif
> > - OFFSET(VCPU_XER, kvm_vcpu, arch.xer);
> > - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr);
> > - OFFSET(VCPU_LR, kvm_vcpu, arch.lr);
> > + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer);
> > + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr);
> > + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link);
> > #ifdef CONFIG_PPC_BOOK3S
> > OFFSET(VCPU_TAR, kvm_vcpu, arch.tar);
> > #endif
> > - OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> > - OFFSET(VCPU_PC, kvm_vcpu, arch.nip);
>
> This should be arch.pc; arch.nip doesn't exist.
Yes. That was introduced during patch split. I will correct them.
>
> > + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr);
>
> I thought the patch description said you weren't moving CR at this
> stage?
Sorry about that. thanks for pointing out.
>
> > + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip);
> > #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> > OFFSET(VCPU_MSR, kvm_vcpu, arch.shregs.msr);
> > OFFSET(VCPU_SRR0, kvm_vcpu, arch.shregs.srr0);
> > @@ -693,11 +693,11 @@ int main(void)
> > #endif /* CONFIG_PPC_BOOK3S_64 */
> >
> > #else /* CONFIG_PPC_BOOK3S */
> > - OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> > - OFFSET(VCPU_XER, kvm_vcpu, arch.xer);
> > - OFFSET(VCPU_LR, kvm_vcpu, arch.lr);
> > - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr);
> > - OFFSET(VCPU_PC, kvm_vcpu, arch.pc);
> > + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr);
>
> Once again VCPU_CR should not be changed.
Yep.
>
> > + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer);
> > + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link);
> > + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr);
> > + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip);
> > OFFSET(VCPU_SPRG9, kvm_vcpu, arch.sprg9);
> > OFFSET(VCPU_LAST_INST, kvm_vcpu, arch.last_inst);
> > OFFSET(VCPU_FAULT_DEAR, kvm_vcpu, arch.fault_dear);
>
> Paul.
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 01/11] KVM: PPC: add pt_regs into kvm_vcpu_arch and move vcpu->arch.gpr[] into it
From: Simon Guo @ 2018-05-03 7:43 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503053401.GB6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 03:34:01PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:34PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > Current regs are scattered at kvm_vcpu_arch structure and it will
> > be more neat to organize them into pt_regs structure.
> >
> > Also it will enable reconstruct MMIO emulation code with
>
> "reimplement" would be clearer than "reconstruct" here, I think.
>
ok.
> > @@ -438,7 +438,7 @@ int main(void)
> > OFFSET(VCPU_TAR, kvm_vcpu, arch.tar);
> > #endif
> > OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> > - OFFSET(VCPU_PC, kvm_vcpu, arch.pc);
> > + OFFSET(VCPU_PC, kvm_vcpu, arch.nip);
>
> This hunk shouldn't be in this patch.
Yes. sorry my patch split has some issue in patch 00/01.
>
> Paul.
Thanks,
- Simon
^ permalink raw reply
* Re: [PATCH 00/11] KVM: PPC: reconstruct mmio emulation with analyse_instr()
From: Simon Guo @ 2018-05-03 7:41 UTC (permalink / raw)
To: Paul Mackerras; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <20180503053117.GA6795@fergus.ozlabs.ibm.com>
On Thu, May 03, 2018 at 03:31:17PM +1000, Paul Mackerras wrote:
> On Wed, Apr 25, 2018 at 07:54:33PM +0800, wei.guo.simon@gmail.com wrote:
> > From: Simon Guo <wei.guo.simon@gmail.com>
> >
> > We already have analyse_instr() which analyzes instructions for the instruction
> > type, size, addtional flags, etc. What kvmppc_emulate_loadstore() did is somehow
> > duplicated and it will be good to utilize analyse_instr() to reconstruct the
> > code. The advantage is that the code logic will be shared and more clean to be
> > maintained.
> >
> > This patch series reconstructs kvmppc_emulate_loadstore() for various load/store
> > instructions.
> >
> > The testcase locates at:
> > https://github.com/justdoitqd/publicFiles/blob/master/test_mmio.c
> >
> > - Tested at both PR/HV KVM.
> > - Also tested with little endian host & big endian guest.
> >
> > Tested instruction list:
> > lbz lbzu lbzx ld ldbrx
> > ldu ldx lfd lfdu lfdx
> > lfiwax lfiwzx lfs lfsu lfsx
> > lha lhau lhax lhbrx lhz
> > lhzu lhzx lvx lwax lwbrx
> > lwz lwzu lwzx lxsdx lxsiwax
> > lxsiwzx lxsspx lxvd2x lxvdsx lxvw4x
> > stb stbu stbx std stdbrx
> > stdu stdx stfd stfdu stfdx
> > stfiwx stfs stfsx sth sthbrx
> > sthu sthx stvx stw stwbrx
> > stwu stwx stxsdx stxsiwx stxsspx
> > stxvd2x stxvw4x
>
> Thanks for doing this. It's nice to see that this makes the code 260
> lines smaller.
>
> I have some comments on the individual patches, which I will give in
> replies to those patches.
>
> Paul.
Thanks for those good comment. I will check them.
BR,
- Simon
^ permalink raw reply
* Re: [PATCH 00/11] KVM: PPC: reconstruct mmio emulation with analyse_instr()
From: Paul Mackerras @ 2018-05-03 5:31 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-1-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:33PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> We already have analyse_instr() which analyzes instructions for the instruction
> type, size, addtional flags, etc. What kvmppc_emulate_loadstore() did is somehow
> duplicated and it will be good to utilize analyse_instr() to reconstruct the
> code. The advantage is that the code logic will be shared and more clean to be
> maintained.
>
> This patch series reconstructs kvmppc_emulate_loadstore() for various load/store
> instructions.
>
> The testcase locates at:
> https://github.com/justdoitqd/publicFiles/blob/master/test_mmio.c
>
> - Tested at both PR/HV KVM.
> - Also tested with little endian host & big endian guest.
>
> Tested instruction list:
> lbz lbzu lbzx ld ldbrx
> ldu ldx lfd lfdu lfdx
> lfiwax lfiwzx lfs lfsu lfsx
> lha lhau lhax lhbrx lhz
> lhzu lhzx lvx lwax lwbrx
> lwz lwzu lwzx lxsdx lxsiwax
> lxsiwzx lxsspx lxvd2x lxvdsx lxvw4x
> stb stbu stbx std stdbrx
> stdu stdx stfd stfdu stfdx
> stfiwx stfs stfsx sth sthbrx
> sthu sthx stvx stw stwbrx
> stwu stwx stxsdx stxsiwx stxsspx
> stxvd2x stxvw4x
Thanks for doing this. It's nice to see that this makes the code 260
lines smaller.
I have some comments on the individual patches, which I will give in
replies to those patches.
Paul.
^ permalink raw reply
* Re: [PATCH 08/11] KVM: PPC: add giveup_ext() hook for PPC KVM ops
From: Paul Mackerras @ 2018-05-03 6:08 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-9-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:41PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> Currently HV will save math regs(FP/VEC/VSX) when trap into host. But
> PR KVM will only save math regs when qemu task switch out of CPU.
>
> To emulate FP/VEC/VSX load, PR KVM need to flush math regs firstly and
> then be able to update saved VCPU FPR/VEC/VSX area reasonably.
>
> This patch adds the giveup_ext() to KVM ops (an empty one for HV KVM)
> and kvmppc_complete_mmio_load() can invoke that hook to flush math
> regs accordingly.
>
> Math regs flush is also necessary for STORE, which will be covered
> in later patch within this patch series.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
I don't see where you have provided a function for Book E.
I would suggest you only set the function pointer to non-NULL when the
function is actually needed, i.e. for PR KVM.
It seems to me that this means that emulation of FP/VMX/VSX loads is
currently broken for PR KVM for the case where kvm_io_bus_read() is
able to supply the data, and the emulation of FP/VMX/VSX stores is
broken for PR KVM for all cases. Do you agree?
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 5b875ba..7eb5507 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -2084,6 +2084,10 @@ static int kvmhv_set_smt_mode(struct kvm *kvm, unsigned long smt_mode,
> return err;
> }
>
> +static void kvmhv_giveup_ext(struct kvm_vcpu *vcpu, ulong msr)
> +{
> +}
> +
> static void unpin_vpa(struct kvm *kvm, struct kvmppc_vpa *vpa)
> {
> if (vpa->pinned_addr)
> @@ -4398,6 +4402,7 @@ static int kvmhv_configure_mmu(struct kvm *kvm, struct kvm_ppc_mmuv3_cfg *cfg)
> .configure_mmu = kvmhv_configure_mmu,
> .get_rmmu_info = kvmhv_get_rmmu_info,
> .set_smt_mode = kvmhv_set_smt_mode,
> + .giveup_ext = kvmhv_giveup_ext,
> };
>
> static int kvm_init_subcore_bitmap(void)
I think HV KVM could leave this pointer as NULL, and then...
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 17f0315..e724601 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -1061,6 +1061,9 @@ static void kvmppc_complete_mmio_load(struct kvm_vcpu *vcpu,
> kvmppc_set_gpr(vcpu, vcpu->arch.io_gpr, gpr);
> break;
> case KVM_MMIO_REG_FPR:
> + if (!is_kvmppc_hv_enabled(vcpu->kvm))
> + vcpu->kvm->arch.kvm_ops->giveup_ext(vcpu, MSR_FP);
> +
This could become
if (vcpu->kvm->arch.kvm_ops->giveup_ext)
vcpu->kvm->arch.kvm_ops->giveup_ext(vcpu, MSR_FP);
and you wouldn't need to fix Book E explicitly.
Paul.
^ permalink raw reply
* Re: [PATCH 06/11] KVM: PPC: add KVMPPC_VSX_COPY_WORD_LOAD_DUMP type support for mmio emulation
From: Paul Mackerras @ 2018-05-03 5:59 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-7-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:39PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> Some VSX instruction like lxvwsx will splat word into VSR. This patch
> adds VSX copy type KVMPPC_VSX_COPY_WORD_LOAD_DUMP to support this.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
Reviewed-by: Paul Mackerras <paulus@ozlabs.org>
^ permalink raw reply
* Re: [PATCH 04/11] KVM: PPC: fix incorrect element_size for stxsiwx in analyse_instr
From: Paul Mackerras @ 2018-05-03 5:50 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-5-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:37PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> stwsiwx will place contents of word element 1 of VSR into word
> storage of EA. So the element size of stwsiwx should be 4.
>
> This patch correct the size from 8 to 4.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> ---
> arch/powerpc/lib/sstep.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index 34d68f1..151d484 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -2178,7 +2178,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> case 140: /* stxsiwx */
> op->reg = rd | ((instr & 1) << 5);
> op->type = MKOP(STORE_VSX, 0, 4);
> - op->element_size = 8;
> + op->element_size = 4;
I made the element_size be 8 deliberately because this way, with
size=4 but element_size=8, the code will naturally choose the correct
word (the least-significant word of the left half) of the register to
store into memory. With this change you then need the special case in
a later patch for stxsiwx, which you shouldn't need if you don't make
this change.
Paul.
^ permalink raw reply
* Re: [PATCH 07/11] KVM: PPC: reconstruct non-SIMD LOAD/STORE instruction mmio emulation with analyse_intr() input
From: Paul Mackerras @ 2018-05-03 6:03 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-8-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:40PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> This patch reconstructs non-SIMD LOAD/STORE instruction MMIO emulation
> with analyse_intr() input. It utilizes the BYTEREV/UPDATE/SIGNEXT
> properties exported by analyse_instr() and invokes
> kvmppc_handle_load(s)/kvmppc_handle_store() accordingly.
>
> It also move CACHEOP type handling into the skeleton.
>
> instruction_type within sstep.h is renamed to avoid conflict with
> kvm_ppc.h.
I'd prefer to change the one in kvm_ppc.h, especially since that one
isn't exactly about the type of instruction, but more about the type
of interrupt led to us trying to fetch the instruction.
> Suggested-by: Paul Mackerras <paulus@ozlabs.org>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
> ---
> arch/powerpc/include/asm/sstep.h | 2 +-
> arch/powerpc/kvm/emulate_loadstore.c | 282 +++++++----------------------------
> 2 files changed, 51 insertions(+), 233 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
> index ab9d849..0a1a312 100644
> --- a/arch/powerpc/include/asm/sstep.h
> +++ b/arch/powerpc/include/asm/sstep.h
> @@ -23,7 +23,7 @@
> #define IS_RFID(instr) (((instr) & 0xfc0007fe) == 0x4c000024)
> #define IS_RFI(instr) (((instr) & 0xfc0007fe) == 0x4c000064)
>
> -enum instruction_type {
> +enum analyse_instruction_type {
> COMPUTE, /* arith/logical/CR op, etc. */
> LOAD, /* load and store types need to be contiguous */
> LOAD_MULTI,
> diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
> index 90b9692..aaaf872 100644
> --- a/arch/powerpc/kvm/emulate_loadstore.c
> +++ b/arch/powerpc/kvm/emulate_loadstore.c
> @@ -31,9 +31,12 @@
> #include <asm/kvm_ppc.h>
> #include <asm/disassemble.h>
> #include <asm/ppc-opcode.h>
> +#include <asm/sstep.h>
> #include "timing.h"
> #include "trace.h"
>
> +int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
> + unsigned int instr);
You shouldn't need this prototype here, since there's one in sstep.h.
> #ifdef CONFIG_PPC_FPU
> static bool kvmppc_check_fp_disabled(struct kvm_vcpu *vcpu)
> {
> @@ -84,8 +87,9 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> struct kvm_run *run = vcpu->run;
> u32 inst;
> int ra, rs, rt;
> - enum emulation_result emulated;
> + enum emulation_result emulated = EMULATE_FAIL;
> int advance = 1;
> + struct instruction_op op;
>
> /* this default type might be overwritten by subcategories */
> kvmppc_set_exit_type(vcpu, EMULATED_INST_EXITS);
> @@ -114,144 +118,64 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
> vcpu->arch.mmio_update_ra = 0;
> vcpu->arch.mmio_host_swabbed = 0;
>
> - switch (get_op(inst)) {
> - case 31:
> - switch (get_xop(inst)) {
> - case OP_31_XOP_LWZX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> - break;
> -
> - case OP_31_XOP_LWZUX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 4, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_LBZX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> - break;
> + emulated = EMULATE_FAIL;
> + vcpu->arch.regs.msr = vcpu->arch.shared->msr;
> + vcpu->arch.regs.ccr = vcpu->arch.cr;
> + if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
> + int type = op.type & INSTR_TYPE_MASK;
> + int size = GETSIZE(op.type);
>
> - case OP_31_XOP_LBZUX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 1, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> + switch (type) {
> + case LOAD: {
> + int instr_byte_swap = op.type & BYTEREV;
>
> - case OP_31_XOP_STDX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 8, 1);
> - break;
> + if (op.type & UPDATE) {
> + vcpu->arch.mmio_ra = op.update_reg;
> + vcpu->arch.mmio_update_ra = 1;
> + }
Any reason we can't just update RA here immediately?
>
> - case OP_31_XOP_STDUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 8, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_STWX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 4, 1);
> - break;
> -
> - case OP_31_XOP_STWUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 4, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_STBX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 1, 1);
> - break;
> -
> - case OP_31_XOP_STBUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 1, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_LHAX:
> - emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
> - break;
> -
> - case OP_31_XOP_LHAUX:
> - emulated = kvmppc_handle_loads(run, vcpu, rt, 2, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> + if (op.type & SIGNEXT)
> + emulated = kvmppc_handle_loads(run, vcpu,
> + op.reg, size, !instr_byte_swap);
> + else
> + emulated = kvmppc_handle_load(run, vcpu,
> + op.reg, size, !instr_byte_swap);
>
> - case OP_31_XOP_LHZX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
> break;
> -
> - case OP_31_XOP_LHZUX:
> - emulated = kvmppc_handle_load(run, vcpu, rt, 2, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_STHX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 2, 1);
> - break;
> -
> - case OP_31_XOP_STHUX:
> - emulated = kvmppc_handle_store(run, vcpu,
> - kvmppc_get_gpr(vcpu, rs), 2, 1);
> - kvmppc_set_gpr(vcpu, ra, vcpu->arch.vaddr_accessed);
> - break;
> -
> - case OP_31_XOP_DCBST:
> - case OP_31_XOP_DCBF:
> - case OP_31_XOP_DCBI:
> + }
> + case STORE:
> + if (op.type & UPDATE) {
> + vcpu->arch.mmio_ra = op.update_reg;
> + vcpu->arch.mmio_update_ra = 1;
> + }
Same comment again about updating RA.
> +
> + /* if need byte reverse, op.val has been reverted by
"reversed" rather than "reverted". "Reverted" means put back to a
former state.
Paul.
^ permalink raw reply
* Re: [PATCH 03/11] KVM: PPC: Fix a mmio_host_swabbed uninitialized usage issue when VMX store
From: Paul Mackerras @ 2018-05-03 5:48 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-4-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:36PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> When KVM emulates VMX store, it will invoke kvmppc_get_vmx_data() to
> retrieve VMX reg val. kvmppc_get_vmx_data() will check mmio_host_swabbed
> to decide which double word of vr[] to be used. But the
> mmio_host_swabbed can be uninitiazed during VMX store procedure:
>
> kvmppc_emulate_loadstore
> \- kvmppc_handle_store128_by2x64
> \- kvmppc_get_vmx_data
>
> This patch corrects this by using kvmppc_need_byteswap() to choose
> double word of vr[] and initialized mmio_host_swabbed to avoid invisble
> trouble.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
The patch is correct, but I think the patch description needs to say
that vcpu->arch.mmio_host_swabbed is not meant to be used at all for
emulation of store instructions, and this patch makes that true for
VMX stores.
Paul.
^ permalink raw reply
* Re: [PATCH 01/11] KVM: PPC: add pt_regs into kvm_vcpu_arch and move vcpu->arch.gpr[] into it
From: Paul Mackerras @ 2018-05-03 5:34 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-2-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:34PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> Current regs are scattered at kvm_vcpu_arch structure and it will
> be more neat to organize them into pt_regs structure.
>
> Also it will enable reconstruct MMIO emulation code with
"reimplement" would be clearer than "reconstruct" here, I think.
> @@ -438,7 +438,7 @@ int main(void)
> OFFSET(VCPU_TAR, kvm_vcpu, arch.tar);
> #endif
> OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> - OFFSET(VCPU_PC, kvm_vcpu, arch.pc);
> + OFFSET(VCPU_PC, kvm_vcpu, arch.nip);
This hunk shouldn't be in this patch.
Paul.
^ permalink raw reply
* Re: [PATCH 02/11] KVM: PPC: mov nip/ctr/lr/xer registers to pt_regs in kvm_vcpu_arch
From: Paul Mackerras @ 2018-05-03 5:46 UTC (permalink / raw)
To: wei.guo.simon; +Cc: kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1524657284-16706-3-git-send-email-wei.guo.simon@gmail.com>
On Wed, Apr 25, 2018 at 07:54:35PM +0800, wei.guo.simon@gmail.com wrote:
> From: Simon Guo <wei.guo.simon@gmail.com>
>
> This patch moves nip/ctr/lr/xer registers from scattered places in
> kvm_vcpu_arch to pt_regs structure.
>
> cr register is "unsigned long" in pt_regs and u32 in vcpu->arch.
> It will need more consideration and may move in later patches.
>
> Signed-off-by: Simon Guo <wei.guo.simon@gmail.com>
Mostly looks fine; some nits below.
> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> index e8a78a5..731f7d4 100644
> --- a/arch/powerpc/kernel/asm-offsets.c
> +++ b/arch/powerpc/kernel/asm-offsets.c
> @@ -431,14 +431,14 @@ int main(void)
> #ifdef CONFIG_ALTIVEC
> OFFSET(VCPU_VRS, kvm_vcpu, arch.vr.vr);
> #endif
> - OFFSET(VCPU_XER, kvm_vcpu, arch.xer);
> - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr);
> - OFFSET(VCPU_LR, kvm_vcpu, arch.lr);
> + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer);
> + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr);
> + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link);
> #ifdef CONFIG_PPC_BOOK3S
> OFFSET(VCPU_TAR, kvm_vcpu, arch.tar);
> #endif
> - OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> - OFFSET(VCPU_PC, kvm_vcpu, arch.nip);
This should be arch.pc; arch.nip doesn't exist.
> + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr);
I thought the patch description said you weren't moving CR at this
stage?
> + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip);
> #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> OFFSET(VCPU_MSR, kvm_vcpu, arch.shregs.msr);
> OFFSET(VCPU_SRR0, kvm_vcpu, arch.shregs.srr0);
> @@ -693,11 +693,11 @@ int main(void)
> #endif /* CONFIG_PPC_BOOK3S_64 */
>
> #else /* CONFIG_PPC_BOOK3S */
> - OFFSET(VCPU_CR, kvm_vcpu, arch.cr);
> - OFFSET(VCPU_XER, kvm_vcpu, arch.xer);
> - OFFSET(VCPU_LR, kvm_vcpu, arch.lr);
> - OFFSET(VCPU_CTR, kvm_vcpu, arch.ctr);
> - OFFSET(VCPU_PC, kvm_vcpu, arch.pc);
> + OFFSET(VCPU_CR, kvm_vcpu, arch.regs.ccr);
Once again VCPU_CR should not be changed.
> + OFFSET(VCPU_XER, kvm_vcpu, arch.regs.xer);
> + OFFSET(VCPU_LR, kvm_vcpu, arch.regs.link);
> + OFFSET(VCPU_CTR, kvm_vcpu, arch.regs.ctr);
> + OFFSET(VCPU_PC, kvm_vcpu, arch.regs.nip);
> OFFSET(VCPU_SPRG9, kvm_vcpu, arch.sprg9);
> OFFSET(VCPU_LAST_INST, kvm_vcpu, arch.last_inst);
> OFFSET(VCPU_FAULT_DEAR, kvm_vcpu, arch.fault_dear);
Paul.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox