* [PATCH 1/6] arm64: dts: meson-gxl-s905x-khadas-vim: fix gpio-keys-polled node
From: Christian Hewitt @ 2019-09-09 15:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1568041287-7805-1-git-send-email-christianshewitt@gmail.com>
Fix DTC warnings:
arch/arm/dts/meson-gxl-s905x-khadas-vim.dtb: Warning (avoid_unnecessary_addr_size):
/gpio-keys-polled: unnecessary #address-cells/#size-cells
without "ranges" or child "reg" property
Fixes: e15d2774b8c ("ARM64: dts: meson-gxl: add support for the Khadas VIM board")
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
index 5499e8d..41be2af 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-khadas-vim.dts
@@ -33,11 +33,9 @@
gpio-keys-polled {
compatible = "gpio-keys-polled";
- #address-cells = <1>;
- #size-cells = <0>;
poll-interval = <100>;
- button@0 {
+ power-button {
label = "power";
linux,code = <KEY_POWER>;
gpios = <&gpio_ao GPIOAO_2 GPIO_ACTIVE_LOW>;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 3/6] arm64: dts: meson-gxm-khadas-vim2: fix uart_A bluetooth node
From: Christian Hewitt @ 2019-09-09 15:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1568041287-7805-1-git-send-email-christianshewitt@gmail.com>
Fixes: 33344e2111a ("arm64: dts: meson-gxm-khadas-vim2: fix Bluetooth support")
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
index 989d33a..d392062 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts
@@ -409,6 +409,9 @@
bluetooth {
compatible = "brcm,bcm43438-bt";
shutdown-gpios = <&gpio GPIOX_17 GPIO_ACTIVE_HIGH>;
+ max-speed = <2000000>;
+ clocks = <&wifi32k>;
+ clock-names = "lpo";
};
};
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 4/6] arm64: dts: meson: libretech-ac: update model description
From: Christian Hewitt @ 2019-09-09 15:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1568041287-7805-1-git-send-email-christianshewitt@gmail.com>
Shorten the model description to improve readability in some app GUIs
that show the string.
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxl-s805x-libretech-ac.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s805x-libretech-ac.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s805x-libretech-ac.dts
index 82b1c48..4d59494 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s805x-libretech-ac.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s805x-libretech-ac.dts
@@ -14,7 +14,7 @@
/ {
compatible = "libretech,aml-s805x-ac", "amlogic,s805x",
"amlogic,meson-gxl";
- model = "Libre Computer Board AML-S805X-AC";
+ model = "Libre Computer AML-S805X-AC";
aliases {
serial0 = &uart_AO;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 5/6] dt-bindings: arm: amlogic: update libretech-cc compatible
From: Christian Hewitt @ 2019-09-09 15:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1568041287-7805-1-git-send-email-christianshewitt@gmail.com>
Update the compatible for the Libre Computer aml-s905x-cc to be more
descriptive using the format introduced with the aml-s805x-ac board.
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
Documentation/devicetree/bindings/arm/amlogic.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/arm/amlogic.yaml b/Documentation/devicetree/bindings/arm/amlogic.yaml
index 325c6fd..b3c9dbb 100644
--- a/Documentation/devicetree/bindings/arm/amlogic.yaml
+++ b/Documentation/devicetree/bindings/arm/amlogic.yaml
@@ -95,7 +95,7 @@ properties:
- amlogic,p212
- hwacom,amazetv
- khadas,vim
- - libretech,cc
+ - libretech,aml-s905x-cc
- nexbox,a95x
- seirobotics,sei510
- const: amlogic,s905x
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 6/6] arm64: dts: meson: libretech-cc: update model and compatible
From: Christian Hewitt @ 2019-09-09 15:01 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Kevin Hilman, devicetree,
linux-arm-kernel, linux-amlogic, linux-kernel
Cc: Chrisitian Hewitt
In-Reply-To: <1568041287-7805-1-git-send-email-christianshewitt@gmail.com>
Shorten the model description to improve readability in some app GUIs
that show the string. Update compatible to be more descriptive, using
the format of the LaFrite board in meson-gxl-s805x-libretech-ac.dts.
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts
index 4b8ce73..e8348b2 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts
@@ -12,8 +12,9 @@
#include "meson-gxl-s905x.dtsi"
/ {
- compatible = "libretech,cc", "amlogic,s905x", "amlogic,meson-gxl";
- model = "Libre Computer Board AML-S905X-CC";
+ compatible = "libretech,aml-s905x-cc", "amlogic,s905x",
+ "amlogic,meson-gxl";
+ model = "Libre Computer AML-S905X-CC";
aliases {
serial0 = &uart_AO;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] usb: dwc3: Add shutdown to platform_driver
From: Vicente Bergas @ 2019-09-09 15:07 UTC (permalink / raw)
To: Felipe Balbi, Robin Murphy
Cc: Matthias Brugger, Heiko Stuebner, MarcZyngier, Catalin Marinas,
linux-usb, Will Deacon, linux-rockchip, Greg Kroah-Hartman,
linux-arm-kernel
In-Reply-To: <645526b8-bfed-4cc6-9500-1843c5fe0da9@gmail.com>
On Tuesday, August 27, 2019 2:16:20 PM CEST, Vicente Bergas wrote:
> On Tuesday, August 27, 2019 1:53:04 PM CEST, Felipe Balbi wrote:
>> Hi,
>>
>> Vicente Bergas <vicencb@gmail.com> writes:
>>> On Saturday, August 17, 2019 7:41:40 PM CEST, Vicente Bergas wrote:
>>>> Otherwise the device keeps writing to memory after kexec and disturbs
>>>> the next kernel.
> ...
>>
>> why don't you just have shutdown use the same exact function as remove?
>> Frankly, though, I still don't fully understand what's going wrong
>> here. Why is the device still alive during kexec?
>>
>> cheers
>
> Hi Felipe,
> the remove and shutdown functions have different prototypes, so
> shutdown is wrapping remove.
> Would it be preferable to cast remove as shutdown?
>
> The issue with kexec is that the device is being used during the livetime
> of the first kernel. When the first kernel executes kexec it calls the
> shutdown function of drivers (instead of remove). Because of this the dwc3
> device keeps doing things like DMA.
> While the second kernel is taking over, it gets its memory corrupted with
> such DMA accesses from the device. When the second kernel reaches the point
> of taking over the dwc3 device, re-initializes it, but it is already too
> late. Still worse, if the second kernel did not have the dwc3 driver, it
> would get endless memory corruptions.
> All in all, devices that can do DMA need to stop doing it on shutdown.
>
> Regards,
> Vicenç.
Hi,
please, can you provide some feedback on this?
Regards,
Vicenç.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/3] arm64: alpine: select AL_POS
From: Arnd Bergmann @ 2019-09-09 15:08 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel@vger.kernel.org, jonnyc, Mauro Carvalho Chehab,
ronenk, Will Deacon, Benjamin Herrenschmidt, DTML, Maxime Ripard,
Rob Herring, Santosh Shilimkar, Thomas Gleixner, hanochu,
Linux ARM, barakw, hhhawa, gregkh, paul.kocialkowski,
Patrick Venture, Olof Johansson, David Miller, David Woodhouse
In-Reply-To: <0d36f94d-596f-0ec7-6951-b097b5ee0d2d@amazon.com>
On Mon, Sep 9, 2019 at 3:59 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 4:45 PM, Arnd Bergmann wrote:
>
> Its not that something will get broken. its error event detector for POS
> events which allows seeing bad accesses to registers.
>
> What is the general rule of which configs to put under select and which
> under defconfig?
>
> I was thinking that "general" SoC support is good under select - those
> things that we always want.
I generally want as little as possible to be selected, basically only
things that are required for linking the kernel and booting it without
potentially destroying the hardware.
In particular, I want most drivers to be enabled as loadable modules
if possible. When you have general-purpose distributions support
your platform, there is no need to have this module built-in while
running on a different chip, even if you always want to load the
module when it's running on yours.
> And specific features, e.g. RAID support or features that supported only
> on specific HW shall go under defconfig.
>
> Similar, I see ARCH_LAYERSCAPE selecting EDAC_SUPPORT.
I think this was done to avoid a link failure. It's also possible that this
is a mistake and just did not get caught in review.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 1/8] mm: Add per-cpu logic to page shuffling
From: Alexander Duyck @ 2019-09-09 15:11 UTC (permalink / raw)
To: David Hildenbrand, Alexander Duyck, virtio-dev, kvm, mst,
catalin.marinas, dave.hansen, linux-kernel, willy, mhocko,
linux-mm, akpm, will, linux-arm-kernel, osalvador
Cc: yang.zhang.wz, pagupta, riel, konrad.wilk, ying.huang,
lcapitulino, wei.w.wang, aarcange, nitesh, pbonzini,
dan.j.williams, fengguang.wu, kirill.shutemov
In-Reply-To: <0df2e5d0-af92-04b4-aa7d-891387874039@redhat.com>
On Mon, 2019-09-09 at 10:14 +0200, David Hildenbrand wrote:
> On 07.09.19 19:25, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Change the logic used to generate randomness in the suffle path so that we
> > can avoid cache line bouncing. The previous logic was sharing the offset
> > and entropy word between all CPUs. As such this can result in cache line
> > bouncing and will ultimately hurt performance when enabled.
>
> So, usually we perform such changes if there is real evidence. Do you
> have any such performance numbers to back your claims?
I'll have to go rerun the test to get the exact numbers. The reason this
came up is that my original test was spanning NUMA nodes and that made
this more expensive as a result since the memory was both not local to the
CPU and was being updated by multiple sockets.
I will try building a pair of host kernels with shuffling enabled and this
patch applied to one and can add that data to the patch description.
- Alex
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 1/8] mm: Add per-cpu logic to page shuffling
From: Alexander Duyck @ 2019-09-09 15:12 UTC (permalink / raw)
To: Kirill A. Shutemov, Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas, mhocko,
linux-mm, will, aarcange, virtio-dev, mst, willy, wei.w.wang,
ying.huang, riel, dan.j.williams, lcapitulino, linux-arm-kernel,
osalvador, nitesh, konrad.wilk, dave.hansen, linux-kernel,
pbonzini, akpm, fengguang.wu, kirill.shutemov
In-Reply-To: <20190909090701.7ebz4foxyu3rxzvc@box>
On Mon, 2019-09-09 at 12:07 +0300, Kirill A. Shutemov wrote:
> On Sat, Sep 07, 2019 at 10:25:12AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Change the logic used to generate randomness in the suffle path so that we
>
> Typo.
>
> > can avoid cache line bouncing. The previous logic was sharing the offset
> > and entropy word between all CPUs. As such this can result in cache line
> > bouncing and will ultimately hurt performance when enabled.
> >
> > To resolve this I have moved to a per-cpu logic for maintaining a unsigned
> > long containing some amount of bits, and an offset value for which bit we
> > can use for entropy with each call.
> >
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> > mm/shuffle.c | 33 +++++++++++++++++++++++----------
> > 1 file changed, 23 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/shuffle.c b/mm/shuffle.c
> > index 3ce12481b1dc..9ba542ecf335 100644
> > --- a/mm/shuffle.c
> > +++ b/mm/shuffle.c
> > @@ -183,25 +183,38 @@ void __meminit __shuffle_free_memory(pg_data_t *pgdat)
> > shuffle_zone(z);
> > }
> >
> > +struct batched_bit_entropy {
> > + unsigned long entropy_bool;
> > + int position;
> > +};
> > +
> > +static DEFINE_PER_CPU(struct batched_bit_entropy, batched_entropy_bool);
> > +
> > void add_to_free_area_random(struct page *page, struct free_area *area,
> > int migratetype)
> > {
> > - static u64 rand;
> > - static u8 rand_bits;
> > + struct batched_bit_entropy *batch;
> > + unsigned long entropy;
> > + int position;
> >
> > /*
> > - * The lack of locking is deliberate. If 2 threads race to
> > - * update the rand state it just adds to the entropy.
> > + * We shouldn't need to disable IRQs as the only caller is
> > + * __free_one_page and it should only be called with the zone lock
> > + * held and either from IRQ context or with local IRQs disabled.
> > */
> > - if (rand_bits == 0) {
> > - rand_bits = 64;
> > - rand = get_random_u64();
> > + batch = raw_cpu_ptr(&batched_entropy_bool);
> > + position = batch->position;
> > +
> > + if (--position < 0) {
> > + batch->entropy_bool = get_random_long();
> > + position = BITS_PER_LONG - 1;
> > }
> >
> > - if (rand & 1)
> > + batch->position = position;
> > + entropy = batch->entropy_bool;
> > +
> > + if (1ul & (entropy >> position))
>
> Maybe something like this would be more readble:
>
> if (entropy & BIT(position))
>
> > add_to_free_area(page, area, migratetype);
> > else
> > add_to_free_area_tail(page, area, migratetype);
> > - rand_bits--;
> > - rand >>= 1;
> > }
> >
> >
Thanks for the review. I will update these two items for v10.
- Alex
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/1] mm/pgtable/debug: Add test validating architecture page table helpers
From: Kirill A. Shutemov @ 2019-09-09 15:13 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Mark Rutland, linux-ia64, linux-sh, Tetsuo Handa, James Hogan,
Heiko Carstens, Michal Hocko, linux-mm, Dave Hansen,
Paul Mackerras, sparclinux, Thomas Gleixner, linux-s390,
Michael Ellerman, x86, Russell King - ARM Linux, Matthew Wilcox,
Steven Price, Jason Gunthorpe, Vlastimil Babka, linux-snps-arc,
Kees Cook, Masahiro Yamada, Mark Brown, Dan Williams,
Gerald Schaefer, linux-arm-kernel, Sri Krishna chowdary,
Ard Biesheuvel, Greg Kroah-Hartman, linux-mips, Ralf Baechle,
linux-kernel, Peter Zijlstra, Mike Rapoport, Paul Burton,
Vineet Gupta, Martin Schwidefsky, Andrew Morton, linuxppc-dev,
David S. Miller
In-Reply-To: <3d5de35f-8192-1c75-50a9-03e66e3b8e5c@arm.com>
On Mon, Sep 09, 2019 at 11:56:50AM +0530, Anshuman Khandual wrote:
>
>
> On 09/07/2019 12:33 AM, Gerald Schaefer wrote:
> > On Fri, 6 Sep 2019 11:58:59 +0530
> > Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> >
> >> On 09/05/2019 10:36 PM, Gerald Schaefer wrote:
> >>> On Thu, 5 Sep 2019 14:48:14 +0530
> >>> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> >>>
> >>>>> [...]
> >>>>>> +
> >>>>>> +#if !defined(__PAGETABLE_PMD_FOLDED) && !defined(__ARCH_HAS_4LEVEL_HACK)
> >>>>>> +static void pud_clear_tests(pud_t *pudp)
> >>>>>> +{
> >>>>>> + memset(pudp, RANDOM_NZVALUE, sizeof(pud_t));
> >>>>>> + pud_clear(pudp);
> >>>>>> + WARN_ON(!pud_none(READ_ONCE(*pudp)));
> >>>>>> +}
> >>>>>
> >>>>> For pgd/p4d/pud_clear(), we only clear if the page table level is present
> >>>>> and not folded. The memset() here overwrites the table type bits, so
> >>>>> pud_clear() will not clear anything on s390 and the pud_none() check will
> >>>>> fail.
> >>>>> Would it be possible to OR a (larger) random value into the table, so that
> >>>>> the lower 12 bits would be preserved?
> >>>>
> >>>> So the suggestion is instead of doing memset() on entry with RANDOM_NZVALUE,
> >>>> it should OR a large random value preserving lower 12 bits. Hmm, this should
> >>>> still do the trick for other platforms, they just need non zero value. So on
> >>>> s390, the lower 12 bits on the page table entry already has valid value while
> >>>> entering this function which would make sure that pud_clear() really does
> >>>> clear the entry ?
> >>>
> >>> Yes, in theory the table entry on s390 would have the type set in the last
> >>> 4 bits, so preserving those would be enough. If it does not conflict with
> >>> others, I would still suggest preserving all 12 bits since those would contain
> >>> arch-specific flags in general, just to be sure. For s390, the pte/pmd tests
> >>> would also work with the memset, but for consistency I think the same logic
> >>> should be used in all pxd_clear_tests.
> >>
> >> Makes sense but..
> >>
> >> There is a small challenge with this. Modifying individual bits on a given
> >> page table entry from generic code like this test case is bit tricky. That
> >> is because there are not enough helpers to create entries with an absolute
> >> value. This would have been easier if all the platforms provided functions
> >> like __pxx() which is not the case now. Otherwise something like this should
> >> have worked.
> >>
> >>
> >> pud_t pud = READ_ONCE(*pudp);
> >> pud = __pud(pud_val(pud) | RANDOM_VALUE (keeping lower 12 bits 0))
> >> WRITE_ONCE(*pudp, pud);
> >>
> >> But __pud() will fail to build in many platforms.
> >
> > Hmm, I simply used this on my system to make pud_clear_tests() work, not
> > sure if it works on all archs:
> >
> > pud_val(*pudp) |= RANDOM_NZVALUE;
>
> Which compiles on arm64 but then fails on x86 because of the way pmd_val()
> has been defined there.
Use instead
*pudp = __pud(pud_val(*pudp) | RANDOM_NZVALUE);
It *should* be more portable.
--
Kirill A. Shutemov
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] KVM: arm/arm64: Allow user injection of external data aborts
From: Christoffer Dall @ 2019-09-09 15:16 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P. Berrangé, Suzuki K Poulose, Marc Zyngier,
James Morse, Julien Thierry, Stefan Hajnoczi, Heinrich Schuchardt,
Alexander Graf, kvmarm, arm-mail-list
In-Reply-To: <CAFEAcA8WcQNJV27q5WJ-SMhOyjFZyh1Pz7H7duK6zW1wiAvqKQ@mail.gmail.com>
On Mon, Sep 09, 2019 at 01:32:46PM +0100, Peter Maydell wrote:
> On Mon, 9 Sep 2019 at 13:13, Christoffer Dall <christoffer.dall@arm.com> wrote:
> >
> > In some scenarios, such as buggy guest or incorrect configuration of the
> > VMM and firmware description data, userspace will detect a memory access
> > to a portion of the IPA, which is not mapped to any MMIO region.
> >
> > For this purpose, the appropriate action is to inject an external abort
> > to the guest. The kernel already has functionality to inject an
> > external abort, but we need to wire up a signal from user space that
> > lets user space tell the kernel to do this.
> >
> > It turns out, we already have the set event functionality which we can
> > perfectly reuse for this.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > ---
> > Documentation/virt/kvm/api.txt | 15 ++++++++++++++-
> > arch/arm/include/uapi/asm/kvm.h | 3 ++-
> > arch/arm/kvm/guest.c | 3 +++
> > arch/arm64/include/uapi/asm/kvm.h | 3 ++-
> > arch/arm64/kvm/guest.c | 3 +++
> > arch/arm64/kvm/inject_fault.c | 4 ++--
> > include/uapi/linux/kvm.h | 1 +
> > virt/kvm/arm/arm.c | 1 +
> > 8 files changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/virt/kvm/api.txt b/Documentation/virt/kvm/api.txt
> > index 02501333f746..edd6cdc470ca 100644
> > --- a/Documentation/virt/kvm/api.txt
> > +++ b/Documentation/virt/kvm/api.txt
> > @@ -955,6 +955,8 @@ The following bits are defined in the flags field:
> >
> > ARM/ARM64:
> >
> > +User space may need to inject several types of events to the guest.
> > +
> > If the guest accesses a device that is being emulated by the host kernel in
> > such a way that a real device would generate a physical SError, KVM may make
> > a virtual SError pending for that VCPU. This system error interrupt remains
> > @@ -989,12 +991,23 @@ Specifying exception.has_esr on a system that does not support it will return
> > -EINVAL. Setting anything other than the lower 24bits of exception.serror_esr
> > will return -EINVAL.
> >
> > +If the guest performed an access to I/O memory which could not be handled by
> > +user space, for example because of missing instruction syndrome decode
> > +information or because there is no device mapped at the accessed IPA, then
> > +user space can ask the kernel to inject an external abort using the address
> > +from the exiting fault on the VCPU. It is a programming error to set
> > +ext_dabt_pending at the same time as any of the serror fields, or to set
> > +ext_dabt_pending on an exit which was not either KVM_EXIT_MMIO or
> > +KVM_EXIT_ARM_NISV. This feature is only available if the system supports
> > +KVM_CAP_ARM_INJECT_EXT_DABT;
> > +
> > struct kvm_vcpu_events {
> > struct {
> > __u8 serror_pending;
> > __u8 serror_has_esr;
> > + __u8 ext_dabt_pending;
> > /* Align it to 8 bytes */
> > - __u8 pad[6];
> > + __u8 pad[5];
> > __u64 serror_esr;
> > } exception;
> > __u32 reserved[12];
>
> This API seems to be missing support for userspace to specify
> whether the ESR_ELx for the guest should have the EA bit set
> (and more generally other syndrome/fault status bits). I think
> if we have an API for "KVM_EXIT_MMIO but the access failed"
> then it should either (a) be architecture agnostic, since
> pretty much any architecture might have a concept of "access
> gave some bus-error-type failure" and it would be nice if userspace
> didn't have to special case them all in arch-specific code,
> or (b) have the same flexibility for specifying exactly what
> kind of fault as the architecture does. This sort of seems to
> fall between two stools. (My ideal for KVM_EXIT_MMIO faults
> would be a generic API which included space for optional
> arch-specific info, which for Arm would pretty much just be
> the EA bit.)
I'm not sure I understand exactly what would be improved by making this
either more architecture speific or more architecture generic. The
EA bit will always be set, that's why the field is called
'ext_dabt_pending'.
I thought as per the previous discussion, that we were specifically
trying to avoid userspace emulating the exception in detail, so I
designed this to provide the minimal effort API for userspace.
Since we already have an architecture specific ioctl, kvm_vcpu_events, I
don't think we're painting ourselves into a corner by using that. Is a
natural consequence of what you're saying not that we should try to make
that whole call architecture generic?
Unless we already have specific examples of how other architectures
would want to use something like this, and given the impact of this
patch, I'm not sure it's worth trying to speculate about that.
>
> As and when we support nested virtualization, any suggestions
> on how this API would extend to support userspace saying
> "deliver fault to guest EL1" vs "deliver fault to guest EL2" ?
>
If we took one of the supported exits from a VM with nested virt
support, it means that you either had a fault from the guest hypervisor,
or a fault from a nested guest where the guest hypervisor has set up a
virtual stage 2 mapping to a hole in the VM's IPA space. In the former
case, the exception would be delivered back to guest hypervisor, and in
the latter case the target depends on the guest hypervisor's
configuration of the virtual HCR_EL2(.TEA), which the kernel should
respect when handling the KVM_SET_VCPU_EVENTS ioctl.
Thanks,
Christoffer
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver
From: Arnd Bergmann @ 2019-09-09 15:16 UTC (permalink / raw)
To: Shenhar, Talel
Cc: Mark Rutland, mjourdan, Catalin Marinas, Linus Walleij,
linux-kernel@vger.kernel.org, jonnyc, Mauro Carvalho Chehab,
ronenk, Will Deacon, Benjamin Herrenschmidt, DTML, Maxime Ripard,
Rob Herring, Santosh Shilimkar, Thomas Gleixner, hanochu,
Linux ARM, barakw, hhhawa, gregkh, paul.kocialkowski,
Patrick Venture, Olof Johansson, David Miller, David Woodhouse
In-Reply-To: <8f7840c3-a682-04a5-18bf-ac7a723725b0@amazon.com>
On Mon, Sep 9, 2019 at 4:11 PM Shenhar, Talel <talel@amazon.com> wrote:
> On 9/9/2019 4:41 PM, Arnd Bergmann wrote:
>
> In current implementation of v1, I am not doing any read barrier, Hence,
> using the non-relaxed will add unneeded memory barrier.
>
> I have no strong objection moving to the non-relaxed version and have an
> unneeded memory barrier, as this path is not "hot" one.
Ok, then please add it.
> Beside of avoiding the unneeded memory barrier, I would be happy to keep
> common behavior for our drivers:
>
> e.g.
>
> https://github.com/torvalds/linux/blob/master/drivers/irqchip/irq-al-fic.c#L49
>
>
> So what do you think we should go with? relaxed or non-relaxed?
The al_fic_set_trigger() function is clearly a slow-path and should use the
non-relaxed functions. In case of al_fic_irq_handler(), the extra barrier
might introduce a measurable overhead, but at the same time I'm
not sure if that one is correct without the barrier:
If you have an MSI-type interrupt for notifying a device driver of
a DMA completion, there might not be any other barrier between
the arrival of the MSI message and the CPU accessing the data.
Depending on how strict the hardware implements MSI and how
the IRQ is chained, this could lead to data corruption.
If the interrupt is only used for level or edge triggered interrupts,
this is ok since you already need another register read in
the driver before it can safely access a DMA buffer.
In either case, if you can prove that it's safe to use the relaxed
version here and you think that it may help, it would be good to
add a comment explaining the reasoning.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH -next] reset: reset-scmi: add missing handle initialisation
From: Sudeep Holla @ 2019-09-09 15:21 UTC (permalink / raw)
To: linux-arm-kernel, Etienne Carriere, Philipp Zabel
Cc: linux-kernel, Sudeep Holla
scmi_reset_data->handle needs to be initialised at probe, so that it
can be used to access scmi reset protocol apis using the same later.
Since it was tested with a module that obtained handle elsewhere,
it was missed easily. Add the missing scmi_reset_data->handle
initialisation to fix the issue.
Fixes: c8ae9c2da1cc ("reset: Add support for resets provided by SCMI")
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Reported-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/reset/reset-scmi.c | 1 +
1 file changed, 1 insertion(+)
Hi Philipp,
I can either take this via ARM SoC as the driver is getting merged through
ARM SoC tree, or you can apply this once it gets landed in mainline.
I am fine with whatever you prefer.
Regards,
Sudeep
diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
index c6d3c8427f14..b46df80ec6c3 100644
--- a/drivers/reset/reset-scmi.c
+++ b/drivers/reset/reset-scmi.c
@@ -102,6 +102,7 @@ static int scmi_reset_probe(struct scmi_device *sdev)
data->rcdev.owner = THIS_MODULE;
data->rcdev.of_node = np;
data->rcdev.nr_resets = handle->reset_ops->num_domains_get(handle);
+ data->handle = handle;
return devm_reset_controller_register(dev, &data->rcdev);
}
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH -next] firmware: arm_scmi: reset: fix reset_state assignment in scmi_domain_reset
From: Sudeep Holla @ 2019-09-09 15:21 UTC (permalink / raw)
To: linux-arm-kernel, Etienne Carriere; +Cc: linux-kernel, Sudeep Holla
Fix the copy paste typo that incorrectly assigns domain_id with the
passed 'state' parameter instead of reset_state.
Fixes: 95a15d80aa0d ("firmware: arm_scmi: Add RESET protocol in SCMI v2.0")
Reported-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/firmware/arm_scmi/reset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 64cc81915581..ab42c21c5517 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -150,7 +150,7 @@ static int scmi_domain_reset(const struct scmi_handle *handle, u32 domain,
dom = t->tx.buf;
dom->domain_id = cpu_to_le32(domain);
dom->flags = cpu_to_le32(flags);
- dom->domain_id = cpu_to_le32(state);
+ dom->reset_state = cpu_to_le32(state);
if (rdom->async_reset)
ret = scmi_do_xfer_with_response(handle, t);
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v9 2/8] mm: Adjust shuffle code to allow for future coalescing
From: Alexander Duyck @ 2019-09-09 15:22 UTC (permalink / raw)
To: Kirill A. Shutemov, Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas, mhocko,
linux-mm, will, aarcange, virtio-dev, mst, willy, wei.w.wang,
ying.huang, riel, dan.j.williams, lcapitulino, linux-arm-kernel,
osalvador, nitesh, konrad.wilk, dave.hansen, linux-kernel,
pbonzini, akpm, fengguang.wu, kirill.shutemov
In-Reply-To: <20190909094700.bbslsxpuwvxmodal@box>
On Mon, 2019-09-09 at 12:47 +0300, Kirill A. Shutemov wrote:
> On Sat, Sep 07, 2019 at 10:25:20AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Move the head/tail adding logic out of the shuffle code and into the
> > __free_one_page function since ultimately that is where it is really
> > needed anyway. By doing this we should be able to reduce the overhead
> > and can consolidate all of the list addition bits in one spot.
> >
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> > include/linux/mmzone.h | 12 --------
> > mm/page_alloc.c | 70 +++++++++++++++++++++++++++---------------------
> > mm/shuffle.c | 9 +-----
> > mm/shuffle.h | 12 ++++++++
> > 4 files changed, 53 insertions(+), 50 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index bda20282746b..125f300981c6 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -116,18 +116,6 @@ static inline void add_to_free_area_tail(struct page *page, struct free_area *ar
> > area->nr_free++;
> > }
> >
> > -#ifdef CONFIG_SHUFFLE_PAGE_ALLOCATOR
> > -/* Used to preserve page allocation order entropy */
> > -void add_to_free_area_random(struct page *page, struct free_area *area,
> > - int migratetype);
> > -#else
> > -static inline void add_to_free_area_random(struct page *page,
> > - struct free_area *area, int migratetype)
> > -{
> > - add_to_free_area(page, area, migratetype);
> > -}
> > -#endif
> > -
> > /* Used for pages which are on another list */
> > static inline void move_to_free_area(struct page *page, struct free_area *area,
> > int migratetype)
>
> Looks like add_to_free_area() and add_to_free_area_tail() can be moved to
> mm/page_alloc.c as all users are there now. And the same for struct
> free_area definition (but not declaration).
>
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index c5d62f1c2851..4e4356ba66c7 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -878,6 +878,36 @@ static inline struct capture_control *task_capc(struct zone *zone)
> > #endif /* CONFIG_COMPACTION */
> >
> > /*
> > + * If this is not the largest possible page, check if the buddy
> > + * of the next-highest order is free. If it is, it's possible
> > + * that pages are being freed that will coalesce soon. In case,
> > + * that is happening, add the free page to the tail of the list
> > + * so it's less likely to be used soon and more likely to be merged
> > + * as a higher order page
> > + */
> > +static inline bool
> > +buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn,
> > + struct page *page, unsigned int order)
> > +{
> > + struct page *higher_page, *higher_buddy;
> > + unsigned long combined_pfn;
> > +
> > + if (order >= MAX_ORDER - 2)
> > + return false;
> > +
> > + if (!pfn_valid_within(buddy_pfn))
> > + return false;
> > +
> > + combined_pfn = buddy_pfn & pfn;
> > + higher_page = page + (combined_pfn - pfn);
> > + buddy_pfn = __find_buddy_pfn(combined_pfn, order + 1);
> > + higher_buddy = higher_page + (buddy_pfn - combined_pfn);
> > +
> > + return pfn_valid_within(buddy_pfn) &&
> > + page_is_buddy(higher_page, higher_buddy, order + 1);
> > +}
>
> Okay, that's much easier to read.
>
> > +
> > +/*
> > * Freeing function for a buddy system allocator.
> > *
> > * The concept of a buddy system is to maintain direct-mapped table
> > @@ -906,11 +936,12 @@ static inline void __free_one_page(struct page *page,
> > struct zone *zone, unsigned int order,
> > int migratetype)
> > {
> > - unsigned long combined_pfn;
> > + struct capture_control *capc = task_capc(zone);
> > unsigned long uninitialized_var(buddy_pfn);
> > - struct page *buddy;
> > + unsigned long combined_pfn;
> > + struct free_area *area;
> > unsigned int max_order;
> > - struct capture_control *capc = task_capc(zone);
> > + struct page *buddy;
> >
> > max_order = min_t(unsigned int, MAX_ORDER, pageblock_order + 1);
> >
> > @@ -979,35 +1010,12 @@ static inline void __free_one_page(struct page *page,
> > done_merging:
> > set_page_order(page, order);
> >
> > - /*
> > - * If this is not the largest possible page, check if the buddy
> > - * of the next-highest order is free. If it is, it's possible
> > - * that pages are being freed that will coalesce soon. In case,
> > - * that is happening, add the free page to the tail of the list
> > - * so it's less likely to be used soon and more likely to be merged
> > - * as a higher order page
> > - */
> > - if ((order < MAX_ORDER-2) && pfn_valid_within(buddy_pfn)
> > - && !is_shuffle_order(order)) {
> > - struct page *higher_page, *higher_buddy;
> > - combined_pfn = buddy_pfn & pfn;
> > - higher_page = page + (combined_pfn - pfn);
> > - buddy_pfn = __find_buddy_pfn(combined_pfn, order + 1);
> > - higher_buddy = higher_page + (buddy_pfn - combined_pfn);
> > - if (pfn_valid_within(buddy_pfn) &&
> > - page_is_buddy(higher_page, higher_buddy, order + 1)) {
> > - add_to_free_area_tail(page, &zone->free_area[order],
> > - migratetype);
> > - return;
> > - }
> > - }
> > -
> > - if (is_shuffle_order(order))
> > - add_to_free_area_random(page, &zone->free_area[order],
> > - migratetype);
> > + area = &zone->free_area[order];
> > + if (is_shuffle_order(order) ? shuffle_pick_tail() :
> > + buddy_merge_likely(pfn, buddy_pfn, page, order))
>
> Too loaded condition to my taste. Maybe
>
> bool to_tail;
> ...
> if (is_shuffle_order(order))
> to_tail = shuffle_pick_tail();
> else if (buddy_merge_likely(pfn, buddy_pfn, page, order))
> to_tail = true;
> else
> to_tail = false;
I can do that, although I would tweak this slightly and do something more
like:
if (is_shuffle_order(order))
to_tail = shuffle_pick_tail();
else
to_tail = buddy+_merge_likely(pfn, buddy_pfn, page, order);
>
> if (to_tail)
> add_to_free_area_tail(page, area, migratetype);
> else
> add_to_free_area(page, area, migratetype);
>
> > + add_to_free_area_tail(page, area, migratetype);
> > else
> > - add_to_free_area(page, &zone->free_area[order], migratetype);
> > -
> > + add_to_free_area(page, area, migratetype);
> > }
> >
> > /*
> > diff --git a/mm/shuffle.c b/mm/shuffle.c
> > index 9ba542ecf335..345cb4347455 100644
> > --- a/mm/shuffle.c
> > +++ b/mm/shuffle.c
> > @@ -4,7 +4,6 @@
> > #include <linux/mm.h>
> > #include <linux/init.h>
> > #include <linux/mmzone.h>
> > -#include <linux/random.h>
> > #include <linux/moduleparam.h>
> > #include "internal.h"
> > #include "shuffle.h"
>
> Why do you move #include <linux/random.h> from .c to .h?
> It's not obvious to me.
Because I had originally put the shuffle logic in an inline function. I
can undo that now as I when back to doing the randomness in the .c
sometime v5 I believe.
> > @@ -190,8 +189,7 @@ struct batched_bit_entropy {
> >
> > static DEFINE_PER_CPU(struct batched_bit_entropy, batched_entropy_bool);
> >
> > -void add_to_free_area_random(struct page *page, struct free_area *area,
> > - int migratetype)
> > +bool __shuffle_pick_tail(void)
> > {
> > struct batched_bit_entropy *batch;
> > unsigned long entropy;
> > @@ -213,8 +211,5 @@ void add_to_free_area_random(struct page *page, struct free_area *area,
> > batch->position = position;
> > entropy = batch->entropy_bool;
> >
> > - if (1ul & (entropy >> position))
> > - add_to_free_area(page, area, migratetype);
> > - else
> > - add_to_free_area_tail(page, area, migratetype);
> > + return 1ul & (entropy >> position);
> > }
> > diff --git a/mm/shuffle.h b/mm/shuffle.h
> > index 777a257a0d2f..0723eb97f22f 100644
> > --- a/mm/shuffle.h
> > +++ b/mm/shuffle.h
> > @@ -3,6 +3,7 @@
> > #ifndef _MM_SHUFFLE_H
> > #define _MM_SHUFFLE_H
> > #include <linux/jump_label.h>
> > +#include <linux/random.h>
> >
> > /*
> > * SHUFFLE_ENABLE is called from the command line enabling path, or by
> > @@ -22,6 +23,7 @@ enum mm_shuffle_ctl {
> > DECLARE_STATIC_KEY_FALSE(page_alloc_shuffle_key);
> > extern void page_alloc_shuffle(enum mm_shuffle_ctl ctl);
> > extern void __shuffle_free_memory(pg_data_t *pgdat);
> > +extern bool __shuffle_pick_tail(void);
> > static inline void shuffle_free_memory(pg_data_t *pgdat)
> > {
> > if (!static_branch_unlikely(&page_alloc_shuffle_key))
> > @@ -43,6 +45,11 @@ static inline bool is_shuffle_order(int order)
> > return false;
> > return order >= SHUFFLE_ORDER;
> > }
> > +
> > +static inline bool shuffle_pick_tail(void)
> > +{
> > + return __shuffle_pick_tail();
> > +}
>
> I don't see a reason in __shuffle_pick_tail() existing if you call it
> unconditionally.
That is for compilation purposes. The function is not used in the
shuffle_pick_tail below that always returns false.
> > #else
> > static inline void shuffle_free_memory(pg_data_t *pgdat)
> > {
> > @@ -60,5 +67,10 @@ static inline bool is_shuffle_order(int order)
> > {
> > return false;
> > }
> > +
> > +static inline bool shuffle_pick_tail(void)
> > +{
> > + return false;
> > +}
> > #endif
> > #endif /* _MM_SHUFFLE_H */
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 5/8] arm64: Move hugetlb related definitions out of pgtable.h to page-defs.h
From: Alexander Duyck @ 2019-09-09 15:27 UTC (permalink / raw)
To: David Hildenbrand, Alexander Duyck, virtio-dev, kvm, mst,
catalin.marinas, dave.hansen, linux-kernel, willy, mhocko,
linux-mm, akpm, will, linux-arm-kernel, osalvador
Cc: yang.zhang.wz, pagupta, riel, konrad.wilk, ying.huang,
lcapitulino, wei.w.wang, aarcange, nitesh, pbonzini,
dan.j.williams, fengguang.wu, kirill.shutemov
In-Reply-To: <90785d30-cde9-f380-5f4a-8af989b11729@redhat.com>
On Mon, 2019-09-09 at 10:52 +0200, David Hildenbrand wrote:
> On 07.09.19 19:25, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Move the static definition for things such as HUGETLB_PAGE_ORDER out of
> > asm/pgtable.h and place it in page-defs.h. By doing this the includes
> > become much easier to deal with as currently arm64 is the only architecture
> > that didn't include this definition in the asm/page.h file or a file
> > included by it.
> >
> > It also makes logical sense as PAGE_SHIFT was already defined in
> > page-defs.h so now we also have HPAGE_SHIFT defined there as well.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> > arch/arm64/include/asm/page-def.h | 9 +++++++++
> > arch/arm64/include/asm/pgtable.h | 9 ---------
> > 2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/page-def.h b/arch/arm64/include/asm/page-def.h
> > index f99d48ecbeef..1c5b079e2482 100644
> > --- a/arch/arm64/include/asm/page-def.h
> > +++ b/arch/arm64/include/asm/page-def.h
> > @@ -20,4 +20,13 @@
> > #define CONT_SIZE (_AC(1, UL) << (CONT_SHIFT + PAGE_SHIFT))
> > #define CONT_MASK (~(CONT_SIZE-1))
> >
> > +/*
> > + * Hugetlb definitions.
> > + */
> > +#define HUGE_MAX_HSTATE 4
> > +#define HPAGE_SHIFT PMD_SHIFT
> > +#define HPAGE_SIZE (_AC(1, UL) << HPAGE_SHIFT)
> > +#define HPAGE_MASK (~(HPAGE_SIZE - 1))
> > +#define HUGETLB_PAGE_ORDER (HPAGE_SHIFT - PAGE_SHIFT)
> > +
>
> I wonder if you should initially limit "config PAGE_REPORTING" to x86
> only and unlock it for the other targets once we actually test it there.
> Or did you test PAGE_REPORTING on other architectures as well?
>
I haven't, but essentially the effects should be the same regardless of
architecture. In addition since this is a feature that can be
enabled/disabled via QEMU I am not sure there is much harm other than
getting additional testing by enabling for all of the architectures at
once.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 2/2] clk: at91: select parent if main oscillator or bypass is enabled
From: Eugen.Hristev @ 2019-09-09 15:30 UTC (permalink / raw)
To: mturquette, sboyd, alexandre.belloni, linux-clk, linux-arm-kernel,
linux-kernel
Cc: Eugen.Hristev, Ludovic.Desroches
In-Reply-To: <1568042692-11784-1-git-send-email-eugen.hristev@microchip.com>
From: Eugen Hristev <eugen.hristev@microchip.com>
Selecting the right parent for the main clock is done using only
main oscillator enabled bit.
In case we have this oscillator bypassed by an external signal (no driving
on the XOUT line), we still use external clock, but with BYPASS bit set.
So, in this case we must select the same parent as before.
Create a macro that will select the right parent considering both bits from
the MOR register.
Use this macro when looking for the right parent.
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/clk/at91/clk-main.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c
index ebe9b99..87083b3 100644
--- a/drivers/clk/at91/clk-main.c
+++ b/drivers/clk/at91/clk-main.c
@@ -21,6 +21,10 @@
#define MOR_KEY_MASK (0xff << 16)
+#define clk_main_parent_select(s) (((s) & \
+ (AT91_PMC_MOSCEN | \
+ AT91_PMC_OSCBYPASS)) ? 1 : 0)
+
struct clk_main_osc {
struct clk_hw hw;
struct regmap *regmap;
@@ -113,7 +117,7 @@ static int clk_main_osc_is_prepared(struct clk_hw *hw)
regmap_read(regmap, AT91_PMC_SR, &status);
- return (status & AT91_PMC_MOSCS) && (tmp & AT91_PMC_MOSCEN);
+ return (status & AT91_PMC_MOSCS) && clk_main_parent_select(tmp);
}
static const struct clk_ops main_osc_ops = {
@@ -450,7 +454,7 @@ static u8 clk_sam9x5_main_get_parent(struct clk_hw *hw)
regmap_read(clkmain->regmap, AT91_CKGR_MOR, &status);
- return status & AT91_PMC_MOSCEN ? 1 : 0;
+ return clk_main_parent_select(status);
}
static const struct clk_ops sam9x5_main_ops = {
@@ -492,7 +496,7 @@ at91_clk_register_sam9x5_main(struct regmap *regmap,
clkmain->hw.init = &init;
clkmain->regmap = regmap;
regmap_read(clkmain->regmap, AT91_CKGR_MOR, &status);
- clkmain->parent = status & AT91_PMC_MOSCEN ? 1 : 0;
+ clkmain->parent = clk_main_parent_select(status);
hw = &clkmain->hw;
ret = clk_hw_register(NULL, &clkmain->hw);
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/2] clk: at91: fix update bit maps on CFG_MOR write
From: Eugen.Hristev @ 2019-09-09 15:30 UTC (permalink / raw)
To: mturquette, sboyd, alexandre.belloni, linux-clk, linux-arm-kernel,
linux-kernel
Cc: Eugen.Hristev, Ludovic.Desroches
From: Eugen Hristev <eugen.hristev@microchip.com>
The regmap update bits call was not selecting the proper mask, considering
the bits which was updating.
Update the mask from call to also include OSCBYPASS.
Removed MOSCEN which was not updated.
Fixes: 1bdf02326b71 ("clk: at91: make use of syscon/regmap internally")
Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
drivers/clk/at91/clk-main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/at91/clk-main.c b/drivers/clk/at91/clk-main.c
index f607ee7..ebe9b99 100644
--- a/drivers/clk/at91/clk-main.c
+++ b/drivers/clk/at91/clk-main.c
@@ -152,7 +152,7 @@ at91_clk_register_main_osc(struct regmap *regmap,
if (bypass)
regmap_update_bits(regmap,
AT91_CKGR_MOR, MOR_KEY_MASK |
- AT91_PMC_MOSCEN,
+ AT91_PMC_OSCBYPASS,
AT91_PMC_OSCBYPASS | AT91_PMC_KEY);
hw = &osc->hw;
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v9 7/8] virtio-balloon: Pull page poisoning config out of free page hinting
From: Alexander Duyck @ 2019-09-09 15:31 UTC (permalink / raw)
To: David Hildenbrand, Alexander Duyck, virtio-dev, kvm, mst,
catalin.marinas, dave.hansen, linux-kernel, willy, mhocko,
linux-mm, akpm, will, linux-arm-kernel, osalvador
Cc: yang.zhang.wz, pagupta, riel, konrad.wilk, ying.huang,
lcapitulino, wei.w.wang, aarcange, nitesh, pbonzini,
dan.j.williams, fengguang.wu, kirill.shutemov
In-Reply-To: <4dfcf372-97be-65ab-1349-75f24aa4f98a@redhat.com>
On Mon, 2019-09-09 at 10:59 +0200, David Hildenbrand wrote:
> On 07.09.19 19:26, Alexander Duyck wrote:
> > From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> >
> > Currently the page poisoning setting wasn't being enabled unless free page
> > hinting was enabled. However we will need the page poisoning tracking logic
> > as well for unused page reporting. As such pull it out and make it a
> > separate bit of config in the probe function.
> >
> > In addition we can actually wrap the code in a check for NO_SANITY. If we
> > don't care what is actually in the page we can just default to 0 and leave
> > it there.
> >
> > Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> > ---
> > drivers/virtio/virtio_balloon.c | 22 +++++++++++++++-------
> > 1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 226fbb995fb0..d2547df7de93 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -842,7 +842,6 @@ static int virtio_balloon_register_shrinker(struct virtio_balloon *vb)
> > static int virtballoon_probe(struct virtio_device *vdev)
> > {
> > struct virtio_balloon *vb;
> > - __u32 poison_val;
> > int err;
> >
> > if (!vdev->config->get) {
> > @@ -909,11 +908,18 @@ static int virtballoon_probe(struct virtio_device *vdev)
> > VIRTIO_BALLOON_CMD_ID_STOP);
> > spin_lock_init(&vb->free_page_list_lock);
> > INIT_LIST_HEAD(&vb->free_page_list);
> > - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> > - memset(&poison_val, PAGE_POISON, sizeof(poison_val));
> > - virtio_cwrite(vb->vdev, struct virtio_balloon_config,
> > - poison_val, &poison_val);
> > - }
> > + }
> > + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_PAGE_POISON)) {
> > + __u32 poison_val;
> > +
> > + /*
> > + * Let hypervisor know that we are expecting a specific
> > + * value to be written back in unused pages.
> > + */
>
> "Let the hypervisor know" ... ?
>
> > + memset(&poison_val, PAGE_POISON, sizeof(poison_val));
> > +
> > + virtio_cwrite(vb->vdev, struct virtio_balloon_config,
> > + poison_val, &poison_val);
> > }
> > /*
> > * We continue to use VIRTIO_BALLOON_F_DEFLATE_ON_OOM to decide if a
> > @@ -1014,7 +1020,9 @@ static int virtballoon_restore(struct virtio_device *vdev)
> >
> > static int virtballoon_validate(struct virtio_device *vdev)
> > {
> > - if (!page_poisoning_enabled())
> > + /* Notify host if we care about poison value */
>
> "Tell the host whether we care about poisoned pages." ?
>
> > + if (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
> > + !page_poisoning_enabled())
> > __virtio_clear_bit(vdev, VIRTIO_BALLOON_F_PAGE_POISON);
> >
> > __virtio_clear_bit(vdev, VIRTIO_F_IOMMU_PLATFORM);
> >
>
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
Thanks. I will update the comments for v10.
- Alex
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 2/8] mm: Adjust shuffle code to allow for future coalescing
From: Kirill A. Shutemov @ 2019-09-09 15:35 UTC (permalink / raw)
To: Alexander Duyck
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas,
Alexander Duyck, mhocko, linux-mm, will, aarcange, virtio-dev,
mst, willy, wei.w.wang, ying.huang, riel, dan.j.williams,
lcapitulino, linux-arm-kernel, osalvador, nitesh, konrad.wilk,
dave.hansen, linux-kernel, pbonzini, akpm, fengguang.wu,
kirill.shutemov
In-Reply-To: <22a896255cba877cf820f552667e1bc14268fa20.camel@linux.intel.com>
On Mon, Sep 09, 2019 at 08:22:11AM -0700, Alexander Duyck wrote:
> > > + area = &zone->free_area[order];
> > > + if (is_shuffle_order(order) ? shuffle_pick_tail() :
> > > + buddy_merge_likely(pfn, buddy_pfn, page, order))
> >
> > Too loaded condition to my taste. Maybe
> >
> > bool to_tail;
> > ...
> > if (is_shuffle_order(order))
> > to_tail = shuffle_pick_tail();
> > else if (buddy_merge_likely(pfn, buddy_pfn, page, order))
> > to_tail = true;
> > else
> > to_tail = false;
>
> I can do that, although I would tweak this slightly and do something more
> like:
> if (is_shuffle_order(order))
> to_tail = shuffle_pick_tail();
> else
> to_tail = buddy+_merge_likely(pfn, buddy_pfn, page, order);
Okay. Looks fine.
> > if (to_tail)
> > add_to_free_area_tail(page, area, migratetype);
> > else
> > add_to_free_area(page, area, migratetype);
> >
> > > + add_to_free_area_tail(page, area, migratetype);
> > > else
> > > - add_to_free_area(page, &zone->free_area[order], migratetype);
> > > -
> > > + add_to_free_area(page, area, migratetype);
> > > }
> > >
> > > /*
> > > diff --git a/mm/shuffle.c b/mm/shuffle.c
> > > index 9ba542ecf335..345cb4347455 100644
> > > --- a/mm/shuffle.c
> > > +++ b/mm/shuffle.c
> > > @@ -4,7 +4,6 @@
> > > #include <linux/mm.h>
> > > #include <linux/init.h>
> > > #include <linux/mmzone.h>
> > > -#include <linux/random.h>
> > > #include <linux/moduleparam.h>
> > > #include "internal.h"
> > > #include "shuffle.h"
> >
> > Why do you move #include <linux/random.h> from .c to .h?
> > It's not obvious to me.
>
> Because I had originally put the shuffle logic in an inline function. I
> can undo that now as I when back to doing the randomness in the .c
> sometime v5 I believe.
Yes, please. It's needless change now.
>
> > > @@ -190,8 +189,7 @@ struct batched_bit_entropy {
> > >
> > > static DEFINE_PER_CPU(struct batched_bit_entropy, batched_entropy_bool);
> > >
> > > -void add_to_free_area_random(struct page *page, struct free_area *area,
> > > - int migratetype)
> > > +bool __shuffle_pick_tail(void)
> > > {
> > > struct batched_bit_entropy *batch;
> > > unsigned long entropy;
> > > @@ -213,8 +211,5 @@ void add_to_free_area_random(struct page *page, struct free_area *area,
> > > batch->position = position;
> > > entropy = batch->entropy_bool;
> > >
> > > - if (1ul & (entropy >> position))
> > > - add_to_free_area(page, area, migratetype);
> > > - else
> > > - add_to_free_area_tail(page, area, migratetype);
> > > + return 1ul & (entropy >> position);
> > > }
> > > diff --git a/mm/shuffle.h b/mm/shuffle.h
> > > index 777a257a0d2f..0723eb97f22f 100644
> > > --- a/mm/shuffle.h
> > > +++ b/mm/shuffle.h
> > > @@ -3,6 +3,7 @@
> > > #ifndef _MM_SHUFFLE_H
> > > #define _MM_SHUFFLE_H
> > > #include <linux/jump_label.h>
> > > +#include <linux/random.h>
> > >
> > > /*
> > > * SHUFFLE_ENABLE is called from the command line enabling path, or by
> > > @@ -22,6 +23,7 @@ enum mm_shuffle_ctl {
> > > DECLARE_STATIC_KEY_FALSE(page_alloc_shuffle_key);
> > > extern void page_alloc_shuffle(enum mm_shuffle_ctl ctl);
> > > extern void __shuffle_free_memory(pg_data_t *pgdat);
> > > +extern bool __shuffle_pick_tail(void);
> > > static inline void shuffle_free_memory(pg_data_t *pgdat)
> > > {
> > > if (!static_branch_unlikely(&page_alloc_shuffle_key))
> > > @@ -43,6 +45,11 @@ static inline bool is_shuffle_order(int order)
> > > return false;
> > > return order >= SHUFFLE_ORDER;
> > > }
> > > +
> > > +static inline bool shuffle_pick_tail(void)
> > > +{
> > > + return __shuffle_pick_tail();
> > > +}
> >
> > I don't see a reason in __shuffle_pick_tail() existing if you call it
> > unconditionally.
>
> That is for compilation purposes. The function is not used in the
> shuffle_pick_tail below that always returns false.
Wouldn't it be the same if you rename __shuffle_pick_tail() to
shuffle_pick_tail() and put its declaration under the same #ifdef?
--
Kirill A. Shutemov
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v9 2/8] mm: Adjust shuffle code to allow for future coalescing
From: Alexander Duyck @ 2019-09-09 15:37 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: yang.zhang.wz, pagupta, kvm, david, catalin.marinas,
Alexander Duyck, mhocko, linux-mm, will, aarcange, virtio-dev,
mst, willy, wei.w.wang, ying.huang, riel, dan.j.williams,
lcapitulino, linux-arm-kernel, osalvador, nitesh, konrad.wilk,
dave.hansen, linux-kernel, pbonzini, akpm, fengguang.wu,
kirill.shutemov
In-Reply-To: <20190909153529.3crs74uraos27ffh@box>
On Mon, 2019-09-09 at 18:35 +0300, Kirill A. Shutemov wrote:
> On Mon, Sep 09, 2019 at 08:22:11AM -0700, Alexander Duyck wrote:
> > > > + area = &zone->free_area[order];
> > > > + if (is_shuffle_order(order) ? shuffle_pick_tail() :
> > > > + buddy_merge_likely(pfn, buddy_pfn, page, order))
> > >
> > > Too loaded condition to my taste. Maybe
> > >
> > > bool to_tail;
> > > ...
> > > if (is_shuffle_order(order))
> > > to_tail = shuffle_pick_tail();
> > > else if (buddy_merge_likely(pfn, buddy_pfn, page, order))
> > > to_tail = true;
> > > else
> > > to_tail = false;
> >
> > I can do that, although I would tweak this slightly and do something more
> > like:
> > if (is_shuffle_order(order))
> > to_tail = shuffle_pick_tail();
> > else
> > to_tail = buddy+_merge_likely(pfn, buddy_pfn, page, order);
>
> Okay. Looks fine.
>
> > > if (to_tail)
> > > add_to_free_area_tail(page, area, migratetype);
> > > else
> > > add_to_free_area(page, area, migratetype);
> > >
> > > > + add_to_free_area_tail(page, area, migratetype);
> > > > else
> > > > - add_to_free_area(page, &zone->free_area[order], migratetype);
> > > > -
> > > > + add_to_free_area(page, area, migratetype);
> > > > }
> > > >
> > > > /*
> > > > diff --git a/mm/shuffle.c b/mm/shuffle.c
> > > > index 9ba542ecf335..345cb4347455 100644
> > > > --- a/mm/shuffle.c
> > > > +++ b/mm/shuffle.c
> > > > @@ -4,7 +4,6 @@
> > > > #include <linux/mm.h>
> > > > #include <linux/init.h>
> > > > #include <linux/mmzone.h>
> > > > -#include <linux/random.h>
> > > > #include <linux/moduleparam.h>
> > > > #include "internal.h"
> > > > #include "shuffle.h"
> > >
> > > Why do you move #include <linux/random.h> from .c to .h?
> > > It's not obvious to me.
> >
> > Because I had originally put the shuffle logic in an inline function. I
> > can undo that now as I when back to doing the randomness in the .c
> > sometime v5 I believe.
>
> Yes, please. It's needless change now.
>
> > > > @@ -190,8 +189,7 @@ struct batched_bit_entropy {
> > > >
> > > > static DEFINE_PER_CPU(struct batched_bit_entropy, batched_entropy_bool);
> > > >
> > > > -void add_to_free_area_random(struct page *page, struct free_area *area,
> > > > - int migratetype)
> > > > +bool __shuffle_pick_tail(void)
> > > > {
> > > > struct batched_bit_entropy *batch;
> > > > unsigned long entropy;
> > > > @@ -213,8 +211,5 @@ void add_to_free_area_random(struct page *page, struct free_area *area,
> > > > batch->position = position;
> > > > entropy = batch->entropy_bool;
> > > >
> > > > - if (1ul & (entropy >> position))
> > > > - add_to_free_area(page, area, migratetype);
> > > > - else
> > > > - add_to_free_area_tail(page, area, migratetype);
> > > > + return 1ul & (entropy >> position);
> > > > }
> > > > diff --git a/mm/shuffle.h b/mm/shuffle.h
> > > > index 777a257a0d2f..0723eb97f22f 100644
> > > > --- a/mm/shuffle.h
> > > > +++ b/mm/shuffle.h
> > > > @@ -3,6 +3,7 @@
> > > > #ifndef _MM_SHUFFLE_H
> > > > #define _MM_SHUFFLE_H
> > > > #include <linux/jump_label.h>
> > > > +#include <linux/random.h>
> > > >
> > > > /*
> > > > * SHUFFLE_ENABLE is called from the command line enabling path, or by
> > > > @@ -22,6 +23,7 @@ enum mm_shuffle_ctl {
> > > > DECLARE_STATIC_KEY_FALSE(page_alloc_shuffle_key);
> > > > extern void page_alloc_shuffle(enum mm_shuffle_ctl ctl);
> > > > extern void __shuffle_free_memory(pg_data_t *pgdat);
> > > > +extern bool __shuffle_pick_tail(void);
> > > > static inline void shuffle_free_memory(pg_data_t *pgdat)
> > > > {
> > > > if (!static_branch_unlikely(&page_alloc_shuffle_key))
> > > > @@ -43,6 +45,11 @@ static inline bool is_shuffle_order(int order)
> > > > return false;
> > > > return order >= SHUFFLE_ORDER;
> > > > }
> > > > +
> > > > +static inline bool shuffle_pick_tail(void)
> > > > +{
> > > > + return __shuffle_pick_tail();
> > > > +}
> > >
> > > I don't see a reason in __shuffle_pick_tail() existing if you call it
> > > unconditionally.
> >
> > That is for compilation purposes. The function is not used in the
> > shuffle_pick_tail below that always returns false.
>
> Wouldn't it be the same if you rename __shuffle_pick_tail() to
> shuffle_pick_tail() and put its declaration under the same #ifdef?
>
Yeah I guess I can do that. I'll update that for v10.
Thanks.
- Alex
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH -next] reset: reset-scmi: add missing handle initialisation
From: Philipp Zabel @ 2019-09-09 15:39 UTC (permalink / raw)
To: Sudeep Holla, linux-arm-kernel, Etienne Carriere; +Cc: linux-kernel
In-Reply-To: <20190909152107.4968-1-sudeep.holla@arm.com>
Hi Sudeep,
On Mon, 2019-09-09 at 16:21 +0100, Sudeep Holla wrote:
> scmi_reset_data->handle needs to be initialised at probe, so that it
> can be used to access scmi reset protocol apis using the same later.
>
> Since it was tested with a module that obtained handle elsewhere,
> it was missed easily. Add the missing scmi_reset_data->handle
> initialisation to fix the issue.
>
> Fixes: c8ae9c2da1cc ("reset: Add support for resets provided by SCMI")
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Reported-by: Etienne Carriere <etienne.carriere@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/reset/reset-scmi.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Hi Philipp,
>
> I can either take this via ARM SoC as the driver is getting merged through
> ARM SoC tree, or you can apply this once it gets landed in mainline.
> I am fine with whatever you prefer.
Feel free to take this in via ARM SoC directly, I see no need to wait
for this to hit mainline.
Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
From: Andre Przywara @ 2019-09-09 15:42 UTC (permalink / raw)
To: Peng Fan
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
f.fainelli@gmail.com, jassisinghbrar@gmail.com,
linux-kernel@vger.kernel.org, robh+dt@kernel.org, dl-linux-imx,
sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <1567004515-3567-2-git-send-email-peng.fan@nxp.com>
On Wed, 28 Aug 2019 03:02:58 +0000
Peng Fan <peng.fan@nxp.com> wrote:
Hi,
sorry for the late reply, eventually managed to have a closer look on this.
> From: Peng Fan <peng.fan@nxp.com>
>
> The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> actions in software layers running in the EL2 or EL3 exception levels.
> The term "ARM" here relates to the SMC instruction as part of the ARM
> instruction set, not as a standard endorsed by ARM Ltd.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> .../devicetree/bindings/mailbox/arm-smc.yaml | 125 +++++++++++++++++++++
> 1 file changed, 125 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> new file mode 100644
> index 000000000000..f8eb28d5e307
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMC Mailbox Interface
> +
> +maintainers:
> + - Peng Fan <peng.fan@nxp.com>
> +
> +description: |
> + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
> + call) instruction to trigger a mailbox-connected activity in firmware,
> + executing on the very same core as the caller. By nature this operation
> + is synchronous and this mailbox provides no way for asynchronous messages
> + to be delivered the other way round, from firmware to the OS, but
> + asynchronous notification could also be supported. However the value of
> + r0/w0/x0 the firmware returns after the smc call is delivered as a received
> + message to the mailbox framework, so a synchronous communication can be
> + established, for a asynchronous notification, no value will be returned.
> + The exact meaning of both the action the mailbox triggers as well as the
> + return value is defined by their users and is not subject to this binding.
> +
> + One use case of this mailbox is the SCMI interface, which uses shared memory
> + to transfer commands and parameters, and a mailbox to trigger a function
> + call. This allows SoCs without a separate management processor (or when
> + such a processor is not available or used) to use this standardized
> + interface anyway.
> +
> + This binding describes no hardware, but establishes a firmware interface.
> + Upon receiving an SMC using one of the described SMC function identifiers,
> + the firmware is expected to trigger some mailbox connected functionality.
> + The communication follows the ARM SMC calling convention.
> + Firmware expects an SMC function identifier in r0 or w0. The supported
> + identifiers are passed from consumers, or listed in the the arm,func-ids
> + properties as described below. The firmware can return one value in
> + the first SMC result register, it is expected to be an error value,
> + which shall be propagated to the mailbox client.
> +
> + Any core which supports the SMC or HVC instruction can be used, as long as
> + a firmware component running in EL3 or EL2 is handling these calls.
> +
> +properties:
> + compatible:
> + const: arm,smc-mbox
> +
> + "#mbox-cells":
> + const: 1
> +
> + arm,num-chans:
> + description: The number of channels supported.
> + items:
> + minimum: 1
> + maximum: 4096 # Should be enough?
This maximum sounds rather arbitrary. Why do we need one? In the driver this just allocates more memory, so why not just impose no artificial limit at all?
Actually, do we need this property at all? Can't we just rely on the size of arm,func-ids to determine this (using of_property_count_elems_of_size() in the driver)? Having both sounds redundant and brings up the question what to do if they don't match.
> +
> + method:
> + - enum:
> + - smc
> + - hvc
> +
> + transports:
> + - enum:
> + - mem
> + - reg
Shouldn't there be a description on what both mean, exactly?
For instance I would expect a list of registers to be shown for the "reg" case, and be it by referring to the ARM SMCCC.
Also looking at the driver this brings up more questions:
- Which memory does mem refer to? If this is really the means of transport, it should be referenced in this *controller* node and populated by the driver. Looking at the example below and the driver code, it actually isn't used that way, instead the memory is used and controlled by the mailbox *client*.
- What is the actual difference between the two transports? For "mem" we just populate the registers with 0, for "reg" we use the data. Couldn't this be left to the client?
There are more points which makes me think this property is actually redundant, see my comments on patch 2/2.
> +
> + arm,func-ids:
> + description: |
> + An array of 32-bit values specifying the function IDs used by each
> + mailbox channel. Those function IDs follow the ARM SMC calling
> + convention standard [1].
> +
> + There is one identifier per channel and the number of supported
> + channels is determined by the length of this array.
I think this makes it obvious that arm,num-chans is not needed.
Also this somewhat contradicts the driver implementation, which allows the array to be shorter, marking this as UINT_MAX and later on using the first data item as a function identifier. This is somewhat surprising and not documented (unless I missed something).
So I would suggest:
- We drop the transports property, and always put the client provided data in the registers, according to the SMCCC. Document this here.
A client not needing those could always puts zeros (or garbage) in there, the respective firmware would just ignore the registers.
- We drop "arm,num-chans", as this is just redundant with the length of the func-ids array.
- We don't impose an arbitrary limit on the number of channels. From the firmware point of view this is just different function IDs, from Linux' point of view just the size of the memory used. Both don't need to be limited artificially IMHO.
- We mark arm,func-ids as required, as this needs to be fixed, allocated number.
For the question of "always one channel per controller" vs. "allow multiple channels per controller": I don't really have a strong opinion, but lean towards allowing multiple channels. This would allow to group functions belonging together, separating them from totally distinct controller uses (think virtual GPIO vs. SCMI).
And it would still allow the special case of multiple single-channel controllers to be naturally specified.
> + $ref: /schemas/types.yaml#/definitions/uint32-array
> + minItems: 0
> + maxItems: 4096 # Should be enough?
> +
> +required:
> + - compatible
> + - "#mbox-cells"
> + - arm,num-chans
> + - transports
> + - method
According to the above description arm,func-ids would also be required?
Cheers,
Andre.
> +
> +examples:
> + - |
> + sram@910000 {
> + compatible = "mmio-sram";
> + reg = <0x0 0x93f000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x0 0x93f000 0x1000>;
> +
> + cpu_scp_lpri: scp-shmem@0 {
> + compatible = "arm,scmi-shmem";
> + reg = <0x0 0x200>;
> + };
> +
> + cpu_scp_hpri: scp-shmem@200 {
> + compatible = "arm,scmi-shmem";
> + reg = <0x200 0x200>;
> + };
> + };
> +
> + firmware {
> + smc_mbox: mailbox {
> + #mbox-cells = <1>;
> + compatible = "arm,smc-mbox";
> + method = "smc";
> + arm,num-chans = <0x2>;
> + transports = "mem";
> + /* Optional */
> + arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> + };
> +
> + scmi {
> + compatible = "arm,scmi";
> + mboxes = <&smc_mbox 0>, <&smc_mbox 1>;
> + mbox-names = "tx", "rx";
> + shmem = <&cpu_scp_lpri>, <&cpu_scp_hpri>;
> + };
> + };
> +
> +...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 2/2] mailbox: introduce ARM SMC based mailbox
From: Andre Przywara @ 2019-09-09 15:42 UTC (permalink / raw)
To: Peng Fan
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
f.fainelli@gmail.com, jassisinghbrar@gmail.com,
linux-kernel@vger.kernel.org, robh+dt@kernel.org, dl-linux-imx,
sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <1567004515-3567-3-git-send-email-peng.fan@nxp.com>
On Wed, 28 Aug 2019 03:03:02 +0000
Peng Fan <peng.fan@nxp.com> wrote:
Hi,
> From: Peng Fan <peng.fan@nxp.com>
>
> This mailbox driver implements a mailbox which signals transmitted data
> via an ARM smc (secure monitor call) instruction. The mailbox receiver
> is implemented in firmware and can synchronously return data when it
> returns execution to the non-secure world again.
> An asynchronous receive path is not implemented.
> This allows the usage of a mailbox to trigger firmware actions on SoCs
> which either don't have a separate management processor or on which such
> a core is not available. A user of this mailbox could be the SCP
> interface.
>
> Modified from Andre Przywara's v2 patch
> https://lore.kernel.org/patchwork/patch/812999/
>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/mailbox/Kconfig | 7 ++
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/arm-smc-mailbox.c | 215 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 224 insertions(+)
> create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index ab4eb750bbdd..7707ee26251a 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -16,6 +16,13 @@ config ARM_MHU
> The controller has 3 mailbox channels, the last of which can be
> used in Secure mode only.
>
> +config ARM_SMC_MBOX
> + tristate "Generic ARM smc mailbox"
> + depends on OF && HAVE_ARM_SMCCC
> + help
> + Generic mailbox driver which uses ARM smc calls to call into
> + firmware for triggering mailboxes.
> +
> config IMX_MBOX
> tristate "i.MX Mailbox"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index c22fad6f696b..93918a84c91b 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
>
> obj-$(CONFIG_ARM_MHU) += arm_mhu.o
>
> +obj-$(CONFIG_ARM_SMC_MBOX) += arm-smc-mailbox.o
> +
> obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
>
> obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX) += armada-37xx-rwtm-mailbox.o
> diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
> new file mode 100644
> index 000000000000..76a2ae11ee4d
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2016,2017 ARM Ltd.
> + * Copyright 2019 NXP
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define ARM_SMC_MBOX_MEM_TRANS BIT(0)
> +
> +struct arm_smc_chan_data {
> + u32 function_id;
> + u32 chan_id;
> + u32 flags;
> +};
> +
> +struct arm_smccc_mbox_cmd {
> + unsigned long a0, a1, a2, a3, a4, a5, a6, a7;
I think this is one or even two registers too long?
The SMCCC speaks of the function ID in x0/r0 and six arguments, with a "client ID" being an optional seventh argument. Looking at the description there I believe we cannot use the client ID here for this purpose, as this is supposed to be set by a hypervisor before passing on an SMC to EL3 firmware. KVM does not allow passing on SMCs in this way.
Also, while using "long" in here seems to make sense from the mailbox and SMC point of view, aliasing this to the mailbox client provided data seems dangerous to me, as this exposes the difference between arm32 and arm64 to the client. I think this is not what we want, the client should not be architecture specific.
> +};
> +
> +typedef unsigned long (smc_mbox_fn)(unsigned long, unsigned long,
> + unsigned long, unsigned long,
> + unsigned long, unsigned long,
> + unsigned long, unsigned long);
> +static smc_mbox_fn *invoke_smc_mbox_fn;
> +
> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> +{
> + struct arm_smc_chan_data *chan_data = link->con_priv;
> + struct arm_smccc_mbox_cmd *cmd = data;
> + unsigned long ret;
> + u32 function_id;
> + u32 chan_id;
> +
> + if (chan_data->flags & ARM_SMC_MBOX_MEM_TRANS) {
> + if (chan_data->function_id != UINT_MAX)
> + function_id = chan_data->function_id;
> + else
> + function_id = cmd->a0;
This is somewhat surprising, dangerous and undocumented. We should *not* allow mailbox clients to specify the function ID. They could end up using PSCI function IDs, for instance, that sounds especially scary if a client driver allows userland to set parameters of some sort.
The function ID is presumably allocated and fixed in the firmware, so it should not be dynamic. Any dynamic properties should be done within a function ID on the protocol level, by using r1/x1, for instance.
> + chan_id = chan_data->chan_id;
Why is this here? Where is this documented? Isn't this redundant with function ID? Or is this meant to be a replacement for it when a client provided function ID is used (which is not desired, as mentioned above)?
> + ret = invoke_smc_mbox_fn(function_id, chan_id, 0, 0, 0, 0,
> + 0, 0);
> + } else {
> + ret = invoke_smc_mbox_fn(cmd->a0, cmd->a1, cmd->a2, cmd->a3,
> + cmd->a4, cmd->a5, cmd->a6, cmd->a7);
> + }
As mentioned in my reply to the binding patch, I don't see this is necessary. Instead of ignoring the client provided data, we should just always pass it on. If clients and protocols don't use them, the client could zero it as well, letting the firmware side ignore it.
Also this underlines the problem with using "long" above: For 32-bit and 64-bit kernels the layout would be different.
I think the size of each argument should be determined by the calling convention class (bit 30) of the function ID.
The client doesn't know about that one (it's a controller/firmware property), so this driver here should split up the stream of data according to SMC64 vs. SMC32.
Does that make sense?
> +
> + mbox_chan_received_data(link, (void *)ret);
Not a mailbox expert, but shouldn't we mark the TX operation as complete here? Clearly by returning from the SMC the firmware has received the request.
Whether the requested action has completed, is a protocol / mailbox client question.
> +
> + return 0;
> +}
> +
> +static unsigned long __invoke_fn_hvc(unsigned long function_id,
> + unsigned long arg0, unsigned long arg1,
> + unsigned long arg2, unsigned long arg3,
> + unsigned long arg4, unsigned long arg5,
> + unsigned long arg6)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
> + arg5, arg6, &res);
> + return res.a0;
> +}
> +
> +static unsigned long __invoke_fn_smc(unsigned long function_id,
> + unsigned long arg0, unsigned long arg1,
> + unsigned long arg2, unsigned long arg3,
> + unsigned long arg4, unsigned long arg5,
> + unsigned long arg6)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
> + arg5, arg6, &res);
> + return res.a0;
> +}
> +
> +static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
> + .send_data = arm_smc_send_data,
> +};
> +
> +static int arm_smc_mbox_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mbox_controller *mbox;
> + struct arm_smc_chan_data *chan_data;
> + const char *method;
> + bool mem_trans = false;
> + int ret, i;
> + u32 val;
> +
> + if (!of_property_read_u32(dev->of_node, "arm,num-chans", &val)) {
> + if (!val) {
> + dev_err(dev, "invalid arm,num-chans value %u\n", val);
> + return -EINVAL;
> + }
> + } else {
> + return -EINVAL;
> + }
As mentioned, this property should be removed, ...
> +
> + if (!of_property_read_string(dev->of_node, "transports", &method)) {
> + if (!strcmp("mem", method)) {
> + mem_trans = true;
> + } else if (!strcmp("reg", method)) {
> + mem_trans = false;
> + } else {
> + dev_warn(dev, "invalid \"transports\" property: %s\n",
> + method);
> +
> + return -EINVAL;
> + }
> + } else {
> + return -EINVAL;
> + }
... and this one as well.
> +
> + if (!of_property_read_string(dev->of_node, "method", &method)) {
> + if (!strcmp("hvc", method)) {
> + invoke_smc_mbox_fn = __invoke_fn_hvc;
> + } else if (!strcmp("smc", method)) {
> + invoke_smc_mbox_fn = __invoke_fn_smc;
> + } else {
> + dev_warn(dev, "invalid \"method\" property: %s\n",
> + method);
Just a nit, but if this is fatal for the driver, it should be dev_err().
> +
> + return -EINVAL;
> + }
> + } else {
> + return -EINVAL;
> + }
> +
> + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> + if (!mbox)
> + return -ENOMEM;
> +
> + mbox->num_chans = val;
This could be replaced with:
mbox->num_chans = of_property_count_elems_of_size(dev_of_node(dev),
"arm,func-ids", sizeof(u32));
> + mbox->chans = devm_kcalloc(dev, mbox->num_chans, sizeof(*mbox->chans),
> + GFP_KERNEL);
> + if (!mbox->chans)
> + return -ENOMEM;
> +
> + chan_data = devm_kcalloc(dev, mbox->num_chans, sizeof(*chan_data),
> + GFP_KERNEL);
> + if (!chan_data)
> + return -ENOMEM;
> +
> + for (i = 0; i < mbox->num_chans; i++) {
> + u32 function_id;
> +
> + ret = of_property_read_u32_index(dev->of_node,
> + "arm,func-ids", i,
> + &function_id);
> + if (ret)
> + chan_data[i].function_id = UINT_MAX;
> +
> + else
> + chan_data[i].function_id = function_id;
As mentioned above, this should go. Any non-0 return value should stop the driver probing.
Cheers,
Andre.
> +
> + chan_data[i].chan_id = i;
> +
> + if (mem_trans)
> + chan_data[i].flags |= ARM_SMC_MBOX_MEM_TRANS;
> + mbox->chans[i].con_priv = &chan_data[i];
> + }
> +
> + mbox->txdone_poll = false;
> + mbox->txdone_irq = false;
> + mbox->ops = &arm_smc_mbox_chan_ops;
> + mbox->dev = dev;
> +
> + platform_set_drvdata(pdev, mbox);
> +
> + ret = devm_mbox_controller_register(dev, mbox);
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "ARM SMC mailbox enabled with %d chan%s.\n",
> + mbox->num_chans, mbox->num_chans == 1 ? "" : "s");
> +
> + return ret;
> +}
> +
> +static int arm_smc_mbox_remove(struct platform_device *pdev)
> +{
> + struct mbox_controller *mbox = platform_get_drvdata(pdev);
> +
> + mbox_controller_unregister(mbox);
> + return 0;
> +}
> +
> +static const struct of_device_id arm_smc_mbox_of_match[] = {
> + { .compatible = "arm,smc-mbox", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, arm_smc_mbox_of_match);
> +
> +static struct platform_driver arm_smc_mbox_driver = {
> + .driver = {
> + .name = "arm-smc-mbox",
> + .of_match_table = arm_smc_mbox_of_match,
> + },
> + .probe = arm_smc_mbox_probe,
> + .remove = arm_smc_mbox_remove,
> +};
> +module_platform_driver(arm_smc_mbox_driver);
> +
> +MODULE_AUTHOR("Andre Przywara <andre.przywara@arm.com>");
> +MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
> +MODULE_LICENSE("GPL v2");
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 05/11] kselftest: arm64: mangle_pstate_ssbs_regs
From: Cristian Marussi @ 2019-09-09 15:51 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114836.GV27757@arm.com>
Hi
On 04/09/2019 12:48, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:26pm +0100, Cristian Marussi wrote:
>> Add a simple mangle testcase which messes with the ucontext_t from within
>> the signal handler, trying to set the PSTATE SSBS bit.
>> Expect SIGILL if SSBS feature is unsupported or that, on test PASS, the
>> value set in PSTATE.SSBS in the signal frame is preserved by sigreturn.
>>
>> Additionally, in order to support this test specific needs:
>> - extend signal testing framework to allow the definition of a custom per
>> test initialization function to be run at the end of test setup.
>> - introduced a set_regval() helper to set system register values in a
>> toolchain independent way.
>> - introduce also a new common utility function: get_current_context()
>> which can be used to grab a ucontext without the help of libc, and
>> detect if such ucontext has been actively used to jump back into it.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v3 --> v4
>> - fix commit message
>> - missing include signal.h
>> - added .init per-test init-func
>> - added set_regval() helper
>> - added SSBS clear to 0 custom .init function
>> - removed volatile qualifier associated with sig_atomic_t data
>> - added dsb inside handler to ensure the writes related to the
>> grabbed ucontext have completed
>> - added test description
>> ---
>> .../selftests/arm64/signal/test_signals.h | 20 +++-
>> .../arm64/signal/test_signals_utils.c | 98 +++++++++++++++++++
>> .../arm64/signal/test_signals_utils.h | 2 +
>> .../testcases/mangle_pstate_ssbs_regs.c | 69 +++++++++++++
>> 4 files changed, 184 insertions(+), 5 deletions(-)
>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h
>> index a1cf69997604..0767e27fbe78 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals.h
>> @@ -27,6 +27,14 @@
>> : "memory"); \
>> }
>>
>> +#define set_regval(regname, in) \
>> +{ \
>> + asm volatile("msr " __stringify(regname) ", %0" \
>> + : \
>> + : "r" (in) \
>> + : "memory"); \
>> +}
>> +
>> /* Regs encoding and masks naming copied in from sysreg.h */
>> #define SYS_ID_AA64MMFR1_EL1 S3_0_C0_C7_1 /* MRS Emulated */
>> #define SYS_ID_AA64MMFR2_EL1 S3_0_C0_C7_2 /* MRS Emulated */
>> @@ -89,12 +97,16 @@ struct tdescr {
>> /* optional sa_flags for the installed handler */
>> int sa_flags;
>> ucontext_t saved_uc;
>> -
>> - /* a custom setup function to be called before test starts */
>> + /* used by get_current_ctx() */
>> + size_t live_sz;
>> + ucontext_t *live_uc;
>> + sig_atomic_t live_uc_valid;
>> + /* a custom setup: called alternatively to default_setup */
>> int (*setup)(struct tdescr *td);
>> + /* a custom init: called by default test initialization */
>> + void (*init)(struct tdescr *td);
>> /* a custom cleanup function called before test exits */
>> void (*cleanup)(struct tdescr *td);
>> -
>> /* an optional function to be used as a trigger for test starting */
>> int (*trigger)(struct tdescr *td);
>> /*
>> @@ -102,10 +114,8 @@ struct tdescr {
>> * presence of the trigger function above; this is mandatory
>> */
>> int (*run)(struct tdescr *td, siginfo_t *si, ucontext_t *uc);
>> -
>> /* an optional function for custom results' processing */
>> void (*check_result)(struct tdescr *td);
>> -
>> void *priv;
>> };
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> index e2a5f37e6ad3..c6fdcb23f246 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c
>> @@ -11,12 +11,16 @@
>> #include <linux/auxvec.h>
>> #include <ucontext.h>
>>
>> +#include <asm/unistd.h>
>> +
>> #include "test_signals.h"
>> #include "test_signals_utils.h"
>> #include "testcases/testcases.h"
>>
>> extern struct tdescr *current;
>>
>> +static int sig_copyctx = SIGUSR2;
>> +
>> static char *feats_store[FMAX_END] = {
>> " SSBS ",
>> " PAN ",
>> @@ -43,6 +47,81 @@ static inline char *feats_to_string(unsigned long feats)
>> return feats_string;
>> }
>>
>> +/*
>> + * Obtaining a valid and full-blown ucontext_t from userspace is tricky:
>> + * libc getcontext does() not save all the regs and messes with some of
>> + * them (pstate value in particular is not reliable).
>> + * Here we use a service signal to grab the ucontext_t from inside a
>> + * dedicated signal handler, since there, it is populated by Kernel
>> + * itself in setup_sigframe(). The grabbed context is then stored and
>> + * made available in td->live_uc.
>> + *
>> + * Anyway this function really serves a dual purpose:
>> + *
>> + * 1. grab a valid sigcontext into td->live_uc for result analysis: in
>> + * such case it returns 1.
>> + *
>> + * 2. detect if somehow a previously grabbed live_uc context has been
>> + * used actively with a sigreturn: in such a case the execution would have
>> + * magically resumed in the middle of the function itself (seen_already==1):
>> + * in such a case return 0, since in fact we have not just simply grabbed
>> + * the context.
>> + *
>> + * This latter case is useful to detect when a fake_sigreturn test-case has
>> + * unexpectedly survived without hittig a SEGV.
>> + */
>> +bool get_current_context(struct tdescr *td, ucontext_t *dest_uc)
>> +{
>> + static sig_atomic_t seen_already;
>> +
>> + assert(td && dest_uc);
>> + /* it's a genuine invocation..reinit */
>> + seen_already = 0;
>> + td->live_uc_valid = 0;
>> + td->live_sz = sizeof(*dest_uc);
>> + memset(dest_uc, 0x00, td->live_sz);
>> + td->live_uc = dest_uc;
>> + /*
>> + * Grab ucontext_t triggering a signal...
>> + * ASM equivalent of raise(sig_copyctx);
>> + *
>> + * Note that:
>> + * - live_uc_valid is declared sig_atomic_t in struct tdescr
>> + * since it will be changed inside the sig_copyctx handler
>> + * - the kill() syscall invocation returns only after any possible
>> + * registered signal handler for the invoked signal has returned,
>> + * so that live_uc_valid flag is surely up to date when this
>> + * function return it.
>> + * - the additional 'memory' clobber is there to avoid possible
>> + * compiler's assumption on live_uc_valid, seen-already and
>> + * the content pointed by dest_uc, which are all changed inside
>> + * the signal handler, without resorting to the volatile qualifier
>> + * (and keeping quiet checkpatch.pl)
>> + */
>> + asm volatile ("mov x8, %0\n\t"
>> + "svc #0\n\t"
>> + "mov x1, %1\n\t"
>> + "mov x8, %2\n\t"
>> + "svc #0"
>> + :
>> + : "i" (__NR_getpid), "r" (sig_copyctx), "i" (__NR_kill)
>> + : "x1", "x8", "x0", "memory");
>> + /*
>> + * If we get here with seen_already==1 it implies the td->live_uc
>> + * context has been used to get back here....this probably means
>> + * a test has failed to cause a SEGV...anyway the live_uc has not
>> + * just been acquired...so return 0
>> + */
>> + if (seen_already) {
>> + fprintf(stdout,
>> + "Successful sigreturn detected: live_uc is stale !\n");
>> + return 0;
>> + }
>> + seen_already = 1;
>> +
>> + return td->live_uc_valid;
>> +}
>> +
>> static void unblock_signal(int signum)
>> {
>> sigset_t sset;
>> @@ -124,6 +203,17 @@ static void default_handler(int signum, siginfo_t *si, void *uc)
>> * to terminate immediately exiting straight away
>> */
>> default_result(current, 1);
>> + } else if (signum == sig_copyctx && current->live_uc) {
>> + memcpy(current->live_uc, uc, current->live_sz);
>> + ASSERT_GOOD_CONTEXT(current->live_uc);
>> + current->live_uc_valid = 1;
>> + /*
>> + * Ensure above writes have completed before signal
>> + * handler terminates
>> + */
>> + asm volatile ("dsb sy" ::: "memory");
>
> The dsb doesn't help here: this has no effect on how the compiler caches
> variables in registers etc.
>
> Overall, I think some details need a bit of a rethink here.
>
Beside the dsb which I understand is useless, I thought the memory clobber
could have helped at least with the compiler optimization/re-ordering issues
while avoiding the volatile. (but not fully)
> We need some way to ensure coherency of accesses to variables around
> and inside the signal handler here, but since we're running in a single
> thread that may be interrupted by a signal handler (running in the same
> thread), it's compiler<->compiler coherency that's the issue here, not
> cpu<->cpu or cpu<->device coherency.
>
> There may also be atomicity concerns, since the compiler might move
> stuff across and/or duplicate or tear reads/writes around the asm where
> the signal is delivered.
>
> The classic solution to these problems is to use volatile, but this
> is a blunt tool and you often end up having to mark more objects
> volatile than you really want to in order to ensure correctness. The
> ordering behaviour of accesses to volatiles is also ill-specified for
> accesses made in different threads.
>
> That said, efficiency is of no concern here and we're single-threaded,
> so a blunt, simple tool may still be adequate.
>
I'll revert back to use volatile despite checkpatch complaints, and sig_atomic_t
where needed, and add an output param in the inline asm for *dest_uc to avoid
compiler making assumptions about it
>
> Another issue is that nothing stops the stack frame the captured SP
> points to from disappearing between get_current_context() and the
> fake_sigreturn() that tries to jump back to it.
>
> To avoid this issue, we'd probably need to inline more of
> get_current_context(), i.e., turn it into a macro.
>
I made it a static __always_inline.
Moreover I moved away from kill(mypid, USR1) approach since the syscall path
will zero the SVE regs before grabbing the sigframe
(and is bad for future signal/SVE tests).
I'm instead now using brk and catch the SIGTRAP (picked form the arm64/signal
SVE tests patch still to be published)
>> + fprintf(stderr,
>> + "GOOD CONTEXT grabbed from sig_copyctx handler\n");
>> } else {
>> if (signum == current->sig_unsupp && !are_feats_ok(current)) {
>> fprintf(stderr,
>> @@ -222,7 +312,15 @@ static int test_init(struct tdescr *td)
>> !feats_ok ? "NOT " : "");
>> }
>>
>> + if (td->sig_trig == sig_copyctx)
>> + sig_copyctx = SIGUSR1;
>> + unblock_signal(sig_copyctx);
>> +
>> + /* Perform test specific additional initialization */
>> + if (td->init)
>> + td->init(td);
>> td->initialized = 1;
>> +
>> return 1;
>> }
>>
>> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> index 8658d1a7d4b9..ce35be8ebc8e 100644
>> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
>> @@ -10,4 +10,6 @@ int test_setup(struct tdescr *td);
>> void test_cleanup(struct tdescr *td);
>> int test_run(struct tdescr *td);
>> void test_result(struct tdescr *td);
>> +
>> +bool get_current_context(struct tdescr *td, ucontext_t *dest_uc);
>> #endif
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>> new file mode 100644
>> index 000000000000..15e6f62512d5
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>> @@ -0,0 +1,69 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 ARM Limited
>> + *
>> + * Try to mangle the ucontext from inside a signal handler, setting the
>> + * SSBS bit to 1 and veryfing that such modification is preserved.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <signal.h>
>> +#include <ucontext.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +static void mangle_invalid_pstate_ssbs_init(struct tdescr *td)
>> +{
>> + fprintf(stderr, "Clearing SSBS to 0\n");
>> + set_regval(SSBS_SYSREG, 0);
>> +}
>> +
>> +static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,
>> + siginfo_t *si, ucontext_t *uc)
>> +{
>> + ASSERT_GOOD_CONTEXT(uc);
>> +
>> + /* set bit value */
>> + uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
>
> Can we check that uc->uc_mcontext.pstate & PSR_SSBS_BIT is initially 0?
>
> If not, it suggests either a test bug, or modification of the SSBS
> flag by other C code before the test signal was delivered.
>
Here the situation is a bit tricky: architecture defines three levels of
SSBS support:
1. SSBS bit + MRS SSBS instruction or
2. SSBS bit ONLY
3. no support
HW_SSBS capability is reported as supported by kernel only for full support (1.)
Otherwise it is reported as unsupported, even if in 2. the PSTATE.SSBS bit is working
and I can look it up using util get_current_context().
Moreover the test is supposed to check that the sigreturn Kernel path does NOT clear
improperly the SSBS bit while returning: so as long as we can set the SSBS bit in uc PSTATE
we can check if it is cleared.
So in order to gather as much info as possible from the test without incurring in unneeded
SIGILL (attempting to use MRS), my new approach is:
- test anyway no matter if SSBS is supported: check that bit is NOT cleared on sigreturn
- use MRS clear to 0 on test_init only if SSBS declared as supported
- abort when PSTATE.SSBS bit is not cleared on test start (trigger) ONLY if SSBS was declared supported (and so cleared in test_init)
- check test result using:
- MRS SSBS if HW_SSBS supported
- PSTATE.SSBS get_current_context if HW_SSBS NOT supported
- print out always PSTATE retrieved values
This way I was able to avoid SIGILL and properly test at any level of support (1,2,3)
both the PASS and the FAIL path (using a Kernel which does NOT properly preserve the
SSBS bit)
>> + fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",
>> + uc->uc_mcontext.pstate);
>> + /* Save after mangling...it should be preserved */
>> + td->saved_uc = *uc;
>> +
>> + return 1;
>> +}
>> +
>> +static void pstate_ssbs_bit_checks(struct tdescr *td)
>> +{
>> + uint64_t val = 0;
>> + ucontext_t uc;
>> +
>> + /* This check reports some result even if MRS SSBS unsupported */
>> + if (get_current_context(td, &uc))
>> + fprintf(stderr,
>> + "INFO: live_uc - got PSTATE: 0x%016llX -> SSBS %s\n",
>> + uc.uc_mcontext.pstate,
>> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT) ==
>> + (uc.uc_mcontext.pstate & PSR_SSBS_BIT) ?
>> + "PRESERVED" : "CLEARED");
>> +
>> + fprintf(stderr, "Checking with MRS SSBS...\n");
>> + get_regval(SSBS_SYSREG, val);
>> + fprintf(stderr, "INFO: MRS SSBS - got: 0x%016lX\n", val);
>> + /* pass when preserved */
>> + td->pass = (val & PSR_SSBS_BIT) ==
>> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT);
>> +}
>> +
>> +struct tdescr tde = {
>> + .sanity_disabled = true,
>> + .name = "MANGLE_PSTATE_SSBS_REGS",
>> + .descr = "Mangling uc_mcontext changing SSBS.(PRESERVE)",
>
> Can we come up with a clearer description here? I'm not sure how to
> read this.
>
Ok..I was trying to please checkpatch.
> [...]
>
> Cheers
> ---Dave
>
Cheers
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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