* Re: [PATCH] powerpc/mce: save ignore_event flag unconditionally for UE
From: Santosh Sivaraj @ 2021-04-20 11:35 UTC (permalink / raw)
To: Ganesh, linuxppc-dev, mpe; +Cc: mahesh, npiggin
In-Reply-To: <9b8c7347-47fe-822d-7fae-9365bb7cde7c@linux.ibm.com>
Ganesh <ganeshgr@linux.ibm.com> writes:
> On 4/20/21 12:54 PM, Santosh Sivaraj wrote:
>
>> Hi Ganesh,
>>
>> Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
>>
>>> When we hit an UE while using machine check safe copy routines,
>>> ignore_event flag is set and the event is ignored by mce handler,
>>> And the flag is also saved for defered handling and printing of
>>> mce event information, But as of now saving of this flag is done
>>> on checking if the effective address is provided or physical address
>>> is calculated, which is not right.
>>>
>>> Save ignore_event flag regardless of whether the effective address is
>>> provided or physical address is calculated.
>>>
>>> Without this change following log is seen, when the event is to be
>>> ignored.
>>>
>>> [ 512.971365] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
>>> [ 512.971509] MCE: CPU1: NIP: [c0000000000b67c0] memcpy+0x40/0x90
>>> [ 512.971655] MCE: CPU1: Initiator CPU
>>> [ 512.971739] MCE: CPU1: Unknown
>>> [ 512.972209] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
>>> [ 512.972334] MCE: CPU1: NIP: [c0000000000b6808] memcpy+0x88/0x90
>>> [ 512.972456] MCE: CPU1: Initiator CPU
>>> [ 512.972534] MCE: CPU1: Unknown
>>>
>>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>>> ---
>>> arch/powerpc/kernel/mce.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
>>> index 11f0cae086ed..db9363e131ce 100644
>>> --- a/arch/powerpc/kernel/mce.c
>>> +++ b/arch/powerpc/kernel/mce.c
>>> @@ -131,6 +131,8 @@ void save_mce_event(struct pt_regs *regs, long handled,
>>> * Populate the mce error_type and type-specific error_type.
>>> */
>>> mce_set_error_info(mce, mce_err);
>>> + if (mce->error_type == MCE_ERROR_TYPE_UE)
>>> + mce->u.ue_error.ignore_event = mce_err->ignore_event;
>>>
>>> if (!addr)
>>> return;
>>> @@ -159,7 +161,6 @@ void save_mce_event(struct pt_regs *regs, long handled,
>>> if (phys_addr != ULONG_MAX) {
>>> mce->u.ue_error.physical_address_provided = true;
>>> mce->u.ue_error.physical_address = phys_addr;
>>> - mce->u.ue_error.ignore_event = mce_err->ignore_event;
>>> machine_check_ue_event(mce);
>>> }
>>> }
>> Small nit:
>> Setting ignore event can happen before the phys_addr check, under the existing
>> check for MCE_ERROR_TYPE_UE, instead of repeating the same condition again.
>
> In some cases we may not get effective address also, so it is placed before
> effective address check.
Yes, I forgot the last two lines in the changelog after I applied the patch :-)
Thanks,
Santosh
>
>>
>> Except for the above nit
>>
>> Reviewed-by: Santosh Sivaraj <santosh@fossix.org>
>>
>> Thanks,
>> Santosh
>>> --
>>> 2.26.2
^ permalink raw reply
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
From: Matthew Wilcox @ 2021-04-20 11:32 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Arnd Bergmann, Grygorii Strashko, netdev, Ilias Apalodimas,
Linux Kernel Mailing List, open list:BROADCOM NVRAM DRIVER,
Michal Hocko, Linux MM, Mel Gorman, Jesper Dangaard Brouer,
mcroce, arcml, linuxppc-dev, Christoph Hellwig, Linux ARM
In-Reply-To: <CAMuHMdXm1Zg=Wm-=tn5jUJwqVGUvCi5yDaW0PXWC2DEDYGcy5A@mail.gmail.com>
On Tue, Apr 20, 2021 at 09:39:54AM +0200, Geert Uytterhoeven wrote:
> > +++ b/include/linux/mm_types.h
> > @@ -97,10 +97,10 @@ struct page {
> > };
> > struct { /* page_pool used by netstack */
> > /**
> > - * @dma_addr: might require a 64-bit value even on
> > + * @dma_addr: might require a 64-bit value on
> > * 32-bit architectures.
> > */
> > - dma_addr_t dma_addr;
> > + unsigned long dma_addr[2];
>
> So we get two 64-bit words on 64-bit platforms, while only one is
> needed?
Not really. This is part of the 5-word union in struct page, so the space
ends up being reserved anyway, event if it's not "assigned" to dma_addr.
> > + dma_addr_t ret = page->dma_addr[0];
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
>
> We don't seem to have a handy macro for a 32-bit left shift yet...
>
> But you can also avoid the warning using
>
> ret |= (u64)page->dma_addr[1] << 32;
Sure. It doesn't really matter which way we eliminate the warning;
the code is unreachable.
> > +{
> > + page->dma_addr[0] = addr;
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + page->dma_addr[1] = addr >> 16 >> 16;
>
> ... but we do have upper_32_bits() for a 32-bit right shift.
Yep, that's what my current tree looks like.
^ permalink raw reply
* Re: [PATCH] powerpc/mce: save ignore_event flag unconditionally for UE
From: Ganesh @ 2021-04-20 10:35 UTC (permalink / raw)
To: Santosh Sivaraj, linuxppc-dev, mpe; +Cc: mahesh, npiggin
In-Reply-To: <87bla9zae7.fsf@fossix.org>
On 4/20/21 12:54 PM, Santosh Sivaraj wrote:
> Hi Ganesh,
>
> Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
>
>> When we hit an UE while using machine check safe copy routines,
>> ignore_event flag is set and the event is ignored by mce handler,
>> And the flag is also saved for defered handling and printing of
>> mce event information, But as of now saving of this flag is done
>> on checking if the effective address is provided or physical address
>> is calculated, which is not right.
>>
>> Save ignore_event flag regardless of whether the effective address is
>> provided or physical address is calculated.
>>
>> Without this change following log is seen, when the event is to be
>> ignored.
>>
>> [ 512.971365] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
>> [ 512.971509] MCE: CPU1: NIP: [c0000000000b67c0] memcpy+0x40/0x90
>> [ 512.971655] MCE: CPU1: Initiator CPU
>> [ 512.971739] MCE: CPU1: Unknown
>> [ 512.972209] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
>> [ 512.972334] MCE: CPU1: NIP: [c0000000000b6808] memcpy+0x88/0x90
>> [ 512.972456] MCE: CPU1: Initiator CPU
>> [ 512.972534] MCE: CPU1: Unknown
>>
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>> arch/powerpc/kernel/mce.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
>> index 11f0cae086ed..db9363e131ce 100644
>> --- a/arch/powerpc/kernel/mce.c
>> +++ b/arch/powerpc/kernel/mce.c
>> @@ -131,6 +131,8 @@ void save_mce_event(struct pt_regs *regs, long handled,
>> * Populate the mce error_type and type-specific error_type.
>> */
>> mce_set_error_info(mce, mce_err);
>> + if (mce->error_type == MCE_ERROR_TYPE_UE)
>> + mce->u.ue_error.ignore_event = mce_err->ignore_event;
>>
>> if (!addr)
>> return;
>> @@ -159,7 +161,6 @@ void save_mce_event(struct pt_regs *regs, long handled,
>> if (phys_addr != ULONG_MAX) {
>> mce->u.ue_error.physical_address_provided = true;
>> mce->u.ue_error.physical_address = phys_addr;
>> - mce->u.ue_error.ignore_event = mce_err->ignore_event;
>> machine_check_ue_event(mce);
>> }
>> }
> Small nit:
> Setting ignore event can happen before the phys_addr check, under the existing
> check for MCE_ERROR_TYPE_UE, instead of repeating the same condition again.
In some cases we may not get effective address also, so it is placed before effective
address check.
>
> Except for the above nit
>
> Reviewed-by: Santosh Sivaraj <santosh@fossix.org>
>
> Thanks,
> Santosh
>> --
>> 2.26.2
^ permalink raw reply
* Re: [RFC v1 PATCH 3/3] driver: update all the code that use soc_device_match
From: Péter Ujfalusi @ 2021-04-20 9:40 UTC (permalink / raw)
To: Alice Guo (OSS), gregkh, rafael, horia.geanta, aymen.sghaier,
herbert, davem, tony, geert+renesas, mturquette, sboyd, vkoul,
a.hajda, narmstrong, robert.foss, airlied, daniel, khilman, tomba,
jyri.sarha, joro, will, mchehab, ulf.hansson, adrian.hunter,
kishon, kuba, linus.walleij, Roy.Pledge, leoyang.li, ssantosh,
matthias.bgg, edubezval, j-keerthy, balbi, linux, stern, wim,
linux
Cc: linux-usb, linux-watchdog, linux-gpio, netdev, linux-pm,
linux-staging, linux-mmc, linux-kernel, dri-devel,
linux-renesas-soc, linux-phy, iommu, linux-mediatek, linux-crypto,
dmaengine, linux-amlogic, linux-omap, linuxppc-dev, linux-clk,
linux-arm-kernel, linux-media
In-Reply-To: <20210419042722.27554-4-alice.guo@oss.nxp.com>
Hi Alice,
On 4/19/21 7:27 AM, Alice Guo (OSS) wrote:
> From: Alice Guo <alice.guo@nxp.com>
>
> Update all the code that use soc_device_match because add support for
> soc_device_match returning -EPROBE_DEFER.
>
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> ---
> drivers/bus/ti-sysc.c | 2 +-
> drivers/clk/renesas/r8a7795-cpg-mssr.c | 4 +++-
> drivers/clk/renesas/rcar-gen2-cpg.c | 2 +-
> drivers/clk/renesas/rcar-gen3-cpg.c | 2 +-
> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c | 7 ++++++-
> drivers/dma/ti/k3-psil.c | 3 +++
> drivers/dma/ti/k3-udma.c | 2 +-
> drivers/gpu/drm/bridge/nwl-dsi.c | 2 +-
> drivers/gpu/drm/meson/meson_drv.c | 4 +++-
> drivers/gpu/drm/omapdrm/dss/dispc.c | 2 +-
> drivers/gpu/drm/omapdrm/dss/dpi.c | 4 +++-
> drivers/gpu/drm/omapdrm/dss/dsi.c | 3 +++
> drivers/gpu/drm/omapdrm/dss/dss.c | 3 +++
> drivers/gpu/drm/omapdrm/dss/hdmi4_core.c | 3 +++
> drivers/gpu/drm/omapdrm/dss/venc.c | 4 +++-
> drivers/gpu/drm/omapdrm/omap_drv.c | 3 +++
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 4 +++-
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
> drivers/gpu/drm/tidss/tidss_dispc.c | 4 +++-
> drivers/iommu/ipmmu-vmsa.c | 7 +++++--
> drivers/media/platform/rcar-vin/rcar-core.c | 2 +-
> drivers/media/platform/rcar-vin/rcar-csi2.c | 2 +-
> drivers/media/platform/vsp1/vsp1_uif.c | 4 +++-
> drivers/mmc/host/renesas_sdhi_core.c | 2 +-
> drivers/mmc/host/renesas_sdhi_internal_dmac.c | 2 +-
> drivers/mmc/host/sdhci-of-esdhc.c | 21 ++++++++++++++-----
> drivers/mmc/host/sdhci-omap.c | 2 +-
> drivers/mmc/host/sdhci_am654.c | 2 +-
> drivers/net/ethernet/renesas/ravb_main.c | 4 +++-
> drivers/net/ethernet/ti/am65-cpsw-nuss.c | 2 +-
> drivers/net/ethernet/ti/cpsw.c | 2 +-
> drivers/net/ethernet/ti/cpsw_new.c | 2 +-
> drivers/phy/ti/phy-omap-usb2.c | 4 +++-
> drivers/pinctrl/renesas/core.c | 2 +-
> drivers/pinctrl/renesas/pfc-r8a7790.c | 5 ++++-
> drivers/pinctrl/renesas/pfc-r8a7794.c | 5 ++++-
> drivers/soc/fsl/dpio/dpio-driver.c | 13 ++++++++----
> drivers/soc/renesas/r8a774c0-sysc.c | 5 ++++-
> drivers/soc/renesas/r8a7795-sysc.c | 2 +-
> drivers/soc/renesas/r8a77990-sysc.c | 5 ++++-
> drivers/soc/ti/k3-ringacc.c | 2 +-
> drivers/staging/mt7621-pci/pci-mt7621.c | 2 +-
> drivers/thermal/rcar_gen3_thermal.c | 4 +++-
> drivers/thermal/ti-soc-thermal/ti-bandgap.c | 10 +++++++--
> drivers/usb/gadget/udc/renesas_usb3.c | 2 +-
> drivers/usb/host/ehci-platform.c | 4 +++-
> drivers/usb/host/xhci-rcar.c | 2 +-
> drivers/watchdog/renesas_wdt.c | 2 +-
> 48 files changed, 131 insertions(+), 52 deletions(-)
>
...
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 96ad21869ba7..50a4c8f0993d 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -5188,7 +5188,7 @@ static int udma_probe(struct platform_device *pdev)
> ud->match_data = match->data;
>
> soc = soc_device_match(k3_soc_devices);
> - if (!soc) {
> + if (!IS_ERR(soc) && !soc) {
this does not sound right...
if (!soc || IS_ERR(soc))
or
if (IS_ERR_OR_NULL(soc))
is even better.
> dev_err(dev, "No compatible SoC found\n");
> return -ENODEV;
There might be a clever macro for it, but:
return soc ? PTR_ERR(soc) : -ENODEV;
> }
--
Péter
^ permalink raw reply
* Re: [RFC v1 PATCH 3/3] driver: update all the code that use soc_device_match
From: Arnd Bergmann @ 2021-04-20 9:10 UTC (permalink / raw)
To: Dominique MARTINET
Cc: Ulf Hansson, aymen.sghaier, Geert Uytterhoeven, Rafael Wysocki,
David Airlie, Michael Turquette, dri-devel,
Linux Kernel Mailing List, Andrzej Hajda, Networking, linux-phy,
peter.ujfalusi, linux-clk, Linux-Renesas, Wim Van Sebroeck,
Herbert Xu, Horia Geantă, Kevin Hilman, Joerg Roedel,
Neil Armstrong, linux-staging, open list:IOMMU DRIVERS, Kishon,
Tony Lindgren, linux-omap, Geert Uytterhoeven, Jakub Kicinski,
Linus Walleij, Guenter Roeck, Linux Media Mailing List,
LINUXWATCHDOG, Will Deacon, Linux PM list, linuxppc-dev,
Eduardo Valentin, open list:GPIO SUBSYSTEM,
moderated list:ARM/Mediatek SoC..., Santosh Shilimkar,
Matthias Brugger, open list:ARM/Amlogic Meson SoC support,
Mauro Carvalho Chehab, Linux ARM, Alice Guo (OSS), Felipe Balbi,
tomba, Stephen Boyd, gregkh, Alan Stern, USB list, linux-mmc,
Adrian Hunter, Robert Foss, Leo Li, Tony Prisk, Vinod Koul,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Daniel Vetter,
Keerthy, dmaengine, Roy Pledge, jyri.sarha, David Miller
In-Reply-To: <YH4VdPNO9cdzc5MD@atmark-techno.com>
On Tue, Apr 20, 2021 at 1:44 AM Dominique MARTINET
<dominique.martinet@atmark-techno.com> wrote:
> Arnd Bergmann wrote on Mon, Apr 19, 2021 at 02:16:36PM +0200:
> > For built-in drivers, load order depends on the initcall level and
> > link order (how things are lined listed in the Makefile hierarchy).
> >
> > For loadable modules, this is up to user space in the end.
> >
> > Which of the drivers in this scenario are loadable modules?
>
> All the drivers involved in my case are built-in (nvmem, soc and final
> soc_device_match consumer e.g. caam_jr that crashes the kernel if soc is
> not identified properly).
Ok, in that case you may have a chance to just adapt the initcall
levels. This is somewhat fragile if someone else already relies
on a particular order, but it's an easy one-line change to change
a driver e.g. from module_init() or device_initcall() to arch_initcall().
> I frankly don't like the idea of moving nvmem/ above soc/ in
> drivers/Makefile as a "solution" to this (especially as there is one
> that seems to care about what soc they run on...), so I'll have a look
> at links first, hopefully that will work out.
Right, that would be way more fragile.
I think the main problem in this case is the caam driver that really
should not look into the particular SoC type or even machine
compatible string. This is something we can do as a last resort
for compatibility with busted devicetree files, but it appears that
this driver does it as the primary method for identifying different
hardware revisions. I would suggest fixing the binding so that
each SoC that includes one of these devices has a soc specific
compatible string associated with the device that the driver can
use as the primary way of identifying the device.
We probably need to keep the old logic around for old dtb files,
but there can at least be a comment next to that table that
discourages people from adding more entries there.
Arnd
^ permalink raw reply
* Re: [RFC v1 PATCH 3/3] driver: update all the code that use soc_device_match
From: Arnd Bergmann @ 2021-04-20 9:07 UTC (permalink / raw)
To: Dominique MARTINET
Cc: Ulf Hansson, aymen.sghaier, Geert Uytterhoeven, Rafael Wysocki,
David Airlie, Michael Turquette, dri-devel,
Linux Kernel Mailing List, Andrzej Hajda, Networking, linux-phy,
peter.ujfalusi, linux-clk, Linux-Renesas, Wim Van Sebroeck,
Herbert Xu, Horia Geantă, Kevin Hilman, Joerg Roedel,
Neil Armstrong, linux-staging, open list:IOMMU DRIVERS, Kishon,
Tony Lindgren, linux-omap, Geert Uytterhoeven, Jakub Kicinski,
Linus Walleij, Guenter Roeck, Linux Media Mailing List,
LINUXWATCHDOG, Will Deacon, Linux PM list, linuxppc-dev,
Eduardo Valentin, open list:GPIO SUBSYSTEM,
moderated list:ARM/Mediatek SoC..., Santosh Shilimkar,
Matthias Brugger, open list:ARM/Amlogic Meson SoC support,
Mauro Carvalho Chehab, Linux ARM, Alice Guo (OSS), Felipe Balbi,
tomba, Stephen Boyd, gregkh, Alan Stern, USB list, linux-mmc,
Adrian Hunter, Robert Foss, Leo Li, Tony Prisk, Vinod Koul,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Daniel Vetter,
Keerthy, dmaengine, Roy Pledge, jyri.sarha, David Miller
In-Reply-To: <YH4VdPNO9cdzc5MD@atmark-techno.com>
On Tue, Apr 20, 2021 at 1:44 AM Dominique MARTINET
<dominique.martinet@atmark-techno.com> wrote:
> Arnd Bergmann wrote on Mon, Apr 19, 2021 at 02:16:36PM +0200:
> > For built-in drivers, load order depends on the initcall level and
> > link order (how things are lined listed in the Makefile hierarchy).
> >
> > For loadable modules, this is up to user space in the end.
> >
> > Which of the drivers in this scenario are loadable modules?
>
> All the drivers involved in my case are built-in (nvmem, soc and final
> soc_device_match consumer e.g. caam_jr that crashes the kernel if soc is
> not identified properly).
Ok, in that case you may have a chance to just adapt the initcall
levels. This is somewhat fragile if someone else already relies
on a particular order, but it's an easy one-line change to change
a driver e.g. from module_init() or device_initcall() to arch_initcall().
> I frankly don't like the idea of moving nvmem/ above soc/ in
> drivers/Makefile as a "solution" to this (especially as there is one
> that seems to care about what soc they run on...), so I'll have a look
> at links first, hopefully that will work out.
Right, that would be way more fragile.
I think the main problem in this case is the caam driver that really
should not look into the particular SoC type or even machine
compatible string. This is something we can do as a last resort
for compatibility with busted devicetree files, but it appears that
this driver does it as the primary method for identifying different
hardware revisions. I would suggest fixing the binding so that
each SoC that includes one of these devices has a soc specific
compatible string associated with the device that the driver can
use as the primary way of identifying the device.
We probably need to keep the old logic around for old dtb files,
but there can at least be a comment next to that table that
discourages people from adding more entries there.
Arnd
^ permalink raw reply
* Re: swiotlb cleanups v3
From: Christoph Hellwig @ 2021-04-20 9:23 UTC (permalink / raw)
To: Tom Lendacky
Cc: xen-devel, konrad.wilk, iommu, dongli.zhang, tientzu,
linuxppc-dev, hch
In-Reply-To: <0349082c-59c5-20d7-f324-279981c3f6ea@amd.com>
On Sat, Apr 17, 2021 at 11:39:22AM -0500, Tom Lendacky wrote:
> Somewhere between the 1st and 2nd patch, specifying a specific swiotlb
> for an SEV guest is no longer honored. For example, if I start an SEV
> guest with 16GB of memory and specify swiotlb=131072 I used to get a
> 256MB SWIOTLB. However, after the 2nd patch, the swiotlb=131072 is no
> longer honored and I get a 982MB SWIOTLB (as set via sev_setup_arch() in
> arch/x86/mm/mem_encrypt.c).
>
> I can't be sure which patch caused the issue since an SEV guest fails to
> boot with the 1st patch but can boot with the 2nd patch, at which point
> the SWIOTLB comes in at 982MB (I haven't had a chance to debug it and so
> I'm hoping you might be able to quickly spot what's going on).
Can you try this patch?
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 0a5b6f7e75bce6..ac81ef97df32f5 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -71,15 +71,17 @@ struct io_tlb_mem *io_tlb_default_mem;
*/
static unsigned int max_segment;
-static unsigned long default_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT;
+static unsigned long swiotlb_cmdline_size;
static int __init
setup_io_tlb_npages(char *str)
{
if (isdigit(*str)) {
/* avoid tail segment of size < IO_TLB_SEGSIZE */
- default_nslabs =
- ALIGN(simple_strtoul(str, &str, 0), IO_TLB_SEGSIZE);
+ unsigned long nslabs = simple_strtoul(str, &str, 0);
+
+ swiotlb_cmdline_size =
+ ALIGN(nslabs, IO_TLB_SEGSIZE) << IO_TLB_SHIFT;
}
if (*str == ',')
++str;
@@ -108,7 +110,9 @@ void swiotlb_set_max_segment(unsigned int val)
unsigned long swiotlb_size_or_default(void)
{
- return default_nslabs << IO_TLB_SHIFT;
+ if (swiotlb_cmdline_size)
+ return swiotlb_cmdline_size;
+ return IO_TLB_DEFAULT_SIZE;
}
void __init swiotlb_adjust_size(unsigned long size)
@@ -118,9 +122,10 @@ void __init swiotlb_adjust_size(unsigned long size)
* architectures such as those supporting memory encryption to
* adjust/expand SWIOTLB size for their use.
*/
- size = ALIGN(size, IO_TLB_SIZE);
- default_nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
- pr_info("SWIOTLB bounce buffer size adjusted to %luMB", size >> 20);
+ if (!swiotlb_cmdline_size)
+ swiotlb_cmdline_size = ALIGN(size, IO_TLB_SIZE);
+ pr_info("SWIOTLB bounce buffer size adjusted to %luMB",
+ swiotlb_cmdline_size >> 20);
}
void swiotlb_print_info(void)
@@ -209,7 +214,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
void __init
swiotlb_init(int verbose)
{
- size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
+ size_t bytes = PAGE_ALIGN(swiotlb_size_or_default());
void *tlb;
if (swiotlb_force == SWIOTLB_NO_FORCE)
@@ -219,7 +224,7 @@ swiotlb_init(int verbose)
tlb = memblock_alloc_low(bytes, PAGE_SIZE);
if (!tlb)
goto fail;
- if (swiotlb_init_with_tbl(tlb, default_nslabs, verbose))
+ if (swiotlb_init_with_tbl(tlb, bytes >> IO_TLB_SHIFT, verbose))
goto fail_free_mem;
return;
^ permalink raw reply related
* Re: [PATCH V2] mm/page_alloc: Ensure that HUGETLB_PAGE_ORDER is less than MAX_ORDER
From: David Hildenbrand @ 2021-04-20 9:03 UTC (permalink / raw)
To: Christoph Lameter, Anshuman Khandual
Cc: linux-ia64@vger.kernel.org, Mel Gorman, linux-kernel,
Michal Hocko, linux-mm, Paul Mackerras, akpm,
linuxppc-dev @ lists . ozlabs . org, Vlastimil Babka,
Mike Kravetz
In-Reply-To: <alpine.DEB.2.22.394.2104191236500.777076@gentwo.de>
Hi Christoph,
thanks for your insight.
> You can have larger blocks but you would need to allocate multiple
> contigous max order blocks or do it at boot time before the buddy
> allocator is active.
>
> What IA64 did was to do this at boot time thereby avoiding the buddy
> lists. And it had a separate virtual address range and page table for the
> huge pages.
>
> Looks like the current code does these allocations via CMA which should
> also bypass the buddy allocator.
Using CMA doesn't really care about the pageblock size when it comes to
fragmentation avoidance a.k.a. somewhat reliable allocation of memory
chunks with an order > MAX_ORDER - 1.
IOW, when using CMA for hugetlb, we don't need pageblock_order >
MAX_ORDER - 1.
>
>
>>> }
>>>
>>>
>>> But it's kind of weird, isn't it? Let's assume we have MAX_ORDER - 1 correspond to 4 MiB and pageblock_order correspond to 8 MiB.
>>>
>>> Sure, we'd be grouping pages in 8 MiB chunks, however, we cannot even
>>> allocate 8 MiB chunks via the buddy. So only alloc_contig_range()
>>> could really grab them (IOW: gigantic pages).
>>
>> Right.
>
> But then you can avoid the buddy allocator.
>
>>> Further, we have code like deferred_free_range(), where we end up
>>> calling __free_pages_core()->...->__free_one_page() with
>>> pageblock_order. Wouldn't we end up setting the buddy order to
>>> something > MAX_ORDER -1 on that path?
>>
>> Agreed.
>
> We would need to return the supersized block to the huge page pool and not
> to the buddy allocator. There is a special callback in the compound page
> sos that you can call an alternate free function that is not the buddy
> allocator.
Sorry, but that doesn't make any sense. We are talking about bringup
code, where we transition from memblock to the buddy and fill the free
page lists. Looking at the code, deferred initialization of the memmap
is broken on these setups -- so I deferred memmap init is never enabled.
>
>>
>>>
>>> Having pageblock_order > MAX_ORDER feels wrong and looks shaky.
>>>
>> Agreed, definitely does not look right. Lets see what other folks
>> might have to say on this.
>>
>> + Christoph Lameter <cl@linux.com>
>>
>
> It was done for a long time successfully and is running in numerous
> configurations.
Enforcing pageblock_order < MAX_ORDER would mean that runtime allocation
of gigantic (here:huge) pages (HUGETLB_PAGE_ORDER >= MAX_ORDER) via
alloc_contig_pages() becomes less reliable. To compensate, relevant
archs could switch to "hugetlb_cma=", to improve the reliability of
runtime allocation.
I wonder which configurations we are talking about:
a) ia64
At least I couldn't care less; it's a dead architecture -- not
sure how much people care about "more reliable runtime
allocation of gigantic (here: huge) pages". Also, not sure about which
exact configurations.
b) ppc64
We have variable hpage size only with CONFIG_PPC_BOOK3S_64. We
initialize the hugepage either to 1M, 2M or 16M. 16M seems to be the
primary choice.
ppc64 has CONFIG_FORCE_MAX_ZONEORDER
default "9" if PPC64 && PPC_64K_PAGES
-> 16M effective buddy maximum size
default "13" if PPC64 && !PPC_64K_PAGES
-> 16M effective buddy maximum size
So I fail to see in which scenario we even could end up with
pageblock_order < MAX_ORDER. I did not check ppc32.
--
Thanks,
David / dhildenb
^ permalink raw reply
* RE: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
From: David Laight @ 2021-04-20 8:39 UTC (permalink / raw)
To: 'Geert Uytterhoeven', Matthew Wilcox
Cc: Arnd Bergmann, Grygorii Strashko, netdev, Ilias Apalodimas,
Linux Kernel Mailing List, open list:BROADCOM NVRAM DRIVER,
Michal Hocko, Linux MM, Mel Gorman, Jesper Dangaard Brouer,
mcroce@linux.microsoft.com, arcml, linuxppc-dev,
Christoph Hellwig, Linux ARM
In-Reply-To: <CAMuHMdXm1Zg=Wm-=tn5jUJwqVGUvCi5yDaW0PXWC2DEDYGcy5A@mail.gmail.com>
From: Geert Uytterhoeven
> Sent: 20 April 2021 08:40
>
> Hi Willy,
>
> On Sat, Apr 17, 2021 at 4:49 AM Matthew Wilcox <willy@infradead.org> wrote:
> > Replacement patch to fix compiler warning.
> >
> > 32-bit architectures which expect 8-byte alignment for 8-byte integers
> > and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> > page inadvertently expanded in 2019. When the dma_addr_t was added,
> > it forced the alignment of the union to 8 bytes, which inserted a 4 byte
> > gap between 'flags' and the union.
> >
> > Fix this by storing the dma_addr_t in one or two adjacent unsigned longs.
> > This restores the alignment to that of an unsigned long, and also fixes a
> > potential problem where (on a big endian platform), the bit used to denote
> > PageTail could inadvertently get set, and a racing get_user_pages_fast()
> > could dereference a bogus compound_head().
> >
> > Fixes: c25fff7171be ("mm: add dma_addr_t to struct page")
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>
> Thanks for your patch!
>
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -97,10 +97,10 @@ struct page {
> > };
> > struct { /* page_pool used by netstack */
> > /**
> > - * @dma_addr: might require a 64-bit value even on
> > + * @dma_addr: might require a 64-bit value on
> > * 32-bit architectures.
> > */
> > - dma_addr_t dma_addr;
> > + unsigned long dma_addr[2];
>
> So we get two 64-bit words on 64-bit platforms, while only one is
> needed?
>
> Would
>
> unsigned long _dma_addr[sizeof(dma_addr_t) / sizeof(unsigned long)];
>
> work?
>
> Or will the compiler become too overzealous, and warn about the use
> of ...[1] below, even when unreachable?
> I wouldn't mind an #ifdef instead of an if () in the code below, though.
You could use [ARRAY_SIZE()-1] instead of [1].
Or, since IIRC it is the last member of that specific struct, define as:
unsigned long dma_addr[];
...
> > - return page->dma_addr;
> > + dma_addr_t ret = page->dma_addr[0];
> > + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> > + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
>
> We don't seem to have a handy macro for a 32-bit left shift yet...
>
> But you can also avoid the warning using
>
> ret |= (u64)page->dma_addr[1] << 32;
Or:
ret |= page->dma_addr[1] + 0ull << 32;
Which relies in integer promotion rather than a cast.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] powerpc/legacy_serial: Use early_ioremap()
From: Christophe Leroy @ 2021-04-20 8:18 UTC (permalink / raw)
To: Chris Packham, Christophe Leroy, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman
Cc: Hamish Martin, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <32c16eef-f426-4f7e-14aa-b9fbbbec59b8@alliedtelesis.co.nz>
Hi Chris,
Le 10/08/2020 à 04:01, Chris Packham a écrit :
>
> On 24/03/20 10:54 am, Chris Packham wrote:
>> Hi Christophe,
>>
>> On Wed, 2020-02-05 at 12:03 +0000, Christophe Leroy wrote:
>>> [ 0.000000] ioremap() called early from
>>> find_legacy_serial_ports+0x3cc/0x474. Use early_ioremap() instead
>>>
>> I was just about to dig into this error message and found you patch. I
>> applied it to a v5.5 base.
>>
>>> find_legacy_serial_ports() is called early from setup_arch(), before
>>> paging_init(). vmalloc is not available yet, ioremap shouldn't be
>>> used that early.
>>>
>>> Use early_ioremap() and switch to a regular ioremap() later.
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> On my system (Freescale T2080 SOC) this seems to cause a crash/hang in
>> early boot. Unfortunately because this is affecting the boot console I
>> don't get any earlyprintk output.
>
> I've been doing a bit more digging into why Christophe's patch didn't
> work for me. I noticed the powerpc specific early_ioremap_range()
> returns addresses around ioremap_bot. Yet the generic early_ioremap()
> uses addresses around FIXADDR_TOP. If I try the following hack I can
> make Christophe's patch work
>
> diff --git a/arch/powerpc/include/asm/fixmap.h
> b/arch/powerpc/include/asm/fixmap.h
> index 2ef155a3c821..7bc2f3f73c8b 100644
> --- a/arch/powerpc/include/asm/fixmap.h
> +++ b/arch/powerpc/include/asm/fixmap.h
> @@ -27,7 +27,7 @@
> #include <asm/kasan.h>
> #define FIXADDR_TOP (KASAN_SHADOW_START - PAGE_SIZE)
> #else
> -#define FIXADDR_TOP ((unsigned long)(-PAGE_SIZE))
> +#define FIXADDR_TOP (IOREMAP_END - PAGE_SIZE)
> #endif
>
> /*
>
> I'll admit to being out of my depth. It seems that the generic
> early_ioremap() is not quite correctly plumbed in for powerpc.
Yes that's probably true for PPC64.
I see that on PPC32 I had to implement the following changes in order to enable earlier use of
early_ioremap()
https://github.com/torvalds/linux/commit/925ac141d106b55acbe112a9272f970631a3c082
I have the problem with QEMU with the ppce500 machine. It will allow me to investigate it a bit further.
^ permalink raw reply
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
From: Geert Uytterhoeven @ 2021-04-20 7:39 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Arnd Bergmann, Grygorii Strashko, netdev, Ilias Apalodimas,
Linux Kernel Mailing List, open list:BROADCOM NVRAM DRIVER,
Michal Hocko, Linux MM, Mel Gorman, Jesper Dangaard Brouer,
mcroce, arcml, linuxppc-dev, Christoph Hellwig, Linux ARM
In-Reply-To: <20210417024522.GP2531743@casper.infradead.org>
Hi Willy,
On Sat, Apr 17, 2021 at 4:49 AM Matthew Wilcox <willy@infradead.org> wrote:
> Replacement patch to fix compiler warning.
>
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Date: Fri, 16 Apr 2021 16:34:55 -0400
> Subject: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
> To: brouer@redhat.com
> Cc: linux-kernel@vger.kernel.org,
> linux-mm@kvack.org,
> netdev@vger.kernel.org,
> linuxppc-dev@lists.ozlabs.org,
> linux-arm-kernel@lists.infradead.org,
> linux-mips@vger.kernel.org,
> ilias.apalodimas@linaro.org,
> mcroce@linux.microsoft.com,
> grygorii.strashko@ti.com,
> arnd@kernel.org,
> hch@lst.de,
> linux-snps-arc@lists.infradead.org,
> mhocko@kernel.org,
> mgorman@suse.de
>
> 32-bit architectures which expect 8-byte alignment for 8-byte integers
> and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> page inadvertently expanded in 2019. When the dma_addr_t was added,
> it forced the alignment of the union to 8 bytes, which inserted a 4 byte
> gap between 'flags' and the union.
>
> Fix this by storing the dma_addr_t in one or two adjacent unsigned longs.
> This restores the alignment to that of an unsigned long, and also fixes a
> potential problem where (on a big endian platform), the bit used to denote
> PageTail could inadvertently get set, and a racing get_user_pages_fast()
> could dereference a bogus compound_head().
>
> Fixes: c25fff7171be ("mm: add dma_addr_t to struct page")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Thanks for your patch!
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -97,10 +97,10 @@ struct page {
> };
> struct { /* page_pool used by netstack */
> /**
> - * @dma_addr: might require a 64-bit value even on
> + * @dma_addr: might require a 64-bit value on
> * 32-bit architectures.
> */
> - dma_addr_t dma_addr;
> + unsigned long dma_addr[2];
So we get two 64-bit words on 64-bit platforms, while only one is
needed?
Would
unsigned long _dma_addr[sizeof(dma_addr_t) / sizeof(unsigned long)];
work?
Or will the compiler become too overzealous, and warn about the use
of ...[1] below, even when unreachable?
I wouldn't mind an #ifdef instead of an if () in the code below, though.
> };
> struct { /* slab, slob and slub */
> union {
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index b5b195305346..ad6154dc206c 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -198,7 +198,17 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>
> static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
> {
> - return page->dma_addr;
> + dma_addr_t ret = page->dma_addr[0];
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + ret |= (dma_addr_t)page->dma_addr[1] << 16 << 16;
We don't seem to have a handy macro for a 32-bit left shift yet...
But you can also avoid the warning using
ret |= (u64)page->dma_addr[1] << 32;
> + return ret;
> +}
> +
> +static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
> +{
> + page->dma_addr[0] = addr;
> + if (sizeof(dma_addr_t) > sizeof(unsigned long))
> + page->dma_addr[1] = addr >> 16 >> 16;
... but we do have upper_32_bits() for a 32-bit right shift.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] powerpc/mce: save ignore_event flag unconditionally for UE
From: Santosh Sivaraj @ 2021-04-20 7:24 UTC (permalink / raw)
To: Ganesh Goudar, linuxppc-dev, mpe; +Cc: Ganesh Goudar, mahesh, npiggin
In-Reply-To: <20210407045816.352276-1-ganeshgr@linux.ibm.com>
Hi Ganesh,
Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
> When we hit an UE while using machine check safe copy routines,
> ignore_event flag is set and the event is ignored by mce handler,
> And the flag is also saved for defered handling and printing of
> mce event information, But as of now saving of this flag is done
> on checking if the effective address is provided or physical address
> is calculated, which is not right.
>
> Save ignore_event flag regardless of whether the effective address is
> provided or physical address is calculated.
>
> Without this change following log is seen, when the event is to be
> ignored.
>
> [ 512.971365] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
> [ 512.971509] MCE: CPU1: NIP: [c0000000000b67c0] memcpy+0x40/0x90
> [ 512.971655] MCE: CPU1: Initiator CPU
> [ 512.971739] MCE: CPU1: Unknown
> [ 512.972209] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
> [ 512.972334] MCE: CPU1: NIP: [c0000000000b6808] memcpy+0x88/0x90
> [ 512.972456] MCE: CPU1: Initiator CPU
> [ 512.972534] MCE: CPU1: Unknown
>
> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> ---
> arch/powerpc/kernel/mce.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
> index 11f0cae086ed..db9363e131ce 100644
> --- a/arch/powerpc/kernel/mce.c
> +++ b/arch/powerpc/kernel/mce.c
> @@ -131,6 +131,8 @@ void save_mce_event(struct pt_regs *regs, long handled,
> * Populate the mce error_type and type-specific error_type.
> */
> mce_set_error_info(mce, mce_err);
> + if (mce->error_type == MCE_ERROR_TYPE_UE)
> + mce->u.ue_error.ignore_event = mce_err->ignore_event;
>
> if (!addr)
> return;
> @@ -159,7 +161,6 @@ void save_mce_event(struct pt_regs *regs, long handled,
> if (phys_addr != ULONG_MAX) {
> mce->u.ue_error.physical_address_provided = true;
> mce->u.ue_error.physical_address = phys_addr;
> - mce->u.ue_error.ignore_event = mce_err->ignore_event;
> machine_check_ue_event(mce);
> }
> }
Small nit:
Setting ignore event can happen before the phys_addr check, under the existing
check for MCE_ERROR_TYPE_UE, instead of repeating the same condition again.
Except for the above nit
Reviewed-by: Santosh Sivaraj <santosh@fossix.org>
Thanks,
Santosh
> --
> 2.26.2
^ permalink raw reply
* Re: [PATCH] powerpc/mce: save ignore_event flag unconditionally for UE
From: Santosh Sivaraj @ 2021-04-20 7:22 UTC (permalink / raw)
To: Ganesh Goudar, linuxppc-dev, mpe; +Cc: Ganesh Goudar, mahesh, npiggin
In-Reply-To: <20210407045816.352276-1-ganeshgr@linux.ibm.com>
Hi Ganesh,
Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
> When we hit an UE while using machine check safe copy routines,
> ignore_event flag is set and the event is ignored by mce handler,
> And the flag is also saved for defered handling and printing of
> mce event information, But as of now saving of this flag is done
> on checking if the effective address is provided or physical address
> is calculated, which is not right.
>
> Save ignore_event flag regardless of whether the effective address is
> provided or physical address is calculated.
>
> Without this change following log is seen, when the event is to be
> ignored.
>
> [ 512.971365] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
> [ 512.971509] MCE: CPU1: NIP: [c0000000000b67c0] memcpy+0x40/0x90
> [ 512.971655] MCE: CPU1: Initiator CPU
> [ 512.971739] MCE: CPU1: Unknown
> [ 512.972209] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
> [ 512.972334] MCE: CPU1: NIP: [c0000000000b6808] memcpy+0x88/0x90
> [ 512.972456] MCE: CPU1: Initiator CPU
> [ 512.972534] MCE: CPU1: Unknown
>
> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> ---
> arch/powerpc/kernel/mce.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
> index 11f0cae086ed..db9363e131ce 100644
> --- a/arch/powerpc/kernel/mce.c
> +++ b/arch/powerpc/kernel/mce.c
> @@ -131,6 +131,8 @@ void save_mce_event(struct pt_regs *regs, long handled,
> * Populate the mce error_type and type-specific error_type.
> */
> mce_set_error_info(mce, mce_err);
> + if (mce->error_type == MCE_ERROR_TYPE_UE)
> + mce->u.ue_error.ignore_event = mce_err->ignore_event;
>
> if (!addr)
> return;
> @@ -159,7 +161,6 @@ void save_mce_event(struct pt_regs *regs, long handled,
> if (phys_addr != ULONG_MAX) {
> mce->u.ue_error.physical_address_provided = true;
> mce->u.ue_error.physical_address = phys_addr;
> - mce->u.ue_error.ignore_event = mce_err->ignore_event;
> machine_check_ue_event(mce);
> }
> }
Small nit:
Setting ignore event can happen before the phys_addr check, under the existing
check for MCE_ERROR_TYPE_UE, instead of repeating the same condition again.
Except for the above nit
Thanks,
Santosh
> --
> 2.26.2
^ permalink raw reply
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
From: Rasmus Villemoes @ 2021-04-20 7:21 UTC (permalink / raw)
To: Matthew Wilcox (Oracle), brouer
Cc: arnd, grygorii.strashko, netdev, ilias.apalodimas, linux-mips,
linux-kernel, mhocko, linux-mm, mgorman, mcroce, linux-snps-arc,
linuxppc-dev, hch, linux-arm-kernel
In-Reply-To: <20210416230724.2519198-2-willy@infradead.org>
On 17/04/2021 01.07, Matthew Wilcox (Oracle) wrote:
> 32-bit architectures which expect 8-byte alignment for 8-byte integers
> and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> page inadvertently expanded in 2019. When the dma_addr_t was added,
> it forced the alignment of the union to 8 bytes, which inserted a 4 byte
> gap between 'flags' and the union.
>
> Fix this by storing the dma_addr_t in one or two adjacent unsigned longs.
> This restores the alignment to that of an unsigned long, and also fixes a
> potential problem where (on a big endian platform), the bit used to denote
> PageTail could inadvertently get set, and a racing get_user_pages_fast()
> could dereference a bogus compound_head().
>
> Fixes: c25fff7171be ("mm: add dma_addr_t to struct page")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> include/linux/mm_types.h | 4 ++--
> include/net/page_pool.h | 12 +++++++++++-
> net/core/page_pool.c | 12 +++++++-----
> 3 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 6613b26a8894..5aacc1c10a45 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -97,10 +97,10 @@ struct page {
> };
> struct { /* page_pool used by netstack */
> /**
> - * @dma_addr: might require a 64-bit value even on
> + * @dma_addr: might require a 64-bit value on
> * 32-bit architectures.
> */
> - dma_addr_t dma_addr;
> + unsigned long dma_addr[2];
Shouldn't that member get another name (_dma_addr?) to be sure the
buildbots catch every possible (ab)user and get them turned into the new
accessors? Sure, page->dma_addr is now "pointer to unsigned long"
instead of "dma_addr_t", but who knows if there's a
"(long)page->dma_addr" somewhere?
Rasmus
^ permalink raw reply
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
From: Arnd Bergmann @ 2021-04-20 7:07 UTC (permalink / raw)
To: Matthew Wilcox
Cc: grygorii.strashko@ti.com, linux-snps-arc@lists.infradead.org,
netdev@vger.kernel.org, Vineet Gupta, ilias.apalodimas@linaro.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org,
linux-mm@kvack.org, mgorman@suse.de, brouer@redhat.com,
mcroce@linux.microsoft.com, mhocko@kernel.org,
linuxppc-dev@lists.ozlabs.org, hch@lst.de,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20210420031029.GI2531743@casper.infradead.org>
On Tue, Apr 20, 2021 at 5:10 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Tue, Apr 20, 2021 at 02:48:17AM +0000, Vineet Gupta wrote:
> > > 32-bit architectures which expect 8-byte alignment for 8-byte integers
> > > and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
> > > page inadvertently expanded in 2019.
> >
> > FWIW, ARC doesn't require 8 byte alignment for 8 byte integers. This is
> > only needed for 8-byte atomics due to the requirements of LLOCKD/SCOND
> > instructions.
>
> Ah, like x86? OK, great, I'll drop your arch from the list of
> affected. Thanks!
I mistakenly assumed that i386 and m68k were the only supported
architectures with 32-bit alignment on u64. I checked it now and found
$ for i in /home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/*-gcc ; do
echo `echo 'int a = __alignof__(long long);' | $i -xc - -Wall -S -o- |
grep -A1 a: | tail -n 1 | cut -f 3 -d\ `
${i#/home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/} ; done
8 aarch64-linux-gcc
8 alpha-linux-gcc
4 arc-linux-gcc
8 arm-linux-gnueabi-gcc
8 c6x-elf-gcc
4 csky-linux-gcc
4 h8300-linux-gcc
8 hppa-linux-gcc
8 hppa64-linux-gcc
8 i386-linux-gcc
8 ia64-linux-gcc
2 m68k-linux-gcc
4 microblaze-linux-gcc
8 mips-linux-gcc
8 mips64-linux-gcc
8 nds32le-linux-gcc
4 nios2-linux-gcc
4 or1k-linux-gcc
8 powerpc-linux-gcc
8 powerpc64-linux-gcc
8 riscv32-linux-gcc
8 riscv64-linux-gcc
8 s390-linux-gcc
4 sh2-linux-gcc
4 sh4-linux-gcc
8 sparc-linux-gcc
8 sparc64-linux-gcc
8 x86_64-linux-gcc
8 xtensa-linux-gcc
which means that half the 32-bit architectures do this. This may
cause more problems when arc and/or microblaze want to support
64-bit kernels and compat mode in the future on their latest hardware,
as that means duplicating the x86 specific hacks we have for compat.
What is alignof(u64) on 64-bit arc?
Arnd
^ permalink raw reply
* Re: [PATCH v1 2/2] powerpc: Enable OPTPROBES on PPC32
From: Naveen N. Rao @ 2021-04-20 6:51 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <28730d147adeaa6c2d0c98d0aa9e17e9e4bd043a.1617701875.git.christophe.leroy@csgroup.eu>
Christophe Leroy wrote:
> For that, create a 32 bits version of patch_imm64_load_insns()
> and create a patch_imm_load_insns() which calls
> patch_imm32_load_insns() on PPC32 and patch_imm64_load_insns()
> on PPC64.
>
> Adapt optprobes_head.S for PPC32. Use PPC_LL/PPC_STL macros instead
> of raw ld/std, opt out things linked to paca and use stmw/lmw to
> save/restore registers.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/Kconfig | 2 +-
> arch/powerpc/kernel/optprobes.c | 24 +++++++++++++--
> arch/powerpc/kernel/optprobes_head.S | 46 +++++++++++++++++++---------
> 3 files changed, 53 insertions(+), 19 deletions(-)
Thanks for adding support for ppc32. It is good to see that it works
without too many changes.
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index c1344c05226c..49b538e54efb 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -227,7 +227,7 @@ config PPC
> select HAVE_MOD_ARCH_SPECIFIC
> select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
> select HAVE_HARDLOCKUP_DETECTOR_ARCH if PPC64 && PPC_BOOK3S && SMP
> - select HAVE_OPTPROBES if PPC64
> + select HAVE_OPTPROBES
> select HAVE_PERF_EVENTS
> select HAVE_PERF_EVENTS_NMI if PPC64
> select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !HAVE_HARDLOCKUP_DETECTOR_ARCH
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 58fdb9f66e0f..cdf87086fa33 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -141,11 +141,21 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
> }
> }
>
> +static void patch_imm32_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
> +{
> + patch_instruction((struct ppc_inst *)addr,
> + ppc_inst(PPC_RAW_LIS(reg, IMM_H(val))));
> + addr++;
> +
> + patch_instruction((struct ppc_inst *)addr,
> + ppc_inst(PPC_RAW_ORI(reg, reg, IMM_L(val))));
> +}
> +
> /*
> * Generate instructions to load provided immediate 64-bit value
> * to register 'reg' and patch these instructions at 'addr'.
> */
> -static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
> +static void patch_imm64_load_insns(unsigned long long val, int reg, kprobe_opcode_t *addr)
Do you really need this?
> {
> /* lis reg,(op)@highest */
> patch_instruction((struct ppc_inst *)addr,
> @@ -177,6 +187,14 @@ static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *
> ___PPC_RS(reg) | (val & 0xffff)));
> }
>
> +static void patch_imm_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
> +{
> + if (IS_ENABLED(CONFIG_PPC64))
> + patch_imm64_load_insns(val, reg, addr);
> + else
> + patch_imm32_load_insns(val, reg, addr);
> +}
> +
> int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
> {
> struct ppc_inst branch_op_callback, branch_emulate_step, temp;
> @@ -230,7 +248,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
> * Fixup the template with instructions to:
> * 1. load the address of the actual probepoint
> */
> - patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
> + patch_imm_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
>
> /*
> * 2. branch to optimized_callback() and emulate_step()
> @@ -264,7 +282,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
> * 3. load instruction to be emulated into relevant register, and
> */
> temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
> - patch_imm64_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
> + patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>
> /*
> * 4. branch back from trampoline
> diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
> index ff8ba4d3824d..49f31e554573 100644
> --- a/arch/powerpc/kernel/optprobes_head.S
> +++ b/arch/powerpc/kernel/optprobes_head.S
> @@ -30,39 +30,47 @@ optinsn_slot:
> .global optprobe_template_entry
> optprobe_template_entry:
> /* Create an in-memory pt_regs */
> - stdu r1,-INT_FRAME_SIZE(r1)
> + PPC_STLU r1,-INT_FRAME_SIZE(r1)
> SAVE_GPR(0,r1)
> /* Save the previous SP into stack */
> addi r0,r1,INT_FRAME_SIZE
> - std r0,GPR1(r1)
> + PPC_STL r0,GPR1(r1)
> +#ifdef CONFIG_PPC64
> SAVE_10GPRS(2,r1)
> SAVE_10GPRS(12,r1)
> SAVE_10GPRS(22,r1)
> +#else
> + stmw r2, GPR2(r1)
> +#endif
> /* Save SPRS */
> mfmsr r5
> - std r5,_MSR(r1)
> + PPC_STL r5,_MSR(r1)
> li r5,0x700
> - std r5,_TRAP(r1)
> + PPC_STL r5,_TRAP(r1)
> li r5,0
> - std r5,ORIG_GPR3(r1)
> - std r5,RESULT(r1)
> + PPC_STL r5,ORIG_GPR3(r1)
> + PPC_STL r5,RESULT(r1)
> mfctr r5
> - std r5,_CTR(r1)
> + PPC_STL r5,_CTR(r1)
> mflr r5
> - std r5,_LINK(r1)
> + PPC_STL r5,_LINK(r1)
> mfspr r5,SPRN_XER
> - std r5,_XER(r1)
> + PPC_STL r5,_XER(r1)
> mfcr r5
> - std r5,_CCR(r1)
> + PPC_STL r5,_CCR(r1)
> +#ifdef CONFIG_PPC64
> lbz r5,PACAIRQSOFTMASK(r13)
> std r5,SOFTE(r1)
> +#endif
>
> /*
> * We may get here from a module, so load the kernel TOC in r2.
> * The original TOC gets restored when pt_regs is restored
> * further below.
> */
> +#ifdef CONFIG_PPC64
> ld r2,PACATOC(r13)
> +#endif
Are the ISA and ABI documents for ppc32 available publicly? I would have
thought that the TOC issues apply to ppc32 as well, but want to
understand why this isn't a problem there.
>
> .global optprobe_template_op_address
> optprobe_template_op_address:
> @@ -72,9 +80,11 @@ optprobe_template_op_address:
> */
> nop
> nop
> +#ifdef CONFIG_PPC64
> nop
> nop
> nop
> +#endif
> /* 2. pt_regs pointer in r4 */
> addi r4,r1,STACK_FRAME_OVERHEAD
>
> @@ -94,9 +104,11 @@ optprobe_template_insn:
> /* 2, Pass instruction to be emulated in r4 */
> nop
> nop
> +#ifdef CONFIG_PPC64
> nop
> nop
> nop
> +#endif
It would be nice to put these behind a macro so as to avoid these #ifdef
blocks here, as well as with the register save/restore sequence.
- Naveen
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/sstep: Add emulation support for ‘setb’ instruction
From: Naveen N. Rao @ 2021-04-20 6:26 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev, Sathvika Vasireddy
In-Reply-To: <875z0mfzbf.fsf@linkitivity.dja.id.au>
Daniel Axtens wrote:
> Sathvika Vasireddy <sathvika@linux.vnet.ibm.com> writes:
>
>> This adds emulation support for the following instruction:
>> * Set Boolean (setb)
>>
>> Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/lib/sstep.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
>> index c6aebc149d14..263c613d7490 100644
>> --- a/arch/powerpc/lib/sstep.c
>> +++ b/arch/powerpc/lib/sstep.c
>> @@ -1964,6 +1964,18 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
>> op->val = ~(regs->gpr[rd] | regs->gpr[rb]);
>> goto logical_done;
>>
>> + case 128: /* setb */
>> + if (!cpu_has_feature(CPU_FTR_ARCH_300))
>> + goto unknown_opcode;
>
> Ok, if I've understood correctly...
>
>> + ra = ra & ~0x3;
>
> This masks off the bits of RA that are not part of BTF:
>
> ra is in [0, 31] which is [0b00000, 0b11111]
> Then ~0x3 = ~0b00011
> ra = ra & 0b11100
>
> This gives us then,
> ra = btf << 2; or
> btf = ra >> 2;
>
> Let's then check to see if your calculations read the right fields.
>
>> + if ((regs->ccr) & (1 << (31 - ra)))
>> + op->val = -1;
>> + else if ((regs->ccr) & (1 << (30 - ra)))
>> + op->val = 1;
>> + else
>> + op->val = 0;
>
>
> CR field: 7 6 5 4 3 2 1 0
> bit: 0123 0123 0123 0123 0123 0123 0123 0123
> normal bit #: 0.....................................31
> ibm bit #: 31.....................................0
>
> If btf = 0, ra = 0, check normal bits 31 and 30, which are both in CR0.
> CR field: 7 6 5 4 3 2 1 0
> bit: 0123 0123 0123 0123 0123 0123 0123 0123
> ^^
>
> If btf = 7, ra = 0b11100 = 28, so check normal bits 31-28 and 30-28,
> which are 3 and 2.
>
> CR field: 7 6 5 4 3 2 1 0
> bit: 0123 0123 0123 0123 0123 0123 0123 0123
> ^^
>
> If btf = 3, ra = 0b01100 = 12, for normal bits 19 and 18:
>
> CR field: 7 6 5 4 3 2 1 0
> bit: 0123 0123 0123 0123 0123 0123 0123 0123
> ^^
>
> So yes, your calculations, while I struggle to follow _how_ they work,
> do in fact seem to work.
>
> Checkpatch does have one complaint:
>
> CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'regs->ccr'
> #30: FILE: arch/powerpc/lib/sstep.c:1971:
> + if ((regs->ccr) & (1 << (31 - ra)))
>
> I don't really mind the parenteses: I think you are safe to ignore
> checkpatch here unless someone else complains :)
>
> If you do end up respinning the patch, I think it would be good to make
> the maths a bit clearer. I think it works because a left shift of 2 is
> the same as multiplying by 4, but it would be easier to follow if you
> used a temporary variable for btf.
Indeed. I wonder if it is better to follow the ISA itself. Per the ISA,
the bit we are interested in is:
4 x BFA + 32
So, if we use that along with the PPC_BIT() macro, we get:
if (regs->ccr & PPC_BIT(ra + 32))
>> + goto compute_done;
>> +
I can see why you thought this should be in the section with other
logical instructions. However, since this instruction does not modify CR
itself, this is probably better placed earlier -- somewhere near 'mfcr'
instruction emulation.
- Naveen
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: Remove bpf_jit_enable=2 debugging mode
From: Alexei Starovoitov @ 2021-04-20 3:28 UTC (permalink / raw)
To: Christophe Leroy
Cc: Ian Rogers, Song Liu, open list:DOCUMENTATION, Zi Shen Lim,
Alexei Starovoitov, Russell King, David S. Miller, Paul Mackerras,
Sandipan Das, H. Peter Anvin, sparclinux, Shubham Bansal,
Mahesh Bandewar, Will Deacon, linux-riscv, linux-s390,
Ilya Leoshkevich, paulburton, Jonathan Corbet,
Mauro Carvalho Chehab, Masahiro Yamada, X86 ML, John Fastabend,
Andrii Nakryiko, Christian Borntraeger, Quentin Monnet,
Dmitry Vyukov, Catalin Marinas, Naveen N . Rao, Jakub Kicinski,
Tobias Klauser, grantseltzer, Xi Wang, Albert Ou, Kees Cook,
Vasily Gorbik, Luke Nelson, Heiko Carstens, KP Singh, iecedge,
Simon Horman, Borislav Petkov, Alexander Viro, Paul Walmsley,
Jianlin Lv, Nicolas Dichtel, Ingo Molnar, linux-arm-kernel,
Wang YanQing, tsbogend, Daniel Borkmann, Hideaki YOSHIFUJI,
Network Development, David Ahern, linux-mips, LKML, Yonghong Song,
Björn Töpel, Palmer Dabbelt, Thomas Gleixner, bpf,
ppc-dev, Martin KaFai Lau
In-Reply-To: <0dea05ba-9467-0d84-4515-b8766f60318e@csgroup.eu>
On Sat, Apr 17, 2021 at 1:16 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
>
>
> Le 16/04/2021 à 01:49, Alexei Starovoitov a écrit :
> > On Thu, Apr 15, 2021 at 8:41 AM Quentin Monnet <quentin@isovalent.com> wrote:
> >>
> >> 2021-04-15 16:37 UTC+0200 ~ Daniel Borkmann <daniel@iogearbox.net>
> >>> On 4/15/21 11:32 AM, Jianlin Lv wrote:
> >>>> For debugging JITs, dumping the JITed image to kernel log is discouraged,
> >>>> "bpftool prog dump jited" is much better way to examine JITed dumps.
> >>>> This patch get rid of the code related to bpf_jit_enable=2 mode and
> >>>> update the proc handler of bpf_jit_enable, also added auxiliary
> >>>> information to explain how to use bpf_jit_disasm tool after this change.
> >>>>
> >>>> Signed-off-by: Jianlin Lv <Jianlin.Lv@arm.com>
> >>
> >> Hello,
> >>
> >> For what it's worth, I have already seen people dump the JIT image in
> >> kernel logs in Qemu VMs running with just a busybox, not for kernel
> >> development, but in a context where buiding/using bpftool was not
> >> possible.
> >
> > If building/using bpftool is not possible then majority of selftests won't
> > be exercised. I don't think such environment is suitable for any kind
> > of bpf development. Much so for JIT debugging.
> > While bpf_jit_enable=2 is nothing but the debugging tool for JIT developers.
> > I'd rather nuke that code instead of carrying it from kernel to kernel.
> >
>
> When I implemented JIT for PPC32, it was extremely helpfull.
>
> As far as I understand, for the time being bpftool is not usable in my environment because it
> doesn't support cross compilation when the target's endianess differs from the building host
> endianess, see discussion at
> https://lore.kernel.org/bpf/21e66a09-514f-f426-b9e2-13baab0b938b@csgroup.eu/
>
> That's right that selftests can't be exercised because they don't build.
>
> The question might be candid as I didn't investigate much about the replacement of "bpf_jit_enable=2
> debugging mode" by bpftool, how do we use bpftool exactly for that ? Especially when using the BPF
> test module ?
the kernel developers can add any amount of printk and dumps to debug
their code,
but such debugging aid should not be part of the production kernel.
That sysctl was two things at once: debugging tool for kernel devs and
introspection for users.
bpftool jit dump solves the 2nd part. It provides JIT introspection to users.
Debugging of the kernel can be done with any amount of auxiliary code
including calling print_hex_dump() during jiting.
^ permalink raw reply
* Re: [PATCH v4] powerpc/kexec_file: use current CPU info while setting up FDT
From: Sourabh Jain @ 2021-04-20 5:50 UTC (permalink / raw)
To: Michael Ellerman, Hari Bathini; +Cc: mahesh, stable, bauerman, linuxppc-dev
In-Reply-To: <877dkyea7g.fsf@mpe.ellerman.id.au>
On 19/04/21 5:51 pm, Michael Ellerman wrote:
> Hari Bathini <hbathini@linux.ibm.com> writes:
>> On 19/04/21 2:06 pm, Sourabh Jain wrote:
>>> kexec_file_load uses initial_boot_params in setting up the device-tree
>>> for the kernel to be loaded. Though initial_boot_params holds info
>>> about CPUs at the time of boot, it doesn't account for hot added CPUs.
>>>
>>> So, kexec'ing with kexec_file_load syscall would leave the kexec'ed
>>> kernel with inaccurate CPU info. Also, if kdump kernel is loaded with
>>> kexec_file_load syscall and the system crashes on a hot added CPU,
>>> capture kernel hangs failing to identify the boot CPU.
>>>
>>> Kernel panic - not syncing: sysrq triggered crash
>>> CPU: 24 PID: 6065 Comm: echo Kdump: loaded Not tainted 5.12.0-rc5upstream #54
>>> Call Trace:
>>> [c0000000e590fac0] [c0000000007b2400] dump_stack+0xc4/0x114 (unreliable)
>>> [c0000000e590fb00] [c000000000145290] panic+0x16c/0x41c
>>> [c0000000e590fba0] [c0000000008892e0] sysrq_handle_crash+0x30/0x40
>>> [c0000000e590fc00] [c000000000889cdc] __handle_sysrq+0xcc/0x1f0
>>> [c0000000e590fca0] [c00000000088a538] write_sysrq_trigger+0xd8/0x178
>>> [c0000000e590fce0] [c0000000005e9b7c] proc_reg_write+0x10c/0x1b0
>>> [c0000000e590fd10] [c0000000004f26d0] vfs_write+0xf0/0x330
>>> [c0000000e590fd60] [c0000000004f2aec] ksys_write+0x7c/0x140
>>> [c0000000e590fdb0] [c000000000031ee0] system_call_exception+0x150/0x290
>>> [c0000000e590fe10] [c00000000000ca5c] system_call_common+0xec/0x278
>>> --- interrupt: c00 at 0x7fff905b9664
>>> NIP: 00007fff905b9664 LR: 00007fff905320c4 CTR: 0000000000000000
>>> REGS: c0000000e590fe80 TRAP: 0c00 Not tainted (5.12.0-rc5upstream)
>>> MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 28000242
>>> XER: 00000000
>>> IRQMASK: 0
>>> GPR00: 0000000000000004 00007ffff5fedf30 00007fff906a7300 0000000000000001
>>> GPR04: 000001002a7355b0 0000000000000002 0000000000000001 00007ffff5fef616
>>> GPR08: 0000000000000001 0000000000000000 0000000000000000 0000000000000000
>>> GPR12: 0000000000000000 00007fff9073a160 0000000000000000 0000000000000000
>>> GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>>> GPR20: 0000000000000000 00007fff906a4ee0 0000000000000002 0000000000000001
>>> GPR24: 00007fff906a0898 0000000000000000 0000000000000002 000001002a7355b0
>>> GPR28: 0000000000000002 00007fff906a1790 000001002a7355b0 0000000000000002
>>> NIP [00007fff905b9664] 0x7fff905b9664
>>> LR [00007fff905320c4] 0x7fff905320c4
>>> --- interrupt: c00
>>>
>>> To avoid this from happening, extract current CPU info from of_root
>>> device node and use it for setting up the fdt in kexec_file_load case.
>>>
>>> Fixes: 6ecd0163d360 ("powerpc/kexec_file: Add appropriate regions for memory reserve map")
>>>
>>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>>> Reviewed-by: Hari Bathini <hbathini@linux.ibm.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>> arch/powerpc/kexec/file_load_64.c | 98 +++++++++++++++++++++++++++++++
>>> 1 file changed, 98 insertions(+)
>>>
>>> ---
>>> Changelog:
>>>
>>> v1 -> v3
>>> - https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-April/227756.html
>>>
>>> v3 -> v4
>>> - Rearranged if-else statement in update_cpus_node function to avoid
>>> redundant checks for positive cpus_offset.
>>> ---
>>>
>>> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
>>> index 02b9e4d0dc40..195ef303d530 100644
>>> --- a/arch/powerpc/kexec/file_load_64.c
>>> +++ b/arch/powerpc/kexec/file_load_64.c
>>> @@ -960,6 +960,99 @@ unsigned int kexec_fdt_totalsize_ppc64(struct kimage *image)
>>> return fdt_size;
>>> }
>>>
>>> +/**
>>> + * add_node_prop - Read property from device node structure and add
>>> + * them to fdt.
>>> + * @fdt: Flattened device tree of the kernel
>>> + * @node_offset: offset of the node to add a property at
>>> + * np: device node pointer
>>> + *
>>> + * Returns 0 on success, negative errno on error.
>>> + */
>>> +static int add_node_prop(void *fdt, int node_offset, const struct device_node *np)
>>> +{
>>> + int ret = 0;
>>> + struct property *pp;
>>> + unsigned long flags;
>>> +
>>> + if (!np)
>>> + return -EINVAL;
>>> +
>>> + raw_spin_lock_irqsave(&devtree_lock, flags);
>>> + for (pp = np->properties; pp; pp = pp->next) {
>>> + ret = fdt_setprop(fdt, node_offset, pp->name,
>>> + pp->value, pp->length);
>>> + if (ret < 0) {
>>> + pr_err("Unable to add %s property: %s\n",
>>> + pp->name, fdt_strerror(ret));
>>> + goto out;
>>> + }
>>> + }
>>> +out:
>>> + raw_spin_unlock_irqrestore(&devtree_lock, flags);
>>> + return ret;
>>> +}
>>> +
>>> +/**
>>> + * update_cpus_node - Update cpus node of flattened device-tree using of_root
>>> + * device node.
>>> + * @fdt: Flattened device tree of the kernel.
>>> + *
>>> + * Returns 0 on success, negative errno on error.
>>> + */
>>> +static int update_cpus_node(void *fdt)
>>> +{
>>> + struct device_node *cpus_node, *dn;
>>> + int cpus_offset, cpus_subnode_off, ret = 0;
>>> +
>>> + cpus_offset = fdt_path_offset(fdt, "/cpus");
>>> + if (cpus_offset < 0 && cpus_offset != -FDT_ERR_NOTFOUND) {
>>> + pr_err("Malformed device tree: error reading /cpus node: %s\n",
>>> + fdt_strerror(cpus_offset));
>>> + return cpus_offset;
>>> + } else {
>>
>>> + if (cpus_offset > 0) {
>>> + ret = fdt_del_node(fdt, cpus_offset);
>>> + if (ret < 0) {
>>> + pr_err("Error deleting /cpus node: %s\n",
>>> + fdt_strerror(ret));
>>> + return -EINVAL;
>>> + }
>>> + }
>>> +
>>> + /* Add cpus node to fdt */
>>> + cpus_offset = fdt_add_subnode(fdt, fdt_path_offset(fdt, "/"),
>>> + "cpus");
>>> + if (cpus_offset < 0) {
>>> + pr_err("Error creating /cpus node: %s\n",
>>> + fdt_strerror(cpus_offset));
>>> + return -EINVAL;
>>> + }
>>> +
>>> + /* Add cpus node properties */
>>> + cpus_node = of_find_node_by_path("/cpus");
>>> + ret = add_node_prop(fdt, cpus_offset, cpus_node);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + /* Loop through all subnodes of cpus and add them to fdt */
>>> + for_each_node_by_type(dn, "cpu") {
>>> + cpus_subnode_off = fdt_add_subnode(fdt,
>>> + cpus_offset,
>>> + dn->full_name);
>>> + if (cpus_subnode_off < 0) {
>>> + pr_err("Unable to add %s subnode: %s\n",
>>> + dn->full_name, fdt_strerror(cpus_subnode_off));
>>> + return cpus_subnode_off;
>>> + }
>>> + ret = add_node_prop(fdt, cpus_subnode_off, dn);
>>> + if (ret < 0)
>>> + return ret;
>>> + }
>> The above code block doesn't really need an explicit else condition..
> Yeah, use the early return to avoid having to indent all the rest of
> that logic.
>
> Please send a v5.
>
> Thanks for the review.
>
> v5 sent.
>
> - Sourabh Jain
^ permalink raw reply
* [PATCH v5] powerpc/kexec_file: use current CPU info while setting up FDT
From: Sourabh Jain @ 2021-04-20 5:46 UTC (permalink / raw)
To: mpe; +Cc: mahesh, Sourabh Jain, linuxppc-dev, stable, hbathini, bauerman
kexec_file_load uses initial_boot_params in setting up the device-tree
for the kernel to be loaded. Though initial_boot_params holds info
about CPUs at the time of boot, it doesn't account for hot added CPUs.
So, kexec'ing with kexec_file_load syscall would leave the kexec'ed
kernel with inaccurate CPU info. Also, if kdump kernel is loaded with
kexec_file_load syscall and the system crashes on a hot added CPU,
capture kernel hangs failing to identify the boot CPU.
Kernel panic - not syncing: sysrq triggered crash
CPU: 24 PID: 6065 Comm: echo Kdump: loaded Not tainted 5.12.0-rc5upstream #54
Call Trace:
[c0000000e590fac0] [c0000000007b2400] dump_stack+0xc4/0x114 (unreliable)
[c0000000e590fb00] [c000000000145290] panic+0x16c/0x41c
[c0000000e590fba0] [c0000000008892e0] sysrq_handle_crash+0x30/0x40
[c0000000e590fc00] [c000000000889cdc] __handle_sysrq+0xcc/0x1f0
[c0000000e590fca0] [c00000000088a538] write_sysrq_trigger+0xd8/0x178
[c0000000e590fce0] [c0000000005e9b7c] proc_reg_write+0x10c/0x1b0
[c0000000e590fd10] [c0000000004f26d0] vfs_write+0xf0/0x330
[c0000000e590fd60] [c0000000004f2aec] ksys_write+0x7c/0x140
[c0000000e590fdb0] [c000000000031ee0] system_call_exception+0x150/0x290
[c0000000e590fe10] [c00000000000ca5c] system_call_common+0xec/0x278
--- interrupt: c00 at 0x7fff905b9664
NIP: 00007fff905b9664 LR: 00007fff905320c4 CTR: 0000000000000000
REGS: c0000000e590fe80 TRAP: 0c00 Not tainted (5.12.0-rc5upstream)
MSR: 800000000280f033 <SF,VEC,VSX,EE,PR,FP,ME,IR,DR,RI,LE> CR: 28000242
XER: 00000000
IRQMASK: 0
GPR00: 0000000000000004 00007ffff5fedf30 00007fff906a7300 0000000000000001
GPR04: 000001002a7355b0 0000000000000002 0000000000000001 00007ffff5fef616
GPR08: 0000000000000001 0000000000000000 0000000000000000 0000000000000000
GPR12: 0000000000000000 00007fff9073a160 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
GPR20: 0000000000000000 00007fff906a4ee0 0000000000000002 0000000000000001
GPR24: 00007fff906a0898 0000000000000000 0000000000000002 000001002a7355b0
GPR28: 0000000000000002 00007fff906a1790 000001002a7355b0 0000000000000002
NIP [00007fff905b9664] 0x7fff905b9664
LR [00007fff905320c4] 0x7fff905320c4
--- interrupt: c00
To avoid this from happening, extract current CPU info from of_root
device node and use it for setting up the fdt in kexec_file_load case.
Fixes: 6ecd0163d360 ("powerpc/kexec_file: Add appropriate regions for memory reserve map")
Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
Reviewed-by: Hari Bathini <hbathini@linux.ibm.com>
Cc: <stable@vger.kernel.org>
---
arch/powerpc/kexec/file_load_64.c | 97 +++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
---
Changelog:
v1 -> v3
- https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-April/227756.html
v3 -> v4
- Rearranged if-else statement in update_cpus_node function to avoid
redundant checks for positive cpus_offset.
v4 -> v5
- removed unnecessary else condition in update_cpus_node function
---
diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index 02b9e4d0dc40..6af3792dbf18 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -960,6 +960,98 @@ unsigned int kexec_fdt_totalsize_ppc64(struct kimage *image)
return fdt_size;
}
+/**
+ * add_node_prop - Read property from device node structure and add
+ * them to fdt.
+ * @fdt: Flattened device tree of the kernel
+ * @node_offset: offset of the node to add a property at
+ * np: device node pointer
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+static int add_node_prop(void *fdt, int node_offset, const struct device_node *np)
+{
+ int ret = 0;
+ struct property *pp;
+ unsigned long flags;
+
+ if (!np)
+ return -EINVAL;
+
+ raw_spin_lock_irqsave(&devtree_lock, flags);
+ for (pp = np->properties; pp; pp = pp->next) {
+ ret = fdt_setprop(fdt, node_offset, pp->name,
+ pp->value, pp->length);
+ if (ret < 0) {
+ pr_err("Unable to add %s property: %s\n",
+ pp->name, fdt_strerror(ret));
+ goto out;
+ }
+ }
+out:
+ raw_spin_unlock_irqrestore(&devtree_lock, flags);
+ return ret;
+}
+
+/**
+ * update_cpus_node - Update cpus node of flattened device-tree using of_root
+ * device node.
+ * @fdt: Flattened device tree of the kernel.
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+static int update_cpus_node(void *fdt)
+{
+ struct device_node *cpus_node, *dn;
+ int cpus_offset, cpus_subnode_off, ret = 0;
+
+ cpus_offset = fdt_path_offset(fdt, "/cpus");
+ if (cpus_offset < 0 && cpus_offset != -FDT_ERR_NOTFOUND) {
+ pr_err("Malformed device tree: error reading /cpus node: %s\n",
+ fdt_strerror(cpus_offset));
+ return cpus_offset;
+ }
+
+ if (cpus_offset > 0) {
+ ret = fdt_del_node(fdt, cpus_offset);
+ if (ret < 0) {
+ pr_err("Error deleting /cpus node: %s\n",
+ fdt_strerror(ret));
+ return -EINVAL;
+ }
+ }
+
+ /* Add cpus node to fdt */
+ cpus_offset = fdt_add_subnode(fdt, fdt_path_offset(fdt, "/"),
+ "cpus");
+ if (cpus_offset < 0) {
+ pr_err("Error creating /cpus node: %s\n",
+ fdt_strerror(cpus_offset));
+ return -EINVAL;
+ }
+
+ /* Add cpus node properties */
+ cpus_node = of_find_node_by_path("/cpus");
+ ret = add_node_prop(fdt, cpus_offset, cpus_node);
+ if (ret < 0)
+ return ret;
+
+ /* Loop through all subnodes of cpus and add them to fdt */
+ for_each_node_by_type(dn, "cpu") {
+ cpus_subnode_off = fdt_add_subnode(fdt, cpus_offset,
+ dn->full_name);
+ if (cpus_subnode_off < 0) {
+ pr_err("Unable to add %s subnode: %s\n",
+ dn->full_name, fdt_strerror(cpus_subnode_off));
+ return cpus_subnode_off;
+ }
+ ret = add_node_prop(fdt, cpus_subnode_off, dn);
+ if (ret < 0)
+ return ret;
+ }
+ return ret;
+}
+
/**
* setup_new_fdt_ppc64 - Update the flattend device-tree of the kernel
* being loaded.
@@ -1020,6 +1112,11 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
}
}
+ /* Update cpus nodes information to account hotplug CPUs. */
+ ret = update_cpus_node(fdt);
+ if (ret < 0)
+ return ret;
+
/* Update memory reserve map */
ret = get_reserved_memory_ranges(&rmem);
if (ret)
--
2.26.3
^ permalink raw reply related
* Re: [PATCH 1/1] powerpc/pseries/iommu: Fix window size for direct mapping with pmem
From: Leonardo Bras @ 2021-04-20 5:29 UTC (permalink / raw)
To: Alexey Kardashevskiy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <58b28a98-37aa-055f-5dec-d8c0005c9519@ozlabs.ru>
On Tue, 2021-04-20 at 15:18 +1000, Alexey Kardashevskiy wrote:
>
> On 20/04/2021 14:54, Leonardo Bras wrote:
> > As of today, if the DDW is big enough to fit (1 << MAX_PHYSMEM_BITS) it's
> > possible to use direct DMA mapping even with pmem region.
> >
> > But, if that happens, the window size (len) is set to
> > (MAX_PHYSMEM_BITS - page_shift) instead of MAX_PHYSMEM_BITS, causing a
> > pagesize times smaller DDW to be created, being insufficient for correct
> > usage.
> >
> > Fix this so the correct window size is used in this case.
>
> Good find indeed.
>
> afaict this does not create a huge problem though as
> query.largest_available_block is always smaller than (MAX_PHYSMEM_BITS -
> page_shift) where it matters (phyp).
>
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
Thanks for reviewing!
Leonardo Bras
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-20 5:20 UTC (permalink / raw)
To: Dan Carpenter, Michael Ellerman
Cc: robh, kbuild-all, lkp, devicetree, linuxppc-dev, bauerman,
Daniel Axtens
In-Reply-To: <20210420050015.GA1959@kadam>
On 4/19/21 10:00 PM, Dan Carpenter wrote:
> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
>>>
>>>> Daniel Axtens <dja@axtens.net> writes:
>>>>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>>>>>>
>>>>>> Sorry - missed copying device-tree and powerpc mailing lists.
>>>>>>
>>>>>>> There are a few "goto out;" statements before the local variable "fdt"
>>>>>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>>>>>>> elf64_load(). This will result in an uninitialized "fdt" being passed
>>>>>>> to kvfree() in this function if there is an error before the call to
>>>>>>> of_kexec_alloc_and_setup_fdt().
>>>>>>>
>>>>>>> Initialize the local variable "fdt" to NULL.
>>>>>>>
>>>>> I'm a huge fan of initialising local variables! But I'm struggling to
>>>>> find the code path that will lead to an uninit fdt being returned...
>>>>>
>>>>> The out label reads in part:
>>>>>
>>>>> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
>>>>> return ret ? ERR_PTR(ret) : fdt;
>>>>>
>>>>> As far as I can tell, any time we get a non-zero ret, we're going to
>>>>> return an error pointer rather than the uninitialised value...
>>>
>>> As Dan pointed out, the new code is in linux-next.
>>>
>>> I have copied the new one below - the function doesn't return fdt, but
>>> instead sets it in the arch specific field (please see the link to the
>>> updated elf_64.c below).
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
>>>
>>>>>
>>>>> (btw, it does look like we might leak fdt if we have an error after we
>>>>> successfully kmalloc it.)
>>>>>
>>>>> Am I missing something? Can you link to the report for the kernel test
>>>>> robot or from Dan?
>>>
>>> /*
>>> * Once FDT buffer has been successfully passed to
>>> kexec_add_buffer(),
>>> * the FDT buffer address is saved in image->arch.fdt. In that
>>> case,
>>> * the memory cannot be freed here in case of any other error.
>>> */
>>> if (ret && !image->arch.fdt)
>>> kvfree(fdt);
>>>
>>> return ret ? ERR_PTR(ret) : NULL;
>>>
>>> In case of an error, the memory allocated for fdt is freed unless it has
>>> already been passed to kexec_add_buffer().
>>
>> It feels like the root of the problem is that the kvfree of fdt is in
>> the wrong place. It's only allocated later in the function, so the error
>> path should reflect that. Something like the patch below.
>>
>> cheers
>>
>>
>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>> index 5a569bb51349..02662e72c53d 100644
>> --- a/arch/powerpc/kexec/elf_64.c
>> +++ b/arch/powerpc/kexec/elf_64.c
>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>> initrd_len, cmdline);
>> if (ret)
>> - goto out;
>> + goto out_free_fdt;
>>
>> fdt_pack(fdt);
>>
>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>> ret = kexec_add_buffer(&kbuf);
>> if (ret)
>> - goto out;
>> + goto out_free_fdt;
>>
>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
>> image->arch.fdt = fdt;
>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>> if (ret)
>> pr_err("Error setting up the purgatory.\n");
>>
>> + goto out;
>
> This will leak. It would need to be something like:
>
> if (ret) {
> pr_err("Error setting up the purgatory.\n");
> goto out_free_fdt;
> }
Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
be freed here - it will be freed when the kexec cleanup function is called.
>
> goto out;
>
> But we should also fix the uninitialized variable of "elf_info" if
> kexec_build_elf_info() fails.
kexec_build_elf_info() frees elf_info and zeroes it in error paths,
except when elf_read_ehdr() fails. So, I think it is better to
initialize the local variable "elf_info" before calling
kexec_build_elf_info().
memset(&elf_info, 0, sizeof(elf_info));
thanks,
-lakshmi
>
>> +
>> +out_free_fdt:
>> + kvfree(fdt);
>> out:
>> kfree(modified_cmdline);
>> kexec_free_elf_info(&elf_info);
>>
>> - /*
>> - * Once FDT buffer has been successfully passed to kexec_add_buffer(),
>> - * the FDT buffer address is saved in image->arch.fdt. In that case,
>> - * the memory cannot be freed here in case of any other error.
>> - */
>> - if (ret && !image->arch.fdt)
>> - kvfree(fdt);
>> -
>> return ret ? ERR_PTR(ret) : NULL;
>> }
>
> regards,
> dan carpenter
>
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/pseries/iommu: Fix window size for direct mapping with pmem
From: Alexey Kardashevskiy @ 2021-04-20 5:18 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210420045404.438735-1-leobras.c@gmail.com>
On 20/04/2021 14:54, Leonardo Bras wrote:
> As of today, if the DDW is big enough to fit (1 << MAX_PHYSMEM_BITS) it's
> possible to use direct DMA mapping even with pmem region.
>
> But, if that happens, the window size (len) is set to
> (MAX_PHYSMEM_BITS - page_shift) instead of MAX_PHYSMEM_BITS, causing a
> pagesize times smaller DDW to be created, being insufficient for correct
> usage.
>
> Fix this so the correct window size is used in this case.
Good find indeed.
afaict this does not create a huge problem though as
query.largest_available_block is always smaller than (MAX_PHYSMEM_BITS -
page_shift) where it matters (phyp).
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Fixes: bf6e2d562bbc4("powerpc/dma: Fallback to dma_ops when persistent memory present")
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/platforms/pseries/iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 9fc5217f0c8e..836cbbe0ecc5 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -1229,7 +1229,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> if (pmem_present) {
> if (query.largest_available_block >=
> (1ULL << (MAX_PHYSMEM_BITS - page_shift)))
> - len = MAX_PHYSMEM_BITS - page_shift;
> + len = MAX_PHYSMEM_BITS;
> else
> dev_info(&dev->dev, "Skipping ibm,pmemory");
> }
>
--
Alexey
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Dan Carpenter @ 2021-04-20 5:00 UTC (permalink / raw)
To: Michael Ellerman
Cc: devicetree, kbuild-all, lkp, robh, Lakshmi Ramasubramanian,
linuxppc-dev, bauerman, Daniel Axtens
In-Reply-To: <87pmypdf93.fsf@mpe.ellerman.id.au>
On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> > On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >
> >> Daniel Axtens <dja@axtens.net> writes:
> >>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >>>>
> >>>> Sorry - missed copying device-tree and powerpc mailing lists.
> >>>>
> >>>>> There are a few "goto out;" statements before the local variable "fdt"
> >>>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> >>>>> elf64_load(). This will result in an uninitialized "fdt" being passed
> >>>>> to kvfree() in this function if there is an error before the call to
> >>>>> of_kexec_alloc_and_setup_fdt().
> >>>>>
> >>>>> Initialize the local variable "fdt" to NULL.
> >>>>>
> >>> I'm a huge fan of initialising local variables! But I'm struggling to
> >>> find the code path that will lead to an uninit fdt being returned...
> >>>
> >>> The out label reads in part:
> >>>
> >>> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> >>> return ret ? ERR_PTR(ret) : fdt;
> >>>
> >>> As far as I can tell, any time we get a non-zero ret, we're going to
> >>> return an error pointer rather than the uninitialised value...
> >
> > As Dan pointed out, the new code is in linux-next.
> >
> > I have copied the new one below - the function doesn't return fdt, but
> > instead sets it in the arch specific field (please see the link to the
> > updated elf_64.c below).
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
> >
> >>>
> >>> (btw, it does look like we might leak fdt if we have an error after we
> >>> successfully kmalloc it.)
> >>>
> >>> Am I missing something? Can you link to the report for the kernel test
> >>> robot or from Dan?
> >
> > /*
> > * Once FDT buffer has been successfully passed to
> > kexec_add_buffer(),
> > * the FDT buffer address is saved in image->arch.fdt. In that
> > case,
> > * the memory cannot be freed here in case of any other error.
> > */
> > if (ret && !image->arch.fdt)
> > kvfree(fdt);
> >
> > return ret ? ERR_PTR(ret) : NULL;
> >
> > In case of an error, the memory allocated for fdt is freed unless it has
> > already been passed to kexec_add_buffer().
>
> It feels like the root of the problem is that the kvfree of fdt is in
> the wrong place. It's only allocated later in the function, so the error
> path should reflect that. Something like the patch below.
>
> cheers
>
>
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 5a569bb51349..02662e72c53d 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> initrd_len, cmdline);
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> fdt_pack(fdt);
>
> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> ret = kexec_add_buffer(&kbuf);
> if (ret)
> - goto out;
> + goto out_free_fdt;
>
> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> image->arch.fdt = fdt;
> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> if (ret)
> pr_err("Error setting up the purgatory.\n");
>
> + goto out;
This will leak. It would need to be something like:
if (ret) {
pr_err("Error setting up the purgatory.\n");
goto out_free_fdt;
}
goto out;
But we should also fix the uninitialized variable of "elf_info" if
kexec_build_elf_info() fails.
> +
> +out_free_fdt:
> + kvfree(fdt);
> out:
> kfree(modified_cmdline);
> kexec_free_elf_info(&elf_info);
>
> - /*
> - * Once FDT buffer has been successfully passed to kexec_add_buffer(),
> - * the FDT buffer address is saved in image->arch.fdt. In that case,
> - * the memory cannot be freed here in case of any other error.
> - */
> - if (ret && !image->arch.fdt)
> - kvfree(fdt);
> -
> return ret ? ERR_PTR(ret) : NULL;
> }
regards,
dan carpenter
^ permalink raw reply
* Re: PPC_FPU, ALTIVEC: enable_kernel_fp, put_vr, get_vr
From: Christophe Leroy @ 2021-04-20 4:55 UTC (permalink / raw)
To: Randy Dunlap, Michael Ellerman, Segher Boessenkool; +Cc: PowerPC, LKML
In-Reply-To: <bd83b06d-ed36-e600-e988-c1e0014fb9cf@infradead.org>
Le 19/04/2021 à 23:39, Randy Dunlap a écrit :
> On 4/19/21 6:16 AM, Michael Ellerman wrote:
>> Randy Dunlap <rdunlap@infradead.org> writes:
>
>>> Sure. I'll post them later today.
>>> They keep FPU and ALTIVEC as independent (build) features.
>>
>> Those patches look OK.
>>
>> But I don't think it makes sense to support that configuration, FPU=n
>> ALTVEC=y. No one is ever going to make a CPU like that. We have enough
>> testing surface due to configuration options, without adding artificial
>> combinations that no one is ever going to use.
>>
>> IMHO :)
>>
>> So I'd rather we just make ALTIVEC depend on FPU.
>
> That's rather simple. See below.
> I'm doing a bunch of randconfig builds with it now.
>
> ---
> From: Randy Dunlap <rdunlap@infradead.org>
> Subject: [PATCH] powerpc: make ALTIVEC depend PPC_FPU
>
> On a kernel config with ALTIVEC=y and PPC_FPU not set/enabled,
> there are build errors:
>
> drivers/cpufreq/pmac32-cpufreq.c:262:2: error: implicit declaration of function 'enable_kernel_fp' [-Werror,-Wimplicit-function-declaration]
> enable_kernel_fp();
> ../arch/powerpc/lib/sstep.c: In function 'do_vec_load':
> ../arch/powerpc/lib/sstep.c:637:3: error: implicit declaration of function 'put_vr' [-Werror=implicit-function-declaration]
> 637 | put_vr(rn, &u.v);
> | ^~~~~~
> ../arch/powerpc/lib/sstep.c: In function 'do_vec_store':
> ../arch/powerpc/lib/sstep.c:660:3: error: implicit declaration of function 'get_vr'; did you mean 'get_oc'? [-Werror=implicit-function-declaration]
> 660 | get_vr(rn, &u.v);
> | ^~~~~~
>
> In theory ALTIVEC is independent of PPC_FPU but in practice nobody
> is going to build such a machine, so make ALTIVEC require PPC_FPU
> by depending on PPC_FPU.
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: Segher Boessenkool <segher@kernel.crashing.org>
> Cc: lkp@intel.com
> ---
> arch/powerpc/platforms/86xx/Kconfig | 1 +
> arch/powerpc/platforms/Kconfig.cputype | 2 ++
> 2 files changed, 3 insertions(+)
>
> --- linux-next-20210416.orig/arch/powerpc/platforms/86xx/Kconfig
> +++ linux-next-20210416/arch/powerpc/platforms/86xx/Kconfig
> @@ -4,6 +4,7 @@ menuconfig PPC_86xx
> bool "86xx-based boards"
> depends on PPC_BOOK3S_32
> select FSL_SOC
> + select PPC_FPU
> select ALTIVEC
> help
> The Freescale E600 SoCs have 74xx cores.
> --- linux-next-20210416.orig/arch/powerpc/platforms/Kconfig.cputype
> +++ linux-next-20210416/arch/powerpc/platforms/Kconfig.cputype
> @@ -186,6 +186,7 @@ config E300C3_CPU
> config G4_CPU
> bool "G4 (74xx)"
> depends on PPC_BOOK3S_32
> + select PPC_FPU
> select ALTIVEC
>
> endchoice
> @@ -309,6 +310,7 @@ config PHYS_64BIT
>
> config ALTIVEC
> bool "AltiVec Support"
> + depends on PPC_FPU
Shouldn't we do it the other way round ? In extenso make ALTIVEC select PPC_FPU and avoid the two
selects that are above ?
> depends on PPC_BOOK3S_32 || PPC_BOOK3S_64 || (PPC_E500MC && PPC64)
> help
> This option enables kernel support for the Altivec extensions to the
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox