* Re: [PATCH v3 10/18] powerpc/powernv: Convert/move set_bypass() callback to take_ownership()
From: Benjamin Herrenschmidt @ 2014-07-28 4:19 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: linuxppc-dev, Paul Mackerras, Gavin Shan
In-Reply-To: <53D5C994.7050507@ozlabs.ru>
On Mon, 2014-07-28 at 13:55 +1000, Alexey Kardashevskiy wrote:
> On 07/28/2014 11:18 AM, Benjamin Herrenschmidt wrote:
> > On Thu, 2014-07-24 at 18:48 +1000, Alexey Kardashevskiy wrote:
> >> At the moment the iommu_table struct has a set_bypass() which enables/
> >> disables DMA bypass on IODA2 PHB. This is exposed to POWERPC IOMMU code
> >> which calls this callback when external IOMMU users such as VFIO are
> >> about to get over a PHB.
> >>
> >> Since the set_bypass() is not really an iommu_table function but PE's
> >> function, and we have an ops struct per IOMMU owner, let's move
> >> set_bypass() to the spapr_tce_iommu_ops struct.
> >>
> >> As arch/powerpc/kernel/iommu.c is more about POWERPC IOMMU tables and
> >> has very little to do with PEs, this moves take_ownership() calls to
> >> the VFIO SPAPR TCE driver.
> >>
> >> This renames set_bypass() to take_ownership() as it is not necessarily
> >> just enabling bypassing, it can be something else/more so let's give it
> >> a generic name. The bool parameter is inverted.
> >
> > I disagree with the name change. take_ownership() is the right semantic
> > at the high level, but down to the actual table, it *is* about disabling
> > bypass.
>
> It is still pnv_pci_ioda2_set_bypass() :-/
>
> take_ownership() is a callback of PNV IOMMU group.
>
> s/take_ownership/set_bypass/ ?
>
Hrm, ... ok. It's a bit messy but should do for now.
> >
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> Reviewed-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >> arch/powerpc/include/asm/iommu.h | 1 -
> >> arch/powerpc/include/asm/tce.h | 2 ++
> >> arch/powerpc/kernel/iommu.c | 12 ------------
> >> arch/powerpc/platforms/powernv/pci-ioda.c | 18 +++++++++++-------
> >> drivers/vfio/vfio_iommu_spapr_tce.c | 16 ++++++++++++++++
> >> 5 files changed, 29 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> >> index 84ee339..2b0b01d 100644
> >> --- a/arch/powerpc/include/asm/iommu.h
> >> +++ b/arch/powerpc/include/asm/iommu.h
> >> @@ -77,7 +77,6 @@ struct iommu_table {
> >> #ifdef CONFIG_IOMMU_API
> >> struct iommu_group *it_group;
> >> #endif
> >> - void (*set_bypass)(struct iommu_table *tbl, bool enable);
> >> };
> >>
> >> /* Pure 2^n version of get_order */
> >> diff --git a/arch/powerpc/include/asm/tce.h b/arch/powerpc/include/asm/tce.h
> >> index 8bfe98f..5ee4987 100644
> >> --- a/arch/powerpc/include/asm/tce.h
> >> +++ b/arch/powerpc/include/asm/tce.h
> >> @@ -58,6 +58,8 @@ struct spapr_tce_iommu_ops {
> >> struct iommu_table *(*get_table)(
> >> struct spapr_tce_iommu_group *data,
> >> phys_addr_t addr);
> >> + void (*take_ownership)(struct spapr_tce_iommu_group *data,
> >> + bool enable);
> >> };
> >>
> >> struct spapr_tce_iommu_group {
> >> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >> index e203314..06984d5 100644
> >> --- a/arch/powerpc/kernel/iommu.c
> >> +++ b/arch/powerpc/kernel/iommu.c
> >> @@ -1116,14 +1116,6 @@ int iommu_take_ownership(struct iommu_table *tbl)
> >> memset(tbl->it_map, 0xff, sz);
> >> iommu_clear_tces_and_put_pages(tbl, tbl->it_offset, tbl->it_size);
> >>
> >> - /*
> >> - * Disable iommu bypass, otherwise the user can DMA to all of
> >> - * our physical memory via the bypass window instead of just
> >> - * the pages that has been explicitly mapped into the iommu
> >> - */
> >> - if (tbl->set_bypass)
> >> - tbl->set_bypass(tbl, false);
> >> -
> >> return 0;
> >> }
> >> EXPORT_SYMBOL_GPL(iommu_take_ownership);
> >> @@ -1138,10 +1130,6 @@ void iommu_release_ownership(struct iommu_table *tbl)
> >> /* Restore bit#0 set by iommu_init_table() */
> >> if (tbl->it_offset == 0)
> >> set_bit(0, tbl->it_map);
> >> -
> >> - /* The kernel owns the device now, we can restore the iommu bypass */
> >> - if (tbl->set_bypass)
> >> - tbl->set_bypass(tbl, true);
> >> }
> >> EXPORT_SYMBOL_GPL(iommu_release_ownership);
> >>
> >> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> >> index 495137b..f828c57 100644
> >> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> >> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> >> @@ -709,10 +709,8 @@ static void pnv_pci_ioda_setup_dma_pe(struct pnv_phb *phb,
> >> __free_pages(tce_mem, get_order(TCE32_TABLE_SIZE * segs));
> >> }
> >>
> >> -static void pnv_pci_ioda2_set_bypass(struct iommu_table *tbl, bool enable)
> >> +static void pnv_pci_ioda2_set_bypass(struct pnv_ioda_pe *pe, bool enable)
> >> {
> >> - struct pnv_ioda_pe *pe = container_of(tbl, struct pnv_ioda_pe,
> >> - tce32.table);
> >> uint16_t window_id = (pe->pe_number << 1 ) + 1;
> >> int64_t rc;
> >>
> >> @@ -752,15 +750,21 @@ static void pnv_pci_ioda2_setup_bypass_pe(struct pnv_phb *phb,
> >> /* TVE #1 is selected by PCI address bit 59 */
> >> pe->tce_bypass_base = 1ull << 59;
> >>
> >> - /* Install set_bypass callback for VFIO */
> >> - pe->tce32.table.set_bypass = pnv_pci_ioda2_set_bypass;
> >> -
> >> /* Enable bypass by default */
> >> - pnv_pci_ioda2_set_bypass(&pe->tce32.table, true);
> >> + pnv_pci_ioda2_set_bypass(pe, true);
> >> +}
> >> +
> >> +static void pnv_ioda2_take_ownership(struct spapr_tce_iommu_group *data,
> >> + bool enable)
> >> +{
> >> + struct pnv_ioda_pe *pe = data->iommu_owner;
> >> +
> >> + pnv_pci_ioda2_set_bypass(pe, !enable);
> >> }
> >>
> >> static struct spapr_tce_iommu_ops pnv_pci_ioda2_ops = {
> >> .get_table = pnv_ioda1_iommu_get_table,
> >> + .take_ownership = pnv_ioda2_take_ownership,
> >> };
> >>
> >> static void pnv_pci_ioda2_setup_dma_pe(struct pnv_phb *phb,
> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> index 917c854..05b2f11 100644
> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> @@ -78,6 +78,13 @@ static long tce_iommu_account_memlimit(struct iommu_table *tbl, bool inc)
> >> return ret;
> >> }
> >>
> >> +static void tce_iommu_take_ownership_notify(struct spapr_tce_iommu_group *data,
> >> + bool enable)
> >> +{
> >> + if (data && data->ops && data->ops->take_ownership)
> >> + data->ops->take_ownership(data, enable);
> >> +}
> >> +
> >> static int tce_iommu_enable(struct tce_container *container)
> >> {
> >> int ret = 0;
> >> @@ -386,6 +393,12 @@ static int tce_iommu_attach_group(void *iommu_data,
> >> ret = iommu_take_ownership(tbl);
> >> if (!ret)
> >> container->grp = iommu_group;
> >> + /*
> >> + * Disable iommu bypass, otherwise the user can DMA to all of
> >> + * our physical memory via the bypass window instead of just
> >> + * the pages that has been explicitly mapped into the iommu
> >> + */
> >> + tce_iommu_take_ownership_notify(data, true);
> >> }
> >>
> >> mutex_unlock(&container->lock);
> >> @@ -423,6 +436,9 @@ static void tce_iommu_detach_group(void *iommu_data,
> >> BUG_ON(!tbl);
> >>
> >> iommu_release_ownership(tbl);
> >> +
> >> + /* Kernel owns the device now, we can restore bypass */
> >> + tce_iommu_take_ownership_notify(data, false);
> >> }
> >> mutex_unlock(&container->lock);
> >> }
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
>
>
^ permalink raw reply
* Re: [PATCH v3 03/18] KVM: PPC: Account TCE pages in locked_vm
From: Alexey Kardashevskiy @ 2014-07-28 4:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul Mackerras, linuxppc-dev, Gavin Shan
In-Reply-To: <1406508232.4935.23.camel@pasglop>
On 07/28/2014 10:43 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2014-07-24 at 18:47 +1000, Alexey Kardashevskiy wrote:
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>
> You need a description.
>
>> arch/powerpc/kvm/book3s_64_vio.c | 35 ++++++++++++++++++++++++++++++++++-
>> 1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
>> index 516f2ee..48b7ed4 100644
>> --- a/arch/powerpc/kvm/book3s_64_vio.c
>> +++ b/arch/powerpc/kvm/book3s_64_vio.c
>> @@ -45,18 +45,48 @@ static long kvmppc_stt_npages(unsigned long window_size)
>> * sizeof(u64), PAGE_SIZE) / PAGE_SIZE;
>> }
>>
>> +/*
>> + * Checks ulimit in order not to let the user space to pin all
>> + * available memory for TCE tables.
>> + */
>> +static long kvmppc_account_memlimit(long npages)
>> +{
>> + unsigned long ret = 0, locked, lock_limit;
>> +
>> + if (!current->mm)
>> + return -ESRCH; /* process exited */
>> +
>> + down_write(¤t->mm->mmap_sem);
>> + locked = current->mm->locked_vm + npages;
>> + lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
>> + pr_warn("RLIMIT_MEMLOCK (%ld) exceeded\n",
>> + rlimit(RLIMIT_MEMLOCK));
>> + ret = -ENOMEM;
>> + } else {
>> + current->mm->locked_vm += npages;
>> + }
>> + up_write(¤t->mm->mmap_sem);
>> +
>> + return ret;
>> +}
>> +
>> static void release_spapr_tce_table(struct kvmppc_spapr_tce_table *stt)
>> {
>> struct kvm *kvm = stt->kvm;
>> int i;
>> + long npages = kvmppc_stt_npages(stt->window_size);
>>
>> mutex_lock(&kvm->lock);
>> list_del(&stt->list);
>> - for (i = 0; i < kvmppc_stt_npages(stt->window_size); i++)
>> + for (i = 0; i < npages; i++)
>> __free_page(stt->pages[i]);
>> +
>> kfree(stt);
>> mutex_unlock(&kvm->lock);
>>
>> + kvmppc_account_memlimit(-(npages + 1));
>> +
>> kvm_put_kvm(kvm);
>> }
>>
>> @@ -112,6 +142,9 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
>> }
>>
>> npages = kvmppc_stt_npages(args->window_size);
>> + ret = kvmppc_account_memlimit(npages + 1);
>> + if (ret)
>> + goto fail;
>
> This is called for VFIO only or is it also called when creating TCE
> tables for emulated devices ? Because in the latter case, you don't
> want to account the pages as locked, do you ?
At the moment TCE-containing pages (for emulated TCE) are allocated with
alloc_page() which is kernel memory and therefore always locked, no?
> Also, you need to explain what +1
>
> Finally, do I correctly deduce that creating 10 TCE tables of 2G
> each will end up accounting 20G as locked even if the guest for
> example only has 4G of RAM ?
The user is required to set the limit to 20G, correct. But this does not
mean all 20G will be pinned. Ugly but better than nothing. As I remember
from you explanations, even if we give up real/virtual mode handlers for
H_PUT_TCE&Co, we cannot rely of existing counters in page struct in order
to understand whether we need to account a page again or not so we are
stuck with this code till we have a "clone DDW window" API.
But this patch is not about guest pages, it is about pages with TCEs, there
was no counting for this at all.
>
>> stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *),
>> GFP_KERNEL);
>
> Ben.
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v3 02/18] KVM: PPC: Use RCU when adding to arch.spapr_tce_tables
From: Benjamin Herrenschmidt @ 2014-07-28 4:30 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Paul Mackerras, linuxppc-dev, Gavin Shan
In-Reply-To: <53D5CD55.9050909@ozlabs.ru>
On Mon, 2014-07-28 at 14:11 +1000, Alexey Kardashevskiy wrote:
> ===
> At the moment the spapr_tce_tables list is not protected against races
> which may happen if the userspace issues the KVM_CREATE_SPAPR_TCE ioctl
> to KVM from different threads.
>
> This makes use of _rcu helpers for list_add()/list_del().
> ===
>
> and it is missing this bit as well (was under impression that all tables
> get freed at once at KVM exit but this might not be the case for malicious
> guest):
>
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -211,7 +211,7 @@ static void release_spapr_tce_table(struct
> kvmppc_spapr_tce_table *stt)
> long npages = kvmppc_stt_npages(stt->size);
>
> mutex_lock(&kvm->lock);
> - list_del(&stt->list);
> + list_del_rcu(&stt->list);
>
>
>
> Would this be sufficient?
Well, not really, we'd need to see the corresponding RCU'isms when
walking the list including the rcu read lock ... which could be
problematic if we are in real mode....
Cheers,
Ben.
>
>
> >
> > Cheers,
> > Ben.
> >
> >> ---
> >> arch/powerpc/kvm/book3s_64_vio.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> >> index 54cf9bc..516f2ee 100644
> >> --- a/arch/powerpc/kvm/book3s_64_vio.c
> >> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> >> @@ -131,7 +131,7 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> >> kvm_get_kvm(kvm);
> >>
> >> mutex_lock(&kvm->lock);
> >> - list_add(&stt->list, &kvm->arch.spapr_tce_tables);
> >> + list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
> >>
> >> mutex_unlock(&kvm->lock);
> >>
> >
> >
>
>
^ permalink raw reply
* Re: [PATCH v3 12/18] powerpc/iommu: Fix missing permission bits in iommu_put_tce_user_mode()
From: Alexey Kardashevskiy @ 2014-07-28 4:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul Mackerras, linuxppc-dev, Gavin Shan
In-Reply-To: <1406510394.4935.35.camel@pasglop>
On 07/28/2014 11:19 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2014-07-24 at 18:48 +1000, Alexey Kardashevskiy wrote:
>> This adds missing permission bits to the translated TCE.
>
> Is this a bug fix for existing stuff ? If yes, submit it separately.
There is 15/18 patch which fixes possible bug with leaking pages, and that
patch won't work until this one is applied.
Merge this one and "[PATCH v3 15/18] powerpc/iommu: Implement put_page() if
TCE had non-zero value"?
>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> arch/powerpc/kernel/iommu.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
>> index 0cda2e8..5af2319 100644
>> --- a/arch/powerpc/kernel/iommu.c
>> +++ b/arch/powerpc/kernel/iommu.c
>> @@ -1088,6 +1088,7 @@ int iommu_put_tce_user_mode(struct iommu_table *tbl, unsigned long entry,
>> return -EFAULT;
>> }
>> hwaddr = (unsigned long) page_address(page) + offset;
>> + hwaddr |= tce & (TCE_PCI_READ | TCE_PCI_WRITE);
>>
>> ret = iommu_tce_build(tbl, entry, hwaddr, direction);
>> if (ret)
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v3 03/18] KVM: PPC: Account TCE pages in locked_vm
From: Benjamin Herrenschmidt @ 2014-07-28 4:34 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Paul Mackerras, linuxppc-dev, Gavin Shan
In-Reply-To: <53D5D04F.7000507@ozlabs.ru>
On Mon, 2014-07-28 at 14:23 +1000, Alexey Kardashevskiy wrote:
> On 07/28/2014 10:43 AM, Benjamin Herrenschmidt wrote:
> > On Thu, 2014-07-24 at 18:47 +1000, Alexey Kardashevskiy wrote:
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >
> > You need a description.
> >
> >> arch/powerpc/kvm/book3s_64_vio.c | 35 ++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 34 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> >> index 516f2ee..48b7ed4 100644
> >> --- a/arch/powerpc/kvm/book3s_64_vio.c
> >> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> >> @@ -45,18 +45,48 @@ static long kvmppc_stt_npages(unsigned long window_size)
> >> * sizeof(u64), PAGE_SIZE) / PAGE_SIZE;
> >> }
> >>
> >> +/*
> >> + * Checks ulimit in order not to let the user space to pin all
> >> + * available memory for TCE tables.
> >> + */
> >> +static long kvmppc_account_memlimit(long npages)
> >> +{
> >> + unsigned long ret = 0, locked, lock_limit;
> >> +
> >> + if (!current->mm)
> >> + return -ESRCH; /* process exited */
> >> +
> >> + down_write(¤t->mm->mmap_sem);
> >> + locked = current->mm->locked_vm + npages;
> >> + lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >> + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
> >> + pr_warn("RLIMIT_MEMLOCK (%ld) exceeded\n",
> >> + rlimit(RLIMIT_MEMLOCK));
> >> + ret = -ENOMEM;
> >> + } else {
> >> + current->mm->locked_vm += npages;
> >> + }
> >> + up_write(¤t->mm->mmap_sem);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> static void release_spapr_tce_table(struct kvmppc_spapr_tce_table *stt)
> >> {
> >> struct kvm *kvm = stt->kvm;
> >> int i;
> >> + long npages = kvmppc_stt_npages(stt->window_size);
> >>
> >> mutex_lock(&kvm->lock);
> >> list_del(&stt->list);
> >> - for (i = 0; i < kvmppc_stt_npages(stt->window_size); i++)
> >> + for (i = 0; i < npages; i++)
> >> __free_page(stt->pages[i]);
> >> +
> >> kfree(stt);
> >> mutex_unlock(&kvm->lock);
> >>
> >> + kvmppc_account_memlimit(-(npages + 1));
> >> +
> >> kvm_put_kvm(kvm);
> >> }
> >>
> >> @@ -112,6 +142,9 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
> >> }
> >>
> >> npages = kvmppc_stt_npages(args->window_size);
> >> + ret = kvmppc_account_memlimit(npages + 1);
> >> + if (ret)
> >> + goto fail;
> >
> > This is called for VFIO only or is it also called when creating TCE
> > tables for emulated devices ? Because in the latter case, you don't
> > want to account the pages as locked, do you ?
>
> At the moment TCE-containing pages (for emulated TCE) are allocated with
> alloc_page() which is kernel memory and therefore always locked, no?
So the npages up there is the number of TCE-containing pages, not the
number of mapped-by-TCE pages ? In that case it makes sense yes.
>
> > Also, you need to explain what +1
> >
> > Finally, do I correctly deduce that creating 10 TCE tables of 2G
> > each will end up accounting 20G as locked even if the guest for
> > example only has 4G of RAM ?
>
>
> The user is required to set the limit to 20G, correct. But this does not
> mean all 20G will be pinned. Ugly but better than nothing. As I remember
> from you explanations, even if we give up real/virtual mode handlers for
> H_PUT_TCE&Co, we cannot rely of existing counters in page struct in order
> to understand whether we need to account a page again or not so we are
> stuck with this code till we have a "clone DDW window" API.
Right but please put that explanation somewhere in one of the changeset
comments or as comments near the code.
> But this patch is not about guest pages, it is about pages with TCEs, there
> was no counting for this at all.
Ok.
>
>
> >
> >> stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *),
> >> GFP_KERNEL);
> >
> > Ben.
> >
> >
>
>
^ permalink raw reply
* Re: [PATCH v3 12/18] powerpc/iommu: Fix missing permission bits in iommu_put_tce_user_mode()
From: Benjamin Herrenschmidt @ 2014-07-28 4:35 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Paul Mackerras, linuxppc-dev, Gavin Shan
In-Reply-To: <53D5D25D.90107@ozlabs.ru>
On Mon, 2014-07-28 at 14:32 +1000, Alexey Kardashevskiy wrote:
> On 07/28/2014 11:19 AM, Benjamin Herrenschmidt wrote:
> > On Thu, 2014-07-24 at 18:48 +1000, Alexey Kardashevskiy wrote:
> >> This adds missing permission bits to the translated TCE.
> >
> > Is this a bug fix for existing stuff ? If yes, submit it separately.
>
>
> There is 15/18 patch which fixes possible bug with leaking pages, and that
> patch won't work until this one is applied.
Please collapse them then.
> Merge this one and "[PATCH v3 15/18] powerpc/iommu: Implement put_page() if
> TCE had non-zero value"?
Right, and if the result is a bug fix on top of something already
upstream, please send it separately.
Cheers,
Ben.
>
>
> >
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> arch/powerpc/kernel/iommu.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >> index 0cda2e8..5af2319 100644
> >> --- a/arch/powerpc/kernel/iommu.c
> >> +++ b/arch/powerpc/kernel/iommu.c
> >> @@ -1088,6 +1088,7 @@ int iommu_put_tce_user_mode(struct iommu_table *tbl, unsigned long entry,
> >> return -EFAULT;
> >> }
> >> hwaddr = (unsigned long) page_address(page) + offset;
> >> + hwaddr |= tce & (TCE_PCI_READ | TCE_PCI_WRITE);
> >>
> >> ret = iommu_tce_build(tbl, entry, hwaddr, direction);
> >> if (ret)
> >
> >
>
>
^ permalink raw reply
* [PATCH] powerpc: kvm: make the setup of hpte under the protection of KVMPPC_RMAP_LOCK_BIT
From: Liu Ping Fan @ 2014-07-28 6:09 UTC (permalink / raw)
To: linuxppc-dev, kvm-ppc; +Cc: Paul Mackerras, Alexander Graf
In current code, the setup of hpte is under the risk of race with
mmu_notifier_invalidate, i.e we may setup a hpte with a invalid pfn.
Resolve this issue by sync the two actions by KVMPPC_RMAP_LOCK_BIT.
Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
arch/powerpc/kvm/book3s_64_mmu_hv.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 8056107..e6dcff4 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -754,19 +754,24 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
if (hptep[0] & HPTE_V_VALID) {
/* HPTE was previously valid, so we need to invalidate it */
- unlock_rmap(rmap);
hptep[0] |= HPTE_V_ABSENT;
kvmppc_invalidate_hpte(kvm, hptep, index);
/* don't lose previous R and C bits */
r |= hptep[1] & (HPTE_R_R | HPTE_R_C);
+
+ hptep[1] = r;
+ eieio();
+ hptep[0] = hpte[0];
+ asm volatile("ptesync" : : : "memory");
+ unlock_rmap(rmap);
} else {
+ hptep[1] = r;
+ eieio();
+ hptep[0] = hpte[0];
+ asm volatile("ptesync" : : : "memory");
kvmppc_add_revmap_chain(kvm, rev, rmap, index, 0);
}
- hptep[1] = r;
- eieio();
- hptep[0] = hpte[0];
- asm volatile("ptesync" : : : "memory");
preempt_enable();
if (page && hpte_is_writable(r))
SetPageDirty(page);
--
1.8.1.4
^ permalink raw reply related
* Re: [PATCH] powerpc: kvm: make the setup of hpte under the protection of KVMPPC_RMAP_LOCK_BIT
From: Benjamin Herrenschmidt @ 2014-07-28 6:42 UTC (permalink / raw)
To: Liu Ping Fan; +Cc: Paul Mackerras, linuxppc-dev, Alexander Graf, kvm-ppc
In-Reply-To: <1406527744-25316-1-git-send-email-pingfank@linux.vnet.ibm.com>
On Mon, 2014-07-28 at 14:09 +0800, Liu Ping Fan wrote:
> In current code, the setup of hpte is under the risk of race with
> mmu_notifier_invalidate, i.e we may setup a hpte with a invalid pfn.
> Resolve this issue by sync the two actions by KVMPPC_RMAP_LOCK_BIT.
Please describe the race you think you see. I'm quite sure both Paul and
I went over that code and somewhat convinced ourselves that it was ok
but it's possible that we were both wrong :-)
Cheers,
Ben.
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 8056107..e6dcff4 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -754,19 +754,24 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
>
> if (hptep[0] & HPTE_V_VALID) {
> /* HPTE was previously valid, so we need to invalidate it */
> - unlock_rmap(rmap);
> hptep[0] |= HPTE_V_ABSENT;
> kvmppc_invalidate_hpte(kvm, hptep, index);
> /* don't lose previous R and C bits */
> r |= hptep[1] & (HPTE_R_R | HPTE_R_C);
> +
> + hptep[1] = r;
> + eieio();
> + hptep[0] = hpte[0];
> + asm volatile("ptesync" : : : "memory");
> + unlock_rmap(rmap);
> } else {
> + hptep[1] = r;
> + eieio();
> + hptep[0] = hpte[0];
> + asm volatile("ptesync" : : : "memory");
> kvmppc_add_revmap_chain(kvm, rev, rmap, index, 0);
> }
>
> - hptep[1] = r;
> - eieio();
> - hptep[0] = hpte[0];
> - asm volatile("ptesync" : : : "memory");
> preempt_enable();
> if (page && hpte_is_writable(r))
> SetPageDirty(page);
^ permalink raw reply
* Re: Kernel crash caused by cpufreq
From: Michael Ellerman @ 2014-07-28 7:03 UTC (permalink / raw)
To: Gavin Shan, svaidy; +Cc: linuxppc-dev
In-Reply-To: <20140725010757.GA19679@shangw>
On Fri, 2014-07-25 at 11:07 +1000, Gavin Shan wrote:
> I'm tracing one LSI interrupt issue on P8 box, and eventually into the
> following kernel crash. Not sure if there is one fix against this? :-)
Vaidy wrote that I'm pretty sure (on CC).
> Starting Linux PPC64 #401 SMP Fri Jul 25 10:52:28 EST 2014
> -----------------------------------------------------
> ppc64_pft_size = 0x0
> physicalMemorySize = 0x800000000
> htab_address = 0xc0000007fe000000
> htab_hash_mask = 0x3ffff
> -----------------------------------------------------
> <- setup_system()
> Initializing cgroup subsys cpuset
> Initializing cgroup subsys cpuacct
> Linux version 3.16.0-rc6-00076-g4226dbe-dirty (shangw@shangw) (gcc version 4.5.2 (crosstool-NG 1.19.0) ) #401 SMP Fri Jul 25 10:52:28 EST 2014
> :
> < Unrelated log stripped >
Are you sure there's nothing else in the log that might be related? See the
messages in init_powernv_pstates() for example.
> ------------[ cut here ]------------
> kernel BUG at drivers/cpufreq/powernv-cpufreq.c:134!
> cpu 0x1: Vector: 700 (Program Check) at [c0000007f8483370]
> pc: c00000000096f32c: .pstate_id_to_freq+0x2c/0x50
> lr: c00000000096f37c: .powernv_read_cpu_freq+0x2c/0x50
> sp: c0000007f84835f0
> msr: 9000000000029032
> current = 0xc0000007f8400000
> paca = 0xc00000000ff00400 softe: 0 irq_happened: 0x01
> pid = 1, comm = swapper/0
> kernel BUG at drivers/cpufreq/powernv-cpufreq.c:134!
> enter ? for help
> [link register ] c00000000096f37c .powernv_read_cpu_freq+0x2c/0x50
> [c0000007f84835f0] c000000001076070 key_type_dns_resolver+0xef20/0x40d20 (unreliable)
> [c0000007f8483670] c00000000010812c .generic_exec_single+0x8c/0x270
> [c0000007f8483730] c0000000001083d0 .smp_call_function_single+0x90/0xb0
> [c0000007f84837b0] c000000000108cec .smp_call_function_any+0x15c/0x200
> [c0000007f8483860] c00000000096f27c .powernv_cpufreq_get+0x3c/0x60
> [c0000007f84838f0] c000000000969dc4 .__cpufreq_add_dev.clone.11+0x574/0xa20
> [c0000007f84839e0] c0000000005f7a4c .subsys_interface_register+0xec/0x130
> [c0000007f8483a90] c000000000967af8 .cpufreq_register_driver+0x168/0x2d0
> [c0000007f8483b30] c000000000f774cc .powernv_cpufreq_init+0x210/0x244
> [c0000007f8483be0] c00000000000bc08 .do_one_initcall+0xc8/0x240
> [c0000007f8483ce0] c000000000f44054 .kernel_init_freeable+0x268/0x33c
> [c0000007f8483db0] c00000000000c4dc .kernel_init+0x1c/0x110
> [c0000007f8483e30] c00000000000a428 .ret_from_kernel_thread+0x58/0xb0
> 1:mon> e
> cpu 0x1: Vector: 700 (Program Check) at [c0000007f8483370]
> pc: c00000000096f32c: .pstate_id_to_freq+0x2c/0x50
> lr: c00000000096f37c: .powernv_read_cpu_freq+0x2c/0x50
> sp: c0000007f84835f0
> msr: 9000000000029032
> current = 0xc0000007f8400000
> paca = 0xc00000000ff00400 softe: 0 irq_happened: 0x01
> pid = 1, comm = swapper/0
> kernel BUG at drivers/cpufreq/powernv-cpufreq.c:134!
> 1:mon> r
> R00 = 0000000000000042 R16 = 0000000000000000
> R01 = c0000007f84835f0 R17 = 0000000000000000
> R02 = c00000000116c430 R18 = 0000000000000000
> R03 = ffffffffffffffbe R19 = 0000000000000001
> R04 = 0000000000000000 R20 = c00000078bd61e58
> R05 = c00000000114ca78 R21 = c000000001008910
> R06 = c0000007f84838d0 R22 = c0000000011c1b74
> R07 = 0000000000000001 R23 = c000000002200030
> R08 = c0000000011c1910 R24 = 0000000000000001
> R09 = c000000001f2970c R25 = c000000001008730
> R10 = 0000000000000001 R26 = c000000001f29478
> R11 = 0000000000000042 R27 = c0000007f84838d0
> R12 = 0000000044002084 R28 = c00000000114ca78
> R13 = c00000000ff00400 R29 = 0000000000000000
> R14 = c00000000000c4c0 R30 = c0000000010a90f0
> R15 = 0000000000000000 R31 = c0000007f84838d0
> pc = c00000000096f32c .pstate_id_to_freq+0x2c/0x50
> cfar= c00000000096f324 .pstate_id_to_freq+0x24/0x50
> lr = c00000000096f37c .powernv_read_cpu_freq+0x2c/0x50
> msr = 9000000000029032 cr = 44002082
> ctr = c00000000096f350 xer = 0000000020000000 trap = 700
> 1:mon>
Gavin, in future for a dump like this it's very helpful to see the actual code
that hit the bug. You can get that with:
1:mon> di $.pstate_id_to_freq
Vaidy, judging by r3 it looks like i became negative. That would obviously
happen if powernv_pstate_info.max was zero?
cheers
^ permalink raw reply
* Re: [PATCH v2 5/7] powerpc/corenet: Add MDIO bus muxing support to the board device tree(s)
From: Emil Medve @ 2014-07-28 6:51 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <1405982754.29414.33.camel@snotra.buserror.net>
Hello Scott,
Scott Wood <scottwood <at> freescale.com> writes:
> On Wed, 2014-07-16 at 15:17 -0500, Shruti Kanetkar wrote:
> > + mdio <at> fd000 {
> > + /* For 10g interfaces */
> > + phy_xaui_slot1: xaui-phy <at> slot1 {
> > + status = "disabled";
> > + compatible = "ethernet-phy-ieee802.3-c45";
> > + reg = <0x7>; /* default switch setting on slot1 of AMC2PEX */
> > + };
>
> Why xaui-phy and not ethernet-phy?
>
> As for the device_type discussion from v1, there is a generic binding
> that says device_type "should" be ethernet-phy.
I have no strong feelings about this and we can use ethernet-phy, but:
1. The binding is old/stale (?) as it still uses device_type and the kernel
doesn't seem to use anymore the device_type for PHY(s)
2. The binding asks "ethernet-phy" for the device_type property, not for the
name. As such TBI PHY(s) use (upstream) the tbi-phy@ node name
> BTW, that binding
> (net/phy.txt) could use some cleaning up -- it still has references to
> things like linux,phandle.
>
> > diff --git a/arch/powerpc/boot/dts/b4qds.dtsi
b/arch/powerpc/boot/dts/b4qds.dtsi
> > index 8b47edc..6188583 100644
> > --- a/arch/powerpc/boot/dts/b4qds.dtsi
> > +++ b/arch/powerpc/boot/dts/b4qds.dtsi
> > <at> <at> -39,6 +39,13 <at> <at>
> > #size-cells = <2>;
> > interrupt-parent = <&mpic>;
> >
> > + aliases {
> > + phy_sgmii_10 = &phy_sgmii_10;
> > + phy_sgmii_11 = &phy_sgmii_11;
> > + phy_sgmii_1c = &phy_sgmii_1c;
> > + phy_sgmii_1d = &phy_sgmii_1d;
> > + };
>
> Is the encoding of these alias strings considered ABI (for either the
> OS's use or U-Boot's)?
U-Boot uses these aliases
> If so, please document it.
Will do
> If not, how do you
> see this being used? What does the hex number mean from the user's
> perspective?
>
> > + mdio0: mdio <at> fc000 {
> > + };
>
> Why is the empty node needed?
For the label
Cheers,
^ permalink raw reply
* Re: [PATCH] powerpc: kvm: make the setup of hpte under the protection of KVMPPC_RMAP_LOCK_BIT
From: Liu ping fan @ 2014-07-28 7:58 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Paul Mackerras, linuxppc-dev, Alexander Graf, kvm-ppc
In-Reply-To: <1406529741.4935.48.camel@pasglop>
Hope I am right. Take the following seq as an example
if (hptep[0] & HPTE_V_VALID) {
/* HPTE was previously valid, so we need to invalidate it */
unlock_rmap(rmap);
hptep[0] |= HPTE_V_ABSENT;
kvmppc_invalidate_hpte(kvm, hptep, index);
/* don't lose previous R and C bits */
r |= hptep[1] & (HPTE_R_R | HPTE_R_C);
} else {
kvmppc_add_revmap_chain(kvm, rev, rmap, index, 0);
}
---------------------------------------------> if we try_to_unmap on
pfn at here, then @r contains a invalid pfn
hptep[1] = r;
eieio();
hptep[0] = hpte[0];
asm volatile("ptesync" : : : "memory");
Thx.
Fan
On Mon, Jul 28, 2014 at 2:42 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Mon, 2014-07-28 at 14:09 +0800, Liu Ping Fan wrote:
>> In current code, the setup of hpte is under the risk of race with
>> mmu_notifier_invalidate, i.e we may setup a hpte with a invalid pfn.
>> Resolve this issue by sync the two actions by KVMPPC_RMAP_LOCK_BIT.
>
> Please describe the race you think you see. I'm quite sure both Paul and
> I went over that code and somewhat convinced ourselves that it was ok
> but it's possible that we were both wrong :-)
>
> Cheers,
> Ben.
>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/kvm/book3s_64_mmu_hv.c | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> index 8056107..e6dcff4 100644
>> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>> @@ -754,19 +754,24 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>
>> if (hptep[0] & HPTE_V_VALID) {
>> /* HPTE was previously valid, so we need to invalidate it */
>> - unlock_rmap(rmap);
>> hptep[0] |= HPTE_V_ABSENT;
>> kvmppc_invalidate_hpte(kvm, hptep, index);
>> /* don't lose previous R and C bits */
>> r |= hptep[1] & (HPTE_R_R | HPTE_R_C);
>> +
>> + hptep[1] = r;
>> + eieio();
>> + hptep[0] = hpte[0];
>> + asm volatile("ptesync" : : : "memory");
>> + unlock_rmap(rmap);
>> } else {
>> + hptep[1] = r;
>> + eieio();
>> + hptep[0] = hpte[0];
>> + asm volatile("ptesync" : : : "memory");
>> kvmppc_add_revmap_chain(kvm, rev, rmap, index, 0);
>> }
>>
>> - hptep[1] = r;
>> - eieio();
>> - hptep[0] = hpte[0];
>> - asm volatile("ptesync" : : : "memory");
>> preempt_enable();
>> if (page && hpte_is_writable(r))
>> SetPageDirty(page);
>
>
^ permalink raw reply
* RE: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers
From: mihai.caraman @ 2014-07-28 8:54 UTC (permalink / raw)
To: Scott Wood
Cc: linuxppc-dev@lists.ozlabs.org, Alexander Graf,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
In-Reply-To: <1406333436.29414.151.camel@snotra.buserror.net>
PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0K
PiBTZW50OiBTYXR1cmRheSwgSnVseSAyNiwgMjAxNCAzOjExIEFNDQo+IFRvOiBDYXJhbWFuIE1p
aGFpIENsYXVkaXUtQjAyMDA4DQo+IENjOiBBbGV4YW5kZXIgR3JhZjsga3ZtLXBwY0B2Z2VyLmtl
cm5lbC5vcmc7IGt2bUB2Z2VyLmtlcm5lbC5vcmc7DQo+IGxpbnV4cHBjLWRldkBsaXN0cy5vemxh
YnMub3JnDQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggMS82IHYyXSBLVk06IFBQQzogQm9vazNFOiBV
c2UgY29tbW9uIGRlZmluZXMgZm9yDQo+IFNQRS9GUC9BbHRpVmVjIGludCBudW1iZXJzDQo+IA0K
PiBPbiBUaHUsIDIwMTQtMDctMjQgYXQgMDQ6MTYgLTA1MDAsIENhcmFtYW4gTWloYWkgQ2xhdWRp
dS1CMDIwMDggd3JvdGU6DQo+ID4gU2NvdHQsIEFsZXgncyByZXF1ZXN0IHRvIGRlZmluZSBTUEUg
aGFuZGxlcnMgb25seSBmb3IgZTUwMHYyIGltcGxpZXMNCj4gY2hhbmdlcw0KPiA+IGluIDMyLWJp
dCBGU0wga2VybmVsIHRvIGhhdmUgZXhjbHVzaXZlIGNvbmZpZ3VyYXRpb25zIGZvciBlMjAwL2U1
MDB2Mg0KPiBhbmQNCj4gPiBlNTAwbWMvZTU1MDAuIFdlIHdvdWxkIHByb2JhYmx5IG5lZWQgc29t
ZXRoaW5nIGxpa2UgdGhpcywgd2hhdCdzIHlvdXINCj4gdGFrZSBvbiBpdD8NCj4gDQo+IFRoYXQg
aXMgYWxyZWFkeSBhIGNvbXBpbGUtdGltZSBkZWNpc2lvbi4NCg0KWWVzLCBidXQgaXMgbm90IGZ1
bGx5IGltcGxlbWVudGVkLg0KDQo+IA0KPiA+IGRpZmYgLS1naXQgYS9hcmNoL3Bvd2VycGMva2Vy
bmVsL2hlYWRfZnNsX2Jvb2tlLlMNCj4gYi9hcmNoL3Bvd2VycGMva2VybmVsL2hlYWRfZnNsX2Jv
b2tlLlMNCj4gPiBpbmRleCBiNDk3MTg4Li45ZDQxMDE1IDEwMDY0NA0KPiA+IC0tLSBhL2FyY2gv
cG93ZXJwYy9rZXJuZWwvaGVhZF9mc2xfYm9va2UuUw0KPiA+ICsrKyBiL2FyY2gvcG93ZXJwYy9r
ZXJuZWwvaGVhZF9mc2xfYm9va2UuUw0KPiA+IEBAIC02MTMsNiArNjEzLDggQEAgRU5EX0ZUUl9T
RUNUSU9OX0lGU0VUKENQVV9GVFJfRU1CX0hWKQ0KPiA+ICAgICAgICAgbWZzcHIgICByMTAsIFNQ
Uk5fU1BSR19SU0NSQVRDSDANCj4gPiAgICAgICAgIGIgICAgICAgSW5zdHJ1Y3Rpb25TdG9yYWdl
DQo+ID4NCj4gPiArLyogRGVmaW5lIFNQRSBoYW5kbGVycyBvbmx5IGZvciBlNTAwdjIgYW5kIGUy
MDAgKi8NCj4gPiArI2lmbmRlZiBDT05GSUdfUFBDX0U1MDBNQw0KPiA+ICAjaWZkZWYgQ09ORklH
X1NQRQ0KPiA+ICAgICAgICAgLyogU1BFIFVuYXZhaWxhYmxlICovDQo+ID4gICAgICAgICBTVEFS
VF9FWENFUFRJT04oU1BFVW5hdmFpbGFibGUpDQo+ID4gQEAgLTYyNiw3ICs2MjgsOSBAQCBFTkRf
RlRSX1NFQ1RJT05fSUZTRVQoQ1BVX0ZUUl9FTUJfSFYpDQo+ID4gICAgICAgICBFWENFUFRJT04o
MHgyMDIwLCBTUEVfQUxUSVZFQ19VTkFWQUlMLCBTUEVVbmF2YWlsYWJsZSwgXA0KPiA+ICAgICAg
ICAgICAgICAgICAgIHVua25vd25fZXhjZXB0aW9uLCBFWENfWEZFUl9FRSkNCj4gPiAgI2VuZGlm
IC8qIENPTkZJR19TUEUgKi8NCj4gPiArI2VuZGlmDQo+IA0KPiBUaGlzIGlzIHJlZHVuZGFudDoN
Cj4gDQo+ICAgICAgICAgY29uZmlnIFNQRQ0KPiAgICAgICAgICAgICAgICAgYm9vbCAiU1BFIFN1
cHBvcnQiDQo+ICAgICAgICAgICAgICAgICBkZXBlbmRzIG9uIEUyMDAgfHwgKEU1MDAgJiYgIVBQ
Q19FNTAwTUMpDQo+ICAgICAgICAgICAgICAgICBkZWZhdWx0IHkNCg0KSSBzZWUgd2hhdCB5b3Ug
bWVhbiwgYnV0IGl0J3Mgbm90IHJlZHVuZGFudC4gQWxleCB3YXMgbG9va2luZyB0byByZW1vdmUg
U1BFDQpoYW5kbGVycyBmb3IgZTUwMG1jKyBhbmQgdGhlIHByb3Bvc2FsIGhhbmRsZWQgIVNQRSBj
YXNlLiBJbiB0aGUgbmV3DQpsaWdodCBJIGZpbmQgdGhpcyB2YXJpYW50IG1vcmUgcmVhZGFibGU6
DQoNCisvKiBEZWZpbmUgU1BFIGhhbmRsZXJzIGZvciBlMjAwIGFuZCBlNTAwdjIgKi8NCiAjaWZk
ZWYgQ09ORklHX1NQRQ0KICAgICAgICAvKiBTUEUgVW5hdmFpbGFibGUgKi8NCiAgICAgICAgU1RB
UlRfRVhDRVBUSU9OKFNQRVVuYXZhaWxhYmxlKQ0KQEAgLTYyMiwxMSArNjIzLDEzIEBAIEVORF9G
VFJfU0VDVElPTl9JRlNFVChDUFVfRlRSX0VNQl9IVikNCiAgICAgICAgYiAgICAgICBmYXN0X2V4
Y2VwdGlvbl9yZXR1cm4NCiAxOiAgICAgYWRkaSAgICByMyxyMSxTVEFDS19GUkFNRV9PVkVSSEVB
RA0KICAgICAgICBFWENfWEZFUl9FRV9MSVRFKDB4MjAxMCwgS2VybmVsU1BFKQ0KLSNlbHNlDQor
I2VsaWYgZGVmaW5lZChDT05GSUdfRTIwMCkgfHwgXA0KKyAgICAgIChkZWZpbmVkKENPTkZJR19F
NTAwKSAmJiAhZGVmaW5lZChDT05GSUdfUFBDX0U1MDBNQykpDQogICAgICAgIEVYQ0VQVElPTigw
eDIwMjAsIFNQRV9BTFRJVkVDX1VOQVZBSUwsIFNQRVVuYXZhaWxhYmxlLCBcDQogICAgICAgICAg
ICAgICAgICB1bmtub3duX2V4Y2VwdGlvbiwgRVhDX1hGRVJfRUUpDQogI2VuZGlmIC8qIENPTkZJ
R19TUEUgKi8NCg0KPiANCj4gPiBkaWZmIC0tZ2l0IGEvYXJjaC9wb3dlcnBjL2tlcm5lbC9jcHV0
YWJsZS5jDQo+IGIvYXJjaC9wb3dlcnBjL2tlcm5lbC9jcHV0YWJsZS5jDQo+ID4gaW5kZXggYzFm
YWFkZS4uM2FiNjVjMiAxMDA2NDQNCj4gPiAtLS0gYS9hcmNoL3Bvd2VycGMva2VybmVsL2NwdXRh
YmxlLmMNCj4gPiArKysgYi9hcmNoL3Bvd2VycGMva2VybmVsL2NwdXRhYmxlLmMNCj4gPiBAQCAt
MjAzMCw2ICsyMDMwLDcgQEAgc3RhdGljIHN0cnVjdCBjcHVfc3BlYyBfX2luaXRkYXRhIGNwdV9z
cGVjc1tdID0gew0KPiA+ICAjZW5kaWYgLyogQ09ORklHX1BQQzMyICovDQo+ID4gICNpZmRlZiBD
T05GSUdfRTUwMA0KPiA+ICAjaWZkZWYgQ09ORklHX1BQQzMyDQo+ID4gKyNpZm5kZWYgQ09ORklH
X1BQQ19FNTAwTUMNCj4gPiAgICAgICAgIHsgICAgICAgLyogZTUwMCAqLw0KPiA+ICAgICAgICAg
ICAgICAgICAucHZyX21hc2sgICAgICAgICAgICAgICA9IDB4ZmZmZjAwMDAsDQo+ID4gICAgICAg
ICAgICAgICAgIC5wdnJfdmFsdWUgICAgICAgICAgICAgID0gMHg4MDIwMDAwMCwNCj4gPiBAQCAt
MjA2OSw2ICsyMDcwLDcgQEAgc3RhdGljIHN0cnVjdCBjcHVfc3BlYyBfX2luaXRkYXRhIGNwdV9z
cGVjc1tdID0gew0KPiA+ICAgICAgICAgICAgICAgICAubWFjaGluZV9jaGVjayAgICAgICAgICA9
IG1hY2hpbmVfY2hlY2tfZTUwMCwNCj4gPiAgICAgICAgICAgICAgICAgLnBsYXRmb3JtICAgICAg
ICAgICAgICAgPSAicHBjODU0OCIsDQo+ID4gICAgICAgICB9LA0KPiA+ICsjZW5kaWYgLyogIUNP
TkZJR19QUENfRTUwME1DICovDQo+ID4gICAgICAgICB7ICAgICAgIC8qIGU1MDBtYyAqLw0KPiA+
ICAgICAgICAgICAgICAgICAucHZyX21hc2sgICAgICAgICAgICAgICA9IDB4ZmZmZjAwMDAsDQo+
ID4gICAgICAgICAgICAgICAgIC5wdnJfdmFsdWUgICAgICAgICAgICAgID0gMHg4MDIzMDAwMCwN
Cj4gPg0KPiANCj4gVGhpcyBsb29rcyBhIGJpdCBzdHJhbmdlIC0tIGU1MDB2MiBnZXRzIGV4Y2x1
ZGVkIGlmIFBQQ19FNTAwTUMsIGJ1dA0KPiBlNTAwbWMgZ2V0cyBpbmNsdWRlZCBpbiAhUFBDX0U1
MDBNQz8NCg0KUmlnaHQsIG15IG1haW4gcHVycG9zZSB3YXMgdG8gZ2V0IHJpZCBvZiBfX3NldHVw
X2U1MDBfaXZvcnMgb24gUFBDX0U1MDBNQw0Kd2hpY2ggcmVmZXJzIFNQRVVuYXZhaWxhYmxlLiBJ
IHdpbGwgYWRkIGFuICNlbHNlIHRvIGV4Y2x1ZGUgZTUwMG1jLg0KDQpUaGUgImdlbmVyaWMgRTUw
MCBQUEMiIGRlZmF1bHQgY3B1IGFkdmVydGlzZXMgUFBDX0ZFQVRVUkVfSEFTX1NQRV9DT01QLg0K
RG8gd2Ugd2FudCB0byBleGNsdWRlZCBpdCBpZiBQUENfRTUwME1DPyBJcyB0aGlzIGNwdSB1c2Vm
dWwgd2l0aG91dCBjcHVfc2V0dXAoKQ0KYW5kIGlmIHNvIHdoeSBkb24ndCB3ZSBhbHNvIGhhdmUg
YSBkZWZhdWx0IGZvciA2NC1iaXQ/DQoNCi1NaWtlDQo=
^ permalink raw reply
* Re: [PATCH v3 04/18] vfio: powerpc: Move locked_vm accounting to a helper
From: Alexey Kardashevskiy @ 2014-07-28 9:12 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Paul Mackerras, linuxppc-dev, Gavin Shan
In-Reply-To: <1406508410.4935.24.camel@pasglop>
On 07/28/2014 10:46 AM, Benjamin Herrenschmidt wrote:
> On Thu, 2014-07-24 at 18:47 +1000, Alexey Kardashevskiy wrote:
>> Additional DMA windows support is coming and accounting will include them
>> so let's move this code to a helper for reuse.
>
> This code looks a LOT like the one you added in the previous patch
> (ie. kvmppc_account_memlimit()), though in a different place. I
> wonder if we should re-arrange things so that there is really
> only one version of it..
One copy is in VFIO driver, another in KVM, they meet in VFIO KVM device
(which may miss) or IOMMU code (drivers/iommu/iommu.c or
arch/powerpc/kernel/iommu.c) but this math has nothing to do with IOMMU. I
fail to see good common place for this.
>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> drivers/vfio/vfio_iommu_spapr_tce.c | 54 ++++++++++++++++++++++++-------------
>> 1 file changed, 36 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index a84788b..c8f7284 100644
>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -47,10 +47,40 @@ struct tce_container {
>> bool enabled;
>> };
>>
>> +/*
>> + * Checks ulimit in order not to let the user space to pin all
>> + * available memory for TCE tables.
>> + */
>> +static long tce_iommu_account_memlimit(struct iommu_table *tbl, bool inc)
>> +{
>> + unsigned long ret = 0, locked, lock_limit;
>> + long npages;
>> +
>> + if (!current->mm)
>> + return -ESRCH; /* process exited */
>> +
>> + npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT;
>> + if (!inc)
>> + npages = -npages;
>> +
>> + down_write(¤t->mm->mmap_sem);
>> + locked = current->mm->locked_vm + npages;
>> + lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> + if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
>> + pr_warn("RLIMIT_MEMLOCK (%ld) exceeded\n",
>> + rlimit(RLIMIT_MEMLOCK));
>> + ret = -ENOMEM;
>> + } else {
>> + current->mm->locked_vm += npages;
>> + }
>> + up_write(¤t->mm->mmap_sem);
>> +
>> + return ret;
>> +}
>> +
>> static int tce_iommu_enable(struct tce_container *container)
>> {
>> int ret = 0;
>> - unsigned long locked, lock_limit, npages;
>> struct iommu_table *tbl = container->tbl;
>>
>> if (!container->tbl)
>> @@ -80,20 +110,11 @@ static int tce_iommu_enable(struct tce_container *container)
>> * that would effectively kill the guest at random points, much better
>> * enforcing the limit based on the max that the guest can map.
>> */
>> - down_write(¤t->mm->mmap_sem);
>> - npages = (tbl->it_size << IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT;
>> - locked = current->mm->locked_vm + npages;
>> - lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>> - if (locked > lock_limit && !capable(CAP_IPC_LOCK)) {
>> - pr_warn("RLIMIT_MEMLOCK (%ld) exceeded\n",
>> - rlimit(RLIMIT_MEMLOCK));
>> - ret = -ENOMEM;
>> - } else {
>> + ret = tce_iommu_account_memlimit(tbl, true);
>> + if (ret)
>> + return ret;
>>
>> - current->mm->locked_vm += npages;
>> - container->enabled = true;
>> - }
>> - up_write(¤t->mm->mmap_sem);
>> + container->enabled = true;
>>
>> return ret;
>> }
>> @@ -108,10 +129,7 @@ static void tce_iommu_disable(struct tce_container *container)
>> if (!container->tbl || !current->mm)
>> return;
>>
>> - down_write(¤t->mm->mmap_sem);
>> - current->mm->locked_vm -= (container->tbl->it_size <<
>> - IOMMU_PAGE_SHIFT_4K) >> PAGE_SHIFT;
>> - up_write(¤t->mm->mmap_sem);
>> + tce_iommu_account_memlimit(container->tbl, false);
>> }
>>
>> static void *tce_iommu_open(unsigned long arg)
>
>
--
Alexey
^ permalink raw reply
* Re: Kernel crash caused by cpufreq
From: Vaidyanathan Srinivasan @ 2014-07-28 10:28 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Gavin Shan
In-Reply-To: <1406530990.23799.8.camel@concordia>
* Michael Ellerman <mpe@ellerman.id.au> [2014-07-28 17:03:10]:
> On Fri, 2014-07-25 at 11:07 +1000, Gavin Shan wrote:
> > I'm tracing one LSI interrupt issue on P8 box, and eventually into the
> > following kernel crash. Not sure if there is one fix against this? :-)
>
> Vaidy wrote that I'm pretty sure (on CC).
Yes, I did :)
> > Starting Linux PPC64 #401 SMP Fri Jul 25 10:52:28 EST 2014
> > -----------------------------------------------------
> > ppc64_pft_size = 0x0
> > physicalMemorySize = 0x800000000
> > htab_address = 0xc0000007fe000000
> > htab_hash_mask = 0x3ffff
> > -----------------------------------------------------
> > <- setup_system()
> > Initializing cgroup subsys cpuset
> > Initializing cgroup subsys cpuacct
> > Linux version 3.16.0-rc6-00076-g4226dbe-dirty (shangw@shangw) (gcc version 4.5.2 (crosstool-NG 1.19.0) ) #401 SMP Fri Jul 25 10:52:28 EST 2014
> > :
> > < Unrelated log stripped >
>
> Are you sure there's nothing else in the log that might be related? See the
> messages in init_powernv_pstates() for example.
Most likely PMSR register is showing out of bound values because of
potential firmware (OPAL) issue.
> > ------------[ cut here ]------------
> > kernel BUG at drivers/cpufreq/powernv-cpufreq.c:134!
> > cpu 0x1: Vector: 700 (Program Check) at [c0000007f8483370]
> > pc: c00000000096f32c: .pstate_id_to_freq+0x2c/0x50
> > lr: c00000000096f37c: .powernv_read_cpu_freq+0x2c/0x50
> > sp: c0000007f84835f0
> > msr: 9000000000029032
> > current = 0xc0000007f8400000
> > paca = 0xc00000000ff00400 softe: 0 irq_happened: 0x01
> > pid = 1, comm = swapper/0
> > kernel BUG at drivers/cpufreq/powernv-cpufreq.c:134!
> > enter ? for help
> > [link register ] c00000000096f37c .powernv_read_cpu_freq+0x2c/0x50
> > [c0000007f84835f0] c000000001076070 key_type_dns_resolver+0xef20/0x40d20 (unreliable)
> > [c0000007f8483670] c00000000010812c .generic_exec_single+0x8c/0x270
> > [c0000007f8483730] c0000000001083d0 .smp_call_function_single+0x90/0xb0
> > [c0000007f84837b0] c000000000108cec .smp_call_function_any+0x15c/0x200
> > [c0000007f8483860] c00000000096f27c .powernv_cpufreq_get+0x3c/0x60
> > [c0000007f84838f0] c000000000969dc4 .__cpufreq_add_dev.clone.11+0x574/0xa20
> > [c0000007f84839e0] c0000000005f7a4c .subsys_interface_register+0xec/0x130
> > [c0000007f8483a90] c000000000967af8 .cpufreq_register_driver+0x168/0x2d0
> > [c0000007f8483b30] c000000000f774cc .powernv_cpufreq_init+0x210/0x244
> > [c0000007f8483be0] c00000000000bc08 .do_one_initcall+0xc8/0x240
> > [c0000007f8483ce0] c000000000f44054 .kernel_init_freeable+0x268/0x33c
> > [c0000007f8483db0] c00000000000c4dc .kernel_init+0x1c/0x110
> > [c0000007f8483e30] c00000000000a428 .ret_from_kernel_thread+0x58/0xb0
> > 1:mon> e
> > cpu 0x1: Vector: 700 (Program Check) at [c0000007f8483370]
> > pc: c00000000096f32c: .pstate_id_to_freq+0x2c/0x50
> > lr: c00000000096f37c: .powernv_read_cpu_freq+0x2c/0x50
> > sp: c0000007f84835f0
> > msr: 9000000000029032
> > current = 0xc0000007f8400000
> > paca = 0xc00000000ff00400 softe: 0 irq_happened: 0x01
> > pid = 1, comm = swapper/0
> > kernel BUG at drivers/cpufreq/powernv-cpufreq.c:134!
> > 1:mon> r
> > R00 = 0000000000000042 R16 = 0000000000000000
> > R01 = c0000007f84835f0 R17 = 0000000000000000
> > R02 = c00000000116c430 R18 = 0000000000000000
> > R03 = ffffffffffffffbe R19 = 0000000000000001
> > R04 = 0000000000000000 R20 = c00000078bd61e58
> > R05 = c00000000114ca78 R21 = c000000001008910
> > R06 = c0000007f84838d0 R22 = c0000000011c1b74
> > R07 = 0000000000000001 R23 = c000000002200030
> > R08 = c0000000011c1910 R24 = 0000000000000001
> > R09 = c000000001f2970c R25 = c000000001008730
> > R10 = 0000000000000001 R26 = c000000001f29478
> > R11 = 0000000000000042 R27 = c0000007f84838d0
> > R12 = 0000000044002084 R28 = c00000000114ca78
> > R13 = c00000000ff00400 R29 = 0000000000000000
> > R14 = c00000000000c4c0 R30 = c0000000010a90f0
> > R15 = 0000000000000000 R31 = c0000007f84838d0
> > pc = c00000000096f32c .pstate_id_to_freq+0x2c/0x50
> > cfar= c00000000096f324 .pstate_id_to_freq+0x24/0x50
> > lr = c00000000096f37c .powernv_read_cpu_freq+0x2c/0x50
> > msr = 9000000000029032 cr = 44002082
> > ctr = c00000000096f350 xer = 0000000020000000 trap = 700
> > 1:mon>
>
> Gavin, in future for a dump like this it's very helpful to see the actual code
> that hit the bug. You can get that with:
>
> 1:mon> di $.pstate_id_to_freq
>
>
> Vaidy, judging by r3 it looks like i became negative. That would obviously
> happen if powernv_pstate_info.max was zero?
yes, negative is ok. Something has gone wrong with the PState
firmware/hardware. A BUG_ON() is too severe for this error. I will
change code to not stop the system for this error and also investigate
what is happening at runtime.
--Vaidy
^ permalink raw reply
* Re: [PATCH v3] Use the POWER8 Micro Partition Prefetch Engine in KVM HV on POWER8
From: Alexander Graf @ 2014-07-28 12:30 UTC (permalink / raw)
To: Stewart Smith, linuxppc-dev, paulus, kvm-ppc
In-Reply-To: <m3k37bmhnb.fsf@oc8180480414.ibm.com>
On 18.07.14 06:10, Stewart Smith wrote:
> Alexander Graf <agraf@suse.de> writes:
>>> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
>>> index 1eaea2d..5769497 100644
>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>> @@ -305,6 +305,8 @@ struct kvmppc_vcore {
>>> u32 arch_compat;
>>> ulong pcr;
>>> ulong dpdes; /* doorbell state (POWER8) */
>>> + unsigned long mpp_buffer; /* Micro Partition Prefetch buffer */
>> Just make this a void*?
> get_free_pages returns an unsigned long and free_pages accepts an
> unsigned long, so I was just avoiding the cast. Is the style in this
> case to do void* rather than unsigned long and cast it everywhere?
>
> In v4 of patch I've gone to void* anyway.
It's probably just a matter of personal taste, but I personally prefer
to keep pointers to memory locations in pointers.
>
>>> @@ -1516,6 +1540,37 @@ static int on_primary_thread(void)
>>> return 1;
>>> }
>>>
>>> +static void ppc_start_saving_l2_cache(struct kvmppc_vcore *vc)
>> Please use the "kvmppc" name space.
> ack, done.
>
>>> + phys_addr_t phy_addr, tmp;
>>> +
>>> + phy_addr = (phys_addr_t)virt_to_phys((void *)vc->mpp_buffer);
>>> +
>>> + tmp = phy_addr & PPC_MPPE_ADDRESS_MASK;
>> I would prefer if you give the variable a more telling name.
> ack.
>
>>> +
>>> + mtspr(SPRN_MPPR, tmp | PPC_MPPR_FETCH_ABORT);
>>> +
>>> + asm volatile(PPC_LOGMPP(R1) : : "r" (tmp | PPC_LOGMPP_LOG_L2));
>> Can you move this asm() into a static inline function in generic code
>> somewhere?
> okay. It seems the best place may be powerpc/include/asm/cache.h -
> simply because it deals with cache things. I'm open to better
> suggestions :)
>
>>> +
>>> + vc->mpp_buffer_is_valid = true;
>> Where does this ever get unset? And what point does this variable make?
>> Can't you just check on if (vc->mpp_buffer)?
> The problem with having moved the memory allocation for mpp_buffer to
> vcore setup is that we'll have vc->mpp_buffer != NULL but have some
> random contents in it, so when we first start executing the vcore, we
> shouldn't initiate prefetching (hence mpp_buffer_is_valid).
>
> If we point the prefetch engine to random memory contents, we get the
> most amazing array of incomprehensible illegal accesses :)
I see :). That makes a lot of sense indeed. Maybe rename the variable to
mpp_content_is_valid to indicate that we are not looking at a valid
buffer, but valid content?
Alex
^ permalink raw reply
* Re: [PATCH v4 0/2] Use the POWER8 Micro Partition Prefetch Engine in KVM HV
From: Alexander Graf @ 2014-07-28 12:34 UTC (permalink / raw)
To: Stewart Smith, linuxppc-dev, paulus, kvm-ppc
In-Reply-To: <1405657123-20087-1-git-send-email-stewart@linux.vnet.ibm.com>
On 18.07.14 06:18, Stewart Smith wrote:
> changes since v3:
> - use kvmppc namespace
> - MPP_BUFFER_ORDER of 3 not 4, as we only need 32k and it's already 32k aligned
> - split out kvmppc_vcore_create in separate patch
> - give a variable a better name: s/tmp/mpp_addr/
> - logmpp becomes static inline function
>
> Stewart Smith (2):
> Split out struct kvmppc_vcore creation to separate function
> Use the POWER8 Micro Partition Prefetch Engine in KVM HV on POWER8
I only realized just now that I've commented on v3 :). Sorry. I guess
the name isn't really important enough for a respin. If you come up with
a really nice one, I'd be happy to apply another patch that renames it.
Thanks, applied all to kvm-ppc-queue.
Alex
^ permalink raw reply
* Re: [PATCH 1/4] KVM: PPC: BOOK3S: PR: Emulate virtual timebase register
From: Alexander Graf @ 2014-07-28 13:25 UTC (permalink / raw)
To: Aneesh Kumar K.V, benh, paulus; +Cc: linuxppc-dev, kvm, kvm-ppc
In-Reply-To: <874mzyxati.fsf@linux.vnet.ibm.com>
On 06.06.14 18:27, Aneesh Kumar K.V wrote:
> Alexander Graf <agraf@suse.de> writes:
>
>> On 05.06.14 14:08, Aneesh Kumar K.V wrote:
>>> virtual time base register is a per VM, per cpu register that needs
>>> to be saved and restored on vm exit and entry. Writing to VTB is not
>>> allowed in the privileged mode.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>> For some reason BUG() doesn't always trigger the "execution stops here"
>> logic in gcc. So I've squashed this patch into yours.
>>
>>
>> Alex
>>
>>
>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> index 3e7085d..99de6ad 100644
>> --- a/arch/powerpc/include/asm/reg.h
>> +++ b/arch/powerpc/include/asm/reg.h
>> @@ -1206,6 +1206,7 @@ static inline unsigned long mfvtb (void)
>> * capture that.
>> */
>> BUG();
>> + return 0;
>> }
>>
>> #ifdef __powerpc64__
> you can then drop the include header change. ie,
>
> #include <asm/bug.h>
Yeah, things are even worse than I thought. I've eventually squashed the
following in. a NOP'ed mfspr() won't keep the branch from blr'ing, so
we'd never hit the BUG() anyway.
Alex
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1f34ef7..c8f3381 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -25,7 +25,6 @@
#ifdef CONFIG_8xx
#include <asm/reg_8xx.h>
#endif /* CONFIG_8xx */
-#include <asm/bug.h>
#define MSR_SF_LG 63 /* Enable 64 bit mode */
#define MSR_ISF_LG 61 /* Interrupt 64b mode valid on
630 */
@@ -1210,12 +1209,6 @@ static inline unsigned long mfvtb (void)
if (cpu_has_feature(CPU_FTR_ARCH_207S))
return mfspr(SPRN_VTB);
#endif
- /*
- * The above mfspr will be a no-op on anything before Power8
- * That can result in random values returned. We need to
- * capture that.
- */
- BUG();
return 0;
}
^ permalink raw reply related
* Re: [PATCH] KVM: PPC: BOOK3S: HV: Update compute_tlbie_rb to handle 16MB base page
From: Alexander Graf @ 2014-07-28 14:09 UTC (permalink / raw)
To: Aneesh Kumar K.V, benh, paulus; +Cc: linuxppc-dev, kvm, kvm-ppc
In-Reply-To: <1403876103-32459-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
On 27.06.14 15:35, Aneesh Kumar K.V wrote:
> When calculating the lower bits of AVA field, use the shift
> count based on the base page size. Also add the missing segment
> size and remove stale comment.
>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
Thanks, applied to kvm-ppc-queue.
Alex
^ permalink raw reply
* [PATCH] powerpc/e6500: Work around erratum A-008139
From: Scott Wood @ 2014-07-28 15:46 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Scott Wood
Erratum A-008139 can cause duplicate TLB entries if an indirect
entry is overwritten using tlbwe while the other thread is using it to
do a lookup. Work around this by using tlbilx to invalidate prior
to overwriting.
To avoid the need to save another register to hold MAS1 during the
workaround code, TID clearing has been moved from tlb_miss_kernel_e6500
until after the SMT section.
Signed-off-by: Scott Wood <scottwood@freescale.com>
---
arch/powerpc/mm/tlb_low_64e.S | 68 +++++++++++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/mm/tlb_low_64e.S b/arch/powerpc/mm/tlb_low_64e.S
index 57c4d66..89bf95b 100644
--- a/arch/powerpc/mm/tlb_low_64e.S
+++ b/arch/powerpc/mm/tlb_low_64e.S
@@ -299,7 +299,9 @@ itlb_miss_fault_bolted:
* r10 = crap (free to use)
*/
tlb_miss_common_e6500:
-BEGIN_FTR_SECTION
+ crmove cr2*4+2,cr0*4+2 /* cr2.eq != 0 if kernel address */
+
+BEGIN_FTR_SECTION /* CPU_FTR_SMT */
/*
* Search if we already have an indirect entry for that virtual
* address, and if we do, bail out.
@@ -324,17 +326,62 @@ BEGIN_FTR_SECTION
b 1b
.previous
+ /*
+ * Erratum A-008139 says that we can't use tlbwe to change
+ * an indirect entry in any way (including replacing or
+ * invalidating) if the other thread could be in the process
+ * of a lookup. The workaround is to invalidate the entry
+ * with tlbilx before overwriting.
+ */
+
+ lbz r15,TCD_ESEL_NEXT(r11)
+ rlwinm r10,r15,16,0xff0000
+ oris r10,r10,MAS0_TLBSEL(1)@h
+ mtspr SPRN_MAS0,r10
+ isync
+ tlbre
mfspr r15,SPRN_MAS1
- mfspr r10,SPRN_MAS2
+ andis. r15,r15,MAS1_VALID@h
+ beq 5f
+
+BEGIN_FTR_SECTION_NESTED(532)
+ mfspr r10,SPRN_MAS8
+ rlwinm r10,r10,0,0x80000fff /* tgs,tlpid -> sgs,slpid */
+ mtspr SPRN_MAS5,r10
+END_FTR_SECTION_NESTED(CPU_FTR_EMB_HV,CPU_FTR_EMB_HV,532)
- tlbsx 0,r16
- mtspr SPRN_MAS2,r10
mfspr r10,SPRN_MAS1
- mtspr SPRN_MAS1,r15
+ rlwinm r15,r10,0,0x3fff0000 /* tid -> spid */
+ rlwimi r15,r10,20,0x00000003 /* ind,ts -> sind,sas */
+ mfspr r10,SPRN_MAS6
+ mtspr SPRN_MAS6,r15
+
+ mfspr r15,SPRN_MAS2
+ isync
+ tlbilxva 0,r15
+ isync
+
+ mtspr SPRN_MAS6,r10
- andis. r10,r10,MAS1_VALID@h
+5:
+BEGIN_FTR_SECTION_NESTED(532)
+ li r10,0
+ mtspr SPRN_MAS8,r10
+ mtspr SPRN_MAS5,r10
+END_FTR_SECTION_NESTED(CPU_FTR_EMB_HV,CPU_FTR_EMB_HV,532)
+
+ tlbsx 0,r16
+ mfspr r10,SPRN_MAS1
+ andis. r15,r10,MAS1_VALID@h
bne tlb_miss_done_e6500
-END_FTR_SECTION_IFSET(CPU_FTR_SMT)
+FTR_SECTION_ELSE
+ mfspr r10,SPRN_MAS1
+ALT_FTR_SECTION_END_IFSET(CPU_FTR_SMT)
+
+ oris r10,r10,MAS1_VALID@h
+ beq cr2,4f
+ rlwinm r10,r10,0,16,1 /* Clear TID */
+4: mtspr SPRN_MAS1,r10
/* Now, we need to walk the page tables. First check if we are in
* range.
@@ -410,12 +457,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_SMT)
rfi
tlb_miss_kernel_e6500:
- mfspr r10,SPRN_MAS1
ld r14,PACA_KERNELPGD(r13)
- cmpldi cr0,r15,8 /* Check for vmalloc region */
- rlwinm r10,r10,0,16,1 /* Clear TID */
- mtspr SPRN_MAS1,r10
- beq+ tlb_miss_common_e6500
+ cmpldi cr1,r15,8 /* Check for vmalloc region */
+ beq+ cr1,tlb_miss_common_e6500
tlb_miss_fault_e6500:
tlb_unlock_e6500
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for SPE/FP/AltiVec int numbers
From: Scott Wood @ 2014-07-28 22:42 UTC (permalink / raw)
To: Caraman Mihai Claudiu-B02008
Cc: linuxppc-dev@lists.ozlabs.org, Alexander Graf,
kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
In-Reply-To: <1941bf37d0bb4e2cba64add056a5986c@BY2PR03MB508.namprd03.prod.outlook.com>
On Mon, 2014-07-28 at 03:54 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Saturday, July 26, 2014 3:11 AM
> > To: Caraman Mihai Claudiu-B02008
> > Cc: Alexander Graf; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org;
> > linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [PATCH 1/6 v2] KVM: PPC: Book3E: Use common defines for
> > SPE/FP/AltiVec int numbers
> >
> > On Thu, 2014-07-24 at 04:16 -0500, Caraman Mihai Claudiu-B02008 wrote:
> > > Scott, Alex's request to define SPE handlers only for e500v2 implies
> > changes
> > > in 32-bit FSL kernel to have exclusive configurations for e200/e500v2
> > and
> > > e500mc/e5500. We would probably need something like this, what's your
> > take on it?
> >
> > That is already a compile-time decision.
>
> Yes, but is not fully implemented.
We might be missing some kconfig language to prohibit e500v2 boards from
being enabled in an e500mc kernel, but if you try to do so it won't work
(except for obsolete hacks like running QEMU's mpc8544ds platform with
"-cpu e500mc").
> > > diff --git a/arch/powerpc/kernel/head_fsl_booke.S
> > b/arch/powerpc/kernel/head_fsl_booke.S
> > > index b497188..9d41015 100644
> > > --- a/arch/powerpc/kernel/head_fsl_booke.S
> > > +++ b/arch/powerpc/kernel/head_fsl_booke.S
> > > @@ -613,6 +613,8 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
> > > mfspr r10, SPRN_SPRG_RSCRATCH0
> > > b InstructionStorage
> > >
> > > +/* Define SPE handlers only for e500v2 and e200 */
> > > +#ifndef CONFIG_PPC_E500MC
> > > #ifdef CONFIG_SPE
> > > /* SPE Unavailable */
> > > START_EXCEPTION(SPEUnavailable)
> > > @@ -626,7 +628,9 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
> > > EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
> > > unknown_exception, EXC_XFER_EE)
> > > #endif /* CONFIG_SPE */
> > > +#endif
> >
> > This is redundant:
> >
> > config SPE
> > bool "SPE Support"
> > depends on E200 || (E500 && !PPC_E500MC)
> > default y
>
> I see what you mean, but it's not redundant.
OK, I didn't realize there was an #else that wasn't included in the
context. It would have been nice if the comment at the end were
"!CONFIG_SPE" rather than "CONFIG_SPE".
> Alex was looking to remove SPE
> handlers for e500mc+ and the proposal handled !SPE case. In the new
> light I find this variant more readable:
>
> +/* Define SPE handlers for e200 and e500v2 */
> #ifdef CONFIG_SPE
> /* SPE Unavailable */
> START_EXCEPTION(SPEUnavailable)
> @@ -622,11 +623,13 @@ END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
> b fast_exception_return
> 1: addi r3,r1,STACK_FRAME_OVERHEAD
> EXC_XFER_EE_LITE(0x2010, KernelSPE)
> -#else
> +#elif defined(CONFIG_E200) || \
> + (defined(CONFIG_E500) && !defined(CONFIG_PPC_E500MC))
> EXCEPTION(0x2020, SPE_ALTIVEC_UNAVAIL, SPEUnavailable, \
> unknown_exception, EXC_XFER_EE)
> #endif /* CONFIG_SPE */
Yes, or better define a CONFIG_SPE_POSSIBLE so that the list only has to
exist in one place, and the intent is clearer.
> > > diff --git a/arch/powerpc/kernel/cputable.c
> > b/arch/powerpc/kernel/cputable.c
> > > index c1faade..3ab65c2 100644
> > > --- a/arch/powerpc/kernel/cputable.c
> > > +++ b/arch/powerpc/kernel/cputable.c
> > > @@ -2030,6 +2030,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
> > > #endif /* CONFIG_PPC32 */
> > > #ifdef CONFIG_E500
> > > #ifdef CONFIG_PPC32
> > > +#ifndef CONFIG_PPC_E500MC
> > > { /* e500 */
> > > .pvr_mask = 0xffff0000,
> > > .pvr_value = 0x80200000,
> > > @@ -2069,6 +2070,7 @@ static struct cpu_spec __initdata cpu_specs[] = {
> > > .machine_check = machine_check_e500,
> > > .platform = "ppc8548",
> > > },
> > > +#endif /* !CONFIG_PPC_E500MC */
> > > { /* e500mc */
> > > .pvr_mask = 0xffff0000,
> > > .pvr_value = 0x80230000,
> > >
> >
> > This looks a bit strange -- e500v2 gets excluded if PPC_E500MC, but
> > e500mc gets included in !PPC_E500MC?
>
> Right, my main purpose was to get rid of __setup_e500_ivors on PPC_E500MC
> which refers SPEUnavailable. I will add an #else to exclude e500mc.
>
> The "generic E500 PPC" default cpu advertises PPC_FEATURE_HAS_SPE_COMP.
> Do we want to excluded it if PPC_E500MC? Is this cpu useful without cpu_setup()
> and if so why don't we also have a default for 64-bit?
I don't think that default will do anything useful.
-Scott
^ permalink raw reply
* Re: [PATCH 1/4] KVM: PPC: BOOK3S: PR: Emulate virtual timebase register
From: Stewart Smith @ 2014-07-28 22:59 UTC (permalink / raw)
To: Alexander Graf, Aneesh Kumar K.V, benh, paulus; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <53D64F4C.80208@suse.de>
Alexander Graf <agraf@suse.de> writes:
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -25,7 +25,6 @@
> #ifdef CONFIG_8xx
> #include <asm/reg_8xx.h>
> #endif /* CONFIG_8xx */
> -#include <asm/bug.h>
>
> #define MSR_SF_LG 63 /* Enable 64 bit mode */
> #define MSR_ISF_LG 61 /* Interrupt 64b mode valid on
> 630 */
> @@ -1210,12 +1209,6 @@ static inline unsigned long mfvtb (void)
> if (cpu_has_feature(CPU_FTR_ARCH_207S))
> return mfspr(SPRN_VTB);
> #endif
> - /*
> - * The above mfspr will be a no-op on anything before Power8
> - * That can result in random values returned. We need to
> - * capture that.
> - */
> - BUG();
> return 0;
> }
(i missed CCing aneesh on this mail in reply to the build robot, so
inserting the same reply here)
the only place that calls it also does the cpu_has_feature() check and
returns 0 ifndef CONFIG_PPC_BOOK3S_64 or !cpu_has_feature().
Looking get_vtb (the only caller) and the places it's called (as well as
PowerISA 2.07) I think in the emulation code we're missing invoking the
"system privileged instruction error handler" as the VTB SPR has spr bit
0 set to 1 (page 107 of PowerISA 2.07, mfspr docs).
That being said... any guest sholud do the cpu_has_feature check
themselves, so this probably isn't an issue in the real world.
Certainly the host really shouldn't BUG() for what is really a guest
issue (actually.. this would be a good DoS attack on < Power8 host).
Reviewed-by: Stewart Smith <stewart@linux.vnet.ibm.com>
^ permalink raw reply
* [PATCH] powerpc/pci: reorder pci bus/bridge unregistration during PHB removal
From: Tyrel Datwyler @ 2014-07-29 0:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: nfont, paulus, Tyrel Datwyler
Commit bcdde7e made __sysfs_remove_dir() recursive and introduced a BUG_ON
during PHB removal while attempting to delete the power managment attribute
group of the bus. This is a result of tearing the bridge and bus devices down
out of order in remove_phb_dynamic. Since, the the bus resides below the bridge
in the sysfs device tree it should be torn down first.
This patch simply moves the device_unregister call for the PHB bridge device
after the device_unregister call for the PHB bus.
Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
---
arch/powerpc/platforms/pseries/pci_dlpar.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/pci_dlpar.c b/arch/powerpc/platforms/pseries/pci_dlpar.c
index 203cbf0..89e2381 100644
--- a/arch/powerpc/platforms/pseries/pci_dlpar.c
+++ b/arch/powerpc/platforms/pseries/pci_dlpar.c
@@ -118,10 +118,10 @@ int remove_phb_dynamic(struct pci_controller *phb)
}
}
- /* Unregister the bridge device from sysfs and remove the PCI bus */
- device_unregister(b->bridge);
+ /* Remove the PCI bus and unregister the bridge device from sysfs */
phb->bus = NULL;
pci_remove_bus(b);
+ device_unregister(b->bridge);
/* Now release the IO resource */
if (res->flags & IORESOURCE_IO)
--
1.7.12.4
^ permalink raw reply related
* Re: [PATCH] powerpc/pci: reorder pci bus/bridge unregistration during PHB removal
From: Michael Ellerman @ 2014-07-29 1:44 UTC (permalink / raw)
To: Tyrel Datwyler; +Cc: nfont, linuxppc-dev, paulus
In-Reply-To: <1406594911-7097-1-git-send-email-tyreld@linux.vnet.ibm.com>
On Mon, 2014-07-28 at 20:48 -0400, Tyrel Datwyler wrote:
> Commit bcdde7e made __sysfs_remove_dir() recursive and introduced a BUG_ON
> during PHB removal while attempting to delete the power managment attribute
> group of the bus. This is a result of tearing the bridge and bus devices down
> out of order in remove_phb_dynamic. Since, the the bus resides below the bridge
> in the sysfs device tree it should be torn down first.
>
> This patch simply moves the device_unregister call for the PHB bridge device
> after the device_unregister call for the PHB bus.
>
> Signed-off-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
That commit went into 3.13, so shouldn't this be CC to stable?
And also:
Fixes: bcdde7e221a8 ("sysfs: make __sysfs_remove_dir() recursive")
cheers
^ permalink raw reply
* Re: [PATCH v4] arm64, ia64, ppc, s390, sh, tile, um, x86, mm: Remove default gate area
From: Michael Ellerman @ 2014-07-29 2:19 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-ia64, linux-sh, Catalin Marinas, Heiko Carstens, linux-mm,
Paul Mackerras, H. Peter Anvin, linux-arch, linux-s390,
Richard Weinberger, x86, Ingo Molnar, Fenghua Yu,
user-mode-linux-devel, Will Deacon, Jeff Dike, Chris Metcalf,
Thomas Gleixner, linux-arm-kernel, Tony Luck, Nathan Lynch,
linux-kernel, Martin Schwidefsky, linux390, Andrew Morton,
linuxppc-dev
In-Reply-To: <6435254cc74d6e9172931f27be3854d522ad299b.1406232860.git.luto@amacapital.net>
On Thu, 2014-07-24 at 13:56 -0700, Andy Lutomirski wrote:
> The core mm code will provide a default gate area based on
> FIXADDR_USER_START and FIXADDR_USER_END if
> !defined(__HAVE_ARCH_GATE_AREA) && defined(AT_SYSINFO_EHDR).
>
> This default is only useful for ia64. arm64, ppc, s390, sh, tile,
> 64-bit UML, and x86_32 have their own code just to disable it. arm,
> 32-bit UML, and x86_64 have gate areas, but they have their own
> implementations.
>
> This gets rid of the default and moves the code into ia64.
>
> This should save some code on architectures without a gate area: it's
> now possible to inline the gate_area functions in the default case.
Looks good to me. Booted and everything seems happy, I still have a vdso etc.
Tested-by: Michael Ellerman <mpe@ellerman.id.au> (for powerpc)
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: thp: Add write barrier after updating the valid bit
From: Aneesh Kumar K.V @ 2014-07-29 6:55 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, paulus
In-Reply-To: <1406066112.22200.28.camel@pasglop>
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Wed, 2014-07-23 at 00:23 +0530, Aneesh Kumar K.V wrote:
>> > A better place for this would be right before the last write to the PMD
>> > (that's also clearing BUSY) in __hash_page_thp(). Basically, it's the
>> > normal lock ordering that's missing here, nothing specific to
>> > mark_hpte_slot_valid() but instead, any state relative to the BUSY bit
>> > in the PMD (including the actual hash writes in update_pp etc...)
>> >
>>
>> IIUC updatepp already have required barriers. ie in updatepp we do tlbie
>> which should take care of the ordering right ?
>
> Only if it succeeds but that doesn't matter, I'd rather we get the
> semantics right. The clearing of the busy bit is an unlock, it should
> have the appropriate barriers like it does in other variants of hash
> page.
ok
>>
>> Now the reason i moved that spm_wmb() to mark_hpte_slot_valid was to
>> pair it with smb_rmb() in get_hpte_slot_array().
>
> Which is also probably in the wrong place. Care to explain to me the
> exact relationship ?
We want to make sure for usage like below we don't reorder the load.
if (pmd_trans_huge(*pmdp)){
get_hpte_slot_array(pmdp)
}
-aneesh
^ 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