* Re: [PATCH -next] powerpc: Convert to DEFINE_SHOW_ATTRIBUTE
From: Paul Mackerras @ 2020-09-02 4:51 UTC (permalink / raw)
To: Qinglang Miao; +Cc: Greg Kroah-Hartman, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <20200716090712.14375-1-miaoqinglang@huawei.com>
On Thu, Jul 16, 2020 at 05:07:12PM +0800, Qinglang Miao wrote:
> From: Chen Huang <chenhuang5@huawei.com>
>
> Use DEFINE_SHOW_ATTRIBUTE macro to simplify the code.
>
> Signed-off-by: Chen Huang <chenhuang5@huawei.com>
For the arch/powerpc/kvm part:
Acked-by: Paul Mackerras <paulus@ozlabs.org>
I expect Michael Ellerman will take the patch through his tree.
Paul.
^ permalink raw reply
* Re: [PATCH 2/7] powerpc/watchpoint/kvm: Add infrastructure to support 2nd DAWR
From: Ravi Bangoria @ 2020-09-02 5:09 UTC (permalink / raw)
To: Paul Mackerras
Cc: christophe.leroy, Ravi Bangoria, mikey, rogealve, kvm,
linux-kernel, npiggin, kvm-ppc, linux-kselftest, jniethe5,
pedromfc, pbonzini, linuxppc-dev
In-Reply-To: <20200902020124.GB272502@thinks.paulus.ozlabs.org>
Hi Paul,
On 9/2/20 7:31 AM, Paul Mackerras wrote:
> On Thu, Jul 23, 2020 at 03:50:53PM +0530, Ravi Bangoria wrote:
>> kvm code assumes single DAWR everywhere. Add code to support 2nd DAWR.
>> DAWR is a hypervisor resource and thus H_SET_MODE hcall is used to set/
>> unset it. Introduce new case H_SET_MODE_RESOURCE_SET_DAWR1 for 2nd DAWR.
>
> Is this the same interface as will be defined in PAPR and available
> under PowerVM, or is it a new/different interface for KVM?
Yes, kvm hcall interface for 2nd DAWR is same as PowerVM, as defined in PAPR.
>
>> Also, kvm will support 2nd DAWR only if CPU_FTR_DAWR1 is set.
>
> In general QEMU wants to be able to control all aspects of the virtual
> machine presented to the guest, meaning that just because a host has a
> particular hardware capability does not mean we should automatically
> present that capability to the guest.
>
> In this case, QEMU will want a way to control whether the guest sees
> the availability of the second DAWR/X registers or not, i.e. whether a
> H_SET_MODE to set DAWR[X]1 will succeed or fail.
Patch #3 adds new kvm capability KVM_CAP_PPC_DAWR1 that can be checked
by Qemu. Also, as suggested by David in Qemu patch[1], I'm planning to
add new machine capability in Qemu:
-machine cap-dawr1=ON/OFF
cap-dawr1 will be default ON when PPC_FEATURE2_ARCH_3_10 is set and OFF
otherwise.
Is this correct approach?
[1]: https://lore.kernel.org/kvm/20200724045613.GA8983@umbus.fritz.box
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH 1/7] powerpc/watchpoint/kvm: Rename current DAWR macros and variables
From: Ravi Bangoria @ 2020-09-02 5:09 UTC (permalink / raw)
To: Paul Mackerras
Cc: christophe.leroy, Ravi Bangoria, mikey, rogealve, kvm,
linux-kernel, npiggin, kvm-ppc, linux-kselftest, jniethe5,
pedromfc, pbonzini, linuxppc-dev
In-Reply-To: <20200902014908.GA272502@thinks.paulus.ozlabs.org>
Hi Paul,
On 9/2/20 7:19 AM, Paul Mackerras wrote:
> On Thu, Jul 23, 2020 at 03:50:52PM +0530, Ravi Bangoria wrote:
>> Power10 is introducing second DAWR. Use real register names (with
>> suffix 0) from ISA for current macros and variables used by kvm.
>
> Most of this looks fine, but I think we should not change the existing
> names in arch/powerpc/include/uapi/asm/kvm.h (and therefore also
> Documentation/virt/kvm/api.rst).
Missed that I'm changing uapi. I'll rename only those macros/variables
which are not uapi.
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH 2/7] powerpc/watchpoint/kvm: Add infrastructure to support 2nd DAWR
From: Ravi Bangoria @ 2020-09-02 5:12 UTC (permalink / raw)
To: paulus
Cc: christophe.leroy, Ravi Bangoria, mikey, rogealve, kvm,
linux-kernel, npiggin, kvm-ppc, linux-kselftest, jniethe5,
pedromfc, pbonzini, linuxppc-dev
In-Reply-To: <20200723102058.312282-3-ravi.bangoria@linux.ibm.com>
Hi Paul,
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index 33793444144c..03f401d7be41 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -538,6 +538,8 @@ struct hv_guest_state {
> s64 tb_offset;
> u64 dawr0;
> u64 dawrx0;
> + u64 dawr1;
> + u64 dawrx1;
> u64 ciabr;
> u64 hdec_expiry;
> u64 purr;
After this struct, there is a macro HV_GUEST_STATE_VERSION, I guess that
also needs to be incremented because I'm adding new members in the struct?
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH 0/7] powerpc/watchpoint: 2nd DAWR kvm enablement + selftests
From: Ravi Bangoria @ 2020-09-02 5:13 UTC (permalink / raw)
To: Paul Mackerras
Cc: christophe.leroy, Ravi Bangoria, mikey, rogealve, kvm,
linux-kernel, npiggin, kvm-ppc, linux-kselftest, jniethe5,
pedromfc, pbonzini, linuxppc-dev
In-Reply-To: <20200902023259.GC272502@thinks.paulus.ozlabs.org>
Hi Paul,
On 9/2/20 8:02 AM, Paul Mackerras wrote:
> On Thu, Jul 23, 2020 at 03:50:51PM +0530, Ravi Bangoria wrote:
>> Patch #1, #2 and #3 enables p10 2nd DAWR feature for Book3S kvm guest. DAWR
>> is a hypervisor resource and thus H_SET_MODE hcall is used to set/unset it.
>> A new case H_SET_MODE_RESOURCE_SET_DAWR1 is introduced in H_SET_MODE hcall
>> for setting/unsetting 2nd DAWR. Also, new capability KVM_CAP_PPC_DAWR1 has
>> been added to query 2nd DAWR support via kvm ioctl.
>>
>> This feature also needs to be enabled in Qemu to really use it. I'll reply
>> link to qemu patches once I post them in qemu-devel mailing list.
>>
>> Patch #4, #5, #6 and #7 adds selftests to test 2nd DAWR.
>
> If/when you resubmit these patches, please split the KVM patches into
> a separate series, since the KVM patches would go via my tree whereas
> I expect the selftests/powerpc patches would go through Michael
> Ellerman's tree.
Sure. Will split it.
Thanks,
Ravi
^ permalink raw reply
* Re: [PATCH 0/2] link vdso with linker
From: Nathan Chancellor @ 2020-09-02 5:21 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
linux-kernel, Nicholas Piggin, clang-built-linux, Paul Mackerras,
linuxppc-dev
In-Reply-To: <20200901222523.1941988-1-ndesaulniers@google.com>
On Tue, Sep 01, 2020 at 03:25:21PM -0700, Nick Desaulniers wrote:
> Kees Cook is working on series that adds --orphan-section=warn to arm,
> arm64, and x86. I noticed that ppc vdso were still using cc-ldoption
> for these which I removed. It seems this results in that flag being
> silently dropped.
>
> I'm very confident with the first patch, but the second needs closer
> review around the error mentioned below the fold related to the .got
> section.
>
> Nick Desaulniers (2):
> powerpc/vdso64: link vdso64 with linker
> powerpc/vdso32: link vdso64 with linker
>
> arch/powerpc/include/asm/vdso.h | 17 ++---------------
> arch/powerpc/kernel/vdso32/Makefile | 7 +++++--
> arch/powerpc/kernel/vdso32/vdso32.lds.S | 3 ++-
> arch/powerpc/kernel/vdso64/Makefile | 8 ++++++--
> arch/powerpc/kernel/vdso64/vdso64.lds.S | 1 -
> 5 files changed, 15 insertions(+), 21 deletions(-)
>
> --
> 2.28.0.402.g5ffc5be6b7-goog
>
ppc44x_defconfig and powernv_defconfig start failing with this series
when LD=ld.lld is used.
$ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- LLVM=1 O=out/ppc32 distclean ppc44x_defconfig uImage
ld.lld: error: relocation R_PPC_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso32/datapage.o
>>> referenced by arch/powerpc/kernel/vdso32/gettimeofday.o:(__kernel_gettimeofday)
ld.lld: error: relocation R_PPC_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso32/datapage.o
>>> referenced by arch/powerpc/kernel/vdso32/gettimeofday.o:(__kernel_clock_gettime)
ld.lld: error: relocation R_PPC_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso32/datapage.o
>>> referenced by arch/powerpc/kernel/vdso32/gettimeofday.o:(__kernel_clock_getres)
ld.lld: error: relocation R_PPC_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso32/datapage.o
>>> referenced by arch/powerpc/kernel/vdso32/gettimeofday.o:(__kernel_time)
...
$ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- LLVM=1 O=out/ppc64le distclean powernv_defconfig zImage.epapr
ld.lld: error: relocation R_PPC64_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso64/datapage.o
>>> referenced by arch/powerpc/kernel/vdso64/gettimeofday.o:(__kernel_gettimeofday)
ld.lld: error: relocation R_PPC64_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso64/datapage.o
>>> referenced by arch/powerpc/kernel/vdso64/gettimeofday.o:(__kernel_clock_gettime)
ld.lld: error: relocation R_PPC64_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso64/datapage.o
>>> referenced by arch/powerpc/kernel/vdso64/gettimeofday.o:(__kernel_clock_getres)
ld.lld: error: relocation R_PPC64_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso64/datapage.o
>>> referenced by arch/powerpc/kernel/vdso64/gettimeofday.o:(__kernel_time)
ld.lld: error: relocation R_PPC64_REL16_LO cannot be used against symbol __kernel_datapage_offset; recompile with -fPIC
>>> defined in arch/powerpc/kernel/vdso64/datapage.o
>>> referenced by arch/powerpc/kernel/vdso64/cacheflush.o:(__kernel_sync_dicache)
...
We need Fangrui's patch to fix ppc44x_defconfig:
https://lore.kernel.org/lkml/20200205005054.k72fuikf6rwrgfe4@google.com/
That exact same fix is needed in arch/powerpc/kernel/vdso64/datapage.S
to fix powernv_defconfig.
Cheers,
Nathan
^ permalink raw reply
* Re: [PATCH v1 08/10] powerpc/pseries/iommu: Add ddw_property_create() and refactor enable_ddw()
From: Leonardo Bras @ 2020-09-02 5:27 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1a469384-91ad-81f0-2a42-4c985cbc92da@ozlabs.ru>
On Mon, 2020-08-31 at 14:34 +1000, Alexey Kardashevskiy wrote:
>
> On 29/08/2020 01:25, Leonardo Bras wrote:
> > On Mon, 2020-08-24 at 15:07 +1000, Alexey Kardashevskiy wrote:
> > > On 18/08/2020 09:40, Leonardo Bras wrote:
> > > > Code used to create a ddw property that was previously scattered in
> > > > enable_ddw() is now gathered in ddw_property_create(), which deals with
> > > > allocation and filling the property, letting it ready for
> > > > of_property_add(), which now occurs in sequence.
> > > >
> > > > This created an opportunity to reorganize the second part of enable_ddw():
> > > >
> > > > Without this patch enable_ddw() does, in order:
> > > > kzalloc() property & members, create_ddw(), fill ddwprop inside property,
> > > > ddw_list_add(), do tce_setrange_multi_pSeriesLP_walk in all memory,
> > > > of_add_property().
> > > >
> > > > With this patch enable_ddw() does, in order:
> > > > create_ddw(), ddw_property_create(), of_add_property(), ddw_list_add(),
> > > > do tce_setrange_multi_pSeriesLP_walk in all memory.
> > > >
> > > > This change requires of_remove_property() in case anything fails after
> > > > of_add_property(), but we get to do tce_setrange_multi_pSeriesLP_walk
> > > > in all memory, which looks the most expensive operation, only if
> > > > everything else succeeds.
> > > >
> > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > > > ---
> > > > arch/powerpc/platforms/pseries/iommu.c | 97 +++++++++++++++-----------
> > > > 1 file changed, 57 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > > > index 4031127c9537..3a1ef02ad9d5 100644
> > > > --- a/arch/powerpc/platforms/pseries/iommu.c
> > > > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > > > @@ -1123,6 +1123,31 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
> > > > ret);
> > > > }
> > > >
> > > > +static int ddw_property_create(struct property **ddw_win, const char *propname,
> > >
> > > @propname is always the same, do you really want to pass it every time?
> >
> > I think it reads better, like "create a ddw property with this name".
>
> This reads as "there are at least two ddw properties".
>
> > Also, it makes possible to create ddw properties with other names, in
> > case we decide to create properties with different names depending on
> > the window created.
>
> It is one window at any given moment, why call it different names... I
> get the part that it is not always "direct" anymore but still...
>
It seems the case as one of the options you suggested on patch [09/10]
>> I suspect it breaks kexec (from older kernel to this one) so you
>> either need to check for both DT names or just keep the old one.
>
> > Also, it's probably optimized / inlined at this point.
> > Is it ok doing it like this?
> >
> > > > + u32 liobn, u64 dma_addr, u32 page_shift, u32 window_shift)
> > > > +{
> > > > + struct dynamic_dma_window_prop *ddwprop;
> > > > + struct property *win64;
> > > > +
> > > > + *ddw_win = win64 = kzalloc(sizeof(*win64), GFP_KERNEL);
> > > > + if (!win64)
> > > > + return -ENOMEM;
> > > > +
> > > > + win64->name = kstrdup(propname, GFP_KERNEL);
> > >
> > > Not clear why "win64->name = DIRECT64_PROPNAME" would not work here, the
> > > generic OF code does not try kfree() it but it is probably out of scope
> > > here.
> >
> > Yeah, I had that question too.
> > Previous code was like that, and I as trying not to mess too much on
> > how it's done.
> >
> > > > + ddwprop = kzalloc(sizeof(*ddwprop), GFP_KERNEL);
> > > > + win64->value = ddwprop;
> > > > + win64->length = sizeof(*ddwprop);
> > > > + if (!win64->name || !win64->value)
> > > > + return -ENOMEM;
> > >
> > > Up to 2 memory leaks here. I see the cleanup at "out_free_prop:" but
> > > still looks fragile. Instead you could simply return win64 as the only
> > > error possible here is -ENOMEM and returning NULL is equally good.
> >
> > I agree. It's better if this function have it's own cleaning routine.
> > It will be fixed for next version.
> >
> > >
> > > > +
> > > > + ddwprop->liobn = cpu_to_be32(liobn);
> > > > + ddwprop->dma_base = cpu_to_be64(dma_addr);
> > > > + ddwprop->tce_shift = cpu_to_be32(page_shift);
> > > > + ddwprop->window_shift = cpu_to_be32(window_shift);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > /*
> > > > * If the PE supports dynamic dma windows, and there is space for a table
> > > > * that can map all pages in a linear offset, then setup such a table,
> > > > @@ -1140,12 +1165,11 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > struct ddw_query_response query;
> > > > struct ddw_create_response create;
> > > > int page_shift;
> > > > - u64 max_addr;
> > > > + u64 max_addr, win_addr;
> > > > struct device_node *dn;
> > > > u32 ddw_avail[DDW_APPLICABLE_SIZE];
> > > > struct direct_window *window;
> > > > - struct property *win64;
> > > > - struct dynamic_dma_window_prop *ddwprop;
> > > > + struct property *win64 = NULL;
> > > > struct failed_ddw_pdn *fpdn;
> > > > bool default_win_removed = false;
> > > >
> > > > @@ -1244,38 +1268,34 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > goto out_failed;
> > > > }
> > > > len = order_base_2(max_addr);
> > > > - win64 = kzalloc(sizeof(struct property), GFP_KERNEL);
> > > > - if (!win64) {
> > > > - dev_info(&dev->dev,
> > > > - "couldn't allocate property for 64bit dma window\n");
> > > > +
> > > > + ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
> > > > + if (ret != 0)
> > >
> > > It is usually just "if (ret)"
> >
> > It was previously like that, and all query_ddw() checks return value
> > this way.
>
> ah I see.
>
> > Should I update them all or just this one?
>
> Pick one variant and make sure all new lines use just that. In this
> patch you add both variants. Thanks,
Ok, I will do that from now on.
Thanks!
^ permalink raw reply
* Re: [PATCH v1 09/10] powerpc/pseries/iommu: Make use of DDW even if it does not map the partition
From: Leonardo Bras @ 2020-09-02 6:11 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley,
Thiago Jung Bauermann, Ram Pai, Brian King,
Murilo Fossa Vicentini, David Dai
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <d309c872-c574-4eb5-0e6e-0a36f73e168e@ozlabs.ru>
On Mon, 2020-08-31 at 14:35 +1000, Alexey Kardashevskiy wrote:
>
> On 29/08/2020 04:36, Leonardo Bras wrote:
> > On Mon, 2020-08-24 at 15:17 +1000, Alexey Kardashevskiy wrote:
> > > On 18/08/2020 09:40, Leonardo Bras wrote:
> > > > As of today, if the biggest DDW that can be created can't map the whole
> > > > partition, it's creation is skipped and the default DMA window
> > > > "ibm,dma-window" is used instead.
> > > >
> > > > DDW is 16x bigger than the default DMA window,
> > >
> > > 16x only under very specific circumstances which are
> > > 1. phyp
> > > 2. sriov
> > > 3. device class in hmc (or what that priority number is in the lpar config).
> >
> > Yeah, missing details.
> >
> > > > having the same amount of
> > > > pages, but increasing the page size to 64k.
> > > > Besides larger DMA window,
> > >
> > > "Besides being larger"?
> >
> > You are right there.
> >
> > > > it performs better for allocations over 4k,
> > >
> > > Better how?
> >
> > I was thinking for allocations larger than (512 * 4k), since >2
> > hypercalls are needed here, and for 64k pages would still be just 1
> > hypercall up to (512 * 64k).
> > But yeah, not the usual case anyway.
>
> Yup.
>
>
> > > > so it would be nice to use it instead.
> > >
> > > I'd rather say something like:
> > > ===
> > > So far we assumed we can map the guest RAM 1:1 to the bus which worked
> > > with a small number of devices. SRIOV changes it as the user can
> > > configure hundreds VFs and since phyp preallocates TCEs and does not
> > > allow IOMMU pages bigger than 64K, it has to limit the number of TCEs
> > > per a PE to limit waste of physical pages.
> > > ===
> >
> > I mixed this in my commit message, it looks like this:
> >
> > ===
> > powerpc/pseries/iommu: Make use of DDW for indirect mapping
> >
> > So far it's assumed possible to map the guest RAM 1:1 to the bus, which
> > works with a small number of devices. SRIOV changes it as the user can
> > configure hundreds VFs and since phyp preallocates TCEs and does not
> > allow IOMMU pages bigger than 64K, it has to limit the number of TCEs
> > per a PE to limit waste of physical pages.
> >
> > As of today, if the assumed direct mapping is not possible, DDW
> > creation is skipped and the default DMA window "ibm,dma-window" is used
> > instead.
> >
> > The default DMA window uses 4k pages instead of 64k pages, and since
> > the amount of pages is the same,
>
> Is the amount really the same? I thought you can prioritize some VFs
> over others (== allocate different number of TCEs). Does it really
> matter if it is the same?
On a conversation with Travis Pizel, he explained how it's supposed to
work, and I understood this:
When a VF is created, it will be assigned a capacity, like 4%, 20%, and
so on. The number of 'TCE entries' that are available to that partition
are proportional to that capacity.
If we use the default DMA window, the IOMMU pagesize/entry will be 4k,
and if we use DDW, we will get 64k pagesize. As the number of entries
will be the same (for the same capacity), the total space that can be
addressed by the IOMMU will be 16 times bigger. This sometimes enable
direct mapping, but sometimes it's still not enough.
On Travis words :
"A low capacity VF, with less resources available, will certainly have
less DMA window capability than a high capacity VF. But, an 8GB DMA
window (with 64k pages) is still 16x larger than an 512MB window (with
4K pages).
A high capacity VF - for example, one that Leonardo has in his scenario
- will go from 8GB (using 4K pages) to 128GB (using 64K pages) - again,
16x larger - but it's obviously still possible to create a partition
that exceeds 128GB of memory in size."
>
>
> > making use of DDW instead of the
> > default DMA window for indirect mapping will expand in 16x the amount
> > of memory that can be mapped on DMA.
>
> Stop saying "16x", it is not guaranteed by anything :)
>
>
> > The DDW created will be used for direct mapping by default. [...]
> > ===
> >
> > What do you think?
> >
> > > > The DDW created will be used for direct mapping by default.
> > > > If it's not available, indirect mapping will be used instead.
> > > >
> > > > For indirect mapping, it's necessary to update the iommu_table so
> > > > iommu_alloc() can use the DDW created. For this,
> > > > iommu_table_update_window() is called when everything else succeeds
> > > > at enable_ddw().
> > > >
> > > > Removing the default DMA window for using DDW with indirect mapping
> > > > is only allowed if there is no current IOMMU memory allocated in
> > > > the iommu_table. enable_ddw() is aborted otherwise.
> > > >
> > > > As there will never have both direct and indirect mappings at the same
> > > > time, the same property name can be used for the created DDW.
> > > >
> > > > So renaming
> > > > define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
> > > > to
> > > > define DMA64_PROPNAME "linux,dma64-ddr-window-info"
> > > > looks the right thing to do.
> > >
> > > I know I suggested this but this does not look so good anymore as I
> > > suspect it breaks kexec (from older kernel to this one) so you either
> > > need to check for both DT names or just keep the old one. Changing the
> > > macro name is fine.
> > >
> >
> > Yeah, having 'direct' in the name don't really makes sense if it's used
> > for indirect mapping. I will just add this new define instead of
> > replacing the old one, and check for both.
> > Is that ok?
>
> No, having two of these does not seem right or useful. It is pseries
> which does not use petitboot (relies on grub instead so until the target
> kernel is started, there will be no ddw) so realistically we need this
> property for kexec/kdump which uses the same kernel but different
> initramdisk so for that purpose we need the same property name.
>
> But I can see myself annoyed when I try petitboot in the hacked pseries
> qemu and things may crash :) On this basis I'd suggest keeping the name
> and adding a comment next to it that it is not always "direct" anymore.
>
Keeping the same name should bring more problems than solve.
If we have indirect mapping and kexec() to an older kernel, it will
think direct mapping is enabled, and trying to use a DMA address
without doing H_PUT_* first may cause a crash.
I tested with a new property name, and it doesn't crash.
As the property is not found, it does try to create a new DDW, which
fails and it falls back to using the default DMA window.
The device that need the IOMMU don't work well, but when iommu_map()
fails, it doesn't try to use the DMA address as valid.
>
> > > > To make sure the property differentiates both cases, a new u32 for flags
> > > > was added at the end of the property, where BIT(0) set means direct
> > > > mapping.
> > > >
> > > > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> > > > ---
> > > > arch/powerpc/platforms/pseries/iommu.c | 108 +++++++++++++++++++------
> > > > 1 file changed, 84 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> > > > index 3a1ef02ad9d5..9544e3c91ced 100644
> > > > --- a/arch/powerpc/platforms/pseries/iommu.c
> > > > +++ b/arch/powerpc/platforms/pseries/iommu.c
> > > > @@ -350,8 +350,11 @@ struct dynamic_dma_window_prop {
> > > > __be64 dma_base; /* address hi,lo */
> > > > __be32 tce_shift; /* ilog2(tce_page_size) */
> > > > __be32 window_shift; /* ilog2(tce_window_size) */
> > > > + __be32 flags; /* DDW properties, see bellow */
> > > > };
> > > >
> > > > +#define DDW_FLAGS_DIRECT 0x01
> > >
> > > This is set if ((1<<window_shift) >= ddw_memory_hotplug_max()), you
> > > could simply check window_shift and drop the flags.
> > >
> >
> > Yeah, it's better this way, I will revert this.
> >
> > > > +
> > > > struct direct_window {
> > > > struct device_node *device;
> > > > const struct dynamic_dma_window_prop *prop;
> > > > @@ -377,7 +380,7 @@ static LIST_HEAD(direct_window_list);
> > > > static DEFINE_SPINLOCK(direct_window_list_lock);
> > > > /* protects initializing window twice for same device */
> > > > static DEFINE_MUTEX(direct_window_init_mutex);
> > > > -#define DIRECT64_PROPNAME "linux,direct64-ddr-window-info"
> > > > +#define DMA64_PROPNAME "linux,dma64-ddr-window-info"
> > > >
> > > > static int tce_clearrange_multi_pSeriesLP(unsigned long start_pfn,
> > > > unsigned long num_pfn, const void *arg)
> > > > @@ -836,7 +839,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
> > > > if (ret)
> > > > return;
> > > >
> > > > - win = of_find_property(np, DIRECT64_PROPNAME, NULL);
> > > > + win = of_find_property(np, DMA64_PROPNAME, NULL);
> > > > if (!win)
> > > > return;
> > > >
> > > > @@ -852,7 +855,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
> > > > np, ret);
> > > > }
> > > >
> > > > -static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
> > > > +static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr, bool *direct_mapping)
> > > > {
> > > > struct direct_window *window;
> > > > const struct dynamic_dma_window_prop *direct64;
> > > > @@ -864,6 +867,7 @@ static bool find_existing_ddw(struct device_node *pdn, u64 *dma_addr)
> > > > if (window->device == pdn) {
> > > > direct64 = window->prop;
> > > > *dma_addr = be64_to_cpu(direct64->dma_base);
> > > > + *direct_mapping = be32_to_cpu(direct64->flags) & DDW_FLAGS_DIRECT;
> > > > found = true;
> > > > break;
> > > > }
> > > > @@ -901,8 +905,8 @@ static int find_existing_ddw_windows(void)
> > > > if (!firmware_has_feature(FW_FEATURE_LPAR))
> > > > return 0;
> > > >
> > > > - for_each_node_with_property(pdn, DIRECT64_PROPNAME) {
> > > > - direct64 = of_get_property(pdn, DIRECT64_PROPNAME, &len);
> > > > + for_each_node_with_property(pdn, DMA64_PROPNAME) {
> > > > + direct64 = of_get_property(pdn, DMA64_PROPNAME, &len);
> > > > if (!direct64)
> > > > continue;
> > > >
> > > > @@ -1124,7 +1128,8 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
> > > > }
> > > >
> > > > static int ddw_property_create(struct property **ddw_win, const char *propname,
> > > > - u32 liobn, u64 dma_addr, u32 page_shift, u32 window_shift)
> > > > + u32 liobn, u64 dma_addr, u32 page_shift,
> > > > + u32 window_shift, bool direct_mapping)
> > > > {
> > > > struct dynamic_dma_window_prop *ddwprop;
> > > > struct property *win64;
> > > > @@ -1144,6 +1149,36 @@ static int ddw_property_create(struct property **ddw_win, const char *propname,
> > > > ddwprop->dma_base = cpu_to_be64(dma_addr);
> > > > ddwprop->tce_shift = cpu_to_be32(page_shift);
> > > > ddwprop->window_shift = cpu_to_be32(window_shift);
> > > > + if (direct_mapping)
> > > > + ddwprop->flags = cpu_to_be32(DDW_FLAGS_DIRECT);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int iommu_table_update_window(struct iommu_table **tbl, int nid, unsigned long liobn,
> > > > + unsigned long win_addr, unsigned long page_shift,
> > > > + unsigned long window_size)
> > >
> > > Rather strange helper imho. I'd extract the most of
> > > iommu_table_setparms_lpar() into iommu_table_setparms() (except
> > > of_parse_dma_window) and call new helper from where you call
> > > iommu_table_update_window; and do
> > > iommu_pseries_alloc_table/iommu_tce_table_put there.
> > >
> >
> > I don't see how to extract iommu_table_setparms_lpar() into
> > iommu_table_setparms(), they look to be used for different machine
> > types.
> >
> > Do mean you extracting most of iommu_table_setparms_lpar() (and maybe
> > iommu_table_setparms() ) into a new helper, which is called in both
> > functions and use it instead of iommu_table_update_window() ?
>
> Yes, this.
I will do that then, seems better. :)
>
>
> > > > +{
> > > > + struct iommu_table *new_tbl, *old_tbl;
> > > > +
> > > > + new_tbl = iommu_pseries_alloc_table(nid);
> > > > + if (!new_tbl)
> > > > + return -ENOMEM;
> > > > +
> > > > + old_tbl = *tbl;
> > > > + new_tbl->it_index = liobn;
> > > > + new_tbl->it_offset = win_addr >> page_shift;
> > > > + new_tbl->it_page_shift = page_shift;
> > > > + new_tbl->it_size = window_size >> page_shift;
> > > > + new_tbl->it_base = old_tbl->it_base;
> > >
> > > Should not be used in pseries.
> > >
> >
> > The point here is to migrate the values from the older tbl to the
>
> The actual window/table is new (on the hypervisor side), you are not
> migrating a single TCE, you deleted one whole window and created another
> whole window, calling it "migration" is confusing, especially when PAPR
> actually defines TCE migration.
Ok, I understand it's confusing now. I will avoid using this term from
now on.
>
>
> > newer. I Would like to understand why this is bad, if it will still be
> > 'unused' as the older tbl.
>
> Having explicit values is more readable imho.
Ok, I understand why it should be improved.!
Alexey, thank you for reviewing, and for helping me with my questions!
Best regards,
>
>
> > > > + new_tbl->it_busno = old_tbl->it_busno;
> > > > + new_tbl->it_blocksize = old_tbl->it_blocksize;
> > >
> > > 16 for pseries and does not change (may be even make it a macro).
> > >
> > > > + new_tbl->it_type = old_tbl->it_type;
> > >
> > > TCE_PCI.
> > >
> >
> > Same as above.
> >
> > > > + new_tbl->it_ops = old_tbl->it_ops;
> > > > +
> > > > + iommu_init_table(new_tbl, nid, old_tbl->it_reserved_start, old_tbl->it_reserved_end);
> > > > + iommu_tce_table_put(old_tbl);
> > > > + *tbl = new_tbl;
> > > >
> > > > return 0;
> > > > }
> > > > @@ -1171,12 +1206,16 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > struct direct_window *window;
> > > > struct property *win64 = NULL;
> > > > struct failed_ddw_pdn *fpdn;
> > > > - bool default_win_removed = false;
> > > > + bool default_win_removed = false, maps_whole_partition = false;
> > >
> > > s/maps_whole_partition/direct_mapping/
> > >
> >
> > Sure, I will get it replaced.
> >
> > > > + struct pci_dn *pci = PCI_DN(pdn);
> > > > + struct iommu_table *tbl = pci->table_group->tables[0];
> > > >
> > > > mutex_lock(&direct_window_init_mutex);
> > > >
> > > > - if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset))
> > > > - goto out_unlock;
> > > > + if (find_existing_ddw(pdn, &dev->dev.archdata.dma_offset, &maps_whole_partition)) {
> > > > + mutex_unlock(&direct_window_init_mutex);
> > > > + return maps_whole_partition;
> > > > + }
> > > >
> > > > /*
> > > > * If we already went through this for a previous function of
> > > > @@ -1258,16 +1297,24 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > query.page_size);
> > > > goto out_failed;
> > > > }
> > > > +
> > > > /* verify the window * number of ptes will map the partition */
> > > > - /* check largest block * page size > max memory hotplug addr */
> > > > max_addr = ddw_memory_hotplug_max();
> > > > if (query.largest_available_block < (max_addr >> page_shift)) {
> > > > - dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu "
> > > > - "%llu-sized pages\n", max_addr, query.largest_available_block,
> > > > - 1ULL << page_shift);
> > > > - goto out_failed;
> > > > + dev_dbg(&dev->dev, "can't map partition max 0x%llx with %llu %llu-sized pages\n",
> > > > + max_addr, query.largest_available_block,
> > > > + 1ULL << page_shift);
> > > > +
> > > > + len = order_base_2(query.largest_available_block << page_shift);
> > > > + } else {
> > > > + maps_whole_partition = true;
> > > > + len = order_base_2(max_addr);
> > > > }
> > > > - len = order_base_2(max_addr);
> > > > +
> > > > + /* DDW + IOMMU on single window may fail if there is any allocation */
> > > > + if (default_win_removed && !maps_whole_partition &&
> > > > + iommu_table_in_use(tbl))
> > > > + goto out_failed;
> > > >
> > > > ret = create_ddw(dev, ddw_avail, &create, page_shift, len);
> > > > if (ret != 0)
> > > > @@ -1277,8 +1324,8 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > create.liobn, dn);
> > > >
> > > > win_addr = ((u64)create.addr_hi << 32) | create.addr_lo;
> > > > - ret = ddw_property_create(&win64, DIRECT64_PROPNAME, create.liobn, win_addr,
> > > > - page_shift, len);
> > > > + ret = ddw_property_create(&win64, DMA64_PROPNAME, create.liobn, win_addr,
> > > > + page_shift, len, maps_whole_partition);
> > > > if (ret) {
> > > > dev_info(&dev->dev,
> > > > "couldn't allocate property, property name, or value\n");
> > > > @@ -1297,12 +1344,25 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > > if (!window)
> > > > goto out_prop_del;
> > > >
> > > > - ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
> > > > - win64->value, tce_setrange_multi_pSeriesLP_walk);
> > > > - if (ret) {
> > > > - dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
> > > > - dn, ret);
> > > > - goto out_free_window;
> > > > + if (maps_whole_partition) {
> > > > + /* DDW maps the whole partition, so enable direct DMA mapping */
> > > > + ret = walk_system_ram_range(0, memblock_end_of_DRAM() >> PAGE_SHIFT,
> > > > + win64->value, tce_setrange_multi_pSeriesLP_walk);
> > > > + if (ret) {
> > > > + dev_info(&dev->dev, "failed to map direct window for %pOF: %d\n",
> > > > + dn, ret);
> > > > + goto out_free_window;
> > > > + }
> > > > + } else {
> > > > + /* New table for using DDW instead of the default DMA window */
> > > > + if (iommu_table_update_window(&tbl, pci->phb->node, create.liobn,
> > > > + win_addr, page_shift, 1UL << len))
> > > > + goto out_free_window;
> > > > +
> > > > + set_iommu_table_base(&dev->dev, tbl);
> > > > + WARN_ON(dev->dev.archdata.dma_offset >= SZ_4G);
> > >
> > > What is this check for exactly? Why 4G, not >= 0, for example?
> >
> > I am not really sure, you suggested adding it here:
> > http://patchwork.ozlabs.org/project/linuxppc-dev/patch/20200716071658.467820-6-leobras.c@gmail.com/#2488874
>
> Ah right I did suggest this :) My bad. I think I suggested it before
> suggesting to keep the reserved area boundaries checked/adjusted to the
> window boundaries, may as well drop this. Thanks,
>
>
> > I can remove it if it's ok.
> >
> > > > + goto out_unlock;
> > > > +
> > > > }
> > > >
> > > > dev->dev.archdata.dma_offset = win_addr;
> > > > @@ -1340,7 +1400,7 @@ static bool enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> > > >
> > > > out_unlock:
> > > > mutex_unlock(&direct_window_init_mutex);
> > > > - return win64;
> > > > + return win64 && maps_whole_partition;
> > > > }
> > > >
> > > > static void pci_dma_dev_setup_pSeriesLP(struct pci_dev *dev)
> > > >
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: remove address space overrides using set_fs()
From: Christophe Leroy @ 2020-09-02 6:15 UTC (permalink / raw)
To: Christoph Hellwig, Linus Torvalds, Al Viro, Michael Ellerman, x86
Cc: linux-fsdevel, linux-arch, linuxppc-dev, Kees Cook, linux-kernel
In-Reply-To: <20200827150030.282762-11-hch@lst.de>
Le 27/08/2020 à 17:00, Christoph Hellwig a écrit :
> Stop providing the possibility to override the address space using
> set_fs() now that there is no need for that any more.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> arch/powerpc/Kconfig | 1 -
> arch/powerpc/include/asm/processor.h | 7 ---
> arch/powerpc/include/asm/thread_info.h | 5 +--
> arch/powerpc/include/asm/uaccess.h | 62 ++++++++------------------
> arch/powerpc/kernel/signal.c | 3 --
> arch/powerpc/lib/sstep.c | 6 +--
> 6 files changed, 22 insertions(+), 62 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 7fe3531ad36a77..39727537d39701 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -8,62 +8,36 @@
> #include <asm/extable.h>
> #include <asm/kup.h>
>
> -/*
> - * The fs value determines whether argument validity checking should be
> - * performed or not. If get_fs() == USER_DS, checking is performed, with
> - * get_fs() == KERNEL_DS, checking is bypassed.
> - *
> - * For historical reasons, these macros are grossly misnamed.
> - *
> - * The fs/ds values are now the highest legal address in the "segment".
> - * This simplifies the checking in the routines below.
> - */
> -
> -#define MAKE_MM_SEG(s) ((mm_segment_t) { (s) })
> -
> -#define KERNEL_DS MAKE_MM_SEG(~0UL)
> #ifdef __powerpc64__
> /* We use TASK_SIZE_USER64 as TASK_SIZE is not constant */
> -#define USER_DS MAKE_MM_SEG(TASK_SIZE_USER64 - 1)
> -#else
> -#define USER_DS MAKE_MM_SEG(TASK_SIZE - 1)
> -#endif
> -
> -#define get_fs() (current->thread.addr_limit)
> +#define TASK_SIZE_MAX TASK_SIZE_USER64
>
> -static inline void set_fs(mm_segment_t fs)
> +static inline bool __access_ok(unsigned long addr, unsigned long size)
> {
> - current->thread.addr_limit = fs;
> - /* On user-mode return check addr_limit (fs) is correct */
> - set_thread_flag(TIF_FSCHECK);
> + if (addr >= TASK_SIZE_MAX)
> + return false;
> + /*
> + * This check is sufficient because there is a large enough gap between
> + * user addresses and the kernel addresses.
> + */
> + return size <= TASK_SIZE_MAX;
> }
> -
> -#define uaccess_kernel() (get_fs().seg == KERNEL_DS.seg)
> -#define user_addr_max() (get_fs().seg)
> -
> -#ifdef __powerpc64__
> -/*
> - * This check is sufficient because there is a large enough
> - * gap between user addresses and the kernel addresses
> - */
> -#define __access_ok(addr, size, segment) \
> - (((addr) <= (segment).seg) && ((size) <= (segment).seg))
> -
> #else
> +#define TASK_SIZE_MAX TASK_SIZE
>
> -static inline int __access_ok(unsigned long addr, unsigned long size,
> - mm_segment_t seg)
> +static inline bool __access_ok(unsigned long addr, unsigned long size)
> {
> - if (addr > seg.seg)
> - return 0;
> - return (size == 0 || size - 1 <= seg.seg - addr);
> + if (addr >= TASK_SIZE_MAX)
> + return false;
> + if (size == 0)
> + return false;
__access_ok() was returning true when size == 0 up to now. Any reason to
return false now ?
> + return size <= TASK_SIZE_MAX - addr;
> }
> -
> -#endif
> +#endif /* __powerpc64__ */
>
> #define access_ok(addr, size) \
> (__chk_user_ptr(addr), \
> - __access_ok((__force unsigned long)(addr), (size), get_fs()))
> + __access_ok((unsigned long)(addr), (size)))
>
> /*
> * These are the main single-value transfer routines. They automatically
Christophe
^ permalink raw reply
* Re: [RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type
From: Paul Mackerras @ 2020-09-02 6:13 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <20200820033922.32311-1-jniethe5@gmail.com>
On Thu, Aug 20, 2020 at 01:39:21PM +1000, Jordan Niethe wrote:
> The ppc_inst type was added to help cope with the addition of prefixed
> instructions to the ISA. Convert KVM to use this new type for dealing
> wiht instructions. For now do not try to add further support for
> prefixed instructions.
This change does seem to splatter itself across a lot of code that
mostly or exclusively runs on machines which are not POWER10 and will
never need to handle prefixed instructions, unfortunately. I wonder
if there is a less invasive way to approach this.
In particular we are inflicting this 64-bit struct on 32-bit platforms
unnecessarily (I assume, correct me if I am wrong here).
How would it be to do something like:
typedef unsigned long ppc_inst_t;
so it is 32 bits on 32-bit platforms and 64 bits on 64-bit platforms,
and then use that instead of 'struct ppc_inst'? You would still need
to change the function declarations but I think most of the function
bodies would not need to be changed. In particular you would avoid a
lot of the churn related to having to add ppc_inst_val() and suchlike.
> -static inline unsigned make_dsisr(unsigned instr)
> +static inline unsigned make_dsisr(struct ppc_inst instr)
> {
> unsigned dsisr;
> + u32 word = ppc_inst_val(instr);
>
>
> /* bits 6:15 --> 22:31 */
> - dsisr = (instr & 0x03ff0000) >> 16;
> + dsisr = (word & 0x03ff0000) >> 16;
>
> if (IS_XFORM(instr)) {
> /* bits 29:30 --> 15:16 */
> - dsisr |= (instr & 0x00000006) << 14;
> + dsisr |= (word & 0x00000006) << 14;
> /* bit 25 --> 17 */
> - dsisr |= (instr & 0x00000040) << 8;
> + dsisr |= (word & 0x00000040) << 8;
> /* bits 21:24 --> 18:21 */
> - dsisr |= (instr & 0x00000780) << 3;
> + dsisr |= (word & 0x00000780) << 3;
> } else {
> /* bit 5 --> 17 */
> - dsisr |= (instr & 0x04000000) >> 12;
> + dsisr |= (word & 0x04000000) >> 12;
> /* bits 1: 4 --> 18:21 */
> - dsisr |= (instr & 0x78000000) >> 17;
> + dsisr |= (word & 0x78000000) >> 17;
> /* bits 30:31 --> 12:13 */
> if (IS_DSFORM(instr))
> - dsisr |= (instr & 0x00000003) << 18;
> + dsisr |= (word & 0x00000003) << 18;
Here I would have done something like:
> -static inline unsigned make_dsisr(unsigned instr)
> +static inline unsigned make_dsisr(struct ppc_inst pi)
> {
> unsigned dsisr;
> + u32 instr = ppc_inst_val(pi);
and left the rest of the function unchanged.
At first I wondered why we still had that function, since IBM Power
CPUs have not set DSISR on an alignment interrupt since POWER3 days.
It turns out it this function is used by PR KVM when it is emulating
one of the old 32-bit PowerPC CPUs (601, 603, 604, 750, 7450 etc.).
> diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
Despite the file name, this code is not used on IBM Power servers.
It is for platforms which run under an ePAPR (not server PAPR)
hypervisor (which would be a KVM variant, but generally book E KVM not
book 3S).
Paul.
^ permalink raw reply
* Re: [RFC PATCH 2/2] KVM: PPC: Book3S HV: Support prefixed instructions
From: Paul Mackerras @ 2020-09-02 6:18 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <20200820033922.32311-2-jniethe5@gmail.com>
On Thu, Aug 20, 2020 at 01:39:22PM +1000, Jordan Niethe wrote:
> There are two main places where instructions are loaded from the guest:
> * Emulate loadstore - such as when performing MMIO emulation
> triggered by an HDSI
> * After an HV emulation assistance interrupt (e40)
>
> If it is a prefixed instruction that triggers these cases, its suffix
> must be loaded. Use the SRR1_PREFIX bit to decide if a suffix needs to
> be loaded. Make sure if this bit is set inject_interrupt() also sets it
> when giving an interrupt to the guest.
>
> ISA v3.10 extends the Hypervisor Emulation Instruction Register (HEIR)
> to 64 bits long to accommodate prefixed instructions. For interrupts
> caused by a word instruction the instruction is loaded into bits 32:63
> and bits 0:31 are zeroed. When caused by a prefixed instruction the
> prefix and suffix are loaded into bits 0:63.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/kvm/book3s.c | 15 +++++++++++++--
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 10 +++++++---
> arch/powerpc/kvm/book3s_hv_builtin.c | 3 +++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 ++++++++++++++
> 4 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 70d8967acc9b..18b1928a571b 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -456,13 +456,24 @@ int kvmppc_load_last_inst(struct kvm_vcpu *vcpu,
> {
> ulong pc = kvmppc_get_pc(vcpu);
> u32 word;
> + u64 doubleword;
> int r;
>
> if (type == INST_SC)
> pc -= 4;
>
> - r = kvmppc_ld(vcpu, &pc, sizeof(u32), &word, false);
> - *inst = ppc_inst(word);
> + if ((kvmppc_get_msr(vcpu) & SRR1_PREFIXED)) {
> + r = kvmppc_ld(vcpu, &pc, sizeof(u64), &doubleword, false);
Should we also have a check here that the doubleword is not crossing a
page boundary? I can't think of a way to get this code to cross a
page boundary, assuming the hardware is working correctly, but it
makes me just a little nervous.
> +#ifdef CONFIG_CPU_LITTLE_ENDIAN
> + *inst = ppc_inst_prefix(doubleword & 0xffffffff, doubleword >> 32);
> +#else
> + *inst = ppc_inst_prefix(doubleword >> 32, doubleword & 0xffffffff);
> +#endif
Ick. Is there a cleaner way to do this?
> + } else {
> + r = kvmppc_ld(vcpu, &pc, sizeof(u32), &word, false);
> + *inst = ppc_inst(word);
> + }
> +
> if (r == EMULATE_DONE)
> return r;
> else
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 775ce41738ce..0802471f4856 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -411,9 +411,13 @@ static int instruction_is_store(struct ppc_inst instr)
> unsigned int mask;
>
> mask = 0x10000000;
> - if ((ppc_inst_val(instr) & 0xfc000000) == 0x7c000000)
> - mask = 0x100; /* major opcode 31 */
> - return (ppc_inst_val(instr) & mask) != 0;
> + if (ppc_inst_prefixed(instr)) {
> + return (ppc_inst_suffix(instr) & mask) != 0;
> + } else {
> + if ((ppc_inst_val(instr) & 0xfc000000) == 0x7c000000)
> + mask = 0x100; /* major opcode 31 */
> + return (ppc_inst_val(instr) & mask) != 0;
> + }
The way the code worked before, the mask depended on whether the
instruction was a D-form (or DS-form or other variant) instruction,
where you can tell loads and stores apart by looking at the major
opcode, or an X-form instruction, where you look at the minor opcode.
Now we are only looking at the minor opcode if it is not a prefixed
instruction. Are there no X-form prefixed loads or stores?
Paul.
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/vdso32: link vdso64 with linker
From: Christophe Leroy @ 2020-09-02 6:46 UTC (permalink / raw)
To: Nick Desaulniers, Michael Ellerman, Nicholas Piggin
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
linux-kernel, clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <20200901222523.1941988-3-ndesaulniers@google.com>
On 09/01/2020 10:25 PM, Nick Desaulniers wrote:
> Rather than invoke the compiler as the driver, use the linker. That way
> we can check --orphan-handling=warn support correctly, as cc-ldoption
> was removed in
> commit 055efab3120b ("kbuild: drop support for cc-ldoption").
>
> Requires dropping the .got section. I couldn't find how it was used in
> the vdso32.
ld crashes:
LD arch/powerpc/kernel/vdso32/vdso32.so.dbg
/bin/sh: line 1: 23780 Segmentation fault (core dumped)
ppc-linux-ld -EB -m elf32ppc -shared -soname linux-vdso32.so.1
--eh-frame-hdr --orphan-handling=warn -T
arch/powerpc/kernel/vdso32/vdso32.lds
arch/powerpc/kernel/vdso32/sigtramp.o
arch/powerpc/kernel/vdso32/gettimeofday.o
arch/powerpc/kernel/vdso32/datapage.o
arch/powerpc/kernel/vdso32/cacheflush.o
arch/powerpc/kernel/vdso32/note.o arch/powerpc/kernel/vdso32/getcpu.o -o
arch/powerpc/kernel/vdso32/vdso32.so.dbg
make[4]: *** [arch/powerpc/kernel/vdso32/vdso32.so.dbg] Error 139
[root@localhost linux-powerpc]# ppc-linux-ld --version
GNU ld (GNU Binutils) 2.26.20160125
Christophe
>
> Fixes: commit f2af201002a8 ("powerpc/build: vdso linker warning for orphan sections")
> Link: https://lore.kernel.org/lkml/CAKwvOdnn3wxYdJomvnveyD_njwRku3fABWT_bS92duihhywLJQ@mail.gmail.com/
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Not sure removing .got is a good idea or not. Otherwise I observe the
> following link error:
> powerpc-linux-gnu-ld: warning: orphan section `.got' from `arch/powerpc/kernel/vdso32/sigtramp.o' being placed in section `.got'
> powerpc-linux-gnu-ld: _GLOBAL_OFFSET_TABLE_ not defined in linker created .got
> powerpc-linux-gnu-ld: final link failed: bad value
>
> sigtramp.c doesn't mention anything from the GOT AFAICT, and doesn't
> look like it contains relocations that do, so I'm not sure where
> references to _GLOBAL_OFFSET_TABLE_ are coming from.
>
> arch/powerpc/kernel/vdso32/Makefile | 7 +++++--
> arch/powerpc/kernel/vdso32/vdso32.lds.S | 3 ++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
> index 87ab1152d5ce..611a5951945a 100644
> --- a/arch/powerpc/kernel/vdso32/Makefile
> +++ b/arch/powerpc/kernel/vdso32/Makefile
> @@ -27,6 +27,9 @@ UBSAN_SANITIZE := n
> ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
> -Wl,-soname=linux-vdso32.so.1 -Wl,--hash-style=both
> asflags-y := -D__VDSO32__ -s
> +ldflags-y := -shared -soname linux-vdso32.so.1 \
> + $(call ld-option, --eh-frame-hdr) \
> + $(call ld-option, --orphan-handling=warn) -T
>
> obj-y += vdso32_wrapper.o
> extra-y += vdso32.lds
> @@ -49,8 +52,8 @@ $(obj-vdso32): %.o: %.S FORCE
> $(call if_changed_dep,vdso32as)
>
> # actual build commands
> -quiet_cmd_vdso32ld = VDSO32L $@
> - cmd_vdso32ld = $(VDSOCC) $(c_flags) $(CC32FLAGS) -o $@ $(call cc-ldoption, -Wl$(comma)--orphan-handling=warn) -Wl,-T$(filter %.lds,$^) $(filter %.o,$^)
> +quiet_cmd_vdso32ld = LD $@
> + cmd_vdso32ld = $(cmd_ld)
> quiet_cmd_vdso32as = VDSO32A $@
> cmd_vdso32as = $(VDSOCC) $(a_flags) $(CC32FLAGS) -c -o $@ $<
>
> diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> index 4c985467a668..0ccdebad18b8 100644
> --- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
> +++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> @@ -61,7 +61,6 @@ SECTIONS
> .fixup : { *(.fixup) }
>
> .dynamic : { *(.dynamic) } :text :dynamic
> - .got : { *(.got) } :text
> .plt : { *(.plt) }
>
> _end = .;
> @@ -108,7 +107,9 @@ SECTIONS
> .debug_varnames 0 : { *(.debug_varnames) }
>
> /DISCARD/ : {
> + *(.got)
> *(.note.GNU-stack)
> + *(.branch_lt)
> *(.data .data.* .gnu.linkonce.d.* .sdata*)
> *(.bss .sbss .dynbss .dynsbss)
> *(.glink .iplt .plt .rela*)
>
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/vdso32: link vdso64 with linker
From: Christophe Leroy @ 2020-09-02 7:56 UTC (permalink / raw)
To: Nick Desaulniers, Michael Ellerman, Nicholas Piggin
Cc: Christophe Leroy, Joe Lawrence, Kees Cook, Fangrui Song,
linux-kernel, clang-built-linux, Paul Mackerras, linuxppc-dev
In-Reply-To: <20200901222523.1941988-3-ndesaulniers@google.com>
On 9/1/20 10:25 PM, Nick Desaulniers wrote:
> Rather than invoke the compiler as the driver, use the linker. That way
> we can check --orphan-handling=warn support correctly, as cc-ldoption
> was removed in
> commit 055efab3120b ("kbuild: drop support for cc-ldoption").
>
> Requires dropping the .got section. I couldn't find how it was used in
> the vdso32.
>
> Fixes: commit f2af201002a8 ("powerpc/build: vdso linker warning for orphan sections")
> Link: https://lore.kernel.org/lkml/CAKwvOdnn3wxYdJomvnveyD_njwRku3fABWT_bS92duihhywLJQ@mail.gmail.com/
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Not sure removing .got is a good idea or not. Otherwise I observe the
> following link error:
> powerpc-linux-gnu-ld: warning: orphan section `.got' from `arch/powerpc/kernel/vdso32/sigtramp.o' being placed in section `.got'
> powerpc-linux-gnu-ld: _GLOBAL_OFFSET_TABLE_ not defined in linker created .got
> powerpc-linux-gnu-ld: final link failed: bad value
>
> sigtramp.c doesn't mention anything from the GOT AFAICT, and doesn't
> look like it contains relocations that do, so I'm not sure where
> references to _GLOBAL_OFFSET_TABLE_ are coming from.
I'm getting the same but only when building for PPC64.
I don't get any reference to sigtramp.o though:
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
VDSO32A arch/powerpc/kernel/vdso32/sigtramp.o
VDSO32A arch/powerpc/kernel/vdso32/gettimeofday.o
VDSO32A arch/powerpc/kernel/vdso32/datapage.o
VDSO32A arch/powerpc/kernel/vdso32/cacheflush.o
VDSO32A arch/powerpc/kernel/vdso32/note.o
VDSO32A arch/powerpc/kernel/vdso32/getcpu.o
LD arch/powerpc/kernel/vdso32/vdso32.so.dbg
powerpc64-linux-ld: _GLOBAL_OFFSET_TABLE_ not defined in linker created .got
powerpc64-linux-ld: final link failed: Bad value
(GCC 8.1, Binutils 2.30)
So it seems that the got section is being created by the linker. Don't
know why though.
With GCC 10.1, binutils 2.34 I get:
LDS arch/powerpc/kernel/vdso32/vdso32.lds
VDSO32A arch/powerpc/kernel/vdso32/sigtramp.o
VDSO32A arch/powerpc/kernel/vdso32/gettimeofday.o
VDSO32A arch/powerpc/kernel/vdso32/datapage.o
VDSO32A arch/powerpc/kernel/vdso32/cacheflush.o
VDSO32A arch/powerpc/kernel/vdso32/note.o
VDSO32A arch/powerpc/kernel/vdso32/getcpu.o
LD arch/powerpc/kernel/vdso32/vdso32.so.dbg
powerpc64-linux-ld: warning: orphan section `.branch_lt' from
`arch/powerpc/kernel/vdso32/sigtramp.o' being placed in section `.branch_lt'
powerpc64-linux-ld: _GLOBAL_OFFSET_TABLE_ not defined in linker created .got
powerpc64-linux-ld: final link failed: bad value
I can't see any .branch_lt section when objdumping sigtramp.o or any
other .o
When I move sigtramp.o at the end of the definition of obj-vdso32 in
Makefile, I then get:
powerpc64-linux-ld: warning: orphan section `.branch_lt' from
`arch/powerpc/kernel/vdso32/gettimeofday.o' being placed in section
`.branch_lt'
powerpc64-linux-ld: _GLOBAL_OFFSET_TABLE_ not defined in linker created .got
powerpc64-linux-ld: final link failed: bad value
gettimeofday.o now being the first object in obj-vdso32
Christophe
>
> arch/powerpc/kernel/vdso32/Makefile | 7 +++++--
> arch/powerpc/kernel/vdso32/vdso32.lds.S | 3 ++-
> 2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
> index 87ab1152d5ce..611a5951945a 100644
> --- a/arch/powerpc/kernel/vdso32/Makefile
> +++ b/arch/powerpc/kernel/vdso32/Makefile
> @@ -27,6 +27,9 @@ UBSAN_SANITIZE := n
> ccflags-y := -shared -fno-common -fno-builtin -nostdlib \
> -Wl,-soname=linux-vdso32.so.1 -Wl,--hash-style=both
> asflags-y := -D__VDSO32__ -s
> +ldflags-y := -shared -soname linux-vdso32.so.1 \
> + $(call ld-option, --eh-frame-hdr) \
> + $(call ld-option, --orphan-handling=warn) -T
>
> obj-y += vdso32_wrapper.o
> extra-y += vdso32.lds
> @@ -49,8 +52,8 @@ $(obj-vdso32): %.o: %.S FORCE
> $(call if_changed_dep,vdso32as)
>
> # actual build commands
> -quiet_cmd_vdso32ld = VDSO32L $@
> - cmd_vdso32ld = $(VDSOCC) $(c_flags) $(CC32FLAGS) -o $@ $(call cc-ldoption, -Wl$(comma)--orphan-handling=warn) -Wl,-T$(filter %.lds,$^) $(filter %.o,$^)
> +quiet_cmd_vdso32ld = LD $@
> + cmd_vdso32ld = $(cmd_ld)
> quiet_cmd_vdso32as = VDSO32A $@
> cmd_vdso32as = $(VDSOCC) $(a_flags) $(CC32FLAGS) -c -o $@ $<
>
> diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> index 4c985467a668..0ccdebad18b8 100644
> --- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
> +++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> @@ -61,7 +61,6 @@ SECTIONS
> .fixup : { *(.fixup) }
>
> .dynamic : { *(.dynamic) } :text :dynamic
> - .got : { *(.got) } :text
> .plt : { *(.plt) }
>
> _end = .;
> @@ -108,7 +107,9 @@ SECTIONS
> .debug_varnames 0 : { *(.debug_varnames) }
>
> /DISCARD/ : {
> + *(.got)
> *(.note.GNU-stack)
> + *(.branch_lt)
> *(.data .data.* .gnu.linkonce.d.* .sdata*)
> *(.bss .sbss .dynbss .dynsbss)
> *(.glink .iplt .plt .rela*)
>
^ permalink raw reply
* Re: [RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type
From: Jordan Niethe @ 2020-09-02 8:00 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, Nicholas Piggin, kvm-ppc
In-Reply-To: <20200902061318.GE272502@thinks.paulus.ozlabs.org>
On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras <paulus@ozlabs.org> wrote:
>
> On Thu, Aug 20, 2020 at 01:39:21PM +1000, Jordan Niethe wrote:
> > The ppc_inst type was added to help cope with the addition of prefixed
> > instructions to the ISA. Convert KVM to use this new type for dealing
> > wiht instructions. For now do not try to add further support for
> > prefixed instructions.
>
> This change does seem to splatter itself across a lot of code that
> mostly or exclusively runs on machines which are not POWER10 and will
> never need to handle prefixed instructions, unfortunately. I wonder
> if there is a less invasive way to approach this.
Something less invasive would be good.
>
> In particular we are inflicting this 64-bit struct on 32-bit platforms
> unnecessarily (I assume, correct me if I am wrong here).
No, that is something that I wanted to to avoid, on 32 bit platforms
it is a 32bit struct:
struct ppc_inst {
u32 val;
#ifdef CONFIG_PPC64
u32 suffix;
#endif
} __packed;
>
> How would it be to do something like:
>
> typedef unsigned long ppc_inst_t;
>
> so it is 32 bits on 32-bit platforms and 64 bits on 64-bit platforms,
> and then use that instead of 'struct ppc_inst'? You would still need
> to change the function declarations but I think most of the function
> bodies would not need to be changed. In particular you would avoid a
> lot of the churn related to having to add ppc_inst_val() and suchlike.
Would the idea be to get rid of `struct ppc_inst` entirely or just not
use it in kvm?
In an earlier series I did something similar (at least code shared
between 32bit and 64bit would need helpers, but 32bit only code need
not change):
#ifdef __powerpc64__
typedef struct ppc_inst {
union {
struct {
u32 word;
u32 pad;
} __packed;
struct {
u32 prefix;
u32 suffix;
} __packed;
};
} ppc_inst;
#else /* !__powerpc64__ */
typedef u32 ppc_inst;
#endif
However mpe wanted to avoid using a typedef
(https://patchwork.ozlabs.org/comment/2391845/)
We did also talk about just using a u64 for instructions
(https://lore.kernel.org/linuxppc-dev/1585028462.t27rstc2uf.astroid@bobo.none/)
but the concern was that as prefixed instructions act as two separate
u32s (prefix is always before the suffix regardless of endianess)
keeping it as a u64 would lead to lot of macros and potential
confusion.
But it does seem if that can avoid a lot of needless churn it might
worth the trade off.
>
> > -static inline unsigned make_dsisr(unsigned instr)
> > +static inline unsigned make_dsisr(struct ppc_inst instr)
> > {
> > unsigned dsisr;
> > + u32 word = ppc_inst_val(instr);
> >
> >
> > /* bits 6:15 --> 22:31 */
> > - dsisr = (instr & 0x03ff0000) >> 16;
> > + dsisr = (word & 0x03ff0000) >> 16;
> >
> > if (IS_XFORM(instr)) {
> > /* bits 29:30 --> 15:16 */
> > - dsisr |= (instr & 0x00000006) << 14;
> > + dsisr |= (word & 0x00000006) << 14;
> > /* bit 25 --> 17 */
> > - dsisr |= (instr & 0x00000040) << 8;
> > + dsisr |= (word & 0x00000040) << 8;
> > /* bits 21:24 --> 18:21 */
> > - dsisr |= (instr & 0x00000780) << 3;
> > + dsisr |= (word & 0x00000780) << 3;
> > } else {
> > /* bit 5 --> 17 */
> > - dsisr |= (instr & 0x04000000) >> 12;
> > + dsisr |= (word & 0x04000000) >> 12;
> > /* bits 1: 4 --> 18:21 */
> > - dsisr |= (instr & 0x78000000) >> 17;
> > + dsisr |= (word & 0x78000000) >> 17;
> > /* bits 30:31 --> 12:13 */
> > if (IS_DSFORM(instr))
> > - dsisr |= (instr & 0x00000003) << 18;
> > + dsisr |= (word & 0x00000003) << 18;
>
> Here I would have done something like:
>
> > -static inline unsigned make_dsisr(unsigned instr)
> > +static inline unsigned make_dsisr(struct ppc_inst pi)
> > {
> > unsigned dsisr;
> > + u32 instr = ppc_inst_val(pi);
>
> and left the rest of the function unchanged.
That is better.
>
> At first I wondered why we still had that function, since IBM Power
> CPUs have not set DSISR on an alignment interrupt since POWER3 days.
> It turns out it this function is used by PR KVM when it is emulating
> one of the old 32-bit PowerPC CPUs (601, 603, 604, 750, 7450 etc.).
>
> > diff --git a/arch/powerpc/kernel/kvm.c b/arch/powerpc/kernel/kvm.c
>
> Despite the file name, this code is not used on IBM Power servers.
> It is for platforms which run under an ePAPR (not server PAPR)
> hypervisor (which would be a KVM variant, but generally book E KVM not
> book 3S).
>
> Paul.
^ permalink raw reply
* Re: [PATCH 05/10] lkdtm: disable set_fs-based tests for !CONFIG_SET_FS
From: Christoph Hellwig @ 2020-09-02 8:09 UTC (permalink / raw)
To: Kees Cook
Cc: linux-arch, linuxppc-dev, the arch/x86 maintainers,
Linux Kernel Mailing List, Al Viro, linux-fsdevel, Linus Torvalds,
Christoph Hellwig
In-Reply-To: <202009011156.0F49882@keescook>
On Tue, Sep 01, 2020 at 11:57:37AM -0700, Kees Cook wrote:
> On Sat, Aug 29, 2020 at 11:24:06AM +0200, Christoph Hellwig wrote:
> > On Thu, Aug 27, 2020 at 11:06:28AM -0700, Linus Torvalds wrote:
> > > On Thu, Aug 27, 2020 at 8:00 AM Christoph Hellwig <hch@lst.de> wrote:
> > > >
> > > > Once we can't manipulate the address limit, we also can't test what
> > > > happens when the manipulation is abused.
> > >
> > > Just remove these tests entirely.
> > >
> > > Once set_fs() doesn't exist on x86, the tests no longer make any sense
> > > what-so-ever, because test coverage will be basically zero.
> > >
> > > So don't make the code uglier just to maintain a fiction that
> > > something is tested when it isn't really.
> >
> > Sure fine with me unless Kees screams.
>
> To clarify: if any of x86, arm64, arm, powerpc, riscv, and s390 are
> using set_fs(), I want to keep this test. "ugly" is fine in lkdtm. :)
And Linus wants them gone entirely, so I'll need a stage fight between
the two of you. At least for this merge window I'm only planning on
x86 and power, plus maybe riscv if I get the work done in time. Although
helper from the maintainers would be welcome. s390 has a driver that
still uses set_fs that will need some surgery, although it shouldn't
be too bad, but arm will be a piece of work. Unless I get help it will
take a while.
^ permalink raw reply
* Re: remove the last set_fs() in common code, and remove it for x86 and powerpc v2
From: Christoph Hellwig @ 2020-09-02 8:10 UTC (permalink / raw)
To: Al Viro
Cc: linux-arch, Kees Cook, x86, linuxppc-dev, linux-kernel,
linux-fsdevel, Linus Torvalds, Christoph Hellwig
In-Reply-To: <20200901172512.GI1236603@ZenIV.linux.org.uk>
On Tue, Sep 01, 2020 at 06:25:12PM +0100, Al Viro wrote:
> On Tue, Sep 01, 2020 at 07:13:00PM +0200, Christophe Leroy wrote:
>
> > 10.92% dd [kernel.kallsyms] [k] iov_iter_zero
>
> Interesting... Could you get an instruction-level profile inside iov_iter_zero(),
> along with the disassembly of that sucker?
So the interesting thing here is with that none of these code paths
should have changed at all, and the biggest items on the profile look
the same modulo some minor reordering.
^ permalink raw reply
* Re: [PATCH] powerpc: Fix random segfault when freeing hugetlb range
From: Christophe Leroy @ 2020-09-02 8:11 UTC (permalink / raw)
To: Aneesh Kumar K.V, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <875z8weua7.fsf@linux.ibm.com>
Le 02/09/2020 à 05:23, Aneesh Kumar K.V a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>
>> The following random segfault is observed from time to time with
>> map_hugetlb selftest:
>>
>> root@localhost:~# ./map_hugetlb 1 19
>> 524288 kB hugepages
>> Mapping 1 Mbytes
>> Segmentation fault
>>
>> [ 31.219972] map_hugetlb[365]: segfault (11) at 117 nip 77974f8c lr 779a6834 code 1 in ld-2.23.so[77966000+21000]
>> [ 31.220192] map_hugetlb[365]: code: 9421ffc0 480318d1 93410028 90010044 9361002c 93810030 93a10034 93c10038
>> [ 31.220307] map_hugetlb[365]: code: 93e1003c 93210024 8123007c 81430038 <80e90004> 814a0004 7f443a14 813a0004
>> [ 31.221911] BUG: Bad rss-counter state mm:(ptrval) type:MM_FILEPAGES val:33
>> [ 31.229362] BUG: Bad rss-counter state mm:(ptrval) type:MM_ANONPAGES val:5
>>
>> This fault is due to hugetlb_free_pgd_range() freeing page tables
>> that are also used by regular pages.
>>
>> As explain in the comment at the beginning of
>> hugetlb_free_pgd_range(), the verification done in free_pgd_range()
>> on floor and ceiling is not done here, which means
>> hugetlb_free_pte_range() can free outside the expected range.
>>
>> As the verification cannot be done in hugetlb_free_pgd_range(), it
>> must be done in hugetlb_free_pte_range().
>>
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>
>> Fixes: b250c8c08c79 ("powerpc/8xx: Manage 512k huge pages as standard pages.")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/mm/hugetlbpage.c | 18 ++++++++++++++++--
>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/hugetlbpage.c b/arch/powerpc/mm/hugetlbpage.c
>> index 26292544630f..e7ae2a2c4545 100644
>> --- a/arch/powerpc/mm/hugetlbpage.c
>> +++ b/arch/powerpc/mm/hugetlbpage.c
>> @@ -330,10 +330,24 @@ static void free_hugepd_range(struct mmu_gather *tlb, hugepd_t *hpdp, int pdshif
>> get_hugepd_cache_index(pdshift - shift));
>> }
>>
>> -static void hugetlb_free_pte_range(struct mmu_gather *tlb, pmd_t *pmd, unsigned long addr)
>> +static void hugetlb_free_pte_range(struct mmu_gather *tlb, pmd_t *pmd,
>> + unsigned long addr, unsigned long end,
>> + unsigned long floor, unsigned long ceiling)
>> {
>> + unsigned long start = addr;
>> pgtable_t token = pmd_pgtable(*pmd);
>>
>> + start &= PMD_MASK;
>> + if (start < floor)
>> + return;
>> + if (ceiling) {
>> + ceiling &= PMD_MASK;
>> + if (!ceiling)
>> + return;
>> + }
>> + if (end - 1 > ceiling - 1)
>> + return;
>> +
>
> We do repeat that for pud/pmd/pte hugetlb_free_range. Can we consolidate
> that with comment explaining we are checking if the pgtable entry is
> mapping outside the range?
I was thinking about refactoring that into a helper and add all the
necessary comments to explain what it does.
Will do that in a followup series if you are OK. This patch is a bug fix
and also have to go through stable.
Christophe
^ permalink raw reply
* Re: [PATCH 0/2] dma-mapping: update default segment_boundary_mask
From: Niklas Schnelle @ 2020-09-02 8:13 UTC (permalink / raw)
To: Nicolin Chen, hch
Cc: linux-ia64, James.Bottomley, paulus, hpa, sparclinux, sfr, deller,
x86, borntraeger, mingo, mattst88, fenghua.yu, gor, linux-s390,
hca, ink, tglx, gerald.schaefer, rth, tony.luck, linux-parisc,
linux-kernel, linux-alpha, bp, linuxppc-dev, davem
In-Reply-To: <20200901221646.26491-1-nicoleotsuka@gmail.com>
On 9/2/20 12:16 AM, Nicolin Chen wrote:
> These two patches are to update default segment_boundary_mask.
>
> PATCH-1 fixes overflow issues in callers of dma_get_seg_boundary.
> Previous version was a series: https://lkml.org/lkml/2020/8/31/1026
>
> Then PATCH-2 sets default segment_boundary_mask to ULONG_MAX.
>
> Nicolin Chen (2):
> dma-mapping: introduce dma_get_seg_boundary_nr_pages()
> dma-mapping: set default segment_boundary_mask to ULONG_MAX
I gave both of your patches a quick test ride on a couple of dev mainframes,
both NVMe, ConnectX and virtio-pci devices all seems to work fine.
I already commented on Christoph's mail that I like the helper approach,
so as for s390 you can add my
Acked-by: Niklas Schnelle <schnelle@linux.ibm.com>
>
> arch/alpha/kernel/pci_iommu.c | 7 +------
> arch/ia64/hp/common/sba_iommu.c | 3 +--
> arch/powerpc/kernel/iommu.c | 9 ++-------
> arch/s390/pci/pci_dma.c | 6 ++----
> arch/sparc/kernel/iommu-common.c | 10 +++-------
> arch/sparc/kernel/iommu.c | 3 +--
> arch/sparc/kernel/pci_sun4v.c | 3 +--
> arch/x86/kernel/amd_gart_64.c | 3 +--
> drivers/parisc/ccio-dma.c | 3 +--
> drivers/parisc/sba_iommu.c | 3 +--
> include/linux/dma-mapping.h | 21 ++++++++++++++++++++-
> 11 files changed, 34 insertions(+), 37 deletions(-)
>
^ permalink raw reply
* Re: [PATCH] powerpc: Fix random segfault when freeing hugetlb range
From: Aneesh Kumar K.V @ 2020-09-02 8:15 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <96409d24-c8bf-7f3a-0a81-0830174d6bcc@csgroup.eu>
On 9/2/20 1:41 PM, Christophe Leroy wrote:
>
>
> Le 02/09/2020 à 05:23, Aneesh Kumar K.V a écrit :
>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>
>>> The following random segfault is observed from time to time with
>>> map_hugetlb selftest:
>>>
>>> root@localhost:~# ./map_hugetlb 1 19
>>> 524288 kB hugepages
>>> Mapping 1 Mbytes
>>> Segmentation fault
>>>
>>> [ 31.219972] map_hugetlb[365]: segfault (11) at 117 nip 77974f8c lr
>>> 779a6834 code 1 in ld-2.23.so[77966000+21000]
>>> [ 31.220192] map_hugetlb[365]: code: 9421ffc0 480318d1 93410028
>>> 90010044 9361002c 93810030 93a10034 93c10038
>>> [ 31.220307] map_hugetlb[365]: code: 93e1003c 93210024 8123007c
>>> 81430038 <80e90004> 814a0004 7f443a14 813a0004
>>> [ 31.221911] BUG: Bad rss-counter state mm:(ptrval)
>>> type:MM_FILEPAGES val:33
>>> [ 31.229362] BUG: Bad rss-counter state mm:(ptrval)
>>> type:MM_ANONPAGES val:5
>>>
>>> This fault is due to hugetlb_free_pgd_range() freeing page tables
>>> that are also used by regular pages.
>>>
>>> As explain in the comment at the beginning of
>>> hugetlb_free_pgd_range(), the verification done in free_pgd_range()
>>> on floor and ceiling is not done here, which means
>>> hugetlb_free_pte_range() can free outside the expected range.
>>>
>>> As the verification cannot be done in hugetlb_free_pgd_range(), it
>>> must be done in hugetlb_free_pte_range().
>>>
>>
>> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>
>>> Fixes: b250c8c08c79 ("powerpc/8xx: Manage 512k huge pages as standard
>>> pages.")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> arch/powerpc/mm/hugetlbpage.c | 18 ++++++++++++++++--
>>> 1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/hugetlbpage.c
>>> b/arch/powerpc/mm/hugetlbpage.c
>>> index 26292544630f..e7ae2a2c4545 100644
>>> --- a/arch/powerpc/mm/hugetlbpage.c
>>> +++ b/arch/powerpc/mm/hugetlbpage.c
>>> @@ -330,10 +330,24 @@ static void free_hugepd_range(struct mmu_gather
>>> *tlb, hugepd_t *hpdp, int pdshif
>>> get_hugepd_cache_index(pdshift - shift));
>>> }
>>> -static void hugetlb_free_pte_range(struct mmu_gather *tlb, pmd_t
>>> *pmd, unsigned long addr)
>>> +static void hugetlb_free_pte_range(struct mmu_gather *tlb, pmd_t *pmd,
>>> + unsigned long addr, unsigned long end,
>>> + unsigned long floor, unsigned long ceiling)
>>> {
>>> + unsigned long start = addr;
>>> pgtable_t token = pmd_pgtable(*pmd);
>>> + start &= PMD_MASK;
>>> + if (start < floor)
>>> + return;
>>> + if (ceiling) {
>>> + ceiling &= PMD_MASK;
>>> + if (!ceiling)
>>> + return;
>>> + }
>>> + if (end - 1 > ceiling - 1)
>>> + return;
>>> +
>>
>> We do repeat that for pud/pmd/pte hugetlb_free_range. Can we consolidate
>> that with comment explaining we are checking if the pgtable entry is
>> mapping outside the range?
>
> I was thinking about refactoring that into a helper and add all the
> necessary comments to explain what it does.
>
> Will do that in a followup series if you are OK. This patch is a bug fix
> and also have to go through stable.
>
agreed.
Thanks.
-aneesh
^ permalink raw reply
* Re: ptrace_syscall_32 is failing
From: Thomas Gleixner @ 2020-09-02 8:29 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-s390, linuxppc-dev, Vasily Gorbik, Brian Gerst,
Heiko Carstens, X86 ML, LKML, Christian Borntraeger,
Paul Mackerras, Catalin Marinas, Andy Lutomirski, Will Deacon,
linux-arm-kernel
In-Reply-To: <CALCETrUpjUPPvnPuS9fP4jgid7U_qdU_yTKSq9PjJ=z2w9HvHg@mail.gmail.com>
On Tue, Sep 01 2020 at 17:09, Andy Lutomirski wrote:
> On Tue, Sep 1, 2020 at 4:50 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> > I think that they almost work for x86, but not quite as
>> > indicated by this bug. Even if we imagine we can somehow hack around
>> > this bug, I imagine we're going to find other problems with this
>> > model, e.g. the potential upcoming exit problem I noted in my review.
>>
>> What's the upcoming problem?
>
> If we ever want to get single-stepping fully correct across syscalls,
> we might need to inject SIGTRAP on syscall return. This would be more
> awkward if we can't run instrumentable code after the syscall part of
> the syscall is done.
We run a lot of instrumentable code after sys_foo() returns. Otherwise
all the TIF work would not be possible at all.
But you might tell me where exactly you want to inject the SIGTRAP in
the syscall exit code flow.
>> I don't think we want that in general. The current variant is perfectly
>> fine for everything except the 32bit fast syscall nonsense. Also
>> irqentry_entry/exit is not equivalent to the syscall_enter/exit
>> counterparts.
>
> If there are any architectures in which actual work is needed to
> figure out whether something is a syscall in the first place, they'll
> want to do the usual kernel entry work before the syscall entry work.
That's low level entry code which does not require RCU, lockdep, tracing
or whatever muck we setup before actual work can be done.
arch_asm_entry()
...
arch_c_entry(cause) {
switch(cause) {
case EXCEPTION: arch_c_exception(...);
case SYSCALL: arch_c_syscall(...);
...
}
You really want to differentiate between exception and syscall
entry/exit.
The splitting of syscall_enter_from_user_mode() is only necessary for
that 32bit fast syscall thing on x86 and there is no point to open code
it with two calls for e.g. do_syscall_64().
> Maybe your patch actually makes this possible -- I haven't digested
> all the details yet.
>
> Who advised you to drop the arch parameter?
Kees, IIRC, but I would have to search through the gazillions of mail
threads to be sure.
>> + syscall_enter_from_user_mode_prepare(regs);
>
> I'm getting lost in all these "enter" functions...
It's not that hard.
syscall_enter_from_user_mode_prepare()
+ syscall_enter_from_user_mode_work()
= syscall_enter_from_user_mode()
That's exactly what you suggested just with the difference that it is
explicit for syscalls and not using irqentry_enter/exit().
If we would do that then instead of having a single call for sane
syscall pathes:
arch_c_entry()
nr = syscall_enter_from_user_mode();
or for that 32bit fast syscall nonsense the split variant:
arch_c_entry()
syscall_enter_from_user_mode_prepare();
do_fast_syscall_muck();
nr = syscall_enter_from_user_mode_work();
we'd have:
arch_c_entry()
irqentry_enter();
local_irq_enble();
nr = syscall_enter_from_user_mode_work();
...
which enforces two calls for sane entries and more code in arch/....
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH] cpuidle-pseries: Fix CEDE latency conversion from tb to us
From: Gautham R Shenoy @ 2020-09-02 8:35 UTC (permalink / raw)
To: Joel Stanley
Cc: Gautham R. Shenoy, linux-pm, Rafael J. Wysocki,
Linux Kernel Mailing List, Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <CACPK8XfZdnKusEuu8i=-aH=Wfr6X6sMrvX=btFq9PtnXJ2w-SQ@mail.gmail.com>
Hello Joel,
On Wed, Sep 02, 2020 at 01:08:35AM +0000, Joel Stanley wrote:
> On Tue, 1 Sep 2020 at 14:09, Gautham R. Shenoy <ego@linux.vnet.ibm.com> wrote:
> >
> > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> >
> > commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> > CEDE(0)") sets the exit latency of CEDE(0) based on the latency values
> > of the Extended CEDE states advertised by the platform. The values
> > advertised by the platform are in timebase ticks. However the cpuidle
> > framework requires the latency values in microseconds.
> >
> > If the tb-ticks value advertised by the platform correspond to a value
> > smaller than 1us, during the conversion from tb-ticks to microseconds,
> > in the current code, the result becomes zero. This is incorrect as it
> > puts a CEDE state on par with the snooze state.
> >
> > This patch fixes this by rounding up the result obtained while
> > converting the latency value from tb-ticks to microseconds.
> >
> > Fixes: commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> > CEDE(0)")
> >
> > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
>
Thanks for reviewing the fix.
> Should you check for the zero case and print a warning?
Yes, that would be better. I will post a v2 with that.
>
> > ---
> > drivers/cpuidle/cpuidle-pseries.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> > index ff6d99e..9043358 100644
> > --- a/drivers/cpuidle/cpuidle-pseries.c
> > +++ b/drivers/cpuidle/cpuidle-pseries.c
> > @@ -361,7 +361,7 @@ static void __init fixup_cede0_latency(void)
> > for (i = 0; i < nr_xcede_records; i++) {
> > struct xcede_latency_record *record = &payload->records[i];
> > u64 latency_tb = be64_to_cpu(record->latency_ticks);
> > - u64 latency_us = tb_to_ns(latency_tb) / NSEC_PER_USEC;
> > + u64 latency_us = DIV_ROUND_UP_ULL(tb_to_ns(latency_tb), NSEC_PER_USEC);
> >
> > if (latency_us < min_latency_us)
> > min_latency_us = latency_us;
> > --
> > 1.9.4
> >
^ permalink raw reply
* [PATCH] mm: check for memory's node later during boot
From: Laurent Dufour @ 2020-09-02 9:09 UTC (permalink / raw)
To: linux-kernel, linuxppc-dev, linux-mm, Greg Kroah-Hartman
Cc: nathanl, cheloha, Andrew Morton, Rafael J. Wysocki
register_mem_sect_under_nodem() is checking the memory block's node id only
if the system state is "SYSTEM_BOOTING". On PowerPC, the memory blocks are
registered while the system state is "SYSTEM_SCHEDULING", the one before
SYSTEM_RUNNING.
The consequence on PowerPC guest with interleaved memory node's ranges is
that some memory block could be assigned to multiple nodes on sysfs. This
lately prevents some memory hot-plug and hot-unplug to succeed because
links are remaining. Such a panic is then displayed:
------------[ cut here ]------------
kernel BUG at /Users/laurent/src/linux-ppc/mm/memory_hotplug.c:1084!
Oops: Exception in kernel mode, sig: 5 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: rpadlpar_io rpaphp pseries_rng rng_core vmx_crypto gf128mul binfmt_misc ip_tables x_tables xfs libcrc32c crc32c_vpmsum autofs4
CPU: 8 PID: 10256 Comm: drmgr Not tainted 5.9.0-rc1+ #25
NIP: c000000000403f34 LR: c000000000403f2c CTR: 0000000000000000
REGS: c0000004876e3660 TRAP: 0700 Not tainted (5.9.0-rc1+)
MSR: 800000000282b033 <SF,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24000448 XER: 20040000
CFAR: c000000000846d20 IRQMASK: 0
GPR00: c000000000403f2c c0000004876e38f0 c0000000012f6f00 ffffffffffffffef
GPR04: 0000000000000227 c0000004805ae680 0000000000000000 00000004886f0000
GPR08: 0000000000000226 0000000000000003 0000000000000002 fffffffffffffffd
GPR12: 0000000088000484 c00000001ec96280 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000004 0000000000000003
GPR20: c00000047814ffe0 c0000007ffff7c08 0000000000000010 c0000000013332c8
GPR24: 0000000000000000 c0000000011f6cc0 0000000000000000 0000000000000000
GPR28: ffffffffffffffef 0000000000000001 0000000150000000 0000000010000000
NIP [c000000000403f34] add_memory_resource+0x244/0x340
LR [c000000000403f2c] add_memory_resource+0x23c/0x340
Call Trace:
[c0000004876e38f0] [c000000000403f2c] add_memory_resource+0x23c/0x340 (unreliable)
[c0000004876e39c0] [c00000000040408c] __add_memory+0x5c/0xf0
[c0000004876e39f0] [c0000000000e2b94] dlpar_add_lmb+0x1b4/0x500
[c0000004876e3ad0] [c0000000000e3888] dlpar_memory+0x1f8/0xb80
[c0000004876e3b60] [c0000000000dc0d0] handle_dlpar_errorlog+0xc0/0x190
[c0000004876e3bd0] [c0000000000dc398] dlpar_store+0x198/0x4a0
[c0000004876e3c90] [c00000000072e630] kobj_attr_store+0x30/0x50
[c0000004876e3cb0] [c00000000051f954] sysfs_kf_write+0x64/0x90
[c0000004876e3cd0] [c00000000051ee40] kernfs_fop_write+0x1b0/0x290
[c0000004876e3d20] [c000000000438dd8] vfs_write+0xe8/0x290
[c0000004876e3d70] [c0000000004391ac] ksys_write+0xdc/0x130
[c0000004876e3dc0] [c000000000034e40] system_call_exception+0x160/0x270
[c0000004876e3e20] [c00000000000d740] system_call_common+0xf0/0x27c
Instruction dump:
48442e35 60000000 0b030000 3cbe0001 7fa3eb78 7bc48402 38a5fffe 7ca5fa14
78a58402 48442db1 60000000 7c7c1b78 <0b030000> 7f23cb78 4bda371d 60000000
---[ end trace 562fd6c109cd0fb2 ]---
To prevent this multiple links, make the node checking done for states
prior to SYSTEM_RUNNING.
Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Fixes: 4fbce633910e ("mm/memory_hotplug.c: make register_mem_sect_under_node() a callback of walk_memory_range()")
---
drivers/base/node.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 508b80f6329b..8e9f39b562ef 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -789,7 +789,7 @@ static int register_mem_sect_under_node(struct memory_block *mem_blk,
* case, during hotplug we know that all pages in the memory
* block belong to the same node.
*/
- if (system_state == SYSTEM_BOOTING) {
+ if (system_state < SYSTEM_RUNNING) {
page_nid = get_nid_for_pfn(pfn);
if (page_nid < 0)
continue;
--
2.28.0
^ permalink raw reply related
* Re: [RFC PATCH 2/2] KVM: PPC: Book3S HV: Support prefixed instructions
From: Jordan Niethe @ 2020-09-02 9:19 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <20200902061829.GF272502@thinks.paulus.ozlabs.org>
On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras <paulus@ozlabs.org> wrote:
>
> On Thu, Aug 20, 2020 at 01:39:22PM +1000, Jordan Niethe wrote:
> > There are two main places where instructions are loaded from the guest:
> > * Emulate loadstore - such as when performing MMIO emulation
> > triggered by an HDSI
> > * After an HV emulation assistance interrupt (e40)
> >
> > If it is a prefixed instruction that triggers these cases, its suffix
> > must be loaded. Use the SRR1_PREFIX bit to decide if a suffix needs to
> > be loaded. Make sure if this bit is set inject_interrupt() also sets it
> > when giving an interrupt to the guest.
> >
> > ISA v3.10 extends the Hypervisor Emulation Instruction Register (HEIR)
> > to 64 bits long to accommodate prefixed instructions. For interrupts
> > caused by a word instruction the instruction is loaded into bits 32:63
> > and bits 0:31 are zeroed. When caused by a prefixed instruction the
> > prefix and suffix are loaded into bits 0:63.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/kvm/book3s.c | 15 +++++++++++++--
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 10 +++++++---
> > arch/powerpc/kvm/book3s_hv_builtin.c | 3 +++
> > arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 ++++++++++++++
> > 4 files changed, 37 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> > index 70d8967acc9b..18b1928a571b 100644
> > --- a/arch/powerpc/kvm/book3s.c
> > +++ b/arch/powerpc/kvm/book3s.c
> > @@ -456,13 +456,24 @@ int kvmppc_load_last_inst(struct kvm_vcpu *vcpu,
> > {
> > ulong pc = kvmppc_get_pc(vcpu);
> > u32 word;
> > + u64 doubleword;
> > int r;
> >
> > if (type == INST_SC)
> > pc -= 4;
> >
> > - r = kvmppc_ld(vcpu, &pc, sizeof(u32), &word, false);
> > - *inst = ppc_inst(word);
> > + if ((kvmppc_get_msr(vcpu) & SRR1_PREFIXED)) {
> > + r = kvmppc_ld(vcpu, &pc, sizeof(u64), &doubleword, false);
>
> Should we also have a check here that the doubleword is not crossing a
> page boundary? I can't think of a way to get this code to cross a
> page boundary, assuming the hardware is working correctly, but it
> makes me just a little nervous.
I didn't think it could happen but I will add a check to be safe.
>
> > +#ifdef CONFIG_CPU_LITTLE_ENDIAN
> > + *inst = ppc_inst_prefix(doubleword & 0xffffffff, doubleword >> 32);
> > +#else
> > + *inst = ppc_inst_prefix(doubleword >> 32, doubleword & 0xffffffff);
> > +#endif
>
> Ick. Is there a cleaner way to do this?
Would it be nicer to read the prefix as u32 then the suffix as a u32 too?
>
> > + } else {
> > + r = kvmppc_ld(vcpu, &pc, sizeof(u32), &word, false);
> > + *inst = ppc_inst(word);
> > + }
> > +
> > if (r == EMULATE_DONE)
> > return r;
> > else
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index 775ce41738ce..0802471f4856 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -411,9 +411,13 @@ static int instruction_is_store(struct ppc_inst instr)
> > unsigned int mask;
> >
> > mask = 0x10000000;
> > - if ((ppc_inst_val(instr) & 0xfc000000) == 0x7c000000)
> > - mask = 0x100; /* major opcode 31 */
> > - return (ppc_inst_val(instr) & mask) != 0;
> > + if (ppc_inst_prefixed(instr)) {
> > + return (ppc_inst_suffix(instr) & mask) != 0;
> > + } else {
> > + if ((ppc_inst_val(instr) & 0xfc000000) == 0x7c000000)
> > + mask = 0x100; /* major opcode 31 */
> > + return (ppc_inst_val(instr) & mask) != 0;
> > + }
>
> The way the code worked before, the mask depended on whether the
> instruction was a D-form (or DS-form or other variant) instruction,
> where you can tell loads and stores apart by looking at the major
> opcode, or an X-form instruction, where you look at the minor opcode.
>
> Now we are only looking at the minor opcode if it is not a prefixed
> instruction. Are there no X-form prefixed loads or stores?
I could not see an X-form load/stores so I went with just that.
But checking the ISA it does mention "..X-form instructions that are
preceded by an MLS-form or MMLS-form prefix..." so I shall use the
other mask too.
>
> Paul.
Thank you for the comments and suggestions.
^ permalink raw reply
* Re: [RFC PATCH 1/2] KVM: PPC: Use the ppc_inst type
From: Paul Mackerras @ 2020-09-02 9:32 UTC (permalink / raw)
To: Jordan Niethe; +Cc: linuxppc-dev, Nicholas Piggin, kvm-ppc
In-Reply-To: <CACzsE9qrgs8ujQ7HeHVo-8oyY2bdwFVnVxR5dEZns5V7qK7Cbg@mail.gmail.com>
On Wed, Sep 02, 2020 at 06:00:24PM +1000, Jordan Niethe wrote:
> On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras <paulus@ozlabs.org> wrote:
> >
> > On Thu, Aug 20, 2020 at 01:39:21PM +1000, Jordan Niethe wrote:
> > > The ppc_inst type was added to help cope with the addition of prefixed
> > > instructions to the ISA. Convert KVM to use this new type for dealing
> > > wiht instructions. For now do not try to add further support for
> > > prefixed instructions.
> >
> > This change does seem to splatter itself across a lot of code that
> > mostly or exclusively runs on machines which are not POWER10 and will
> > never need to handle prefixed instructions, unfortunately. I wonder
> > if there is a less invasive way to approach this.
> Something less invasive would be good.
> >
> > In particular we are inflicting this 64-bit struct on 32-bit platforms
> > unnecessarily (I assume, correct me if I am wrong here).
> No, that is something that I wanted to to avoid, on 32 bit platforms
> it is a 32bit struct:
>
> struct ppc_inst {
> u32 val;
> #ifdef CONFIG_PPC64
> u32 suffix;
> #endif
> } __packed;
> >
> > How would it be to do something like:
> >
> > typedef unsigned long ppc_inst_t;
> >
> > so it is 32 bits on 32-bit platforms and 64 bits on 64-bit platforms,
> > and then use that instead of 'struct ppc_inst'? You would still need
> > to change the function declarations but I think most of the function
> > bodies would not need to be changed. In particular you would avoid a
> > lot of the churn related to having to add ppc_inst_val() and suchlike.
>
> Would the idea be to get rid of `struct ppc_inst` entirely or just not
> use it in kvm?
> In an earlier series I did something similar (at least code shared
> between 32bit and 64bit would need helpers, but 32bit only code need
> not change):
>
> #ifdef __powerpc64__
>
> typedef struct ppc_inst {
> union {
> struct {
> u32 word;
> u32 pad;
> } __packed;
> struct {
> u32 prefix;
> u32 suffix;
> } __packed;
> };
> } ppc_inst;
>
> #else /* !__powerpc64__ */
>
> typedef u32 ppc_inst;
> #endif
>
> However mpe wanted to avoid using a typedef
> (https://patchwork.ozlabs.org/comment/2391845/)
Well it doesn't have to be typedef'd, it could just be "unsigned
long", which is used in other places for things that want to be 32-bit
on 32-bit machines and 64-bit on 64-bit machines.
I do however think that it should be a numeric type so that we can
mask, shift and compare it more easily. I know that's less "abstract"
but it's also a lot less obfuscated and I think that will lead to
clearer code. If you got the opposite advice from Michael Ellerman or
Nick Piggin then I will discuss it with them.
> We did also talk about just using a u64 for instructions
> (https://lore.kernel.org/linuxppc-dev/1585028462.t27rstc2uf.astroid@bobo.none/)
> but the concern was that as prefixed instructions act as two separate
> u32s (prefix is always before the suffix regardless of endianess)
> keeping it as a u64 would lead to lot of macros and potential
> confusion.
> But it does seem if that can avoid a lot of needless churn it might
> worth the trade off.
u32 *ip;
instr = *ip++;
if (is_prefix(instr) && is_suitably_aligned(ip))
instr = (instr << 32) | *ip++;
would avoid the endian issues pretty cleanly I think. In other words
the prefix would always be the high half of the 64-bit value, so you
can't just do a single 64-bit of the instruction on little-endian
platforms; but you can't do a single 64-bit load for other reasons as
well, such as alignment.
Paul.
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/64s/radix: Fix mm_cpumask trimming race vs kthread_use_mm
From: Nicholas Piggin @ 2020-09-02 9:48 UTC (permalink / raw)
To: linux-mm, Michael Ellerman
Cc: Jens Axboe, linux-arch, Peter Zijlstra, Aneesh Kumar K.V,
linux-kernel, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <87pn751zcb.fsf@mpe.ellerman.id.au>
Excerpts from Michael Ellerman's message of September 1, 2020 10:00 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> Commit 0cef77c7798a7 ("powerpc/64s/radix: flush remote CPUs out of
>> single-threaded mm_cpumask") added a mechanism to trim the mm_cpumask of
>> a process under certain conditions. One of the assumptions is that
>> mm_users would not be incremented via a reference outside the process
>> context with mmget_not_zero() then go on to kthread_use_mm() via that
>> reference.
>>
>> That invariant was broken by io_uring code (see previous sparc64 fix),
>> but I'll point Fixes: to the original powerpc commit because we are
>> changing that assumption going forward, so this will make backports
>> match up.
>>
>> Fix this by no longer relying on that assumption, but by having each CPU
>> check the mm is not being used, and clearing their own bit from the mask
>> if it's okay. This fix relies on commit 38cf307c1f20 ("mm: fix
>> kthread_use_mm() vs TLB invalidate") to disable irqs over the mm switch,
>> and ARCH_WANT_IRQS_OFF_ACTIVATE_MM to be enabled.
>
> You could use:
>
> Depends-on: 38cf307c1f20 ("mm: fix kthread_use_mm() vs TLB invalidate")
Good idea I wil.
>> Fixes: 0cef77c7798a7 ("powerpc/64s/radix: flush remote CPUs out of single-threaded mm_cpumask")
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/tlb.h | 13 -------------
>> arch/powerpc/mm/book3s64/radix_tlb.c | 23 ++++++++++++++++-------
>> 2 files changed, 16 insertions(+), 20 deletions(-)
>
> One minor nit below if you're respinning anyway.
>
> You know this stuff better than me, but I still reviewed it and it seems
> good to me.
>
> Reviewed-by: Michael Ellerman <mpe@ellerman.id.au>
Thanks.
>
>> diff --git a/arch/powerpc/include/asm/tlb.h b/arch/powerpc/include/asm/tlb.h
>> index fbc6f3002f23..d97f061fecac 100644
>> --- a/arch/powerpc/include/asm/tlb.h
>> +++ b/arch/powerpc/include/asm/tlb.h
>> @@ -66,19 +66,6 @@ static inline int mm_is_thread_local(struct mm_struct *mm)
>> return false;
>> return cpumask_test_cpu(smp_processor_id(), mm_cpumask(mm));
>> }
>> -static inline void mm_reset_thread_local(struct mm_struct *mm)
>> -{
>> - WARN_ON(atomic_read(&mm->context.copros) > 0);
>> - /*
>> - * It's possible for mm_access to take a reference on mm_users to
>> - * access the remote mm from another thread, but it's not allowed
>> - * to set mm_cpumask, so mm_users may be > 1 here.
>> - */
>> - WARN_ON(current->mm != mm);
>> - atomic_set(&mm->context.active_cpus, 1);
>> - cpumask_clear(mm_cpumask(mm));
>> - cpumask_set_cpu(smp_processor_id(), mm_cpumask(mm));
>> -}
>> #else /* CONFIG_PPC_BOOK3S_64 */
>> static inline int mm_is_thread_local(struct mm_struct *mm)
>> {
>> diff --git a/arch/powerpc/mm/book3s64/radix_tlb.c b/arch/powerpc/mm/book3s64/radix_tlb.c
>> index 0d233763441f..a421a0e3f930 100644
>> --- a/arch/powerpc/mm/book3s64/radix_tlb.c
>> +++ b/arch/powerpc/mm/book3s64/radix_tlb.c
>> @@ -645,19 +645,29 @@ static void do_exit_flush_lazy_tlb(void *arg)
>> struct mm_struct *mm = arg;
>> unsigned long pid = mm->context.id;
>>
>> + /*
>> + * A kthread could have done a mmget_not_zero() after the flushing CPU
>> + * checked mm_users == 1, and be in the process of kthread_use_mm when
> ^
> in mm_is_singlethreaded()
>
> Adding that reference would help join the dots for a new reader I think.
Yes you're right I can change that.
Thanks,
Nick
^ 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