Re: [PATCH] KVM/arm: kernel low level debug support for ARM32 virtual platforms
On Tue, Nov 03, 2015 at 11:33:17AM -0500, Christopher Covington wrote: > > diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug > > index a2e16f9..d126bd4 100644 > > --- a/arch/arm/Kconfig.debug > > +++ b/arch/arm/Kconfig.debug > > @@ -1155,6 +1155,28 @@ choice > > This option selects UART0 on VIA/Wondermedia System-on-a-chip > > devices, including VT8500, WM8505, WM8650 and WM8850. > > > > + config DEBUG_VIRT_UART_QEMU > > + bool "Kernel low-level debugging on QEMU Virtual Platform" > > + depends on ARCH_VIRT > > + select DEBUG_UART_PL01X > > + help > > + Say Y here if you want the debug print routines to direct > > + their output to PL011 UART port on QEMU Virtual Platform. > > + Appropriate address values are: > > + PHYSVIRT > > + 0x900 0xf809 > > I thought the only guarantee the virt machine had about the memory map was > that it would be described in the device tree. This LL debug stuff is used prior to device tree being parsed. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] arm64: KVM: Do not inject a 64bit fault for a 32bit guest
On Thu, Aug 27, 2015 at 03:05:47PM +0100, Marc Zyngier wrote: > When injecting a fault into a 32bit guest, it seems rather idiotic > to also inject a 64bit fault that is only going to corrupt the > guest state, and lead to a situation where we restore an illegal > context. > > Just fix the stupid bug that has been there from day 1. > > Cc: sta...@vger.kernel.org > Reported-by: Russell King s/linux/rmk+kernel/ please Tested-by: Russell King > Signed-off-by: Marc Zyngier > --- > Will: Paolo being on holiday, do you mind merging this one > via your tree? I don't think the commit message does this bug justice. The implication is it's just a guest issue. It isn't, the bug appears to take out the host kernel in a truely spectacular way. http://www.arm.linux.org.uk/developer/build/result.php?type=boot&idx=4871 Tested here, the fix stops the host kernel exploding. The crashed kvm instance can be stopped and a proper kernel can then be booted in a new guest instance. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Remove visible dependency files
After building, there is a lot of clutter from the dependency system. Let's clean this up by using dir/.file.d style dependencies, similar to those used in the Linux kernel. In order to support this, rearrange the dependency generation to create the dependency files as we build rather than as a separate step, and have make clean remove them. Signed-off-by: Russell King --- Makefile | 32 +--- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/Makefile b/Makefile index 4bd86ee..c07fd8e 100644 --- a/Makefile +++ b/Makefile @@ -288,7 +288,13 @@ LIBS += -lpthread LIBS += -lutil -DEPS := $(patsubst %.o,%.d,$(OBJS)) +comma = , + +# The dependency file for the current target +depfile = $(subst $(comma),_,$(dir $@).$(notdir $@).d) + +DEPS := $(foreach obj,$(OBJS),\ + $(subst $(comma),_,$(dir $(obj)).$(notdir $(obj)).d)) DEFINES+= -D_FILE_OFFSET_BITS=64 DEFINES+= -D_GNU_SOURCE @@ -327,6 +333,10 @@ all: arch_support_check $(PROGRAM) $(PROGRAM_ALIAS) $(GUEST_INIT) arch_support_check: $(UNSUPP_ERR) +# CFLAGS used when building objects +# This is intentionally not assigned using := +c_flags= -Wp,-MD,$(depfile) $(CFLAGS) + # When building -static all objects are built with appropriate flags, which # may differ between static & dynamic .o. The objects are separated into # .o and .static.o. See the %.o: %.c rules below. @@ -336,11 +346,11 @@ arch_support_check: STATIC_OBJS = $(patsubst %.o,%.static.o,$(OBJS) $(OBJS_STATOPT)) GUEST_OBJS = guest/guest_init.o -$(PROGRAM)-static: $(DEPS) $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_INIT) +$(PROGRAM)-static: $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_INIT) $(E) " LINK" $@ $(Q) $(CC) -static $(CFLAGS) $(STATIC_OBJS) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_STATOPT) -o $@ -$(PROGRAM): $(DEPS) $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_INIT) +$(PROGRAM): $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_INIT) $(E) " LINK" $@ $(Q) $(CC) $(CFLAGS) $(OBJS) $(OBJS_DYNOPT) $(OTHEROBJS) $(GUEST_OBJS) $(LIBS) $(LIBS_DYNOPT) -o $@ @@ -353,19 +363,11 @@ $(GUEST_INIT): guest/init.c $(Q) $(CC) -static guest/init.c -o $@ $(Q) $(LD) $(LDFLAGS) -r -b binary -o guest/guest_init.o $(GUEST_INIT) -$(DEPS): - -util/rbtree.d: util/rbtree.c - $(Q) $(CC) -M -MT util/rbtree.o $(CFLAGS) $< -o $@ - -%.d: %.c - $(Q) $(CC) -M -MT $(patsubst %.d,%.o,$@) $(CFLAGS) $< -o $@ - %.s: %.c $(Q) $(CC) -o $@ -S $(CFLAGS) -fverbose-asm $< # The header file common-cmds.h is needed for compilation of builtin-help.c. -builtin-help.d: $(KVM_INCLUDE)/common-cmds.h +builtin-help.o: $(KVM_INCLUDE)/common-cmds.h $(OBJS): @@ -375,7 +377,7 @@ ifeq ($(C),1) $(Q) $(CHECK) -c $(CFLAGS) $< -o $@ endif $(E) " CC " $@ - $(Q) $(CC) -c $(CFLAGS) $< -o $@ + $(Q) $(CC) -c $(c_flags) $< -o $@ %.static.o: %.c ifeq ($(C),1) @@ -383,7 +385,7 @@ ifeq ($(C),1) $(Q) $(CHECK) -c $(CFLAGS) $(CFLAGS_STATOPT) $< -o $@ endif $(E) " CC " $@ - $(Q) $(CC) -c $(CFLAGS) $(CFLAGS_STATOPT) $< -o $@ + $(Q) $(CC) -c $(c_flags) $(CFLAGS_STATOPT) $< -o $@ %.o: %.c ifeq ($(C),1) @@ -391,7 +393,7 @@ ifeq ($(C),1) $(Q) $(CHECK) -c $(CFLAGS) $(CFLAGS_DYNOPT) $< -o $@ endif $(E) " CC " $@ - $(Q) $(CC) -c $(CFLAGS) $(CFLAGS_DYNOPT) $< -o $@ + $(Q) $(CC) -c $(c_flags) $(CFLAGS_DYNOPT) $< -o $@ $(KVM_INCLUDE)/common-cmds.h: util/generate-cmdlist.sh command-list.txt -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] virtio: fix fsync() on a directory
dpkg in the guest fails when it tries to use fsync() on a directory:
openat(AT_FDCWD, "/var/lib/dpkg",
O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY|O_CLOEXEC) = 4
fsync(4)= -1 EINVAL (Invalid argument)
stracing lkvm shows that this is converted to:
openat(AT_FDCWD, "/root/rootfs-32//var/lib/dpkg",
O_RDONLY|O_NONBLOCK|O_DIRECTORY|O_CLOEXEC) = 368
fsync(0)= -1 EINVAL (Invalid argument)
In other words, we sync against the wrong file descriptor. This case
is not handled in the kvmtool code, let's add support for it.
Signed-off-by: Russell King
---
virtio/9p.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/virtio/9p.c b/virtio/9p.c
index 2c120fa..5f93e41 100644
--- a/virtio/9p.c
+++ b/virtio/9p.c
@@ -886,17 +886,22 @@ err_out:
static void virtio_p9_fsync(struct p9_dev *p9dev,
struct p9_pdu *pdu, u32 *outlen)
{
- int ret;
+ int ret, fd;
struct p9_fid *fid;
u32 fid_val, datasync;
virtio_p9_pdu_readf(pdu, "dd", &fid_val, &datasync);
fid = get_fid(p9dev, fid_val);
+ if (fid->dir)
+ fd = dirfd(fid->dir);
+ else
+ fd = fid->fd;
+
if (datasync)
- ret = fdatasync(fid->fd);
+ ret = fdatasync(fd);
else
- ret = fsync(fid->fd);
+ ret = fsync(fd);
if (ret < 0)
goto err_out;
*outlen = pdu->write_offset;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: stand-alone kvmtool
On Thu, Feb 19, 2015 at 05:56:45AM -0500, Sasha Levin wrote: > What inconvenience is caused by having it sit inside the kernel tree > beyond an increased requirement in disk space? I've come across this problem with the perf tools - luckily, the perf tools allow the source to be exported from the kernel tree, but it is far from a good solution. The problem is when you're primarily cross-building the kernel on a system where you don't have the target libraries (because, eg, you're running in a build environment for multiple different target systems.) Having to build userspace tools in that scenario is a _major_ pita. Yes, of course it's possible to pull the 1GB of kernel GIT respository down onto the target just to build some silly userspace tool, but when your rootfs lives on an 8GB SD card or a USB memory stick (as is the case with the ARM Juno 64-bit platform), and when the userspace tool somehow depends on the kernel source tree being configured, it really starts getting painful. TBH, I don't much care provided there is a way to export a source tarball for the tool from the kernel (like perf does) which can then be transferred to the target and built there. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 07/26] driver core: amba: add device binding path 'driver_override'
On Tue, Sep 23, 2014 at 04:46:06PM +0200, Antonios Motakis wrote:
> As already demonstrated with PCI [1] and the platform bus [2], a
> driver_override property in sysfs can be used to bypass the id matching
> of a device to a AMBA driver. This can be used by VFIO to bind to any AMBA
> device requested by the user.
>
> [1]
> http://lists-archives.com/linux-kernel/28030441-pci-introduce-new-device-binding-path-using-pci_dev-driver_override.html
> [2] https://www.redhat.com/archives/libvir-list/2014-April/msg00382.html
>
> Signed-off-by: Antonios Motakis
I have to ask why this is even needed in the first place. To take the
example in [2], what's wrong with:
echo fff51000.ethernet >
/sys/bus/platform/devices/fff51000.ethernet/driver/unbind
echo fff51000.ethernet > /sys/bus/platform/drivers/vfio-platform/bind
and similar for AMBA.
All we would need to do is to introduce a way of having a driver accept
explicit bind requests.
In any case:
> +static ssize_t driver_override_store(struct device *_dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct amba_device *dev = to_amba_device(_dev);
> + char *driver_override, *old = dev->driver_override, *cp;
> +
> + if (count > PATH_MAX)
> + return -EINVAL;
> +
> + driver_override = kstrndup(buf, count, GFP_KERNEL);
> + if (!driver_override)
> + return -ENOMEM;
> +
> + cp = strchr(driver_override, '\n');
> + if (cp)
> + *cp = '\0';
I hope that is not replicated everywhere. This allows up to a page to be
allocated, even when the first byte may be a newline. This is wasteful.
How about:
if (count > PATH_MAX)
return -EINVAL;
cp = strnchr(buf, count, '\n');
if (cp)
count = cp - buf - 1;
if (count) {
driver_override = kstrndup(buf, count, GFP_KERNEL);
if (!driver_override)
return -ENOMEM;
} else {
driver_override = NULL;
}
kfree(dev->driver_override);
dev->driver_override = driver_override;
Also:
> +static ssize_t driver_override_show(struct device *_dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct amba_device *dev = to_amba_device(_dev);
> +
> + return sprintf(buf, "%s\n", dev->driver_override);
> +}
Do we really want to do a NULL pointer dereference here?
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: convert all "mov.* pc, reg" to "bx reg" for ARMv6+ (part1)
On Tue, Jul 01, 2014 at 05:42:42PM +0100, Måns Rullgård wrote:
> Russell King writes:
>
> > ARMv6 and greater introduced a new instruction ("bx") which can be used
> > to return from function calls. Recent CPUs perform better when the
> > "bx lr" instruction is used rather than the "mov pc, lr" instruction,
> > and this sequence is strongly recommended to be used by the ARM
> > architecture manual (section A.4.1.1).
> >
> > We provide a new macro "ret" with all its variants for the condition
> > code which will resolve to the appropriate instruction.
>
> When the source register is not "lr" the name "ret" is a misnomer since
> only the "bx lr" instruction is predicted as a function return. The
> "bx" instruction with other source registers uses the normal prediction
> mechanisms, leaving the return stack alone, and should not be used for
> function returns. Any code currently using another register to return
> from a function should probably be modified to use lr instead, unless
> there are special reasons for doing otherwise. If code jumping to an
> address in a non-lr register is not a return, using the "ret" name will
> make for some rather confusing reading.
>
> I would suggest either using a more neutral name than "ret" or adding an
> alias to be used for non-return jumps so as to make the intent clearer.
If you read the patch, the branches which are changed are those which
do indeed return in some way. There are those which do this having
moved lr to a different register.
As you point out, "bx lr" /may/ be treated specially (I've actually been
discussing this with Will Deacon over the last couple of days, who has
also been talking to the hardware people in ARM, and Will is happy with
this patch as in its current form.) This is why I've changed all
"mov pc, reg" instructions which return in some way to use this macro,
and left others (those which are used to call some function and return
back to the same point) alone.
I have thought about introducing a "call" macro for those other sites,
but as there are soo few of them, I've left them as-is for the time
being (this patch is already large enough.)
If there are any in the patch which you have specific concerns about,
please specifically point them out those which give you a concern.
Thanks.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: convert all "mov.* pc, reg" to "bx reg" for ARMv6+ (part2)
--- a/arch/arm/mm/proc-arm926.S +++ b/arch/arm/mm/proc-arm926.S @@ -55,7 +55,7 @@ * cpu_arm926_proc_init() */ ENTRY(cpu_arm926_proc_init) - mov pc, lr + ret lr /* * cpu_arm926_proc_fin() @@ -65,7 +65,7 @@ ENTRY(cpu_arm926_proc_fin) bic r0, r0, #0x1000 @ ...i bic r0, r0, #0x000e @ wca. mcr p15, 0, r0, c1, c0, 0 @ disable caches - mov pc, lr + ret lr /* * cpu_arm926_reset(loc) @@ -89,7 +89,7 @@ ENTRY(cpu_arm926_reset) bic ip, ip, #0x000f @ wcam bic ip, ip, #0x1100 @ ...i...s mcr p15, 0, ip, c1, c0, 0 @ ctrl register - mov pc, r0 + ret r0 ENDPROC(cpu_arm926_reset) .popsection @@ -111,7 +111,7 @@ ENTRY(cpu_arm926_do_idle) mcr p15, 0, r0, c7, c0, 4 @ Wait for interrupt mcr p15, 0, r1, c1, c0, 0 @ Restore ICache enable msr cpsr_c, r3 @ Restore FIQ state - mov pc, lr + ret lr /* * flush_icache_all() @@ -121,7 +121,7 @@ ENTRY(cpu_arm926_do_idle) ENTRY(arm926_flush_icache_all) mov r0, #0 mcr p15, 0, r0, c7, c5, 0 @ invalidate I cache - mov pc, lr + ret lr ENDPROC(arm926_flush_icache_all) /* @@ -151,7 +151,7 @@ __flush_whole_cache: tst r2, #VM_EXEC mcrne p15, 0, ip, c7, c5, 0 @ invalidate I cache mcrne p15, 0, ip, c7, c10, 4 @ drain WB - mov pc, lr + ret lr /* * flush_user_cache_range(start, end, flags) @@ -188,7 +188,7 @@ ENTRY(arm926_flush_user_cache_range) blo 1b tst r2, #VM_EXEC mcrne p15, 0, ip, c7, c10, 4 @ drain WB - mov pc, lr + ret lr /* * coherent_kern_range(start, end) @@ -222,7 +222,7 @@ ENTRY(arm926_coherent_user_range) blo 1b mcr p15, 0, r0, c7, c10, 4 @ drain WB mov r0, #0 - mov pc, lr + ret lr /* * flush_kern_dcache_area(void *addr, size_t size) @@ -242,7 +242,7 @@ ENTRY(arm926_flush_kern_dcache_area) mov r0, #0 mcr p15, 0, r0, c7, c5, 0 @ invalidate I cache mcr p15, 0, r0, c7, c10, 4 @ drain WB - mov pc, lr + ret lr /* * dma_inv_range(start, end) @@ -270,7 +270,7 @@ arm926_dma_inv_range: cmp r0, r1 blo 1b mcr p15, 0, r0, c7, c10, 4 @ drain WB - mov pc, lr + ret lr /* * dma_clean_range(start, end) @@ -291,7 +291,7 @@ arm926_dma_clean_range: blo 1b #endif mcr p15, 0, r0, c7, c10, 4 @ drain WB - mov pc, lr + ret lr /* * dma_flush_range(start, end) @@ -313,7 +313,7 @@ ENTRY(arm926_dma_flush_range) cmp r0, r1 blo 1b mcr p15, 0, r0, c7, c10, 4 @ drain WB - mov pc, lr + ret lr /* * dma_map_area(start, size, dir) @@ -336,7 +336,7 @@ ENDPROC(arm926_dma_map_area) * - dir - DMA direction */ ENTRY(arm926_dma_unmap_area) - mov pc, lr + ret lr ENDPROC(arm926_dma_unmap_area) .globl arm926_flush_kern_cache_louis @@ -353,7 +353,7 @@ ENTRY(cpu_arm926_dcache_clean_area) bhi 1b #endif mcr p15, 0, r0, c7, c10, 4 @ drain WB - mov pc, lr + ret lr /* === PageTable == */ @@ -380,7 +380,7 @@ ENTRY(cpu_arm926_switch_mm) mcr p15, 0, r0, c2, c0, 0 @ load page table pointer mcr p15, 0, ip, c8, c7, 0 @ invalidate I & D TLBs #endif - mov pc, lr + ret lr /* * cpu_arm926_set_pte_ext(ptep, pte, ext) @@ -397,7 +397,7 @@ ENTRY(cpu_arm926_set_pte_ext) #endif mcr p15, 0, r0, c7, c10, 4 @ drain WB #endif - mov pc, lr + ret lr /* Suspend/resume support: taken from arch/arm/plat-s3c24xx/sleep.S */ .globl cpu_arm926_suspend_size @@ -448,7 +448,7 @@ __arm926_setup: #ifdef CONFIG_CPU_CACHE_ROUND_ROBIN orr r0, r0, #0x4000 @ .1.. #endif - mov pc, lr + ret lr .size __arm926_setup, . - __arm926_setup /* diff --git a/arch/arm/mm/proc-arm940.S b/arch/arm/mm/proc-arm940.S index 1c39a704ff6e..e5212d489377 100644 --- a/arch/arm/mm/proc-arm940.S +++ b/arch/arm/mm/proc-arm940.S @@ -31,7 +31,7 @@ */ ENTRY(cpu_arm940_proc_init) ENTRY(cpu_arm940_switch_mm) - mov pc, lr + ret lr /* * cpu_arm940_proc_fin() @@ -41,7 +41,7 @@ ENTRY(cpu_arm940_proc_fin) bic r0, r0, #0x1000 @ i-cache bic r
Re: [PATCH] ARM: kvm: rename cpu_reset to avoid name clash
On Mon, Sep 23, 2013 at 12:59:44PM -0700, Olof Johansson wrote: > Hi Christoffer, > > On Mon, Sep 16, 2013 at 8:47 PM, Christoffer Dall > wrote: > > On 16 September 2013 19:41, Olof Johansson wrote: > >> Hi, > >> > >> On Wed, Sep 11, 2013 at 6:50 PM, Christoffer Dall > >> wrote: > >>> On Wed, Sep 11, 2013 at 03:39:26PM -0700, Olof Johansson wrote: > On Wed, Sep 11, 2013 at 3:27 PM, Olof Johansson wrote: > > cpu_reset is already #defined in as processor.reset, > > so it expands here and causes problems. > > > > Signed-off-by: Olof Johansson > > I just noticed this is broken on 3.10 too, so if/when applying feel free > to add: > > Cc: # 3.10+ > > >>> Thanks for the fix, applied. > >> > >> I haven't seen this hit linux-next yet? > >> > > I was waiting for kvm/next to move to 3.12-rc1 and base the patch for > > kvm-arm-next off there, but I pushed this to kvm-arm-next now and it > > should land in linux-next as soon as they update. > > Another week has passed, and -next and mainline are both still broken. > Can we please see a fix in mainline and -next within the next few > days? I'm getting failing builds too, which will only stop once it hits mainline. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Call for participation] Bi-Weekly KVM/ARM Technical Sync-up
On Wed, Aug 21, 2013 at 05:09:39PM -0700, Christoffer Dall wrote: > Linaro is going to host a bi-weekly sync-up call for technical issues on > KVM/ARM development. The KVM 32-bit and 64-bit maintainers as well as > the QEMU ARM maintainer will typically be on the call. > > The first call will be held Tuesday August 27th. I'll point out that I don't do Tuesdays for phone calls (it's one of the days I regularly take as "weekend time") so you'll never be able to invite me if you keep this on Tuesdays. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/10] uaccess: better might_sleep/might_fault behavior
On Wed, May 22, 2013 at 11:25:36AM +0200, Arnd Bergmann wrote: > Given the most commonly used functions and a couple of architectures > I'm familiar with, these are the ones that currently call might_fault() > > x86-32 x86-64 arm arm64 powerpc s390generic > copy_to_user - x - - - x x > copy_from_user- x - - - x > x > put_user x x x x x x x > get_user x x x x x x x > __copy_to_userx x - - x - > - > __copy_from_user x x - - x - - > __put_user- - x - x - - > __get_user- - x - x - - > > WTF? I think your table is rather screwed - especially on ARM. Tell me - how can __copy_to_user() use might_fault() but copy_to_user() not when copy_to_user() is implemented using __copy_to_user() ? Same for copy_from_user() but the reverse argument - there's nothing special in our copy_from_user() which would make it do might_fault() when __copy_from_user() wouldn't. The correct position for ARM is: our (__)?(pu|ge)t_user all use might_fault(), but (__)?copy_(to|from)_user do not. Neither does (__)?clear_user. We might want to fix those to use might_fault(). -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] ARM: KVM: switch to a dual-step HYP init code
On Fri, Apr 05, 2013 at 10:08:04AM +0100, Marc Zyngier wrote: > On 04/04/13 23:10, Geoff Levand wrote: > > Hi, > > > > On Tue, 2013-04-02 at 14:25 +0100, Marc Zyngier wrote: > >> + @ Jump to the trampoline page > >> + ldr r2, =#PAGE_MASK > >> + adr r3, target > >> + bic r3, r3, r2 > >> + ldr r2, =#TRAMPOLINE_VA > >> + add r3, r3, r2 > >> + mov pc, r3 > > > > I guess you need 'ldr r2, =PAGE_MASK'. > > > > arch/arm/kvm/init.S:114: Error: bad expression -- `ldr > > r2,=#(~((1<<12)-1))' > > arch/arm/kvm/init.S:117: Error: bad expression -- `ldr r2,=#0x' > > Oddly enough, this code compiles perfectly fine on my box. > What's your compiler/binutils versions? The standard format for this is: ldr rd, =value without a '#' and has been that way for as long as I remember binutils accepting that format. It's entirely possible that later binutils has decided to be a bit more flexible by allowing the '#' in there, but that's something which will be incompatible with older versions. Best loose the '#' in there. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fixing KVM/ARM breakage in mainline
On Mon, Feb 25, 2013 at 02:21:17PM +0200, Gleb Natapov wrote: > On Mon, Feb 25, 2013 at 12:18:27PM +, Marc Zyngier wrote: > > Russell, Marcelo, Gleb, > > > > Commit 89f8833 (Merge tag 'kvm-3.9-1' of > > git://git.kernel.org/pub/scm/virt/kvm/kvm) broke KVM/ARM. Nothing > > fundamental, just annoying enough. > > > > I've posted patches against next-20130215 some time ago: > > http://comments.gmane.org/gmane.comp.emulators.kvm.devel/105019 > > > > I've now created a branch based off mainline as of this morning, and > > containing these fixes: > > git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git > > kvm-arm/core-3.9-rc1-fixes > > > > Now the question is: who wants to merge this? The diffstat looks like this: > > > I was going to merge it today. I notice that my randconfig build last night tripped over the: include/linux/kvm_host.h:333:34: error: 'KVM_USER_MEM_SLOTS' undeclared here (not in a function) error which Marc's patches fix. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 07/14] KVM: ARM: World-switch implementation
If you're going to bother commenting on a big long email, please _CHOP OUT_ content which is not relevant to your reply. I paged down 5 pages, hit end, paged up, found no comment from you, so I'm not going to bother reading anything further in this message. Help your readers to read your email. Don't expect them to search a 1600-line email message for a one line reply. (This has been said many times over the history of the Internet. There's etiquette documents on Internet mail stating it too. Please, comply with it or you will find people will ignore your messages.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/14] KVM: ARM: Initial skeleton to compile KVM support
On Wed, Jan 16, 2013 at 01:26:01PM +1030, Rusty Russell wrote:
> Christoffer Dall writes:
>
> > On Mon, Jan 14, 2013 at 11:24 AM, Russell King - ARM Linux
> > wrote:
> >> On Tue, Jan 08, 2013 at 01:38:55PM -0500, Christoffer Dall wrote:
> >>> + /* -ENOENT for unknown features, -EINVAL for invalid combinations.
> >>> */
> >>> + for (i = 0; i < sizeof(init->features)*8; i++) {
> >>> + if (init->features[i / 32] & (1 << (i % 32))) {
> >>
> >> Isn't this an open-coded version of test_bit() ?
> >
> > indeed, nicely spotted:
>
> BTW, I wrote it that was out of excessive paranoia: it's a userspace
> API, and test_bit() won't be right on 64 bit BE systems.
So why is this a concern for 32-bit systems (which are, by definition,
only in arch/arm) ?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 13/14] KVM: ARM: Handle I/O aborts
On Mon, Jan 14, 2013 at 01:25:39PM -0500, Christoffer Dall wrote: > However, unifying all instruction decoding within arch/arm is quite > the heavy task, and requires agreeing on some canonical API that > people can live with and it will likely take a long time. I seem to > recall there were also arguments against unifying kprobe code with > other instruction decoding, as the kprobe code was also written to > work highly optimized under certain assumptions, if I understood > previous comments correctly. Yes, I know Rusty had a go. What I think may make sense is to unify this and the alignment code. They're really after the same things, which are: - Given an instruction, and register set, calculate the address of the access, size, number of accesses, and the source/destination registers. - Update the register set as though the instruction had been executed by the CPU. However, I've changed tack slightly from the above in the last 10 minutes or so. I'm thinking a little more that we might be able to take what we already have in alignment.c and provide it with a set of accessors according to size etc. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 08/14] KVM: ARM: Emulation framework and CP15 emulation
On Mon, Jan 14, 2013 at 12:38:17PM -0500, Christoffer Dall wrote:
> On Mon, Jan 14, 2013 at 11:36 AM, Russell King - ARM Linux
> wrote:
> > On Tue, Jan 08, 2013 at 01:39:31PM -0500, Christoffer Dall wrote:
> >> + /*
> >> + * Check whether this vcpu requires the cache to be flushed on
> >> + * this physical CPU. This is a consequence of doing dcache
> >> + * operations by set/way on this vcpu. We do it here to be in
> >> + * a non-preemptible section.
> >> + */
> >> + if (cpumask_test_cpu(cpu, &vcpu->arch.require_dcache_flush)) {
> >> + cpumask_clear_cpu(cpu, &vcpu->arch.require_dcache_flush);
> >
> > There is cpumask_test_and_clear_cpu() which may be better for this.
>
> nice:
>
> commit d31686fadb74ad564f6a5acabdebe411de86d77d
> Author: Christoffer Dall
> Date: Mon Jan 14 12:36:53 2013 -0500
>
> KVM: ARM: Use cpumask_test_and_clear_cpu
>
> Nicer shorter cleaner code. A.
>
> Cc: Russell King
Great, thanks.
Acked-by: Russell King
> Signed-off-by: Christoffer Dall
>
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index b5c6ab1..fdd4a7c 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -352,10 +352,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>* operations by set/way on this vcpu. We do it here to be in
>* a non-preemptible section.
>*/
> - if (cpumask_test_cpu(cpu, &vcpu->arch.require_dcache_flush)) {
> - cpumask_clear_cpu(cpu, &vcpu->arch.require_dcache_flush);
> + if (cpumask_test_and_clear_cpu(cpu, &vcpu->arch.require_dcache_flush))
> flush_cache_all(); /* We'd really want v7_flush_dcache_all() */
> - }
>
> kvm_arm_set_running_vcpu(vcpu);
> }
>
> --
>
> Thanks,
> -Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 13/14] KVM: ARM: Handle I/O aborts
On Tue, Jan 08, 2013 at 01:40:05PM -0500, Christoffer Dall wrote:
> diff --git a/arch/arm/kvm/decode.c b/arch/arm/kvm/decode.c
> new file mode 100644
> index 000..469cf14
> --- /dev/null
> +++ b/arch/arm/kvm/decode.c
> @@ -0,0 +1,462 @@
> +/*
> + * Copyright (C) 2012 - Virtual Open Systems and Columbia University
> + * Author: Christoffer Dall
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "trace.h"
> +
> +struct arm_instr {
> + /* Instruction decoding */
> + u32 opc;
> + u32 opc_mask;
> +
> + /* Decoding for the register write back */
> + bool register_form;
> + u32 imm;
> + u8 Rm;
> + u8 type;
> + u8 shift_n;
> +
> + /* Common decoding */
> + u8 len;
> + bool sign_extend;
> + bool w;
> +
> + bool (*decode)(struct kvm_decode *decode, struct kvm_exit_mmio *mmio,
> +unsigned long instr, struct arm_instr *ai);
> +};
> +
> +enum SRType {
> + SRType_LSL,
> + SRType_LSR,
> + SRType_ASR,
> + SRType_ROR,
> + SRType_RRX
> +};
> +
> +/* Modelled after DecodeImmShift() in the ARM ARM */
> +static enum SRType decode_imm_shift(u8 type, u8 imm5, u8 *amount)
> +{
> + switch (type) {
> + case 0x0:
> + *amount = imm5;
> + return SRType_LSL;
> + case 0x1:
> + *amount = (imm5 == 0) ? 32 : imm5;
> + return SRType_LSR;
> + case 0x2:
> + *amount = (imm5 == 0) ? 32 : imm5;
> + return SRType_ASR;
> + case 0x3:
> + if (imm5 == 0) {
> + *amount = 1;
> + return SRType_RRX;
> + } else {
> + *amount = imm5;
> + return SRType_ROR;
> + }
> + }
> +
> + return SRType_LSL;
> +}
> +
> +/* Modelled after Shift() in the ARM ARM */
> +static u32 shift(u32 value, u8 N, enum SRType type, u8 amount, bool carry_in)
> +{
> + u32 mask = (1 << N) - 1;
> + s32 svalue = (s32)value;
> +
> + BUG_ON(N > 32);
> + BUG_ON(type == SRType_RRX && amount != 1);
> + BUG_ON(amount > N);
> +
> + if (amount == 0)
> + return value;
> +
> + switch (type) {
> + case SRType_LSL:
> + value <<= amount;
> + break;
> + case SRType_LSR:
> + value >>= amount;
> + break;
> + case SRType_ASR:
> + if (value & (1 << (N - 1)))
> + svalue |= ((-1UL) << N);
> + value = svalue >> amount;
> + break;
> + case SRType_ROR:
> + value = (value >> amount) | (value << (N - amount));
> + break;
> + case SRType_RRX: {
> + u32 C = (carry_in) ? 1 : 0;
> + value = (value >> 1) | (C << (N - 1));
> + break;
> + }
> + }
> +
> + return value & mask;
> +}
> +
> +static bool decode_arm_wb(struct kvm_decode *decode, struct kvm_exit_mmio
> *mmio,
> + unsigned long instr, const struct arm_instr *ai)
> +{
> + u8 Rt = (instr >> 12) & 0xf;
> + u8 Rn = (instr >> 16) & 0xf;
> + u8 W = (instr >> 21) & 1;
> + u8 U = (instr >> 23) & 1;
> + u8 P = (instr >> 24) & 1;
> + u32 base_addr = *kvm_decode_reg(decode, Rn);
> + u32 offset_addr, offset;
> +
> + /*
> + * Technically this is allowed in certain circumstances,
> + * but we don't support it.
> + */
> + if (Rt == 15 || Rn == 15)
> + return false;
> +
> + if (P && !W) {
> + kvm_err("Decoding operation with valid ISV?\n");
> + return false;
> + }
> +
> + decode->rt = Rt;
> +
> + if (ai->register_form) {
> + /* Register operation */
> + enum SRType s_type;
> + u8 shift_n = 0;
> + bool c_bit = *kvm_decode_cpsr(decode) & PSR_C_BIT;
> + u32 s_reg = *kvm_decode_reg(decode, ai->Rm);
> +
> + s_type = decode_imm_shift(ai->type, ai->shift_n, &shift_n);
> + offset = shift(s_reg, 5, s_type, shift_n, c_bit);
> + } else {
> + /* Immediate operation */
> + offset = ai->imm;
> + }
> +
> + /* Handle Writeback */
> + if (U)
> + offset_addr = base_addr + offset;
>
Re: [PATCH v5 08/14] KVM: ARM: Emulation framework and CP15 emulation
On Tue, Jan 08, 2013 at 01:39:31PM -0500, Christoffer Dall wrote:
> + /*
> + * Check whether this vcpu requires the cache to be flushed on
> + * this physical CPU. This is a consequence of doing dcache
> + * operations by set/way on this vcpu. We do it here to be in
> + * a non-preemptible section.
> + */
> + if (cpumask_test_cpu(cpu, &vcpu->arch.require_dcache_flush)) {
> + cpumask_clear_cpu(cpu, &vcpu->arch.require_dcache_flush);
There is cpumask_test_and_clear_cpu() which may be better for this.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 03/14] KVM: ARM: Initial skeleton to compile KVM support
On Tue, Jan 08, 2013 at 01:38:55PM -0500, Christoffer Dall wrote:
> + /* -ENOENT for unknown features, -EINVAL for invalid combinations. */
> + for (i = 0; i < sizeof(init->features)*8; i++) {
> + if (init->features[i / 32] & (1 << (i % 32))) {
Isn't this an open-coded version of test_bit() ?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 02/14] ARM: Section based HYP idmap
On Tue, Jan 08, 2013 at 01:38:48PM -0500, Christoffer Dall wrote:
> + pr_info("Setting up static %sidentity map for 0x%llx - 0x%llx\n",
> + prot ? "HYP " : "",
> + (long long)addr, (long long)end);
There's no point using 0x%llx and casting to 64-bit longs if the arguments
are always going to be 32-bit.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 02/14] ARM: Section based HYP idmap
On Mon, Jan 14, 2013 at 01:07:56PM +0200, Gleb Natapov wrote:
> Ah, of course. This is ident map so by definition it cannot map phys
> addresses above 4G. And since __virt_to_phys() suppose to work only on
> ident map it's OK to returns unsigned long.
Let's get this right... the lack of correct definition in these
comments needs correction.
Firstly, __virt_to_phys() is an ARM-arch private function. Only
ARM-arch private code should be using it - it must not be used outside
that context.
Secondly, it's "public" counterpart, virt_to_phys(), and itself are
valid _only_ for what we call the "kernel direct map", which are the
addresses corresponding with the lowmem pages mapped from PAGE_OFFSET
up to the _start_ of vmalloc space. No other mapping is valid for this.
That means that addresses in the identity mapping, if the identity
mapping is outside of the range {PAGE_OFFSET..vmalloc start}, are _not_
valid for virt_to_phys().
The same is true of their counterparts, __phys_to_virt() and
phys_to_virt(). These are _only_ valid for physical addresses
corresponding to the pages mapped in as "lowmem" and they will return
addresses for that mapping of the pages.
Both these functions are invalid when used on highmem pages.
*virt_to_phys() is invalid when used on pointers returned from
ioremap(), vmalloc(), vmap(), dma_alloc_coherent(), and any other
interface which remaps memory.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [kvmarm] [PATCH v2 2/2] ARM: KVM: Power State Coordination Interface implementation
On Fri, Jan 11, 2013 at 01:07:31PM -0500, Christoffer Dall wrote: > The _very_ good reason here, is that we have two success cases: return > to guest and return to user space. As I said, we can save this state > in another bit somewhere and change all the KVM/ARM code to do so, but > the KVM guys back then would like to use the same convention as other > KVM archs. Can you please credit me for not objecting to returning 0/1 to have different success meanings. What I'm merely objecting to is that "return -1" statement in the code (notice the negative sign.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [kvmarm] [PATCH v2 2/2] ARM: KVM: Power State Coordination Interface implementation
On Fri, Jan 11, 2013 at 12:48:45PM -0500, Christoffer Dall wrote:
> again, that's why I suggest returning a bool instead. You just said
> it: it's a basic handled/not-handled state. Why do you want to return
> -EINVAL if that's not propogated anywhere?
We have a well established principle throughout the kernel interfaces that
functions will return positive values for success and an appropriate -ve
errno for failure.
We *certainly* hate functions which return 0 for failure and non-zero
for success - it makes review a real pain because you start seeing code
doing this:
if (!function()) {
deal_with_failure();
}
and you have to then start looking at the function to properly understand
what it's return semantics are.
We have more than enough proof already that this doesn't work: people
don't care to understand what the return values from functions mean.
You only have to do an audit of a few of the IS_ERR_OR_NULL() uses to
find that out, and you quickly realise that people just use what they
_think_ is the right test and which happens to pass their very simple
testing at the time.
We've avoided major problems so far in the kernel by having most of the
integer-returning functions following the established principle, and
that's good. I really really think that there must be a _very_ good
reason, and overwhelming reason to deviate from the established
principle in any large project.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [kvmarm] [PATCH v2 2/2] ARM: KVM: Power State Coordination Interface implementation
On Fri, Jan 11, 2013 at 12:33:15PM -0500, Christoffer Dall wrote:
> On Fri, Jan 11, 2013 at 12:24 PM, Marc Zyngier wrote:
> > On 11/01/13 17:12, Russell King - ARM Linux wrote:
> >> On Thu, Jan 10, 2013 at 04:06:45PM +, Marc Zyngier wrote:
> >>> +int kvm_psci_call(struct kvm_vcpu *vcpu)
> >>> +{
> >>> +unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
> >>> +unsigned long val;
> >>> +
> >>> +switch (psci_fn) {
> >>> +case KVM_PSCI_FN_CPU_OFF:
> >>> +kvm_psci_vcpu_off(vcpu);
> >>> +val = KVM_PSCI_RET_SUCCESS;
> >>> +break;
> >>> +case KVM_PSCI_FN_CPU_ON:
> >>> +val = kvm_psci_vcpu_on(vcpu);
> >>> +break;
> >>> +case KVM_PSCI_FN_CPU_SUSPEND:
> >>> +case KVM_PSCI_FN_MIGRATE:
> >>> +val = KVM_PSCI_RET_NI;
> >>> +break;
> >>> +
> >>> +default:
> >>> +return -1;
> >>> +}
> >>> +
> >>> +*vcpu_reg(vcpu, 0) = val;
> >>> +return 0;
> >>> +}
> >>
> >> We were discussing recently on #kernel about kernel APIs and the way that
> >> our integer-returning functions pretty much use 0 for success, and -errno
> >> for failures, whereas our pointer-returning functions are a mess.
> >>
> >> And above we have something returning -1 to some other chunk of code
> >> outside
> >> this compilation unit. That doesn't sound particularly clever to me.
> >
> > The original code used to return -EINVAL, see:
> > https://lists.cs.columbia.edu/pipermail/kvmarm/2013-January/004509.html
> >
> > Christoffer (Cc-ed) didn't like this, hence the -1. I'm happy to revert
> > the code to its original state though.
> >
> I don't want to return -EINVAL, because for the rest of the KVM code
> this would mean kill the guest.
>
> The convention used in other archs of KVM as well as for ARM is that
> the handle_exit functions return:
>
> -ERRNO: Error, report this error to user space
> 0: Everything is fine, but return to user space to let it do I/O
> emulation and whatever it wants to do
> 1: Everything is fine, return directly to the guest without going to user
> space
Right, so the above "return -1" _is_ doing the thing that I really hate,
which is it's actually doing a "return -EPERM" without anyone realising
that's what it's doing.
This is precisely why I hate (and pick up on, and have my mail reader to
highlight) any "return -1". It's mostly always a bug.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] ARM: KVM: Power State Coordination Interface implementation
On Thu, Jan 10, 2013 at 04:06:45PM +, Marc Zyngier wrote:
> +int kvm_psci_call(struct kvm_vcpu *vcpu)
> +{
> + unsigned long psci_fn = *vcpu_reg(vcpu, 0) & ~((u32) 0);
> + unsigned long val;
> +
> + switch (psci_fn) {
> + case KVM_PSCI_FN_CPU_OFF:
> + kvm_psci_vcpu_off(vcpu);
> + val = KVM_PSCI_RET_SUCCESS;
> + break;
> + case KVM_PSCI_FN_CPU_ON:
> + val = kvm_psci_vcpu_on(vcpu);
> + break;
> + case KVM_PSCI_FN_CPU_SUSPEND:
> + case KVM_PSCI_FN_MIGRATE:
> + val = KVM_PSCI_RET_NI;
> + break;
> +
> + default:
> + return -1;
> + }
> +
> + *vcpu_reg(vcpu, 0) = val;
> + return 0;
> +}
We were discussing recently on #kernel about kernel APIs and the way that
our integer-returning functions pretty much use 0 for success, and -errno
for failures, whereas our pointer-returning functions are a mess.
And above we have something returning -1 to some other chunk of code outside
this compilation unit. That doesn't sound particularly clever to me.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 12/13] ARM: KVM: vgic: reduce the number of vcpu kick
On Wed, Dec 05, 2012 at 01:40:24PM +, Marc Zyngier wrote: > Admittedly, the whole sequence should be rewritten to be clearer. What > it does is "If we're running a guest and there is no active interrupt, > then kick the guest". On the whole this entire thing should be written clearer; from the explanations you've given it seems that the only reason this code works is because you're relying on several behaviours all coming together to achieve the right result - which makes for fragile code. You're partly relying on atomic types to ensure that the increment and decrement happen exclusively. You're then relying on a combination of IRQ protection and cmpxchg() to ensure that the non-atomic read of the atomic type won't be a problem. This doesn't inspire confidence, and I have big concerns over whether this code will still be understandable in a number of years time. And I still wonder how safe this is even with your explanations. IRQ disabling only works for the local CPU core so I still have questions over this wrt a SMP host OS. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 12/13] ARM: KVM: vgic: reduce the number of vcpu kick
On Wed, Dec 05, 2012 at 12:17:57PM +, Marc Zyngier wrote:
> On 05/12/12 10:58, Russell King - ARM Linux wrote:
> > On Wed, Dec 05, 2012 at 10:43:58AM +, Will Deacon wrote:
> >> On Sat, Nov 10, 2012 at 03:45:39PM +, Christoffer Dall wrote:
> >>> From: Marc Zyngier
> >>>
> >>> If we have level interrupts already programmed to fire on a vcpu,
> >>> there is no reason to kick it after injecting a new interrupt,
> >>> as we're guaranteed that we'll exit when the level interrupt will
> >>> be EOId (VGIC_LR_EOI is set).
> >>>
> >>> The exit will force a reload of the VGIC, injecting the new interrupts.
> >>>
> >>> Signed-off-by: Marc Zyngier
> >>> Signed-off-by: Christoffer Dall
> >>> ---
> >>> arch/arm/include/asm/kvm_vgic.h | 10 ++
> >>> arch/arm/kvm/arm.c | 10 +-
> >>> arch/arm/kvm/vgic.c | 10 --
> >>> 3 files changed, 27 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/arch/arm/include/asm/kvm_vgic.h
> >>> b/arch/arm/include/asm/kvm_vgic.h
> >>> index a8e7a93..7d2662c 100644
> >>> --- a/arch/arm/include/asm/kvm_vgic.h
> >>> +++ b/arch/arm/include/asm/kvm_vgic.h
> >>> @@ -215,6 +215,9 @@ struct vgic_cpu {
> >>> u32 vgic_elrsr[2]; /* Saved only */
> >>> u32 vgic_apr;
> >>> u32 vgic_lr[64];/* A15 has only 4... */
> >>> +
> >>> + /* Number of level-triggered interrupt in progress */
> >>> + atomic_tirq_active_count;
> >>> #endif
> >>> };
> >>>
> >>> @@ -254,6 +257,8 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct
> >>> kvm_run *run,
> >>>
> >>> #define irqchip_in_kernel(k) (!!((k)->arch.vgic.vctrl_base))
> >>> #define vgic_initialized(k) ((k)->arch.vgic.ready)
> >>> +#define vgic_active_irq(v)
> >>> (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) == 0)
> >>
> >> When is the atomic_t initialised to zero? I can only see increments.
> >
> > I'd question whether an atomic type is correct for this; the only
> > protection that it's offering is to ensure that the atomic increment
> > and decrement occur atomically - there's nothing else that they're doing
> > in this code.
> >
> > If those atomic increments and decrements are occuring beneath a common
> > lock, then using atomic types is just mere code obfuscation.
>
> No, they occur on code paths that do not have a common lock (one of them
> being an interrupt handler). This may change though, after one comment
> Will made earlier (the thing about delayed interrupts).
>
> If these two code sections become mutually exclusive, then indeed there
> will be no point in having an atomic type anymore.
>
> > For example, I'd like to question the correctness of this:
> >
> > + if (vgic_active_irq(vcpu) &&
> > + cmpxchg(&vcpu->mode, EXITING_GUEST_MODE, IN_GUEST_MODE)
> > == EXITING_GUEST_MODE)
> >
> > What if vgic_active_irq() reads the atomic type, immediately after it gets
> > decremented to zero before the cmpxchg() is executed? Would that be a
> > problem?
>
> I do not think so. If the value gets decremented, it means we took a
> maintenance interrupt, which means we exited the guest at some point.
> Two possibilities:
>
> - We're not in guest mode anymore (vcpu->mode = OUTSIDE_GUEST_MODE), and
> cmpxchg will fail, hence signaling the guest to reload its state. This
> is not needed (the guest will reload its state anyway), but doesn't
> cause any harm.
What is the relative ordering of the atomic decrement and setting
vcpu->mode to be OUTSIDE_GUEST_MODE ? Is there a window where we have
decremented this atomic type but vcpu->mode is still set to IN_GUEST_MODE.
> - We're back into the guest (vcpu->mode = IN_GUEST_MODE), and cmpxchg
> will fail as well, triggering a reload which is needed this time.
Well, the whole code looks really weird to me, especially that:
+ if (kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE) {
+ if (vgic_active_irq(vcpu) &&
+ cmpxchg(&vcpu->mode, EXITING_GUEST_MODE, IN_GUEST_MODE) ==
EXITING_GUEST_MODE)
+ return 0;
+
+ return 1;
+ }
I've no idea what kvm_vcpu_exiting_guest_mode() is (it doesn't exist in
any tree I have access to)...
In any case, look at the version I converted to spinlocks and see whether
you think the code looks reasonable in that form. If it doesn't then it
isn't reasonable in atomic types either.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 12/13] ARM: KVM: vgic: reduce the number of vcpu kick
For the sake of public education, let me rewrite this patch a bit to
illustrate why atomic_t's are bad, and then people can review this
instead.
Every change I've made here is functionally equivalent to the behaviour
of the atomic type; I have not added any new bugs here that aren't
present in the original code.
It is my hope that through education like this, people will see that
atomic types have no magic properties, and their use does not make
code automatically race free and correct; in fact, the inappropriate
use of atomic types is pure obfuscation and causes confusion.
On Sat, Nov 10, 2012 at 04:45:39PM +0100, Christoffer Dall wrote:
> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
> index a8e7a93..7d2662c 100644
> --- a/arch/arm/include/asm/kvm_vgic.h
> +++ b/arch/arm/include/asm/kvm_vgic.h
> @@ -215,6 +215,9 @@ struct vgic_cpu {
> u32 vgic_elrsr[2]; /* Saved only */
> u32 vgic_apr;
> u32 vgic_lr[64];/* A15 has only 4... */
> +
> + /* Number of level-triggered interrupt in progress */
> + atomic_tirq_active_count;
+ int irq_active_count;
> #endif
> };
>
> @@ -254,6 +257,8 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct
> kvm_run *run,
>
> #define irqchip_in_kernel(k) (!!((k)->arch.vgic.vctrl_base))
> #define vgic_initialized(k) ((k)->arch.vgic.ready)
> +#define vgic_active_irq(v)
> (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) == 0)
> +
+#define vgic_active_irq(v) ((v)->arch.vgic_cpu.irq_active_count)
> #else
> static inline int kvm_vgic_hyp_init(void)
> {
> @@ -305,6 +310,11 @@ static inline bool vgic_initialized(struct kvm *kvm)
> {
> return true;
> }
> +
> +static inline int vgic_active_irq(struct kvm_vcpu *vcpu)
> +{
> + return 0;
> +}
> #endif
>
> #endif
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a633d9d..1716f12 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -94,7 +94,15 @@ int kvm_arch_hardware_enable(void *garbage)
>
> int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
> {
> - return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;
> + if (kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE) {
> + if (vgic_active_irq(vcpu) &&
> + cmpxchg(&vcpu->mode, EXITING_GUEST_MODE, IN_GUEST_MODE) ==
> EXITING_GUEST_MODE)
> + return 0;
So with the above change to the macro, this becomes:
+ if (vcpu->arch.vgic_cpu.irq_active_count &&
+ cmpxchg(&vcpu->mode, EXITING_GUEST_MODE, IN_GUEST_MODE) ==
EXITING_GUEST_MODE)
> +
> + return 1;
> + }
> +
> + return 0;
> }
>
> void kvm_arch_hardware_disable(void *garbage)
> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
> index 415ddb8..146de1d 100644
> --- a/arch/arm/kvm/vgic.c
> +++ b/arch/arm/kvm/vgic.c
> @@ -705,8 +705,10 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8
> sgi_source_id, int irq)
> kvm_debug("LR%d piggyback for IRQ%d %x\n", lr, irq,
> vgic_cpu->vgic_lr[lr]);
> BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
> vgic_cpu->vgic_lr[lr] |= VGIC_LR_PENDING_BIT;
> - if (is_level)
> + if (is_level) {
> vgic_cpu->vgic_lr[lr] |= VGIC_LR_EOI;
> + atomic_inc(&vgic_cpu->irq_active_count);
+ spin_lock_irqsave(&atomic_lock, flags);
+ vgic_cpu->irq_active_count++;
+ spin_unlock_irqrestore(&atomic_lock, flags);
> + }
> return true;
> }
>
> @@ -718,8 +720,10 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8
> sgi_source_id, int irq)
>
> kvm_debug("LR%d allocated for IRQ%d %x\n", lr, irq, sgi_source_id);
> vgic_cpu->vgic_lr[lr] = MK_LR_PEND(sgi_source_id, irq);
> - if (is_level)
> + if (is_level) {
> vgic_cpu->vgic_lr[lr] |= VGIC_LR_EOI;
> + atomic_inc(&vgic_cpu->irq_active_count);
+ spin_lock_irqsave(&atomic_lock, flags);
+ vgic_cpu->irq_active_count++;
+ spin_unlock_irqrestore(&atomic_lock, flags);
> + }
>
> vgic_cpu->vgic_irq_lr_map[irq] = lr;
> clear_bit(lr, (unsigned long *)vgic_cpu->vgic_elrsr);
> @@ -1011,6 +1015,8 @@ static irqreturn_t vgic_maintenance_handler(int irq,
> void *data)
>
> vgic_bitmap_set_irq_val(&dist->irq_active,
> vcpu->vcpu_id, irq, 0);
> + atomic_dec(&vgic_cpu->irq_active_count);
+ spin_lock_irqsave(&atomic_lock, flags);
+ vgic_cpu->irq_active_count--;
+ spin_unlock_irqrestore(&atomic_lock, flags);
> + smp_mb();
> vgic_cpu->vgic_lr[lr] &= ~VGIC_LR_EOI;
> wri
Re: [PATCH v4 12/13] ARM: KVM: vgic: reduce the number of vcpu kick
On Wed, Dec 05, 2012 at 10:43:58AM +, Will Deacon wrote:
> On Sat, Nov 10, 2012 at 03:45:39PM +, Christoffer Dall wrote:
> > From: Marc Zyngier
> >
> > If we have level interrupts already programmed to fire on a vcpu,
> > there is no reason to kick it after injecting a new interrupt,
> > as we're guaranteed that we'll exit when the level interrupt will
> > be EOId (VGIC_LR_EOI is set).
> >
> > The exit will force a reload of the VGIC, injecting the new interrupts.
> >
> > Signed-off-by: Marc Zyngier
> > Signed-off-by: Christoffer Dall
> > ---
> > arch/arm/include/asm/kvm_vgic.h | 10 ++
> > arch/arm/kvm/arm.c | 10 +-
> > arch/arm/kvm/vgic.c | 10 --
> > 3 files changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kvm_vgic.h
> > b/arch/arm/include/asm/kvm_vgic.h
> > index a8e7a93..7d2662c 100644
> > --- a/arch/arm/include/asm/kvm_vgic.h
> > +++ b/arch/arm/include/asm/kvm_vgic.h
> > @@ -215,6 +215,9 @@ struct vgic_cpu {
> > u32 vgic_elrsr[2]; /* Saved only */
> > u32 vgic_apr;
> > u32 vgic_lr[64];/* A15 has only 4... */
> > +
> > + /* Number of level-triggered interrupt in progress */
> > + atomic_tirq_active_count;
> > #endif
> > };
> >
> > @@ -254,6 +257,8 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct
> > kvm_run *run,
> >
> > #define irqchip_in_kernel(k) (!!((k)->arch.vgic.vctrl_base))
> > #define vgic_initialized(k)((k)->arch.vgic.ready)
> > +#define vgic_active_irq(v)
> > (atomic_read(&(v)->arch.vgic_cpu.irq_active_count) == 0)
>
> When is the atomic_t initialised to zero? I can only see increments.
I'd question whether an atomic type is correct for this; the only
protection that it's offering is to ensure that the atomic increment
and decrement occur atomically - there's nothing else that they're doing
in this code.
If those atomic increments and decrements are occuring beneath a common
lock, then using atomic types is just mere code obfuscation.
For example, I'd like to question the correctness of this:
+ if (vgic_active_irq(vcpu) &&
+ cmpxchg(&vcpu->mode, EXITING_GUEST_MODE, IN_GUEST_MODE) ==
EXITING_GUEST_MODE)
What if vgic_active_irq() reads the atomic type, immediately after it gets
decremented to zero before the cmpxchg() is executed? Would that be a
problem?
If yes, yet again this illustrates why the use of atomic types leads people
down the path of believing that their code somehow becomes magically safe
through the use of this smoke-screen. IMHO, every use of atomic_t must be
questioned and carefully analysed before it gets into the kernel - many
are buggy through assumptions that atomic_t buys you something magic.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 14/15] KVM: ARM: Handle I/O aborts
On Mon, Oct 01, 2012 at 01:53:26PM +0100, Dave Martin wrote: > A good starting point would be load/store emulation as this seems to be a > common theme, and we would need a credible deployment for any new > framework so that we know it's fit for purpose. Probably not actually, that code is written to be fast, because things like IP stack throughput depend on it - particularly when your network card can only DMA packets to 32-bit aligned addresses (resulting in virtually all network data being misaligned.) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/15] ARM: add mem_type prot_pte accessor
On Sat, Sep 15, 2012 at 11:34:36AM -0400, Christoffer Dall wrote: > From: Marc Zyngier > > The KVM hypervisor mmu code requires access to the mem_type prot_pte > field when setting up page tables pointing to a device. Unfortunately, > the mem_type structure is opaque. > > Add an accessor (get_mem_type_prot_pte()) to retrieve the prot_pte > value. > > Signed-off-by: Marc Zyngier > Signed-off-by: Christoffer Dall Is there a reason why we need this to be exposed, along with all the page table manipulation in patch 7? Is there a reason why we can't have new MT_ types for PAGE_HYP and the HYP MT_DEVICE type (which is the same as MT_DEVICE but with PTE_USER set) and have the standard ARM/generic kernel code build those mappings? That would (it seems) also avoid the need to export the pXd_clear_bad() acessors too... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
On Thu, Mar 01, 2012 at 10:27:02AM +, Dave Martin wrote: > So, where there's a compelling reason to inline these things, we can use > the existing techniques if we're alert to the risks. But in cases where > there isn't a compelling reason, aren't we just inviting fragility > unnecessarily? The practical experience from the kernel suggests that there isn't a problem - that's not to say that future versions of gcc won't become a problem, and that the compiler guys may refuse to fix it. I think it's a feature that we should be pressing gcc guys for - it's fairly fundamental to any programming which requires interfaces that require certain args in certain registers, or receive results in certain registers. The options over this are basically: 1. refusing to upgrade to any version of gcc which does not allow registers-in-asm 2. doing the store-to-memory reload-in-asm thing 3. hand-coding veneers for every call to marshall the registers Each of those has its down sides, but I suspect with (1), it may be possible to have enough people applying pressure to the compiler guys that they finally see sense on this matter. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
On Wed, Feb 29, 2012 at 02:44:24PM +, Ian Campbell wrote: > > If you need a specific register, this means that you must set up that > > register explicitly inside the asm if you want a guarantee that the > > code will work: > > > > asm volatile ( > > "movw r12, %[hvc_num]\n\t" > > Is gcc (or gas?) smart enough to optimise this away if it turns out that > %[hvc_num] == r12? No, and it won't do, because %[hvc_num] is specified in these operands: > > ... > > "hvc#0" > > :: [hvc_num] "i" (NUMBER) : "r12" to be an integer, not a register. > How are system calls implemented on the userspace side? I confess I > don't know what the ARM syscall ABI looks like -- is it all registers or > is some of it on the stack? It sounds like the solution ought to be > pretty similar though. All registers. We have a few which take a pointer to an in memory array, but those are for some old multiplexed syscalls. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
On Wed, Feb 29, 2012 at 12:58:26PM +, Dave Martin wrote:
> On Wed, Feb 29, 2012 at 09:56:02AM +, Ian Campbell wrote:
> > On Wed, 2012-02-29 at 09:34 +, Dave Martin wrote:
> > > On Tue, Feb 28, 2012 at 12:28:29PM +, Stefano Stabellini wrote:
> >
> > > > I don't have a very strong opinion on which register we should use, but
> > > > I would like to avoid r7 if it is already actively used by gcc.
> > >
> > > But there is no framepointer for Thumb-2 code (?)
> >
> > Peter Maydell suggested there was:
> > > r7 is (used by gcc as) the Thumb frame pointer; I don't know if this
> > > makes it worth avoiding in this context.
> >
> > Sounds like it might be a gcc-ism, possibly a non-default option?
> >
> > Anyway, I think r12 will be fine for our purposes so the point is rather
> > moot.
>
> Just had a chat with some tools guys -- apparently, when passing register
> arguments to gcc inline asms there really isn't a guarantee that those
> variables will be in the expected registers on entry to the inline asm.
The best you can do is:
register unsigned int foo asm("r7") = value;
asm("blah %0" : : "r" (foo));
and ensure that your assembly checks that %0 _is_ r7 and produces a build
error if not. See the __asmeq() macro in asm/system.h to find out how to
do that.
This feature has been missing from ARM GCC for quite a long time - it's
used extensively on x86 GCC, where they have one register class per
register, so they can do stuff like:
asm("blah %0" : : "a" (value));
and be guaranteed that %0 will be eax.
> If you need a specific register, this means that you must set up that
> register explicitly inside the asm if you want a guarantee that the
> code will work:
>
> asm volatile (
> "movw r12, %[hvc_num]\n\t"
> ...
> "hvc#0"
> :: [hvc_num] "i" (NUMBER) : "r12"
> );
>
> Of course, if you need to set up more than about 5 or 6 registers in
> this way, the doubled register footprint means that the compiler will
> have to start spilling stuff to the stack.
No it won't - it will barf instead - think about it. If you're clobbering
r0 - r5, but need to pass in six values in registers, gcc can't use r0-r5
for that, so it must use the remaining registers. It gets rather unhappy
with that, and starts erroring out (iirc 'too many reloads' or some similar
error.) I've been there.
If you want to do it that way, your only option is to store them to memory
and pass the address of the block into the assembly, and reload them there.
Which is extremely sucky and inefficient.
Practically, the register variable plus asm() does seem to work, and seems
to work for virtually all gcc versions out there (there have been the odd
buggy version, but those bugs appear to get fixed.)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
