* [PATCH v26 2/7] arm64: kdump: implement machine_crash_shutdown()
From: AKASHI Takahiro @ 2016-09-20 7:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57DC067B.3050003@arm.com>
On Fri, Sep 16, 2016 at 03:49:31PM +0100, James Morse wrote:
> On 16/09/16 04:21, AKASHI Takahiro wrote:
> > On Wed, Sep 14, 2016 at 07:09:33PM +0100, James Morse wrote:
> >> On 07/09/16 05:29, AKASHI Takahiro wrote:
> >>> Primary kernel calls machine_crash_shutdown() to shut down non-boot cpus
> >>> and save registers' status in per-cpu ELF notes before starting crash
> >>> dump kernel. See kernel_kexec().
> >>> Even if not all secondary cpus have shut down, we do kdump anyway.
> >>>
> >>> As we don't have to make non-boot(crashed) cpus offline (to preserve
> >>> correct status of cpus at crash dump) before shutting down, this patch
> >>> also adds a variant of smp_send_stop().
>
> >>> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> >>> index 04744dc..a908958 100644
> >>> --- a/arch/arm64/include/asm/kexec.h
> >>> +++ b/arch/arm64/include/asm/kexec.h
> >>> @@ -40,7 +40,46 @@
> >>> static inline void crash_setup_regs(struct pt_regs *newregs,
> >>> struct pt_regs *oldregs)
> >>> {
> >>> - /* Empty routine needed to avoid build errors. */
> >>> + if (oldregs) {
> >>> + memcpy(newregs, oldregs, sizeof(*newregs));
> >>> + } else {
> >>> + u64 tmp1, tmp2;
> >>> +
> >>> + __asm__ __volatile__ (
> >>> + "stp x0, x1, [%2, #16 * 0]\n"
> >>> + "stp x2, x3, [%2, #16 * 1]\n"
> >>> + "stp x4, x5, [%2, #16 * 2]\n"
> >>> + "stp x6, x7, [%2, #16 * 3]\n"
> >>> + "stp x8, x9, [%2, #16 * 4]\n"
> >>> + "stp x10, x11, [%2, #16 * 5]\n"
> >>> + "stp x12, x13, [%2, #16 * 6]\n"
> >>> + "stp x14, x15, [%2, #16 * 7]\n"
> >>> + "stp x16, x17, [%2, #16 * 8]\n"
> >>> + "stp x18, x19, [%2, #16 * 9]\n"
> >>> + "stp x20, x21, [%2, #16 * 10]\n"
> >>> + "stp x22, x23, [%2, #16 * 11]\n"
> >>> + "stp x24, x25, [%2, #16 * 12]\n"
> >>> + "stp x26, x27, [%2, #16 * 13]\n"
> >>> + "stp x28, x29, [%2, #16 * 14]\n"
> >>> + "mov %0, sp\n"
> >>> + "stp x30, %0, [%2, #16 * 15]\n"
> >>> +
> >>> + "/* faked current PSTATE */\n"
> >>> + "mrs %0, CurrentEL\n"
> >>> + "mrs %1, DAIF\n"
> >>> + "orr %0, %0, %1\n"
> >>> + "mrs %1, NZCV\n"
> >>> + "orr %0, %0, %1\n"
> >>> +
> >>
> >> What about SPSEL? While we don't use it, it is correctly preserved for
> >> everything except a CPU that calls panic()...
> >
> > My comment above might be confusing, but what I want to fake
> > here is "spsr" as pt_regs.pstate is normally set based on spsr_el1.
> > So there is no corresponding field of SPSEL in spsr.
>
> Here is my logic, I may have missed something obvious, see what you think:
>
> SPSR_EL{1,2} shows the CPU mode 'M' in bits 0-4, From aarch64 bit 4 is always 0.
> From the register definitions in the ARM-ARM C5.2, likely values in 0-3 are:
> 0100 EL1t
> 0101 EL1h
> 1000 EL2t
> 1001 EL2h
>
> I'm pretty sure this least significant bit is what SPSEL changes, so it does get
> implicitly recorded in SPSR.
> CurrentEL returns a value in bits 0-3, of which 0-1 are RES0, so we lose the
> difference between EL?t and EL?h.
OK.
SPSel will be added assuming that CurrentEL is never 0 here.
>
> >
> >>
> >>> + /* pc */
> >>> + "adr %1, 1f\n"
> >>> + "1:\n"
> >>> + "stp %1, %0, [%2, #16 * 16]\n"
> >>> + : "=r" (tmp1), "=r" (tmp2), "+r" (newregs)
> >>> + :
> >>> + : "memory"
>
> Do you need the memory clobber? This asm only modifies values in newregs.
What about this (including the change above):
| "/* faked current PSTATE */\n"
| "mrs %0, CurrentEL\n"
| "mrs %1, SPSEL\n"
| "orr %0, %0, %1\n"
| "mrs %1, DAIF\n"
| "orr %0, %0, %1\n"
| "mrs %1, NZCV\n"
| "orr %0, %0, %1\n"
| /* pc */
| "adr %1, 1f\n"
| "1:\n"
| "stp %1, %0, [%2, #16 * 16]\n"
| : "+r" (tmp1), "+r" (tmp2)
| : "r" (newregs)
| : "memory"
>
> >>> + );
> >>> + }
> >>> }
> >>>
> >>> #endif /* __ASSEMBLY__ */
>
>
> >>> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
>
> >>> +#ifdef CONFIG_KEXEC_CORE
> >>> +void smp_send_crash_stop(void)
> >>> +{
> >>> + cpumask_t mask;
> >>> + unsigned long timeout;
> >>> +
> >>> + if (num_online_cpus() == 1)
> >>> + return;
> >>> +
> >>> + cpumask_copy(&mask, cpu_online_mask);
> >>> + cpumask_clear_cpu(smp_processor_id(), &mask);
> >>> +
> >>> + atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
> >>> +
> >>> + pr_crit("SMP: stopping secondary CPUs\n");
> >>> + smp_cross_call(&mask, IPI_CPU_CRASH_STOP);
> >>> +
> >>> + /* Wait up to one second for other CPUs to stop */
> >>> + timeout = USEC_PER_SEC;
> >>> + while ((atomic_read(&waiting_for_crash_ipi) > 0) && timeout--)
> >>> + udelay(1);
> >>> +
> >>> + if (atomic_read(&waiting_for_crash_ipi) > 0)
> >>> + pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
> >>> + cpumask_pr_args(cpu_online_mask));
> >>> +}
> >>> +#endif
> >>
> >> This is very similar to smp_send_stop() which also has the timeout. Is it
> >> possible to merge them? You could use in_crash_kexec to choose the IPI type.
> >
> > Yeah, we could merge them along with ipi_cpu_(crash_)stop().
> > But the resulting code would be quite noisy if each line
> > is switched by "if (in_crash_kexec)."
> > Otherwise, we may have one big "if" like:
> > void smp_send_stop(void)
> > {
> > if (in_crash_kexec)
> > ...
> > else
> > ...
> > }
> > It seems to me that it is not much different from the current code.
> > What do you think?
>
> Hmm, yes, its too fiddly to keep the existing behaviour of both.
>
> The problems are ipi_cpu_stop() doesn't call cpu_die(), (I can't see a good
> reason for this, but more archaeology is needed), and ipi_cpu_crash_stop()
> doesn't modify the online cpu mask.
>
> I don't suggest we do this yet, but it could be future cleanup if it's proved to
> be safe:
> smp_send_stop() is only called from: machine_halt(), machine_power_off(),
> machine_restart() and panic(). In all those cases the CPUs are never expected to
> come back, so we can probably merge the IPIs. This involves modifying the
> online cpu mask during kdump, (which I think is fine as it uses the atomic
> bitops so we won't get blocked on a lock), and promoting in_crash_kexec to some
> atomic type.
>
> But I think we should leave it as it is for now,
Sure.
Thanks,
-Takahiro AKASHI
>
> Thanks,
>
> James
>
^ permalink raw reply
* [PATCH v26 3/7] arm64: kdump: add kdump support
From: AKASHI Takahiro @ 2016-09-20 7:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57DC06B0.8030504@arm.com>
On Fri, Sep 16, 2016 at 03:50:24PM +0100, James Morse wrote:
> On 07/09/16 05:29, AKASHI Takahiro wrote:
> > On crash dump kernel, all the information about primary kernel's system
> > memory (core image) is available in elf core header.
> > The primary kernel will set aside this header with reserve_elfcorehdr()
> > at boot time and inform crash dump kernel of its location via a new
> > device-tree property, "linux,elfcorehdr".
> >
> > Please note that all other architectures use traditional "elfcorehdr="
> > kernel parameter for this purpose.
> >
> > Then crash dump kernel will access the primary kernel's memory with
> > copy_oldmem_page(), which reads one page by ioremap'ing it since it does
> > not reside in linear mapping on crash dump kernel.
> >
> > We also need our own elfcorehdr_read() here since the header is placed
> > within crash dump kernel's usable memory.
>
> One nit below, looks good.
Fixed.
Thanks,
-Takahiro AKASHI
> Reviewed-by: James Morse <james.morse@arm.com>
>
>
> Thanks,
>
> James
>
>
> > diff --git a/arch/arm64/kernel/crash_dump.c b/arch/arm64/kernel/crash_dump.c
>
> > +/**
> > + * copy_oldmem_page() - copy one page from old kernel memory
> > + * @pfn: page frame number to be copied
> > + * @buf: buffer where the copied page is placed
> > + * @csize: number of bytes to copy
> > + * @offset: offset in bytes into the page
> > + * @userbuf: if set, @buf is in a user address space
> > + *
> > + * This function copies one page from old kernel memory into buffer pointed by
> > + * @buf. If @buf is in userspace, set @userbuf to %1. Returns number of bytes
> > + * copied or negative error in case of failure.
> > + */
> > +ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
> > + size_t csize, unsigned long offset,
> > + int userbuf)
> > +{
> > + void *vaddr;
> > +
> > + if (!csize)
> > + return 0;
> > +
> > + vaddr = memremap(__pfn_to_phys(pfn), PAGE_SIZE, MEMREMAP_WB);
> > + if (!vaddr)
> > + return -ENOMEM;
> > +
> > + if (userbuf) {
>
> > + if (copy_to_user(buf, vaddr + offset, csize)) {
>
> If you re-cast buf with (char __user *), it should stop sparse complaining:
> > ../arch/arm64/kernel/crash_dump.c:45:34: warning: incorrect type in argument 1
> (different address spaces)
> > ../arch/arm64/kernel/crash_dump.c:45:34: expected void [noderef] <asn:1>*to
> > ../arch/arm64/kernel/crash_dump.c:45:34: got char *buf
>
>
^ permalink raw reply
* [PATCH] clocksource: arm_arch_timer: Don't assume clock runs in suspend
From: Marc Zyngier @ 2016-09-20 7:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160919231441.GA60928@google.com>
On 20/09/16 00:14, Brian Norris wrote:
> On Fri, Sep 16, 2016 at 09:06:55AM +0100, Marc Zyngier wrote:
>> Hi Brian,
>
> Hi Marc,
>
> Thanks for the quick response.
>
>> On 16/09/16 06:49, Brian Norris wrote:
>>> Since commit 4fbcdc813fb9 ("clocksource: arm_arch_timer: Use clocksource
>>> for suspend timekeeping"), this driver assumes that the ARM architected
>>> timer keeps running in suspend. This is not the case for some ARM SoCs,
>>> depending on the HW state used for system suspend. Let's not assume that
>>> all SoCs support this, and instead only support this if the device tree
>>> explicitly tells us it's "always on". In all other cases, just fall back
>>> to the RTC. This should be relatively harmless.
>>
>> I'm afraid you're confusing two things:
>> - the counter, which *must* carry on counting no matter what, as
>> (quoting the ARM ARM) "The system counter must be implemented in an
>> always-on power domain"
>> - the timer, which is allowed to be powered off, and can be tagged with
>> the "always-on" property to indicate that it is guaranteed to stay up
>> (which in practice only exists in virtual machines and never on real HW).
>
> Indeed, sorry for that confusion, and thanks for the explanations.
>
>> If your counter does stop counting when suspended, then this is starting
>> to either feel like a HW bug, or someone is killing the clock that feeds
>> this counter when entering suspend.
>>
>> If this is the former, then we need a separate quirk to indicate the
>> non-standard behaviour. If it is the latter, don't do it! ;-)
>
> It's beginning to seem more like a HW quirk which yields nonstandard
> behavior. AIUI, this SoC normally runs the counter off its 24 MHz clock,
> but for low power modes, this "always-on" domain switches over to a 32
> KHz alternative clock. Unfortunately, the counter doesn't actually tick
> when run this way. I'm trying to confirm with the chip designers
> (Rockchip, RK3399) about the nature of the quirk, but I think we'll need
> a separate DT flag for this behavior.
The counter is allowed to be clocked at a different rate, as long as it
is incremented by the frequency ratio on each tick of the new frequency.
In your case, the counter should increment by 750 on each tick of the
32kHz clock. If the rk3399 implementation doesn't do this, then this is
a bug, and we need a quirk to work around it.
Note that such a quirk will have some other impacts, such as the
gettimeofday implementation in the VDSO (which relies on the counter
making forward progress). There could be other issues in the timer
subsystem as well... This doesn't look like a pleasant thing to fix.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v26 6/7] arm64: kdump: update a kernel doc
From: AKASHI Takahiro @ 2016-09-20 8:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57DC18FC.9000807@arm.com>
On Fri, Sep 16, 2016 at 05:08:28PM +0100, James Morse wrote:
> Hi Akashi,
>
> On 07/09/16 05:29, AKASHI Takahiro wrote:
> > This patch adds arch specific descriptions about kdump usage on arm64
> > to kdump.txt.
>
> > diff --git a/Documentation/kdump/kdump.txt b/Documentation/kdump/kdump.txt
>
> > @@ -249,6 +249,13 @@ Dump-capture kernel config options (Arch Dependent, arm)
> >
> > AUTO_ZRELADDR=y
> >
> > +Dump-capture kernel config options (Arch Dependent, arm64)
> > +----------------------------------------------------------
> > +
> > +- Please note that kvm of the dump-capture kernel will not be enabled
> > + on non-VHE systems even if it is configured. This is because the CPU
> > + cannot be reset to EL2 on panic.
>
> Nit:
> cannot be -> will not be
OK.
> We could try to do this, but its more code that could prevent us reaching the
> kdump kernel, so we choose not to.
>
>
> > @@ -370,6 +381,9 @@ For s390x:
> > For arm:
> > "1 maxcpus=1 reset_devices"
> >
> > +For arm64:
> > + "1 maxcpus=1 reset_devices"
> > +
>
> 'maxcpus=1' is a bit fragile. Since 44dbcc93ab67145 ("arm64: Fix behavior of
> maxcpus=N") udev on ubuntu vivid (running on Juno) has taken it upon itself to
> bring the secondary cores online, even when booted with 'maxcpus=1'.
>
> Can we change the recomendation to "1 nosmp reset_devices"?
Well, I have no strong opinion here, but I'm not quite sure whether
this change does make any difference in practice.
Looking at kernel/smp.c, the only difference is setup_max_cpus. But
given that arch_disable_smp_support() is null and smp_cpus_done()
ignores "max_cpus" on arm64, I don't think that the change is very
meaningful.
I might miss something.
Thanks,
-Takahiro AKASHI
>
> Thanks,
>
> James
>
^ permalink raw reply
* [PATCH] drm/exynos: mark exynos_dp_crtc_clock_enable() static
From: Andrzej Hajda @ 2016-09-20 8:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474207110-16111-1-git-send-email-baoyou.xie@linaro.org>
On 18.09.2016 15:58, Baoyou Xie wrote:
> We get 1 warning when building kernel with W=1:
> drivers/gpu/drm/exynos/exynos_dp.c:46:5: warning: no previous prototype for 'exynos_dp_crtc_clock_enable' [-Wmissing-prototypes]
>
> In fact, this function is only used in the file in which it is
> declared and don't need a declaration, but can be made static.
> So this patch marks it 'static'.
>
> Signed-off-by: Baoyou Xie <baoyou.xie@linaro.org>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
--
Regards
Andrzej
^ permalink raw reply
* [PATCH] gpio: Added zynq specific check for special pins on bank zero
From: Nava kishore Manne @ 2016-09-20 8:32 UTC (permalink / raw)
To: linux-arm-kernel
From: Nava kishore Manne <nava.manne@xilinx.com>
This patch adds zynq specific check for bank 0 pins 7 and 8
are special and cannot be used as inputs
Signed-off-by: Nava kishore Manne <navam@xilinx.com>
---
drivers/gpio/gpio-zynq.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index e72794e..eae9d24 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -96,6 +96,10 @@
/* GPIO upper 16 bit mask */
#define ZYNQ_GPIO_UPPER_MASK 0xFFFF0000
+/* For GPIO quirks */
+#define ZYNQ_GPIO BIT(0)
+#define ZYNQMP_GPIO BIT(1)
+
/**
* struct zynq_gpio - gpio device private data structure
* @chip: instance of the gpio_chip
@@ -122,6 +126,7 @@ struct zynq_gpio {
*/
struct zynq_platform_data {
const char *label;
+ u32 quirks;
u16 ngpio;
int max_bank;
int bank_min[ZYNQMP_GPIO_MAX_BANK];
@@ -238,13 +243,19 @@ static void zynq_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
{
u32 reg;
+ bool is_zynq_gpio;
unsigned int bank_num, bank_pin_num;
struct zynq_gpio *gpio = gpiochip_get_data(chip);
+ is_zynq_gpio = gpio->p_data->quirks & ZYNQ_GPIO;
zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num, gpio);
- /* bank 0 pins 7 and 8 are special and cannot be used as inputs */
- if (bank_num == 0 && (bank_pin_num == 7 || bank_pin_num == 8))
+ /*
+ * On zynq bank 0 pins 7 and 8 are special and cannot be used
+ * as inputs.
+ */
+ if (is_zynq_gpio && bank_num == 0 &&
+ (bank_pin_num == 7 || bank_pin_num == 8))
return -EINVAL;
/* clear the bit in direction mode reg to set the pin as input */
@@ -609,6 +620,7 @@ static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
static const struct zynq_platform_data zynqmp_gpio_def = {
.label = "zynqmp_gpio",
+ .quirks = ZYNQMP_GPIO,
.ngpio = ZYNQMP_GPIO_NR_GPIOS,
.max_bank = ZYNQMP_GPIO_MAX_BANK,
.bank_min[0] = ZYNQ_GPIO_BANK0_PIN_MIN(MP),
@@ -627,6 +639,7 @@ static const struct zynq_platform_data zynqmp_gpio_def = {
static const struct zynq_platform_data zynq_gpio_def = {
.label = "zynq_gpio",
+ .quirks = ZYNQ_GPIO,
.ngpio = ZYNQ_GPIO_NR_GPIOS,
.max_bank = ZYNQ_GPIO_MAX_BANK,
.bank_min[0] = ZYNQ_GPIO_BANK0_PIN_MIN(),
--
2.1.2
^ permalink raw reply related
* [PATCH v9 17/19] drm/virtio: kconfig: Fix recursive dependency issue.
From: Peter Griffin @ 2016-09-20 8:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACvgo52_g7anGBd9HTUyYsbp5QmQEk9LR3nNHgSTgjQ29G3ViA@mail.gmail.com>
Hi Emil,
On Tue, 20 Sep 2016, Emil Velikov wrote:
> On 5 September 2016 at 14:16, Peter Griffin <peter.griffin@linaro.org> wrote:
> > ST_SLIM_REMOTEPROC must select REMOTEPROC, which exposes the following
> > recursive dependency.
> >
> From a humble skim through remoteproc, drm and a few other subsystems
> I think the above is wrong. All the drivers (outside of remoteproc),
> that I've seen, depend on the core component, they don't select it.
I will let Bjorn comment on the remoteproc subsystem Kconfig design, and
why it is like it is.
For this particular SLIM_RPROC I have added it to Kconfig in keeping with all
the other drivers in the remoteproc subsystem which has exposed this recursive
dependency issue.
For this particular kconfig symbol a quick grep reveals more drivers in
the kernel using 'select', than 'depend on'
git grep "select VIRTIO" | wc -l
14
git grep "depends on VIRTIO" | wc -l
10
> Furthermore most places explicitly hide the drivers from the menu if
> the core component isn't enabled.
Remoteproc subsystem takes a different approach, the core code is only enabled
if a driver which relies on it is enabled. This IMHO makes sense, as
remoteproc is not widely used (only a few particular ARM SoC's).
It is true that for subsystems which rely on the core component being
explicitly enabled, they often tend to hide drivers which depend on it
from the menu unless it is. This also makes sense.
>
> Is there something that requires such a different/unusual behaviour in
> remoteproc ?
>
I'm not sure it is that unusual...looking at config USB, it selects USB_COMMON in
mfd subsystem, client drivers select MFD_CORE.
I've added Arnd to this thread, as I've seen lots of Kconfig patches from him
recently, maybe he has some thoughts on whether this is the correct fix or not?
regards,
Peter.
^ permalink raw reply
* [RFC] Arm64 boot fail with numa enable in BIOS
From: Will Deacon @ 2016-09-20 8:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <cd53a1ae-e4ff-933c-4d1c-1ddaaa999a43@huawei.com>
Hi Yisheng,
On Tue, Sep 20, 2016 at 11:29:24AM +0800, Yisheng Xie wrote:
> On 2016/9/19 22:07, Mark Rutland wrote:
> > On Mon, Sep 19, 2016 at 09:05:26PM +0800, Yisheng Xie wrote:
> > Can you modify the warning in cpumask.h to dump the bad CPU number? That
> > would make it fairly clear if that's the case.
> >
> hi Mark,
> I dump the bad CPU number, it is 64,
> And the cpumask get from task is 00000000,00000000.
>
> [ 3.873044] select_task_rq: allowed 0, allow_cpumask 00000000,00000000
> [ 3.879727] cpumask_check: cpu 64, nr_cpumask_bits:64, nr_cpu_ids= 64
> [ 3.895989] ------------[ cut here ]------------
> [ 3.900652] WARNING: CPU: 16 PID: 103 at ./include/linux/cpumask.h:122 try_to_wake_up+0x410/0x4ac
Can you look at this patch from David, please:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-September/458110.html
and offer a Tested-by if it fixes your problem?
Thanks,
Will
^ permalink raw reply
* [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: Noralf Trønnes @ 2016-09-20 8:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <c47a0a10-4e0b-9025-8d38-af299d5f55da@martin.sperl.org>
Den 20.09.2016 09:19, skrev Martin Sperl:
> Hi Noralf!
>
> On 19.09.2016 17:26, Noralf Tr?nnes wrote:
>> Some SMBus protocols use Repeated Start Condition to switch from write
>> mode to read mode. Devices like MMA8451 won't work without it.
>>
>> When downstream implemented support for this in i2c-bcm2708, it broke
>> support for some devices, so a module parameter was added and combined
>> transfer was disabled by default.
>> See https://github.com/raspberrypi/linux/issues/599
>> It doesn't seem to have been any investigation into what the problem
>> really was. Later there was added a timeout on the polling loop.
>>
>> One of the devices mentioned to partially stop working was DS1307.
>>
>> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
>> and AT24C32 (eeprom) in parallel without problems.
>>
>> Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
>> ---
>> drivers/i2c/busses/i2c-bcm2835.c | 107
>> +++++++++++++++++++++++++++++++++++----
>> 1 file changed, 98 insertions(+), 9 deletions(-)
> ...
>> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
>> *adap, struct i2c_msg msgs[],
>> int i;
>> int ret = 0;
>> + /* Combined write-read to the same address (smbus) */
>> + if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>> + !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>> + (msgs[0].len <= 16)) {
>> + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>> +
>> + return ret ? ret : 2;
>> + }
>> +
>> for (i = 0; i < num; i++) {
>> - ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
>> + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>> if (ret)
>> break;
>> }
> This does not seem to implement the i2c_msg api correctly.
>
> As per comments in include/uapi/linux/i2c.h on line 58 only the last
> message
> in a group should - by default - send a STOP.
>
Apparently it's a known problem that the i2c controller doesn't support
Repeated Start. It will always issue a Stop when it has transferred DLEN
bytes.
Refs:
http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
and before DONE is set (or the last byte is shifted, I don't know excatly).
Refs:
https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
I found this answer/report by joan that the downstream combined support
isn't reliable:
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
My implementation differs from downstream in that I use local_irq_save()
to protect the polling loop. But that only protects from missing the TA
(downstream can miss the TA and issue a Stop).
So currently in mainline we have a driver that says it support the standard
(I2C_FUNC_I2C), but it really only supports one message transfers since it
can't do ReStart.
What I have done in this patch is to support ReStart for transfers with
2 messages: first write, then read. But maybe a better solution is to just
leave this alone if it is flaky and use bitbanging instead. I don't know.
Noralf.
> As far as I understand you would need to implement the I2C_M_STOP flag
> (by exposing I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func)
> to make this work correctly:
>
> for (i = 0; i < num; i++) {
> + bool send_stop = (i == num - 1) ||msgs[i]
> <http://lxr.free-electrons.com/ident?i=msg>->flags
> <http://lxr.free-electrons.com/ident?i=flags> &I2C_M_STOP
> <http://lxr.free-electrons.com/ident?i=I2C_M_STOP>;
> - ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
> + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], send_stop);
> if (ret)
> break;
> }
>
> The corresponding device driver (or userspace) will need to set the
> flag correctly.
>
> Martin
^ permalink raw reply
* [RESEND PATCH] arm64: kgdb: fix single stepping
From: AKASHI Takahiro @ 2016-09-20 8:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <bb9cf2c6-83b4-1c71-d497-67f751de9b35@windriver.com>
Jason,
On Mon, Sep 19, 2016 at 05:29:36PM -0500, Jason Wessel wrote:
> On 09/15/2016 11:32 PM, AKASHI Takahiro wrote:
> >@@ -176,18 +183,14 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
> >>> * over and over again.
> >>> */
> >>> kgdb_arch_update_addr(linux_regs, remcom_in_buffer);
> >>>- atomic_set(&kgdb_cpu_doing_single_step, -1);
> >>>- kgdb_single_step = 0;
> >>
> >>This is a subtle change, but I assume it is what you intended? All the CPUs will get released into the run state when exiting the kgdb exception handler.
> >You are talking about "- kgdb_single_step = 0." Right?
>
>
> Correct.
>
> >Do you think that there is any (negative) side effect of this change?
>
>
> Not at all. The kernel debugger always skids to a stop, and it is more reliable from a locking perspective if the other CPU threads are released while a single CPU is asked to single step until the next "skid" for all the other CPUs.
>
> When you do not release the other CPUs you can end up single stepping a CPU which dead locks or never exits a lock elsewhere due to what ever it was blocking on never getting freed from another CPU.
Thank you for the explanation. This convinces me very much.
-Takahiro AKASHI
> Cheers,
> Jason.
^ permalink raw reply
* [PATCH] ARM: uniphier: select ARCH_HAS_RESET_CONTROLLER
From: Masahiro Yamada @ 2016-09-20 8:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474356608.4030.7.camel@pengutronix.de>
Hi Philipp,
2016-09-20 16:30 GMT+09:00 Philipp Zabel <p.zabel@pengutronix.de>:
> Hi Masahiro,
>
> Am Dienstag, den 20.09.2016, 13:43 +0900 schrieb Masahiro Yamada:
>> The UniPhier reset driver (drivers/reset/reset-uniphier.c) has been
>> merged. Select ARCH_HAS_RESET_CONTROLLER from the SoC Kconfig.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>> Philipp,
>>
>> IIRC, you mentioned that you were planning to consolidate the double
>> gurad by CONFIG_RESET_CONTROLLER and CONFIG_ARCH_HAS_RESET_CONTROLLER.
>>
>> I have not seen it in the ML, so I am sending this.
>>
>> Please let me know if you have some updates.
>
> I had started to doodle a bit, see
>
> git fetch git://git.pengutronix.de/git/pza/linux.git refs/heads/reset/kconfig
>
> but I haven't found time for cleanup and testing.
OK, I will merge this patch for now.
BTW, I did not understand some of your commits under way.
commit 7fe911f9c83737449565db03bebf953d3d94bbbf
Author: Philipp Zabel <p.zabel@pengutronix.de>
Date: Tue Aug 9 11:18:51 2016 +0200
dmaengine: sunx6i: do not depend on reset controller framework
The reset controller framework provides inline function stubs if
disabled.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
As far as I see from drivers/dma/sun6i-dma.c,
the reset control is mandatory for this driver.
Why are you removing the dependency?
Don't you care if it works on run-time
as long as it can build?
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Marc Zyngier @ 2016-09-20 9:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <0960d2a7-6100-3212-c544-d5377df34d57@suse.de>
On 19/09/16 18:39, Alexander Graf wrote:
>
>
> On 19.09.16 16:48, Marc Zyngier wrote:
>> On 19/09/16 12:14, Alexander Graf wrote:
>>> We have 2 modes for dealing with interrupts in the ARM world. We can either
>>> handle them all using hardware acceleration through the vgic or we can emulate
>>> a gic in user space and only drive CPU IRQ pins from there.
>>>
>>> Unfortunately, when driving IRQs from user space, we never tell user space
>>> about timer events that may result in interrupt line state changes, so we
>>> lose out on timer events if we run with user space gic emulation.
>>>
>>> This patch fixes that by routing vtimer expiration events to user space.
>>> With this patch I can successfully run edk2 and Linux with user space gic
>>> emulation.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>>
>>> - Add back curly brace that got lost
>>>
>>> v2 -> v3:
>>>
>>> - Split into patch set
>>>
>>> v3 -> v4:
>>>
>>> - Improve documentation
>>> ---
>>> Documentation/virtual/kvm/api.txt | 30 ++++++++-
>>> arch/arm/include/asm/kvm_host.h | 3 +
>>> arch/arm/kvm/arm.c | 22 ++++---
>>> arch/arm64/include/asm/kvm_host.h | 3 +
>>> include/uapi/linux/kvm.h | 14 +++++
>>> virt/kvm/arm/arch_timer.c | 125 +++++++++++++++++++++++++++-----------
>>> 6 files changed, 155 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>> index 23937e0..1c0bd86 100644
>>> --- a/Documentation/virtual/kvm/api.txt
>>> +++ b/Documentation/virtual/kvm/api.txt
>>> @@ -3202,9 +3202,14 @@ struct kvm_run {
>>> /* in */
>>> __u8 request_interrupt_window;
>>>
>>> -Request that KVM_RUN return when it becomes possible to inject external
>>> +[x86] Request that KVM_RUN return when it becomes possible to inject external
>>> interrupts into the guest. Useful in conjunction with KVM_INTERRUPT.
>>>
>>> +[arm*] Bits set to 1 in here mask IRQ lines that would otherwise potentially
>>> +trigger forever. These lines are available:
>>> +
>>> + KVM_IRQWINDOW_VTIMER - Masks hw virtual timer irq while in guest
>>> +
>>> __u8 padding1[7];
>>>
>>> /* out */
>>> @@ -3519,6 +3524,18 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
>>> event/message pages and to enable/disable SynIC messages/events processing
>>> in userspace.
>>>
>>> + /* KVM_EXIT_ARM_TIMER */
>>> + struct {
>>> + __u8 timesource;
>>> + } arm_timer;
>>> +
>>> +Indicates that a timer triggered that user space needs to handle and
>>> +potentially mask with vcpu->run->request_interrupt_window to allow the
>>> +guest to proceed. This only happens for timers that got enabled through
>>> +KVM_CAP_ARM_TIMER. The following time sources are available:
>>> +
>>> + KVM_ARM_TIMER_VTIMER - virtual cpu timer
>>> +
>>> /* Fix the size of the union. */
>>> char padding[256];
>>> };
>>> @@ -3739,6 +3756,17 @@ Once this is done the KVM_REG_MIPS_VEC_* and KVM_REG_MIPS_MSA_* registers can be
>>> accessed, and the Config5.MSAEn bit is accessible via the KVM API and also from
>>> the guest.
>>>
>>> +6.11 KVM_CAP_ARM_TIMER
>>> +
>>> +Architectures: arm, arm64
>>> +Target: vcpu
>>> +Parameters: args[0] contains a bitmap of timers to select (see 5.)
>>> +
>>> +This capability allows to route per-core timers into user space. When it's
>>> +enabled and no in-kernel interrupt controller is in use, the timers selected
>>> +by args[0] trigger KVM_EXIT_ARM_TIMER guest exits when they are pending,
>>> +unless masked by vcpu->run->request_interrupt_window (see 5.).
>>> +
>>> 7. Capabilities that can be enabled on VMs
>>> ------------------------------------------
>>>
>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>> index de338d9..77d1f73 100644
>>> --- a/arch/arm/include/asm/kvm_host.h
>>> +++ b/arch/arm/include/asm/kvm_host.h
>>> @@ -180,6 +180,9 @@ struct kvm_vcpu_arch {
>>>
>>> /* Detect first run of a vcpu */
>>> bool has_run_once;
>>> +
>>> + /* User space wants timer notifications */
>>> + bool user_space_arm_timers;
>>
>> Please move this to the timer structure.
>
> Sure.
>
>>
>>> };
>>>
>>> struct kvm_vm_stat {
>>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>>> index c84b6ad..57bdb71 100644
>>> --- a/arch/arm/kvm/arm.c
>>> +++ b/arch/arm/kvm/arm.c
>>> @@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>> case KVM_CAP_ARM_PSCI_0_2:
>>> case KVM_CAP_READONLY_MEM:
>>> case KVM_CAP_MP_STATE:
>>> + case KVM_CAP_ARM_TIMER:
>>> r = 1;
>>> break;
>>> case KVM_CAP_COALESCED_MMIO:
>>> @@ -474,13 +475,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>> return ret;
>>> }
>>>
>>> - /*
>>> - * Enable the arch timers only if we have an in-kernel VGIC
>>> - * and it has been properly initialized, since we cannot handle
>>> - * interrupts from the virtual timer with a userspace gic.
>>> - */
>>> - if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
>>> - ret = kvm_timer_enable(vcpu);
>>> + ret = kvm_timer_enable(vcpu);
>>>
>>> return ret;
>>> }
>>> @@ -601,6 +596,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>> run->exit_reason = KVM_EXIT_INTR;
>>> }
>>>
>>> + if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>>
>> Since this is a very unlikely event (in the grand scheme of things), how
>> about making this unlikely()?
>>
>>> + /* Tell user space about the pending vtimer */
>>> + ret = 0;
>>> + run->exit_reason = KVM_EXIT_ARM_TIMER;
>>> + run->arm_timer.timesource = KVM_ARM_TIMER_VTIMER;
>>> + }
>>
>> More importantly: why does it have to be indirected by a
>> make_request/check_request, and not be handled as part of the
>> kvm_timer_sync() call? We do update the state there, and you could
>> directly find out whether an exit is required.
>
> I can try - it seemed like it could easily become quite racy because we
> call kvm_timer_sync_hwstate() at multiple places.
It shouldn't. We only do it at exactly two locations (depending whether
we've entered the guest or not).
Also, take the following scenario:
(1) guest programs the timer to expire at time T
(2) guest performs an MMIO access which traps
(3) during the world switch, the timer expires and we mark the timer
interrupt as pending
(4) we exit to handle the MMIO, no sign of the timer being pending
Is the timer event lost? Or simply delayed? I think this indicates that
the timer state should always be signalled to userspace, no matter what
the exit reason is.
>
>>
>>> +
>>> if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) ||
>>> vcpu->arch.power_off || vcpu->arch.pause) {
>>> local_irq_enable();
>>> @@ -887,6 +889,12 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>>> return -EINVAL;
>>>
>>> switch (cap->cap) {
>>> + case KVM_CAP_ARM_TIMER:
>>> + r = 0;
>>> + if (cap->args[0] != KVM_ARM_TIMER_VTIMER)
>>> + return -EINVAL;
>>> + vcpu->arch.user_space_arm_timers = true;
>>> + break;
>>> default:
>>> r = -EINVAL;
>>> break;
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index 3eda975..3d01481 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -270,6 +270,9 @@ struct kvm_vcpu_arch {
>>>
>>> /* Detect first run of a vcpu */
>>> bool has_run_once;
>>> +
>>> + /* User space wants timer notifications */
>>> + bool user_space_arm_timers;
>>> };
>>>
>>> #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs)
>>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>>> index 300ef25..00f4948 100644
>>> --- a/include/uapi/linux/kvm.h
>>> +++ b/include/uapi/linux/kvm.h
>>> @@ -205,6 +205,7 @@ struct kvm_hyperv_exit {
>>> #define KVM_EXIT_S390_STSI 25
>>> #define KVM_EXIT_IOAPIC_EOI 26
>>> #define KVM_EXIT_HYPERV 27
>>> +#define KVM_EXIT_ARM_TIMER 28
>>>
>>> /* For KVM_EXIT_INTERNAL_ERROR */
>>> /* Emulate instruction failed. */
>>> @@ -361,6 +362,10 @@ struct kvm_run {
>>> } eoi;
>>> /* KVM_EXIT_HYPERV */
>>> struct kvm_hyperv_exit hyperv;
>>> + /* KVM_EXIT_ARM_TIMER */
>>> + struct {
>>> + __u8 timesource;
>>> + } arm_timer;
>>> /* Fix the size of the union. */
>>> char padding[256];
>>> };
>>> @@ -870,6 +875,7 @@ struct kvm_ppc_smmu_info {
>>> #define KVM_CAP_S390_USER_INSTR0 130
>>> #define KVM_CAP_MSI_DEVID 131
>>> #define KVM_CAP_PPC_HTM 132
>>> +#define KVM_CAP_ARM_TIMER 133
>>>
>>> #ifdef KVM_CAP_IRQ_ROUTING
>>>
>>> @@ -1327,4 +1333,12 @@ struct kvm_assigned_msix_entry {
>>> #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0)
>>> #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1)
>>>
>>> +/* Available with KVM_CAP_ARM_TIMER */
>>> +
>>> +/* Bits for run->request_interrupt_window */
>>> +#define KVM_IRQWINDOW_VTIMER (1 << 0)
>>> +
>>> +/* Bits for run->arm_timer.timesource */
>>> +#define KVM_ARM_TIMER_VTIMER (1 << 0)
>>> +
>>> #endif /* __LINUX_KVM_H */
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 4309b60..cbbb50dd 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -170,16 +170,45 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
>>> {
>>> int ret;
>>> struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>> + struct kvm_run *run = vcpu->run;
>>>
>>> - BUG_ON(!vgic_initialized(vcpu->kvm));
>>> + BUG_ON(irqchip_in_kernel(vcpu->kvm) && !vgic_initialized(vcpu->kvm));
>>>
>>> timer->active_cleared_last = false;
>>> timer->irq.level = new_level;
>>> - trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
>>> + trace_kvm_timer_update_irq(vcpu->vcpu_id, host_vtimer_irq,
>>> timer->irq.level);
>>> - ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
>>> - timer->irq.irq,
>>> - timer->irq.level);
>>> +
>>> + if (irqchip_in_kernel(vcpu->kvm) && vgic_initialized(vcpu->kvm)) {
>>
>> Given how many times you repeat this idiom in this patch, you should
>> have a single condition that encapsulate it once and for all.
>
> Sure.
>
>>
>>> + /* Fire the timer in the VGIC */
>>> +
>>> + ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
>>> + timer->irq.irq,
>>> + timer->irq.level);
>>> + } else if (!vcpu->arch.user_space_arm_timers) {
>>> + /* User space has not activated timer use */
>>> + ret = 0;
>>> + } else {
>>> + /*
>>> + * Set PENDING_TIMER so that user space can handle the event if
>>> + *
>>> + * 1) Level is high
>>> + * 2) The vtimer is not suppressed by user space
>>> + * 3) We are not in the timer trigger exit path
>>> + */
>>> + if (new_level &&
>>> + !(run->request_interrupt_window & KVM_IRQWINDOW_VTIMER) &&
>>> + (run->exit_reason != KVM_EXIT_ARM_TIMER)) {
>>> + /* KVM_REQ_PENDING_TIMER means vtimer triggered */
>>> + kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
>>> + }
>>> +
>>> + /* Force a new level high check on next entry */
>>> + timer->irq.level = 0;
>>
>> I think this could become a bit more simple if you follow the flow I
>> mentioned earlier involving kvm_timer_sync(). Also, I only see how you
>> flag the line as being high, but not how you lower it. Care to explain
>> that flow?
>
> We convert the level triggered timer into an edge up event (kvm exit).
> It's then up to user space to poll for the down event. Usually that will
> happen when the guest fiddles with the interrupt controller (eoi, enable
> line, etc).
Do you do this by inspecting the timer state? Or do you rely on the exit
itself to communicate the timer status to userspace?
> Can you think of a different option? We could maybe verify 2 kvm_run
> elements on guest entry and just see if they're identical: user space
> "active" line and arch timer "IMASK" bit. If not, exit to user space and
> let it update its state?
>
> That should at least get rid of any chance for spurious interrupts.
My gut feeling is that any exit should communicate the state of the
timer interrupt, just like we do it for the in-kernel implementation.
Userspace should "see" the line state, whatever happens.
>
>>
>>> +
>>> + ret = 0;
>>> + }
>>> +
>>> WARN_ON(ret);
>>> }
>>>
>>> @@ -197,7 +226,8 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
>>> * because the guest would never see the interrupt. Instead wait
>>> * until we call this function from kvm_timer_flush_hwstate.
>>> */
>>> - if (!vgic_initialized(vcpu->kvm) || !timer->enabled)
>>> + if ((irqchip_in_kernel(vcpu->kvm) && !vgic_initialized(vcpu->kvm)) ||
>>> + !timer->enabled)
>>> return -ENODEV;
>>>
>>> if (kvm_timer_should_fire(vcpu) != timer->irq.level)
>>> @@ -275,35 +305,57 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>>> * to ensure that hardware interrupts from the timer triggers a guest
>>> * exit.
>>> */
>>> - phys_active = timer->irq.level ||
>>> - kvm_vgic_map_is_active(vcpu, timer->irq.irq);
>>> -
>>> - /*
>>> - * We want to avoid hitting the (re)distributor as much as
>>> - * possible, as this is a potentially expensive MMIO access
>>> - * (not to mention locks in the irq layer), and a solution for
>>> - * this is to cache the "active" state in memory.
>>> - *
>>> - * Things to consider: we cannot cache an "active set" state,
>>> - * because the HW can change this behind our back (it becomes
>>> - * "clear" in the HW). We must then restrict the caching to
>>> - * the "clear" state.
>>> - *
>>> - * The cache is invalidated on:
>>> - * - vcpu put, indicating that the HW cannot be trusted to be
>>> - * in a sane state on the next vcpu load,
>>> - * - any change in the interrupt state
>>> - *
>>> - * Usage conditions:
>>> - * - cached value is "active clear"
>>> - * - value to be programmed is "active clear"
>>> - */
>>> - if (timer->active_cleared_last && !phys_active)
>>> - return;
>>> + if (irqchip_in_kernel(vcpu->kvm) && vgic_initialized(vcpu->kvm)) {
>>> + phys_active = timer->irq.level ||
>>> + kvm_vgic_map_is_active(vcpu, timer->irq.irq);
>>> +
>>> + /*
>>> + * We want to avoid hitting the (re)distributor as much as
>>> + * possible, as this is a potentially expensive MMIO access
>>> + * (not to mention locks in the irq layer), and a solution for
>>> + * this is to cache the "active" state in memory.
>>> + *
>>> + * Things to consider: we cannot cache an "active set" state,
>>> + * because the HW can change this behind our back (it becomes
>>> + * "clear" in the HW). We must then restrict the caching to
>>> + * the "clear" state.
>>> + *
>>> + * The cache is invalidated on:
>>> + * - vcpu put, indicating that the HW cannot be trusted to be
>>> + * in a sane state on the next vcpu load,
>>> + * - any change in the interrupt state
>>> + *
>>> + * Usage conditions:
>>> + * - cached value is "active clear"
>>> + * - value to be programmed is "active clear"
>>> + */
>>> + if (timer->active_cleared_last && !phys_active)
>>> + return;
>>> +
>>> + ret = irq_set_irqchip_state(host_vtimer_irq,
>>> + IRQCHIP_STATE_ACTIVE,
>>> + phys_active);
>>> + } else {
>>> + /* User space tells us whether the timer is in active mode */
>>> + phys_active = vcpu->run->request_interrupt_window &
>>> + KVM_IRQWINDOW_VTIMER;
>>
>> No, this just says "mask the interrupt". It doesn't say anything about
>> the state of the timer. More importantly: you sample the shared page.
>> What guarantees that the information there is preserved? Is userspace
>> writing that bit each time the vcpu thread re-enters the kernel with
>> this interrupt being in flight?
>
> The bit is owned by user space, so it has to guarantee that it stays.
> It's the way the kvm_run struct rolls :).
>
>>
>>> +
>>> + /* However if the line is high, we exit anyway, so we want
>>> + * to keep the IRQ masked */
>>> + phys_active = phys_active || timer->irq.level;
>>
>> Why would you force the interrupt to be masked as soon as the timer is
>> firing? If userspace hasn't masked it, I don't think you should paper
>> over it.
>
> This is to make sure that when we hit the second call to
> kvm_timer_sync_hwstate() in the exit path, we ignore it. We always set
> timer->irq.level = 0 in kvm_timer_update_irq.
I think you got the wrong end of the stick. There is only one call to
sync_hwstate per run (either because you've just exited the guest, or
because you have a something pending and we can't enter the guest).
>
>>
>>> +
>>> + /*
>>> + * So we can just explicitly mask or unmask the IRQ, gaining
>>> + * more compatibility with oddball irq controllers.
>>> + */
>>> + if (phys_active)
>>> + disable_percpu_irq(host_vtimer_irq);
>>> + else
>>> + enable_percpu_irq(host_vtimer_irq, 0);
>>
>> Since you are now targeting random irqchips (as opposed to a GIC
>> specifically), what guarantees that the timer is a per-cpu IRQ?
>
> This is the host interrupt controller - and we're already using percpu
> irqs on it :). Also as it happens the RPi has them percpu (anything else
> wouldn't make sense...).
Not really. The RPi is faking percpu interrupts just to have some level
of compatibility with the host arch timer driver. But nonetheless, if
you're opening the code to something else than a GIC, then you should
check that the interrupt you're getting is percpu.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-20 9:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E0FF95.7040305@arm.com>
On 20.09.16 11:21, Marc Zyngier wrote:
> On 19/09/16 18:39, Alexander Graf wrote:
>>
>>
>> On 19.09.16 16:48, Marc Zyngier wrote:
>>> On 19/09/16 12:14, Alexander Graf wrote:
>>>> We have 2 modes for dealing with interrupts in the ARM world. We can either
>>>> handle them all using hardware acceleration through the vgic or we can emulate
>>>> a gic in user space and only drive CPU IRQ pins from there.
>>>>
>>>> Unfortunately, when driving IRQs from user space, we never tell user space
>>>> about timer events that may result in interrupt line state changes, so we
>>>> lose out on timer events if we run with user space gic emulation.
>>>>
>>>> This patch fixes that by routing vtimer expiration events to user space.
>>>> With this patch I can successfully run edk2 and Linux with user space gic
>>>> emulation.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>>
>>>> - Add back curly brace that got lost
>>>>
>>>> v2 -> v3:
>>>>
>>>> - Split into patch set
>>>>
>>>> v3 -> v4:
>>>>
>>>> - Improve documentation
>>>> ---
>>>> Documentation/virtual/kvm/api.txt | 30 ++++++++-
>>>> arch/arm/include/asm/kvm_host.h | 3 +
>>>> arch/arm/kvm/arm.c | 22 ++++---
>>>> arch/arm64/include/asm/kvm_host.h | 3 +
>>>> include/uapi/linux/kvm.h | 14 +++++
>>>> virt/kvm/arm/arch_timer.c | 125 +++++++++++++++++++++++++++-----------
>>>> 6 files changed, 155 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>>> index 23937e0..1c0bd86 100644
>>>> --- a/Documentation/virtual/kvm/api.txt
>>>> +++ b/Documentation/virtual/kvm/api.txt
>>>> @@ -3202,9 +3202,14 @@ struct kvm_run {
>>>> /* in */
>>>> __u8 request_interrupt_window;
>>>>
>>>> -Request that KVM_RUN return when it becomes possible to inject external
>>>> +[x86] Request that KVM_RUN return when it becomes possible to inject external
>>>> interrupts into the guest. Useful in conjunction with KVM_INTERRUPT.
>>>>
>>>> +[arm*] Bits set to 1 in here mask IRQ lines that would otherwise potentially
>>>> +trigger forever. These lines are available:
>>>> +
>>>> + KVM_IRQWINDOW_VTIMER - Masks hw virtual timer irq while in guest
>>>> +
>>>> __u8 padding1[7];
>>>>
>>>> /* out */
>>>> @@ -3519,6 +3524,18 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
>>>> event/message pages and to enable/disable SynIC messages/events processing
>>>> in userspace.
>>>>
>>>> + /* KVM_EXIT_ARM_TIMER */
>>>> + struct {
>>>> + __u8 timesource;
>>>> + } arm_timer;
>>>> +
>>>> +Indicates that a timer triggered that user space needs to handle and
>>>> +potentially mask with vcpu->run->request_interrupt_window to allow the
>>>> +guest to proceed. This only happens for timers that got enabled through
>>>> +KVM_CAP_ARM_TIMER. The following time sources are available:
>>>> +
>>>> + KVM_ARM_TIMER_VTIMER - virtual cpu timer
>>>> +
>>>> /* Fix the size of the union. */
>>>> char padding[256];
>>>> };
>>>> @@ -3739,6 +3756,17 @@ Once this is done the KVM_REG_MIPS_VEC_* and KVM_REG_MIPS_MSA_* registers can be
>>>> accessed, and the Config5.MSAEn bit is accessible via the KVM API and also from
>>>> the guest.
>>>>
>>>> +6.11 KVM_CAP_ARM_TIMER
>>>> +
>>>> +Architectures: arm, arm64
>>>> +Target: vcpu
>>>> +Parameters: args[0] contains a bitmap of timers to select (see 5.)
>>>> +
>>>> +This capability allows to route per-core timers into user space. When it's
>>>> +enabled and no in-kernel interrupt controller is in use, the timers selected
>>>> +by args[0] trigger KVM_EXIT_ARM_TIMER guest exits when they are pending,
>>>> +unless masked by vcpu->run->request_interrupt_window (see 5.).
>>>> +
>>>> 7. Capabilities that can be enabled on VMs
>>>> ------------------------------------------
>>>>
>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>> index de338d9..77d1f73 100644
>>>> --- a/arch/arm/include/asm/kvm_host.h
>>>> +++ b/arch/arm/include/asm/kvm_host.h
>>>> @@ -180,6 +180,9 @@ struct kvm_vcpu_arch {
>>>>
>>>> /* Detect first run of a vcpu */
>>>> bool has_run_once;
>>>> +
>>>> + /* User space wants timer notifications */
>>>> + bool user_space_arm_timers;
>>>
>>> Please move this to the timer structure.
>>
>> Sure.
>>
>>>
>>>> };
>>>>
>>>> struct kvm_vm_stat {
>>>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>>>> index c84b6ad..57bdb71 100644
>>>> --- a/arch/arm/kvm/arm.c
>>>> +++ b/arch/arm/kvm/arm.c
>>>> @@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>> case KVM_CAP_ARM_PSCI_0_2:
>>>> case KVM_CAP_READONLY_MEM:
>>>> case KVM_CAP_MP_STATE:
>>>> + case KVM_CAP_ARM_TIMER:
>>>> r = 1;
>>>> break;
>>>> case KVM_CAP_COALESCED_MMIO:
>>>> @@ -474,13 +475,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>>> return ret;
>>>> }
>>>>
>>>> - /*
>>>> - * Enable the arch timers only if we have an in-kernel VGIC
>>>> - * and it has been properly initialized, since we cannot handle
>>>> - * interrupts from the virtual timer with a userspace gic.
>>>> - */
>>>> - if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
>>>> - ret = kvm_timer_enable(vcpu);
>>>> + ret = kvm_timer_enable(vcpu);
>>>>
>>>> return ret;
>>>> }
>>>> @@ -601,6 +596,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>> run->exit_reason = KVM_EXIT_INTR;
>>>> }
>>>>
>>>> + if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>>>
>>> Since this is a very unlikely event (in the grand scheme of things), how
>>> about making this unlikely()?
>>>
>>>> + /* Tell user space about the pending vtimer */
>>>> + ret = 0;
>>>> + run->exit_reason = KVM_EXIT_ARM_TIMER;
>>>> + run->arm_timer.timesource = KVM_ARM_TIMER_VTIMER;
>>>> + }
>>>
>>> More importantly: why does it have to be indirected by a
>>> make_request/check_request, and not be handled as part of the
>>> kvm_timer_sync() call? We do update the state there, and you could
>>> directly find out whether an exit is required.
>>
>> I can try - it seemed like it could easily become quite racy because we
>> call kvm_timer_sync_hwstate() at multiple places.
>
> It shouldn't. We only do it at exactly two locations (depending whether
> we've entered the guest or not).
>
> Also, take the following scenario:
> (1) guest programs the timer to expire at time T
> (2) guest performs an MMIO access which traps
> (3) during the world switch, the timer expires and we mark the timer
> interrupt as pending
> (4) we exit to handle the MMIO, no sign of the timer being pending
>
> Is the timer event lost? Or simply delayed? I think this indicates that
> the timer state should always be signalled to userspace, no matter what
> the exit reason is.
That's basically what I'm trying to get running right now, yes. I pushed
the interrupt pending status field into the kvm_sync_regs struct and
check it on every exit in user space - to make sure we catch pending
state changes before mmio reads.
On top of that we also need a force exit event when the state changes,
in case there's no other event pending. Furthermore we probably want to
indicate the user space status of the pending bit into the kernel to not
exit too often.
Something doesn't quite work with that approach yet though - I can't get
edk2 to roll yet.
Alex
^ permalink raw reply
* [PATCH v9 17/19] drm/virtio: kconfig: Fix recursive dependency issue.
From: Jani Nikula @ 2016-09-20 9:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160920083251.GB26093@griffinp-ThinkPad-X1-Carbon-2nd>
On Tue, 20 Sep 2016, Peter Griffin <peter.griffin@linaro.org> wrote:
> Hi Emil,
>
> On Tue, 20 Sep 2016, Emil Velikov wrote:
>
>> On 5 September 2016 at 14:16, Peter Griffin <peter.griffin@linaro.org> wrote:
>> > ST_SLIM_REMOTEPROC must select REMOTEPROC, which exposes the following
>> > recursive dependency.
>
>
>> >
>> From a humble skim through remoteproc, drm and a few other subsystems
>> I think the above is wrong. All the drivers (outside of remoteproc),
>> that I've seen, depend on the core component, they don't select it.
>
> I will let Bjorn comment on the remoteproc subsystem Kconfig design, and
> why it is like it is.
>
> For this particular SLIM_RPROC I have added it to Kconfig in keeping with all
> the other drivers in the remoteproc subsystem which has exposed this recursive
> dependency issue.
>
> For this particular kconfig symbol a quick grep reveals more drivers in
> the kernel using 'select', than 'depend on'
>
> git grep "select VIRTIO" | wc -l
> 14
>
> git grep "depends on VIRTIO" | wc -l
> 10
>
>
>> Furthermore most places explicitly hide the drivers from the menu if
>> the core component isn't enabled.
>
> Remoteproc subsystem takes a different approach, the core code is only enabled
> if a driver which relies on it is enabled. This IMHO makes sense, as
> remoteproc is not widely used (only a few particular ARM SoC's).
>
> It is true that for subsystems which rely on the core component being
> explicitly enabled, they often tend to hide drivers which depend on it
> from the menu unless it is. This also makes sense.
>
>>
>> Is there something that requires such a different/unusual behaviour in
>> remoteproc ?
>>
>
> I'm not sure it is that unusual...looking at config USB, it selects USB_COMMON in
> mfd subsystem, client drivers select MFD_CORE.
>
> I've added Arnd to this thread, as I've seen lots of Kconfig patches from him
> recently, maybe he has some thoughts on whether this is the correct fix or not?
Documentation/kbuild/kconfig-language.txt:
Note:
select should be used with care. select will force
a symbol to a value without visiting the dependencies.
By abusing select you are able to select a symbol FOO even
if FOO depends on BAR that is not set.
In general use select only for non-visible symbols
(no prompts anywhere) and for symbols with no dependencies.
That will limit the usefulness but on the other hand avoid
the illegal configurations all over.
People tend to abuse select because it's "convenient". If you depend,
but some of your dependencies aren't met, you're in for some digging
through Kconfig to find the missing deps. Just to make the option you
want visible in menuconfig. If you instead select something with
dependencies, it'll be right most of the time, and it's "convenient",
until it breaks. (And hey, it usually breaks for someone else with some
other config, so it's still convenient for you.)
Perhaps kconfig should complain about selecting visible symbols and
symbols with dependencies.
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply
* [PATCH v5 3/5] arm64: arch_timer: Work around QorIQ Erratum A-008585
From: Mark Rutland @ 2016-09-20 9:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474312560.4283.10.camel@buserror.net>
On Mon, Sep 19, 2016 at 02:16:00PM -0500, Scott Wood wrote:
> On Mon, 2016-09-19 at 18:07 +0100, Mark Rutland wrote:
> > > > Reconsidering my suggestion, I realise this will also affect the MMIO
> > > > timers, so that doesn't work.
> > > >
> > > > So for the moment, I guess we have to keep fsl_a008585_set_next_event().
> > > What is the problem with MMIO timers? ?needs_fsl_a008585_workaround()
> > > should
> > > always be false for them.
> > As suggested, needs_fsl_a008585_workaround() takes no parameter, and
> > set_next_event is called for both cp15/sysreg and MMIO timers. So it
> > would either be true for all, or false for all.
> >
> > If it's true for all, we'd end up calling fsl_a008585_set_next_event()
> > for the MMIO timers too.
>
> There should not be any MMIO timers on a system where
> fsl_a008585_set_next_event() returns true.
I'm generally not keen on relying on that.
For reference, are no MMIO timers implemented at all, or are they simply
not listed in the DT today?
Thanks,
Mark.
^ permalink raw reply
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Marc Zyngier @ 2016-09-20 9:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <580a8a26-ca8f-a08a-a9da-d19d1a595cb6@suse.de>
On 20/09/16 10:26, Alexander Graf wrote:
>
>
> On 20.09.16 11:21, Marc Zyngier wrote:
>> On 19/09/16 18:39, Alexander Graf wrote:
>>>
>>>
>>> On 19.09.16 16:48, Marc Zyngier wrote:
>>>> On 19/09/16 12:14, Alexander Graf wrote:
>>>>> We have 2 modes for dealing with interrupts in the ARM world. We can either
>>>>> handle them all using hardware acceleration through the vgic or we can emulate
>>>>> a gic in user space and only drive CPU IRQ pins from there.
>>>>>
>>>>> Unfortunately, when driving IRQs from user space, we never tell user space
>>>>> about timer events that may result in interrupt line state changes, so we
>>>>> lose out on timer events if we run with user space gic emulation.
>>>>>
>>>>> This patch fixes that by routing vtimer expiration events to user space.
>>>>> With this patch I can successfully run edk2 and Linux with user space gic
>>>>> emulation.
>>>>>
>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>
>>>>> ---
>>>>>
>>>>> v1 -> v2:
>>>>>
>>>>> - Add back curly brace that got lost
>>>>>
>>>>> v2 -> v3:
>>>>>
>>>>> - Split into patch set
>>>>>
>>>>> v3 -> v4:
>>>>>
>>>>> - Improve documentation
>>>>> ---
>>>>> Documentation/virtual/kvm/api.txt | 30 ++++++++-
>>>>> arch/arm/include/asm/kvm_host.h | 3 +
>>>>> arch/arm/kvm/arm.c | 22 ++++---
>>>>> arch/arm64/include/asm/kvm_host.h | 3 +
>>>>> include/uapi/linux/kvm.h | 14 +++++
>>>>> virt/kvm/arm/arch_timer.c | 125 +++++++++++++++++++++++++++-----------
>>>>> 6 files changed, 155 insertions(+), 42 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>>>> index 23937e0..1c0bd86 100644
>>>>> --- a/Documentation/virtual/kvm/api.txt
>>>>> +++ b/Documentation/virtual/kvm/api.txt
>>>>> @@ -3202,9 +3202,14 @@ struct kvm_run {
>>>>> /* in */
>>>>> __u8 request_interrupt_window;
>>>>>
>>>>> -Request that KVM_RUN return when it becomes possible to inject external
>>>>> +[x86] Request that KVM_RUN return when it becomes possible to inject external
>>>>> interrupts into the guest. Useful in conjunction with KVM_INTERRUPT.
>>>>>
>>>>> +[arm*] Bits set to 1 in here mask IRQ lines that would otherwise potentially
>>>>> +trigger forever. These lines are available:
>>>>> +
>>>>> + KVM_IRQWINDOW_VTIMER - Masks hw virtual timer irq while in guest
>>>>> +
>>>>> __u8 padding1[7];
>>>>>
>>>>> /* out */
>>>>> @@ -3519,6 +3524,18 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
>>>>> event/message pages and to enable/disable SynIC messages/events processing
>>>>> in userspace.
>>>>>
>>>>> + /* KVM_EXIT_ARM_TIMER */
>>>>> + struct {
>>>>> + __u8 timesource;
>>>>> + } arm_timer;
>>>>> +
>>>>> +Indicates that a timer triggered that user space needs to handle and
>>>>> +potentially mask with vcpu->run->request_interrupt_window to allow the
>>>>> +guest to proceed. This only happens for timers that got enabled through
>>>>> +KVM_CAP_ARM_TIMER. The following time sources are available:
>>>>> +
>>>>> + KVM_ARM_TIMER_VTIMER - virtual cpu timer
>>>>> +
>>>>> /* Fix the size of the union. */
>>>>> char padding[256];
>>>>> };
>>>>> @@ -3739,6 +3756,17 @@ Once this is done the KVM_REG_MIPS_VEC_* and KVM_REG_MIPS_MSA_* registers can be
>>>>> accessed, and the Config5.MSAEn bit is accessible via the KVM API and also from
>>>>> the guest.
>>>>>
>>>>> +6.11 KVM_CAP_ARM_TIMER
>>>>> +
>>>>> +Architectures: arm, arm64
>>>>> +Target: vcpu
>>>>> +Parameters: args[0] contains a bitmap of timers to select (see 5.)
>>>>> +
>>>>> +This capability allows to route per-core timers into user space. When it's
>>>>> +enabled and no in-kernel interrupt controller is in use, the timers selected
>>>>> +by args[0] trigger KVM_EXIT_ARM_TIMER guest exits when they are pending,
>>>>> +unless masked by vcpu->run->request_interrupt_window (see 5.).
>>>>> +
>>>>> 7. Capabilities that can be enabled on VMs
>>>>> ------------------------------------------
>>>>>
>>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>>> index de338d9..77d1f73 100644
>>>>> --- a/arch/arm/include/asm/kvm_host.h
>>>>> +++ b/arch/arm/include/asm/kvm_host.h
>>>>> @@ -180,6 +180,9 @@ struct kvm_vcpu_arch {
>>>>>
>>>>> /* Detect first run of a vcpu */
>>>>> bool has_run_once;
>>>>> +
>>>>> + /* User space wants timer notifications */
>>>>> + bool user_space_arm_timers;
>>>>
>>>> Please move this to the timer structure.
>>>
>>> Sure.
>>>
>>>>
>>>>> };
>>>>>
>>>>> struct kvm_vm_stat {
>>>>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>>>>> index c84b6ad..57bdb71 100644
>>>>> --- a/arch/arm/kvm/arm.c
>>>>> +++ b/arch/arm/kvm/arm.c
>>>>> @@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>>> case KVM_CAP_ARM_PSCI_0_2:
>>>>> case KVM_CAP_READONLY_MEM:
>>>>> case KVM_CAP_MP_STATE:
>>>>> + case KVM_CAP_ARM_TIMER:
>>>>> r = 1;
>>>>> break;
>>>>> case KVM_CAP_COALESCED_MMIO:
>>>>> @@ -474,13 +475,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>>>> return ret;
>>>>> }
>>>>>
>>>>> - /*
>>>>> - * Enable the arch timers only if we have an in-kernel VGIC
>>>>> - * and it has been properly initialized, since we cannot handle
>>>>> - * interrupts from the virtual timer with a userspace gic.
>>>>> - */
>>>>> - if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
>>>>> - ret = kvm_timer_enable(vcpu);
>>>>> + ret = kvm_timer_enable(vcpu);
>>>>>
>>>>> return ret;
>>>>> }
>>>>> @@ -601,6 +596,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>> run->exit_reason = KVM_EXIT_INTR;
>>>>> }
>>>>>
>>>>> + if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>>>>
>>>> Since this is a very unlikely event (in the grand scheme of things), how
>>>> about making this unlikely()?
>>>>
>>>>> + /* Tell user space about the pending vtimer */
>>>>> + ret = 0;
>>>>> + run->exit_reason = KVM_EXIT_ARM_TIMER;
>>>>> + run->arm_timer.timesource = KVM_ARM_TIMER_VTIMER;
>>>>> + }
>>>>
>>>> More importantly: why does it have to be indirected by a
>>>> make_request/check_request, and not be handled as part of the
>>>> kvm_timer_sync() call? We do update the state there, and you could
>>>> directly find out whether an exit is required.
>>>
>>> I can try - it seemed like it could easily become quite racy because we
>>> call kvm_timer_sync_hwstate() at multiple places.
>>
>> It shouldn't. We only do it at exactly two locations (depending whether
>> we've entered the guest or not).
>>
>> Also, take the following scenario:
>> (1) guest programs the timer to expire at time T
>> (2) guest performs an MMIO access which traps
>> (3) during the world switch, the timer expires and we mark the timer
>> interrupt as pending
>> (4) we exit to handle the MMIO, no sign of the timer being pending
>>
>> Is the timer event lost? Or simply delayed? I think this indicates that
>> the timer state should always be signalled to userspace, no matter what
>> the exit reason is.
>
> That's basically what I'm trying to get running right now, yes. I pushed
> the interrupt pending status field into the kvm_sync_regs struct and
> check it on every exit in user space - to make sure we catch pending
> state changes before mmio reads.
>
> On top of that we also need a force exit event when the state changes,
> in case there's no other event pending. Furthermore we probably want to
> indicate the user space status of the pending bit into the kernel to not
> exit too often.
All you need is to do is to stash the line state in the run structure.
You shouldn't need any other information. And you trigger the exit on
"timer line high + timer line unmasked" in order to inject the
interrupt. Which is basically what the vgic does. It would greatly help
if you adopted a similar behaviour.
> Something doesn't quite work with that approach yet though - I can't get
> edk2 to roll yet.
Oh well...
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [GIT PULL] ARM: mvebu: soc for v4.9 (#1)
From: Arnd Bergmann @ 2016-09-20 9:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87shsv6ni8.fsf@free-electrons.com>
On Tuesday, September 20, 2016 9:25:51 AM CEST Gregory CLEMENT wrote:
> Hi Arnd,
>
> On lun., sept. 19 2016, Arnd Bergmann <arnd@arndb.de> wrote:
>
> > On Monday, September 19, 2016 11:46:22 PM CEST Arnd Bergmann wrote:
> >> On Wednesday, September 14, 2016 5:34:37 PM CEST Gregory CLEMENT wrote:
> >> > mvebu soc for 4.9 (part 1)
> >> >
> >> > - irq cleanup for old mvebu SoC
> >> > - Convert orion5x based SoC Netgear WNR854T to devicetree
> >> >
> >>
> >> Pulled into next/soc, thanks!
> >
> > Sorry, backed out again after seeing the PCI stuff on the WNR854T in
> > there. I thought the plan was to leave out PCI support from the DT
> > based machine file, and leave the old board in place, or am I
> > missing something?
>
> I might have overlooked the thread, I thouhgt the state of the patch was
> OK as is, and further changes can be done later.
>
> So it seems that it will be 4.10 material.
>
We have gained a little more time since Linus delayed the merge window
by another week, so I think there is still a chance to respin this.
I also really want the NO_IRQ changes to get merged ;-)
Just drop the patch removing wnr854t-setup.c, and add another patch
on top to remove the PCI initialization from wrt350n-v2-setup.c,
and I'll take it.
Arnd
^ permalink raw reply
* [PATCH v5 2/9] drivers: irqchip: Add STM32 external interrupts support
From: Alexandre Torgue @ 2016-09-20 9:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f64e4f9d-d811-2c6e-7c5a-d4ab06716e2c@st.com>
Hi Thomas,
On 09/14/2016 03:44 PM, Alexandre Torgue wrote:
>
>
> On 09/14/2016 03:34 PM, Thomas Gleixner wrote:
>> On Wed, 14 Sep 2016, Alexandre Torgue wrote:
>>> On 09/14/2016 11:19 AM, Thomas Gleixner wrote:
>>>>
>>>> Now what really bugs me is that you do that at all. An interrupt
>>>> which is
>>>> freed must be masked already. Why is it unmasked in the first place?
>>>
>>> Honestly I don't know. When "devm_free_irq" is called to release
>>> virq, there
>>> is no issue and interrupt is well masked. But, when I tried to use
>>> "irq_dispose_mapping(virq)" I observed that .free is called (child
>>> and parent
>>> domain) but interrupt is not masked.
>>
>> Well, you just used some function in some context which is not
>> relevant to
>> the normal operation. So adding that mask() is just paranoia for no
>> value.
>
A gentle reminder ping...
If ".free" callback is not relevant then I 'll remove it from exti domain.
> I agree. I just wanted to "force" a test for .free callback. If it not
> relevant I'll remove ".free" callback of exti domain.
> As a part of this series has already been taken by Linus (pinctrl part),
> I will send a new series only for irqchip part (patches [1] and [2]). Do
> you agree ?
>
Thanks in advance
Alex
> Thanks
> Alex
>
>
>>
>> Thanks,
>>
>> tglx
>>
>
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v6 4/7] arm/arm64: vgic-new: Define required GICv3 reg definitions
From: Marc Zyngier @ 2016-09-20 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474351965-11586-5-git-send-email-vijay.kilari@gmail.com>
On 20/09/16 07:12, vijay.kilari at gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Define register definitions for ICH_VMCR_EL2, ICC_CTLR_EL1 and
> ICH_VTR_EL2, ICC_BPR0_EL1, ICC_BPR1_EL1 registers.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
$SUBJECT should report "irqchip/gic-v3: Add missing system register
definitions", or something similar.
Also, please drop this "vgic-new", as there is no vgic-old anymore.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH v5 2/9] drivers: irqchip: Add STM32 external interrupts support
From: Thomas Gleixner @ 2016-09-20 9:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <39c4ceee-7e05-ebfd-2ea0-3c4e1c4ea619@st.com>
On Tue, 20 Sep 2016, Alexandre Torgue wrote:
> > On 09/14/2016 03:34 PM, Thomas Gleixner wrote:
> > > Well, you just used some function in some context which is not
> > > relevant to
> > > the normal operation. So adding that mask() is just paranoia for no
> > > value.
> >
> A gentle reminder ping...
> If ".free" callback is not relevant then I 'll remove it from exti domain.
I was not talking about the .free callback in general. I was talking about
the masking. But yes, if the thing is otherwise a NOOP, then you can spare
it completely.
Thanks,
tglx
^ permalink raw reply
* [PATCH] arm64, numa: Add cpu_to_node() implementation.
From: Yisheng Xie @ 2016-09-20 9:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474310970-21264-1-git-send-email-ddaney.cavm@gmail.com>
On 2016/9/20 2:49, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> The wq_numa_init() function makes a private CPU to node map by calling
> cpu_to_node() early in the boot process, before the non-boot CPUs are
> brought online. Since the default implementation of cpu_to_node()
> returns zero for CPUs that have never been brought online, the
> workqueue system's view is that *all* CPUs are on node zero.
>
> When the unbound workqueue for a non-zero node is created, the
> tsk_cpus_allowed() for the worker threads is the empty set because
> there are, in the view of the workqueue system, no CPUs on non-zero
> nodes. The code in try_to_wake_up() using this empty cpumask ends up
> using the cpumask empty set value of NR_CPUS as an index into the
> per-CPU area pointer array, and gets garbage as it is one past the end
> of the array. This results in:
>
> [ 0.881970] Unable to handle kernel paging request at virtual address fffffb1008b926a4
> [ 1.970095] pgd = fffffc00094b0000
> [ 1.973530] [fffffb1008b926a4] *pgd=0000000000000000, *pud=0000000000000000, *pmd=0000000000000000
> [ 1.982610] Internal error: Oops: 96000004 [#1] SMP
> [ 1.987541] Modules linked in:
> [ 1.990631] CPU: 48 PID: 295 Comm: cpuhp/48 Tainted: G W 4.8.0-rc6-preempt-vol+ #9
> [ 1.999435] Hardware name: Cavium ThunderX CN88XX board (DT)
> [ 2.005159] task: fffffe0fe89cc300 task.stack: fffffe0fe8b8c000
> [ 2.011158] PC is at try_to_wake_up+0x194/0x34c
> [ 2.015737] LR is at try_to_wake_up+0x150/0x34c
> [ 2.020318] pc : [<fffffc00080e7468>] lr : [<fffffc00080e7424>] pstate: 600000c5
> [ 2.027803] sp : fffffe0fe8b8fb10
> [ 2.031149] x29: fffffe0fe8b8fb10 x28: 0000000000000000
> [ 2.036522] x27: fffffc0008c63bc8 x26: 0000000000001000
> [ 2.041896] x25: fffffc0008c63c80 x24: fffffc0008bfb200
> [ 2.047270] x23: 00000000000000c0 x22: 0000000000000004
> [ 2.052642] x21: fffffe0fe89d25bc x20: 0000000000001000
> [ 2.058014] x19: fffffe0fe89d1d00 x18: 0000000000000000
> [ 2.063386] x17: 0000000000000000 x16: 0000000000000000
> [ 2.068760] x15: 0000000000000018 x14: 0000000000000000
> [ 2.074133] x13: 0000000000000000 x12: 0000000000000000
> [ 2.079505] x11: 0000000000000000 x10: 0000000000000000
> [ 2.084879] x9 : 0000000000000000 x8 : 0000000000000000
> [ 2.090251] x7 : 0000000000000040 x6 : 0000000000000000
> [ 2.095621] x5 : ffffffffffffffff x4 : 0000000000000000
> [ 2.100991] x3 : 0000000000000000 x2 : 0000000000000000
> [ 2.106364] x1 : fffffc0008be4c24 x0 : ffffff0ffffada80
> [ 2.111737]
> [ 2.113236] Process cpuhp/48 (pid: 295, stack limit = 0xfffffe0fe8b8c020)
> [ 2.120102] Stack: (0xfffffe0fe8b8fb10 to 0xfffffe0fe8b90000)
> [ 2.125914] fb00: fffffe0fe8b8fb80 fffffc00080e7648
> .
> .
> .
> [ 2.442859] Call trace:
> [ 2.445327] Exception stack(0xfffffe0fe8b8f940 to 0xfffffe0fe8b8fa70)
> [ 2.451843] f940: fffffe0fe89d1d00 0000040000000000 fffffe0fe8b8fb10 fffffc00080e7468
> [ 2.459767] f960: fffffe0fe8b8f980 fffffc00080e4958 ffffff0ff91ab200 fffffc00080e4b64
> [ 2.467690] f980: fffffe0fe8b8f9d0 fffffc00080e515c fffffe0fe8b8fa80 0000000000000000
> [ 2.475614] f9a0: fffffe0fe8b8f9d0 fffffc00080e58e4 fffffe0fe8b8fa80 0000000000000000
> [ 2.483540] f9c0: fffffe0fe8d10000 0000000000000040 fffffe0fe8b8fa50 fffffc00080e5ac4
> [ 2.491465] f9e0: ffffff0ffffada80 fffffc0008be4c24 0000000000000000 0000000000000000
> [ 2.499387] fa00: 0000000000000000 ffffffffffffffff 0000000000000000 0000000000000040
> [ 2.507309] fa20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 2.515233] fa40: 0000000000000000 0000000000000000 0000000000000000 0000000000000018
> [ 2.523156] fa60: 0000000000000000 0000000000000000
> [ 2.528089] [<fffffc00080e7468>] try_to_wake_up+0x194/0x34c
> [ 2.533723] [<fffffc00080e7648>] wake_up_process+0x28/0x34
> [ 2.539275] [<fffffc00080d3764>] create_worker+0x110/0x19c
> [ 2.544824] [<fffffc00080d69dc>] alloc_unbound_pwq+0x3cc/0x4b0
> [ 2.550724] [<fffffc00080d6bcc>] wq_update_unbound_numa+0x10c/0x1e4
> [ 2.557066] [<fffffc00080d7d78>] workqueue_online_cpu+0x220/0x28c
> [ 2.563234] [<fffffc00080bd288>] cpuhp_invoke_callback+0x6c/0x168
> [ 2.569398] [<fffffc00080bdf74>] cpuhp_up_callbacks+0x44/0xe4
> [ 2.575210] [<fffffc00080be194>] cpuhp_thread_fun+0x13c/0x148
> [ 2.581027] [<fffffc00080dfbac>] smpboot_thread_fn+0x19c/0x1a8
> [ 2.586929] [<fffffc00080dbd64>] kthread+0xdc/0xf0
> [ 2.591776] [<fffffc0008083380>] ret_from_fork+0x10/0x50
> [ 2.597147] Code: b00057e1 91304021 91005021 b8626822 (b8606821)
> [ 2.603464] ---[ end trace 58c0cd36b88802bc ]---
> [ 2.608138] Kernel panic - not syncing: Fatal exception
>
> Fix by supplying a cpu_to_node() implementation that returns correct
> node mappings.
>
> Cc: <stable@vger.kernel.org> # 4.7.x-
> Signed-off-by: David Daney <david.daney@cavium.com>
>
Tested-by: Yisheng Xie <xieyisheng1@huawei.com>
> ---
> arch/arm64/include/asm/topology.h | 3 +++
> arch/arm64/mm/numa.c | 18 ++++++++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index 8b57339..8d935447 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -30,6 +30,9 @@ int pcibus_to_node(struct pci_bus *bus);
> cpu_all_mask : \
> cpumask_of_node(pcibus_to_node(bus)))
>
> +int cpu_to_node(int cpu);
> +#define cpu_to_node cpu_to_node
> +
> #endif /* CONFIG_NUMA */
>
> #include <asm-generic/topology.h>
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index 5bb15ea..e76281b 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -130,6 +130,24 @@ void __init early_map_cpu_to_node(unsigned int cpu, int nid)
> cpu_to_node_map[cpu] = nid;
> }
>
> +int cpu_to_node(int cpu)
> +{
> + int nid;
> +
> + /*
> + * Return 0 for unknown mapping so that we report something
> + * sensible if firmware doesn't supply a proper mapping.
> + */
> + if (cpu < 0 || cpu >= NR_CPUS)
> + return 0;
> +
> + nid = cpu_to_node_map[cpu];
> + if (nid == NUMA_NO_NODE)
> + nid = 0;
> + return nid;
> +}
> +EXPORT_SYMBOL(cpu_to_node);
> +
> /**
> * numa_add_memblk - Set node id to memblk
> * @nid: NUMA node ID of the new memblk
>
^ permalink raw reply
* [PATCH] arm64: kgdb: handle read-only text / modules
From: AKASHI Takahiro @ 2016-09-20 10:03 UTC (permalink / raw)
To: linux-arm-kernel
Handle read-only cases (CONFIG_DEBUG_RODATA/CONFIG_DEBUG_SET_MODULE_RONX)
by using aarch64_insn_write() instead of probe_kernel_write().
See how this works:
commit 2f896d586610 ("arm64: use fixmap for text patching")
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Jason Wessel <jason.wessel@windriver.com>
Cc: <stable@vger.kernel.org> # 4.0-
---
arch/arm64/kernel/kgdb.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 6732a27..133cfe3 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -382,3 +382,23 @@ struct kgdb_arch arch_kgdb_ops = {
KGDB_DYN_BRK_INS_BYTE(3),
}
};
+
+int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
+{
+ int err;
+
+ BUILD_BUG_ON(AARCH64_INSN_SIZE != BREAK_INSTR_SIZE);
+
+ err = aarch64_insn_read((void *)bpt->bpt_addr, (u32 *)bpt->saved_instr);
+ if (err)
+ return err;
+
+ return aarch64_insn_write((void *)bpt->bpt_addr,
+ (u32)AARCH64_BREAK_KGDB_DYN_DBG);
+}
+
+int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
+{
+ return aarch64_insn_write((void *)bpt->bpt_addr,
+ *(u32 *)bpt->saved_instr);
+}
--
2.10.0
^ permalink raw reply related
* [PATCH v4 2/2] KVM: arm/arm64: Route vtimer events to user space
From: Alexander Graf @ 2016-09-20 10:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <57E103E8.7040506@arm.com>
On 09/20/2016 11:39 AM, Marc Zyngier wrote:
> On 20/09/16 10:26, Alexander Graf wrote:
>>
>> On 20.09.16 11:21, Marc Zyngier wrote:
>>> On 19/09/16 18:39, Alexander Graf wrote:
>>>>
>>>> On 19.09.16 16:48, Marc Zyngier wrote:
>>>>> On 19/09/16 12:14, Alexander Graf wrote:
>>>>>> We have 2 modes for dealing with interrupts in the ARM world. We can either
>>>>>> handle them all using hardware acceleration through the vgic or we can emulate
>>>>>> a gic in user space and only drive CPU IRQ pins from there.
>>>>>>
>>>>>> Unfortunately, when driving IRQs from user space, we never tell user space
>>>>>> about timer events that may result in interrupt line state changes, so we
>>>>>> lose out on timer events if we run with user space gic emulation.
>>>>>>
>>>>>> This patch fixes that by routing vtimer expiration events to user space.
>>>>>> With this patch I can successfully run edk2 and Linux with user space gic
>>>>>> emulation.
>>>>>>
>>>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> v1 -> v2:
>>>>>>
>>>>>> - Add back curly brace that got lost
>>>>>>
>>>>>> v2 -> v3:
>>>>>>
>>>>>> - Split into patch set
>>>>>>
>>>>>> v3 -> v4:
>>>>>>
>>>>>> - Improve documentation
>>>>>> ---
>>>>>> Documentation/virtual/kvm/api.txt | 30 ++++++++-
>>>>>> arch/arm/include/asm/kvm_host.h | 3 +
>>>>>> arch/arm/kvm/arm.c | 22 ++++---
>>>>>> arch/arm64/include/asm/kvm_host.h | 3 +
>>>>>> include/uapi/linux/kvm.h | 14 +++++
>>>>>> virt/kvm/arm/arch_timer.c | 125 +++++++++++++++++++++++++++-----------
>>>>>> 6 files changed, 155 insertions(+), 42 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>>>>>> index 23937e0..1c0bd86 100644
>>>>>> --- a/Documentation/virtual/kvm/api.txt
>>>>>> +++ b/Documentation/virtual/kvm/api.txt
>>>>>> @@ -3202,9 +3202,14 @@ struct kvm_run {
>>>>>> /* in */
>>>>>> __u8 request_interrupt_window;
>>>>>>
>>>>>> -Request that KVM_RUN return when it becomes possible to inject external
>>>>>> +[x86] Request that KVM_RUN return when it becomes possible to inject external
>>>>>> interrupts into the guest. Useful in conjunction with KVM_INTERRUPT.
>>>>>>
>>>>>> +[arm*] Bits set to 1 in here mask IRQ lines that would otherwise potentially
>>>>>> +trigger forever. These lines are available:
>>>>>> +
>>>>>> + KVM_IRQWINDOW_VTIMER - Masks hw virtual timer irq while in guest
>>>>>> +
>>>>>> __u8 padding1[7];
>>>>>>
>>>>>> /* out */
>>>>>> @@ -3519,6 +3524,18 @@ Hyper-V SynIC state change. Notification is used to remap SynIC
>>>>>> event/message pages and to enable/disable SynIC messages/events processing
>>>>>> in userspace.
>>>>>>
>>>>>> + /* KVM_EXIT_ARM_TIMER */
>>>>>> + struct {
>>>>>> + __u8 timesource;
>>>>>> + } arm_timer;
>>>>>> +
>>>>>> +Indicates that a timer triggered that user space needs to handle and
>>>>>> +potentially mask with vcpu->run->request_interrupt_window to allow the
>>>>>> +guest to proceed. This only happens for timers that got enabled through
>>>>>> +KVM_CAP_ARM_TIMER. The following time sources are available:
>>>>>> +
>>>>>> + KVM_ARM_TIMER_VTIMER - virtual cpu timer
>>>>>> +
>>>>>> /* Fix the size of the union. */
>>>>>> char padding[256];
>>>>>> };
>>>>>> @@ -3739,6 +3756,17 @@ Once this is done the KVM_REG_MIPS_VEC_* and KVM_REG_MIPS_MSA_* registers can be
>>>>>> accessed, and the Config5.MSAEn bit is accessible via the KVM API and also from
>>>>>> the guest.
>>>>>>
>>>>>> +6.11 KVM_CAP_ARM_TIMER
>>>>>> +
>>>>>> +Architectures: arm, arm64
>>>>>> +Target: vcpu
>>>>>> +Parameters: args[0] contains a bitmap of timers to select (see 5.)
>>>>>> +
>>>>>> +This capability allows to route per-core timers into user space. When it's
>>>>>> +enabled and no in-kernel interrupt controller is in use, the timers selected
>>>>>> +by args[0] trigger KVM_EXIT_ARM_TIMER guest exits when they are pending,
>>>>>> +unless masked by vcpu->run->request_interrupt_window (see 5.).
>>>>>> +
>>>>>> 7. Capabilities that can be enabled on VMs
>>>>>> ------------------------------------------
>>>>>>
>>>>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>>>>> index de338d9..77d1f73 100644
>>>>>> --- a/arch/arm/include/asm/kvm_host.h
>>>>>> +++ b/arch/arm/include/asm/kvm_host.h
>>>>>> @@ -180,6 +180,9 @@ struct kvm_vcpu_arch {
>>>>>>
>>>>>> /* Detect first run of a vcpu */
>>>>>> bool has_run_once;
>>>>>> +
>>>>>> + /* User space wants timer notifications */
>>>>>> + bool user_space_arm_timers;
>>>>> Please move this to the timer structure.
>>>> Sure.
>>>>
>>>>>> };
>>>>>>
>>>>>> struct kvm_vm_stat {
>>>>>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>>>>>> index c84b6ad..57bdb71 100644
>>>>>> --- a/arch/arm/kvm/arm.c
>>>>>> +++ b/arch/arm/kvm/arm.c
>>>>>> @@ -187,6 +187,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>>>>> case KVM_CAP_ARM_PSCI_0_2:
>>>>>> case KVM_CAP_READONLY_MEM:
>>>>>> case KVM_CAP_MP_STATE:
>>>>>> + case KVM_CAP_ARM_TIMER:
>>>>>> r = 1;
>>>>>> break;
>>>>>> case KVM_CAP_COALESCED_MMIO:
>>>>>> @@ -474,13 +475,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> - /*
>>>>>> - * Enable the arch timers only if we have an in-kernel VGIC
>>>>>> - * and it has been properly initialized, since we cannot handle
>>>>>> - * interrupts from the virtual timer with a userspace gic.
>>>>>> - */
>>>>>> - if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
>>>>>> - ret = kvm_timer_enable(vcpu);
>>>>>> + ret = kvm_timer_enable(vcpu);
>>>>>>
>>>>>> return ret;
>>>>>> }
>>>>>> @@ -601,6 +596,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>>>>> run->exit_reason = KVM_EXIT_INTR;
>>>>>> }
>>>>>>
>>>>>> + if (kvm_check_request(KVM_REQ_PENDING_TIMER, vcpu)) {
>>>>> Since this is a very unlikely event (in the grand scheme of things), how
>>>>> about making this unlikely()?
>>>>>
>>>>>> + /* Tell user space about the pending vtimer */
>>>>>> + ret = 0;
>>>>>> + run->exit_reason = KVM_EXIT_ARM_TIMER;
>>>>>> + run->arm_timer.timesource = KVM_ARM_TIMER_VTIMER;
>>>>>> + }
>>>>> More importantly: why does it have to be indirected by a
>>>>> make_request/check_request, and not be handled as part of the
>>>>> kvm_timer_sync() call? We do update the state there, and you could
>>>>> directly find out whether an exit is required.
>>>> I can try - it seemed like it could easily become quite racy because we
>>>> call kvm_timer_sync_hwstate() at multiple places.
>>> It shouldn't. We only do it at exactly two locations (depending whether
>>> we've entered the guest or not).
>>>
>>> Also, take the following scenario:
>>> (1) guest programs the timer to expire at time T
>>> (2) guest performs an MMIO access which traps
>>> (3) during the world switch, the timer expires and we mark the timer
>>> interrupt as pending
>>> (4) we exit to handle the MMIO, no sign of the timer being pending
>>>
>>> Is the timer event lost? Or simply delayed? I think this indicates that
>>> the timer state should always be signalled to userspace, no matter what
>>> the exit reason is.
>> That's basically what I'm trying to get running right now, yes. I pushed
>> the interrupt pending status field into the kvm_sync_regs struct and
>> check it on every exit in user space - to make sure we catch pending
>> state changes before mmio reads.
>>
>> On top of that we also need a force exit event when the state changes,
>> in case there's no other event pending. Furthermore we probably want to
>> indicate the user space status of the pending bit into the kernel to not
>> exit too often.
> All you need is to do is to stash the line state in the run structure.
That's what I do now, yes. The sync_regs struct is basically an arch
specific add-on to the run structure, so we don't modify padding /
alignment with the change.
> You shouldn't need any other information. And you trigger the exit on
> "timer line high + timer line unmasked" in order to inject the
> interrupt. Which is basically what the vgic does. It would greatly help
> if you adopted a similar behaviour.
We also need to know "timer line low + timer line masked", as otherwise
we might get spurious interrupts in the guest, no?
Either way, I agree that this approach in general is saner. I don't
think it's easier to implement though, but we'll get to that when I send
a new version :)
Alex
^ permalink raw reply
* [PATCH] arm64, numa: Add cpu_to_node() implementation.
From: Hanjun Guo @ 2016-09-20 10:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1474310970-21264-1-git-send-email-ddaney.cavm@gmail.com>
On 09/20/2016 02:49 AM, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
>
> The wq_numa_init() function makes a private CPU to node map by calling
> cpu_to_node() early in the boot process, before the non-boot CPUs are
> brought online. Since the default implementation of cpu_to_node()
> returns zero for CPUs that have never been brought online, the
> workqueue system's view is that *all* CPUs are on node zero.
>
> When the unbound workqueue for a non-zero node is created, the
> tsk_cpus_allowed() for the worker threads is the empty set because
> there are, in the view of the workqueue system, no CPUs on non-zero
> nodes. The code in try_to_wake_up() using this empty cpumask ends up
> using the cpumask empty set value of NR_CPUS as an index into the
> per-CPU area pointer array, and gets garbage as it is one past the end
> of the array. This results in:
>
> [ 0.881970] Unable to handle kernel paging request at virtual address fffffb1008b926a4
> [ 1.970095] pgd = fffffc00094b0000
> [ 1.973530] [fffffb1008b926a4] *pgd=0000000000000000, *pud=0000000000000000, *pmd=0000000000000000
> [ 1.982610] Internal error: Oops: 96000004 [#1] SMP
> [ 1.987541] Modules linked in:
> [ 1.990631] CPU: 48 PID: 295 Comm: cpuhp/48 Tainted: G W 4.8.0-rc6-preempt-vol+ #9
> [ 1.999435] Hardware name: Cavium ThunderX CN88XX board (DT)
> [ 2.005159] task: fffffe0fe89cc300 task.stack: fffffe0fe8b8c000
> [ 2.011158] PC is at try_to_wake_up+0x194/0x34c
> [ 2.015737] LR is at try_to_wake_up+0x150/0x34c
> [ 2.020318] pc : [<fffffc00080e7468>] lr : [<fffffc00080e7424>] pstate: 600000c5
> [ 2.027803] sp : fffffe0fe8b8fb10
> [ 2.031149] x29: fffffe0fe8b8fb10 x28: 0000000000000000
> [ 2.036522] x27: fffffc0008c63bc8 x26: 0000000000001000
> [ 2.041896] x25: fffffc0008c63c80 x24: fffffc0008bfb200
> [ 2.047270] x23: 00000000000000c0 x22: 0000000000000004
> [ 2.052642] x21: fffffe0fe89d25bc x20: 0000000000001000
> [ 2.058014] x19: fffffe0fe89d1d00 x18: 0000000000000000
> [ 2.063386] x17: 0000000000000000 x16: 0000000000000000
> [ 2.068760] x15: 0000000000000018 x14: 0000000000000000
> [ 2.074133] x13: 0000000000000000 x12: 0000000000000000
> [ 2.079505] x11: 0000000000000000 x10: 0000000000000000
> [ 2.084879] x9 : 0000000000000000 x8 : 0000000000000000
> [ 2.090251] x7 : 0000000000000040 x6 : 0000000000000000
> [ 2.095621] x5 : ffffffffffffffff x4 : 0000000000000000
> [ 2.100991] x3 : 0000000000000000 x2 : 0000000000000000
> [ 2.106364] x1 : fffffc0008be4c24 x0 : ffffff0ffffada80
> [ 2.111737]
> [ 2.113236] Process cpuhp/48 (pid: 295, stack limit = 0xfffffe0fe8b8c020)
> [ 2.120102] Stack: (0xfffffe0fe8b8fb10 to 0xfffffe0fe8b90000)
> [ 2.125914] fb00: fffffe0fe8b8fb80 fffffc00080e7648
> .
> .
> .
> [ 2.442859] Call trace:
> [ 2.445327] Exception stack(0xfffffe0fe8b8f940 to 0xfffffe0fe8b8fa70)
> [ 2.451843] f940: fffffe0fe89d1d00 0000040000000000 fffffe0fe8b8fb10 fffffc00080e7468
> [ 2.459767] f960: fffffe0fe8b8f980 fffffc00080e4958 ffffff0ff91ab200 fffffc00080e4b64
> [ 2.467690] f980: fffffe0fe8b8f9d0 fffffc00080e515c fffffe0fe8b8fa80 0000000000000000
> [ 2.475614] f9a0: fffffe0fe8b8f9d0 fffffc00080e58e4 fffffe0fe8b8fa80 0000000000000000
> [ 2.483540] f9c0: fffffe0fe8d10000 0000000000000040 fffffe0fe8b8fa50 fffffc00080e5ac4
> [ 2.491465] f9e0: ffffff0ffffada80 fffffc0008be4c24 0000000000000000 0000000000000000
> [ 2.499387] fa00: 0000000000000000 ffffffffffffffff 0000000000000000 0000000000000040
> [ 2.507309] fa20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 2.515233] fa40: 0000000000000000 0000000000000000 0000000000000000 0000000000000018
> [ 2.523156] fa60: 0000000000000000 0000000000000000
> [ 2.528089] [<fffffc00080e7468>] try_to_wake_up+0x194/0x34c
> [ 2.533723] [<fffffc00080e7648>] wake_up_process+0x28/0x34
> [ 2.539275] [<fffffc00080d3764>] create_worker+0x110/0x19c
> [ 2.544824] [<fffffc00080d69dc>] alloc_unbound_pwq+0x3cc/0x4b0
> [ 2.550724] [<fffffc00080d6bcc>] wq_update_unbound_numa+0x10c/0x1e4
> [ 2.557066] [<fffffc00080d7d78>] workqueue_online_cpu+0x220/0x28c
> [ 2.563234] [<fffffc00080bd288>] cpuhp_invoke_callback+0x6c/0x168
> [ 2.569398] [<fffffc00080bdf74>] cpuhp_up_callbacks+0x44/0xe4
> [ 2.575210] [<fffffc00080be194>] cpuhp_thread_fun+0x13c/0x148
> [ 2.581027] [<fffffc00080dfbac>] smpboot_thread_fn+0x19c/0x1a8
> [ 2.586929] [<fffffc00080dbd64>] kthread+0xdc/0xf0
> [ 2.591776] [<fffffc0008083380>] ret_from_fork+0x10/0x50
> [ 2.597147] Code: b00057e1 91304021 91005021 b8626822 (b8606821)
> [ 2.603464] ---[ end trace 58c0cd36b88802bc ]---
> [ 2.608138] Kernel panic - not syncing: Fatal exception
>
> Fix by supplying a cpu_to_node() implementation that returns correct
> node mappings.
>
> Cc: <stable@vger.kernel.org> # 4.7.x-
> Signed-off-by: David Daney <david.daney@cavium.com>
>
> ---
> arch/arm64/include/asm/topology.h | 3 +++
> arch/arm64/mm/numa.c | 18 ++++++++++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index 8b57339..8d935447 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -30,6 +30,9 @@ int pcibus_to_node(struct pci_bus *bus);
> cpu_all_mask : \
> cpumask_of_node(pcibus_to_node(bus)))
>
> +int cpu_to_node(int cpu);
> +#define cpu_to_node cpu_to_node
> +
> #endif /* CONFIG_NUMA */
>
> #include <asm-generic/topology.h>
> diff --git a/arch/arm64/mm/numa.c b/arch/arm64/mm/numa.c
> index 5bb15ea..e76281b 100644
> --- a/arch/arm64/mm/numa.c
> +++ b/arch/arm64/mm/numa.c
> @@ -130,6 +130,24 @@ void __init early_map_cpu_to_node(unsigned int cpu, int nid)
> cpu_to_node_map[cpu] = nid;
> }
>
> +int cpu_to_node(int cpu)
> +{
> + int nid;
> +
> + /*
> + * Return 0 for unknown mapping so that we report something
> + * sensible if firmware doesn't supply a proper mapping.
> + */
> + if (cpu < 0 || cpu >= NR_CPUS)
> + return 0;
> +
> + nid = cpu_to_node_map[cpu];
> + if (nid == NUMA_NO_NODE)
> + nid = 0;
> + return nid;
> +}
> +EXPORT_SYMBOL(cpu_to_node);
> +
> /**
> * numa_add_memblk - Set node id to memblk
> * @nid: NUMA node ID of the new memblk
Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>
Thanks for the fix!
Hanjun
^ permalink raw reply
* [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer
From: Martin Sperl @ 2016-09-20 10:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <fb5e8e93-c6af-0050-1e7d-a69b722feadb@tronnes.org>
On 20.09.2016 10:41, Noralf Tr?nnes wrote:
>
> Den 20.09.2016 09:19, skrev Martin Sperl:
>> Hi Noralf!
>>
>> On 19.09.2016 17:26, Noralf Tr?nnes wrote:
>>> Some SMBus protocols use Repeated Start Condition to switch from write
>>> mode to read mode. Devices like MMA8451 won't work without it.
>>>
>>> When downstream implemented support for this in i2c-bcm2708, it broke
>>> support for some devices, so a module parameter was added and combined
>>> transfer was disabled by default.
>>> See https://github.com/raspberrypi/linux/issues/599
>>> It doesn't seem to have been any investigation into what the problem
>>> really was. Later there was added a timeout on the polling loop.
>>>
>>> One of the devices mentioned to partially stop working was DS1307.
>>>
>>> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
>>> and AT24C32 (eeprom) in parallel without problems.
>>>
>>> Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
>>> ---
>>> drivers/i2c/busses/i2c-bcm2835.c | 107
>>> +++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 98 insertions(+), 9 deletions(-)
>> ...
>>> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
>>> *adap, struct i2c_msg msgs[],
>>> int i;
>>> int ret = 0;
>>> + /* Combined write-read to the same address (smbus) */
>>> + if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>>> + !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>>> + (msgs[0].len <= 16)) {
>>> + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>>> +
>>> + return ret ? ret : 2;
>>> + }
>>> +
>>> for (i = 0; i < num; i++) {
>>> - ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
>>> + ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>>> if (ret)
>>> break;
>>> }
>> This does not seem to implement the i2c_msg api correctly.
>>
>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>> message
>> in a group should - by default - send a STOP.
>>
>
> Apparently it's a known problem that the i2c controller doesn't support
> Repeated Start. It will always issue a Stop when it has transferred DLEN
> bytes.
> Refs:
> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>
>
> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
> and before DONE is set (or the last byte is shifted, I don't know excatly).
> Refs:
> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
>
>
> I found this answer/report by joan that the downstream combined support
> isn't reliable:
> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>
>
> My implementation differs from downstream in that I use local_irq_save()
> to protect the polling loop. But that only protects from missing the TA
> (downstream can miss the TA and issue a Stop).
>
> So currently in mainline we have a driver that says it support the standard
> (I2C_FUNC_I2C), but it really only supports one message transfers since it
> can't do ReStart.
>
> What I have done in this patch is to support ReStart for transfers with
> 2 messages: first write, then read. But maybe a better solution is to just
> leave this alone if it is flaky and use bitbanging instead. I don't know.
I have not said that the approach you have taken is wrong or bad.
I was only telling you that the portion inside the bcm2835_i2c_xfer:
+ /* Combined write-read to the same address (smbus) */
+ if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+ !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+ (msgs[0].len <= 16)) {
+ ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
+
+ return ret ? ret : 2;
+ }
is very specific and maybe could be done in a "generic" manner
supporting more cases.
At least add a dev_warn_once for all num > 1 cases not handled by the
code above.
This gives people an opportunity to detect such a situation if they
find something is not working as expected.
Martin
^ 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