* 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
* Re: [PATCH 2/2] powerpc/vdso32: link vdso64 with linker
From: Christophe Leroy @ 2020-09-02 10:16 UTC (permalink / raw)
To: Nick Desaulniers, Michael Ellerman, Nicholas Piggin
Cc: 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
Finally I spotted it I think:
make arch/powerpc/kernel/vdso32/ V=1
powerpc64-linux-ld -EB -m elf64ppc -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
If I do the same manually but with -m elf32ppc instead of -m elf64ppc,
there is no failure.
Adding -m elf32ppc to ldflags-y also works, allthough I don't like too
much having "-m elf64ppc -m elf32ppc" on the line.
Christophe
^ permalink raw reply
* [Bug 209029] kernel 5.9-rc2 fails to boot on a PowerMac G5 11,2 - BUG: Kernel NULL pointer dereference on read at 0x00000020
From: bugzilla-daemon @ 2020-09-02 10:48 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-209029-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=209029
--- Comment #4 from Erhard F. (erhard_f@mailbox.org) ---
(In reply to Christophe Leroy from comment #3)
> Did you try without CONFIG_DEBUG_VM_PGTABLE ?
Without CONFIG_DEBUG_VM_PGTABLE the G5 boots fine. Thanks!
> If you want CONFIG_DEBUG_VM_PGTABLE, the following series aims at fixing it
> for PPC64:
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=197961
Did not check the series as current ozlabs patches indicate that the
CONFIG_DEBUG_VM_PGTABLE option is removed for the time being.
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [PATCH v4 00/13] mm/debug_vm_pgtable fixes
From: Aneesh Kumar K.V @ 2020-09-02 11:42 UTC (permalink / raw)
To: linux-mm, akpm; +Cc: linuxppc-dev, Aneesh Kumar K.V, Anshuman Khandual
This patch series includes fixes for debug_vm_pgtable test code so that
they follow page table updates rules correctly. The first two patches introduce
changes w.r.t ppc64. The patches are included in this series for completeness. We can
merge them via ppc64 tree if required.
Hugetlb test is disabled on ppc64 because that needs larger change to satisfy
page table update rules.
These tests are broken w.r.t page table update rules and results in kernel
crash as below.
[ 21.083519] kernel BUG at arch/powerpc/mm/pgtable.c:304!
cpu 0x0: Vector: 700 (Program Check) at [c000000c6d1e76c0]
pc: c00000000009a5ec: assert_pte_locked+0x14c/0x380
lr: c0000000005eeeec: pte_update+0x11c/0x190
sp: c000000c6d1e7950
msr: 8000000002029033
current = 0xc000000c6d172c80
paca = 0xc000000003ba0000 irqmask: 0x03 irq_happened: 0x01
pid = 1, comm = swapper/0
kernel BUG at arch/powerpc/mm/pgtable.c:304!
[link register ] c0000000005eeeec pte_update+0x11c/0x190
[c000000c6d1e7950] 0000000000000001 (unreliable)
[c000000c6d1e79b0] c0000000005eee14 pte_update+0x44/0x190
[c000000c6d1e7a10] c000000001a2ca9c pte_advanced_tests+0x160/0x3d8
[c000000c6d1e7ab0] c000000001a2d4fc debug_vm_pgtable+0x7e8/0x1338
[c000000c6d1e7ba0] c0000000000116ec do_one_initcall+0xac/0x5f0
[c000000c6d1e7c80] c0000000019e4fac kernel_init_freeable+0x4dc/0x5a4
[c000000c6d1e7db0] c000000000012474 kernel_init+0x24/0x160
[c000000c6d1e7e20] c00000000000cbd0 ret_from_kernel_thread+0x5c/0x6c
With DEBUG_VM disabled
[ 20.530152] BUG: Kernel NULL pointer dereference on read at 0x00000000
[ 20.530183] Faulting instruction address: 0xc0000000000df330
cpu 0x33: Vector: 380 (Data SLB Access) at [c000000c6d19f700]
pc: c0000000000df330: memset+0x68/0x104
lr: c00000000009f6d8: hash__pmdp_huge_get_and_clear+0xe8/0x1b0
sp: c000000c6d19f990
msr: 8000000002009033
dar: 0
current = 0xc000000c6d177480
paca = 0xc00000001ec4f400 irqmask: 0x03 irq_happened: 0x01
pid = 1, comm = swapper/0
[link register ] c00000000009f6d8 hash__pmdp_huge_get_and_clear+0xe8/0x1b0
[c000000c6d19f990] c00000000009f748 hash__pmdp_huge_get_and_clear+0x158/0x1b0 (unreliable)
[c000000c6d19fa10] c0000000019ebf30 pmd_advanced_tests+0x1f0/0x378
[c000000c6d19fab0] c0000000019ed088 debug_vm_pgtable+0x79c/0x1244
[c000000c6d19fba0] c0000000000116ec do_one_initcall+0xac/0x5f0
[c000000c6d19fc80] c0000000019a4fac kernel_init_freeable+0x4dc/0x5a4
[c000000c6d19fdb0] c000000000012474 kernel_init+0x24/0x160
[c000000c6d19fe20] c00000000000cbd0 ret_from_kernel_thread+0x5c/0x6c
Changes from v3:
* Address review feedback
* Move page table depost and withdraw patch after adding pmdlock to avoid bisect failure.
Changes from v2:
* Fix build failure with different configs and architecture.
Changes from v1:
* Address review feedback
* drop test specific pfn_pte and pfn_pmd.
* Update ppc64 page table helper to add _PAGE_PTE
Aneesh Kumar K.V (13):
powerpc/mm: Add DEBUG_VM WARN for pmd_clear
powerpc/mm: Move setting pte specific flags to pfn_pte
mm/debug_vm_pgtable/ppc64: Avoid setting top bits in radom value
mm/debug_vm_pgtables/hugevmap: Use the arch helper to identify huge
vmap support.
mm/debug_vm_pgtable/savedwrite: Enable savedwrite test with
CONFIG_NUMA_BALANCING
mm/debug_vm_pgtable/THP: Mark the pte entry huge before using
set_pmd/pud_at
mm/debug_vm_pgtable/set_pte/pmd/pud: Don't use set_*_at to update an
existing pte entry
mm/debug_vm_pgtable/locks: Move non page table modifying test together
mm/debug_vm_pgtable/locks: Take correct page table lock
mm/debug_vm_pgtable/thp: Use page table depost/withdraw with THP
mm/debug_vm_pgtable/pmd_clear: Don't use pmd/pud_clear on pte entries
mm/debug_vm_pgtable/hugetlb: Disable hugetlb test on ppc64
mm/debug_vm_pgtable: Avoid none pte in pte_clear_test
arch/powerpc/include/asm/book3s/64/pgtable.h | 29 +++-
arch/powerpc/include/asm/nohash/pgtable.h | 5 -
arch/powerpc/mm/pgtable.c | 5 -
mm/debug_vm_pgtable.c | 171 ++++++++++++-------
4 files changed, 131 insertions(+), 79 deletions(-)
--
2.26.2
^ permalink raw reply
* [PATCH v4 02/13] powerpc/mm: Move setting pte specific flags to pfn_pte
From: Aneesh Kumar K.V @ 2020-09-02 11:42 UTC (permalink / raw)
To: linux-mm, akpm; +Cc: linuxppc-dev, Aneesh Kumar K.V, Anshuman Khandual
In-Reply-To: <20200902114222.181353-1-aneesh.kumar@linux.ibm.com>
powerpc used to set the pte specific flags in set_pte_at(). This is
different from other architectures. To be consistent with other
architecture update pfn_pte to set _PAGE_PTE on ppc64. Also, drop now
unused pte_mkpte.
We add a VM_WARN_ON() to catch the usage of calling set_pte_at()
without setting _PAGE_PTE bit. We will remove that after a few releases.
With respect to huge pmd entries, pmd_mkhuge() takes care of adding the
_PAGE_PTE bit.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 15 +++++++++------
arch/powerpc/include/asm/nohash/pgtable.h | 5 -----
arch/powerpc/mm/pgtable.c | 5 -----
3 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 079211968987..2382fd516f6b 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -619,7 +619,7 @@ static inline pte_t pfn_pte(unsigned long pfn, pgprot_t pgprot)
VM_BUG_ON(pfn >> (64 - PAGE_SHIFT));
VM_BUG_ON((pfn << PAGE_SHIFT) & ~PTE_RPN_MASK);
- return __pte(((pte_basic_t)pfn << PAGE_SHIFT) | pgprot_val(pgprot));
+ return __pte(((pte_basic_t)pfn << PAGE_SHIFT) | pgprot_val(pgprot) | _PAGE_PTE);
}
static inline unsigned long pte_pfn(pte_t pte)
@@ -655,11 +655,6 @@ static inline pte_t pte_mkexec(pte_t pte)
return __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_EXEC));
}
-static inline pte_t pte_mkpte(pte_t pte)
-{
- return __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_PTE));
-}
-
static inline pte_t pte_mkwrite(pte_t pte)
{
/*
@@ -823,6 +818,14 @@ static inline int pte_none(pte_t pte)
static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr,
pte_t *ptep, pte_t pte, int percpu)
{
+
+ VM_WARN_ON(!(pte_raw(pte) & cpu_to_be64(_PAGE_PTE)));
+ /*
+ * Keep the _PAGE_PTE added till we are sure we handle _PAGE_PTE
+ * in all the callers.
+ */
+ pte = __pte_raw(pte_raw(pte) | cpu_to_be64(_PAGE_PTE));
+
if (radix_enabled())
return radix__set_pte_at(mm, addr, ptep, pte, percpu);
return hash__set_pte_at(mm, addr, ptep, pte, percpu);
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index 4b7c3472eab1..6277e7596ae5 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -140,11 +140,6 @@ static inline pte_t pte_mkold(pte_t pte)
return __pte(pte_val(pte) & ~_PAGE_ACCESSED);
}
-static inline pte_t pte_mkpte(pte_t pte)
-{
- return pte;
-}
-
static inline pte_t pte_mkspecial(pte_t pte)
{
return __pte(pte_val(pte) | _PAGE_SPECIAL);
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 9c0547d77af3..ab57b07ef39a 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -184,9 +184,6 @@ void set_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep,
*/
VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
- /* Add the pte bit when trying to set a pte */
- pte = pte_mkpte(pte);
-
/* Note: mm->context.id might not yet have been assigned as
* this context might not have been activated yet when this
* is called.
@@ -275,8 +272,6 @@ void set_huge_pte_at(struct mm_struct *mm, unsigned long addr, pte_t *ptep, pte_
*/
VM_WARN_ON(pte_hw_valid(*ptep) && !pte_protnone(*ptep));
- pte = pte_mkpte(pte);
-
pte = set_pte_filter(pte);
val = pte_val(pte);
--
2.26.2
^ permalink raw reply related
* [PATCH v4 01/13] powerpc/mm: Add DEBUG_VM WARN for pmd_clear
From: Aneesh Kumar K.V @ 2020-09-02 11:42 UTC (permalink / raw)
To: linux-mm, akpm; +Cc: linuxppc-dev, Aneesh Kumar K.V, Anshuman Khandual
In-Reply-To: <20200902114222.181353-1-aneesh.kumar@linux.ibm.com>
With the hash page table, the kernel should not use pmd_clear for clearing
huge pte entries. Add a DEBUG_VM WARN to catch the wrong usage.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 6de56c3b33c4..079211968987 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -868,6 +868,13 @@ static inline bool pte_ci(pte_t pte)
static inline void pmd_clear(pmd_t *pmdp)
{
+ if (IS_ENABLED(CONFIG_DEBUG_VM) && !radix_enabled()) {
+ /*
+ * Don't use this if we can possibly have a hash page table
+ * entry mapping this.
+ */
+ WARN_ON((pmd_val(*pmdp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));
+ }
*pmdp = __pmd(0);
}
@@ -916,6 +923,13 @@ static inline int pmd_bad(pmd_t pmd)
static inline void pud_clear(pud_t *pudp)
{
+ if (IS_ENABLED(CONFIG_DEBUG_VM) && !radix_enabled()) {
+ /*
+ * Don't use this if we can possibly have a hash page table
+ * entry mapping this.
+ */
+ WARN_ON((pud_val(*pudp) & (H_PAGE_HASHPTE | _PAGE_PTE)) == (H_PAGE_HASHPTE | _PAGE_PTE));
+ }
*pudp = __pud(0);
}
--
2.26.2
^ permalink raw reply related
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