Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
Hi Marc, On Wed, Mar 11, 2015 at 11:47 PM, Marc Zyngier wrote: > On 11/03/15 17:57, Feng Kan wrote: >> On Wed, Mar 11, 2015 at 10:31 AM, Marc Zyngier wrote: >>> On 11/03/15 17:19, Feng Kan wrote: On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier wrote: > On 27/01/15 07:03, Pranavkumar Sawargaonkar wrote: >> In APM X-Gene, GIC register space is 64K aligned while the sizes >> mentioned >> in the dt are 4K aligned. This breaks KVM when kernel is built with 64K >> page >> size due to size alignment checking in vgic driver for VCPU Control and >> VCPU register. >> >> This patch corrects the sizes to be inline with the hardware spec. > > This patch may be correct, but it is useless. The firmware on my APM > system (some version of u-boot) repaints the DT at boot time, negating > the effect of this patch. We have updated u-boot to reflect this change. I can supply you with a updated image if you wish. >>> >>> That would be useful, thanks. >>> >>> But more importantly, why bother upstreaming your DT into the kernel >>> tree if your firmware is going to overwrite whatever we provide? >> We did tried to submit a version upstream but was rejected. >> >>> >>> Either the firmware let the user provide its own DT (and doesn't touch >>> it other than to change the CPU enable method, insert a /memreserve/ or >>> similar things), or the firmware always provide its own DT, and doesn't >>> let the user provide its own. Corrupting the user DT is a disaster, as >>> we just found. >> Yes, the intent of the change is listed in the link below. It is not a >> justification by any means, >> just the effects of things appearing in layers. >> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/288574.html > > Yeah. This is as wrong as it can possibly be. Oh well... Yes there is an issue with u-boot patching the dt for end user who wants his DT to be used, for this we can (in fact we should) provide an option in u-boot (may be setting some environment variable) which will make sure end user's DT does not get modified (apart from standard things like patching mac-addresses) by u-boot. Another point I want to reopen here is the how to handle 64K GIC page size pointed out in this thread, what would be the best way to tackle this (adding a new DT string or any other way) ? Thanks, Pranav > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [PATCH v2 5/6] target-arm: kvm64 fix save/restore of SPSR regs
On Mon, Mar 9, 2015 at 8:26 AM, Christoffer Dall
wrote:
> On Wed, Mar 04, 2015 at 02:35:52PM +, Alex Bennée wrote:
>> From: Christoffer Dall
>>
>> The current code was negatively indexing the cpu state array and not
>> synchronizing banked spsr register state with the current mode's spsr
>> state, causing occasional failures with migration.
>>
>> Some munging is done to take care of the aarch64 mapping and also to
>> ensure the most current value of the spsr is updated to the banked
>> registers (relevant for KVM<->TCG migration).
>>
>> Signed-off-by: Christoffer Dall
>> Signed-off-by: Alex Bennée
>>
>> ---
>> v2 (ajb)
>> - minor tweaks and clarifications
>> v3
>> - Use the correct bank index function for setting/getting env->spsr
>> - only deal with spsrs in elevated exception levels
>>
>> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
>> index c60e989..45e5c3f 100644
>> --- a/target-arm/kvm64.c
>> +++ b/target-arm/kvm64.c
>> @@ -140,6 +140,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>> uint64_t val;
>> int i;
>> int ret;
>> +unsigned int el;
>>
>> ARMCPU *cpu = ARM_CPU(cs);
>> CPUARMState *env = &cpu->env;
>> @@ -206,9 +207,27 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>> return ret;
>> }
>>
>> +/* Saved Program State Registers
>> + *
>> + * Before we restore from the banked_spsr[] array we need to
>> + * ensure that any modifications to env->spsr are correctly
>> + * reflected and map aarch64 exception levels if required.
>> + */
>> +el = arm_current_el(env);
>> +if (el > 0) {
>> +if (is_a64(env)) {
>> +g_assert(el == 1);
>> +/* KVM only maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 ATM */
>
> not sure about the 'for aarch64' comment; I would say that it's for
> aarch32 support. Also, you can drop the ATM, since this is user space
> ABI that we don't change easily.
>
>
> don't you need to do env->banked_spsr[0] = env->spsr first?
I agree with Christoffer, env->spsr actually has the most current
value so you need to sync up with it before sending it out.
>
>> +env->banked_spsr[1] = env->banked_spsr[0];
>
>
>> +} else {
>> +i = bank_number(env->uncached_cpsr & CPSR_M);
>> +env->banked_spsr[i] = env->spsr;
>
> so here we don't need to worry about banked_spsr[1] = banked_spsr[0]
> because banked_spsr[0] is meaningless for 32-bit state and we only sync
> banked_spsr[1] and up to KVM, correct? I think this is what may deserve
> a comment.
>
>> +}
>> +}
>> +
>> for (i = 0; i < KVM_NR_SPSR; i++) {
>> reg.id = AARCH64_CORE_REG(spsr[i]);
>> -reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
>> +reg.addr = (uintptr_t) &env->banked_spsr[i+1];
>> ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
>> if (ret) {
>> return ret;
>> @@ -253,6 +272,7 @@ int kvm_arch_get_registers(CPUState *cs)
>> struct kvm_one_reg reg;
>> uint64_t val;
>> uint32_t fpr;
>> +unsigned int el;
>> int i;
>> int ret;
>>
>> @@ -325,15 +345,35 @@ int kvm_arch_get_registers(CPUState *cs)
>> return ret;
>> }
>>
>> +/* Fetch the SPSR registers
>> + *
>> + * KVM has an array of state indexed for all the possible aarch32
>> + * privilage levels. Although not all are valid at all points
>
> privilege
>
>> + * there are some transitions possible which can access old state
>> + * so it is worth keeping them all.
>> + */
>
> dubious comment overall
>
>> for (i = 0; i < KVM_NR_SPSR; i++) {
>> reg.id = AARCH64_CORE_REG(spsr[i]);
>> -reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
>> +reg.addr = (uintptr_t) &env->banked_spsr[i+1];
>> ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
>> if (ret) {
>> return ret;
>> }
>> }
>>
>> +el = arm_current_el(env);
>> +if (el > 0) {
>> +if (is_a64(env)) {
>> +g_assert(el == 1);
>> +/* KVM maps KVM_SPSR_SVC to KVM_SPSR_EL1 for aarch64 */
>
> same as above
If Christoffer's comment is referring to updating env->spsr, it occurs
below based on 'i'.
>
>> +env->banked_spsr[0] = env->banked_spsr[1];
>> +i = aarch64_banked_spsr_index(el);
>> +} else {
>> +i = bank_number(env->uncached_cpsr & CPSR_M);
>
> same potential place for comment as above.
>
>> +}
>> +env->spsr = env->banked_spsr[i];
>> +}
>> +
>> /* Advanced SIMD and FP registers */
>> for (i = 0; i < 32; i++) {
>> reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
>> --
>> 2.3.1
>>
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC/WIP PATCH 6/6] memory: add clear_cache_to_poc
On Fri, Mar 06, 2015 at 01:53:38PM -0500, Andrew Jones wrote:
> Add a function that flushes the cache to PoC. We need a new
> function because __builtin___clear_cache only flushes to
> PoU. Call this function each time an address in a memory
> region that has been flagged as having an incoherent cache
> is written. For starters we only implement it for ARM. Most
> other architectures don't need it anyway.
I started looking for my missing flushes, and see I have stupidity
in this patch. I'm not flushing in the right place at all... My
testing was just [un]lucky, making me think it was on the right
track. I'll send an update to this tomorrow after I remove my head
from a dark hole near my chair.
drew
>
> Signed-off-by: Andrew Jones
> ---
> Currently only implemented for aarch64, doesn't completely work yet.
>
> exec.c | 16 ++--
> include/exec/exec-all.h | 41 +
> 2 files changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index c85321a38ba69..68268a5961ff5 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2261,7 +2261,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
> addr,
>
> #else
>
> -static void invalidate_and_set_dirty(hwaddr addr,
> +static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
> hwaddr length)
> {
> if (cpu_physical_memory_range_includes_clean(addr, length)) {
> @@ -2269,6 +2269,10 @@ static void invalidate_and_set_dirty(hwaddr addr,
> cpu_physical_memory_set_dirty_range_nocode(addr, length);
> }
> xen_modified_memory(addr, length);
> +if (memory_region_has_incoherent_cache(mr)) {
> +char *start = qemu_get_ram_ptr(addr);
> +clear_cache_to_poc(start, start + length);
> +}
> }
>
> static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
> @@ -2348,7 +2352,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr,
> uint8_t *buf,
> /* RAM case */
> ptr = qemu_get_ram_ptr(addr1);
> memcpy(ptr, buf, l);
> -invalidate_and_set_dirty(addr1, l);
> +invalidate_and_set_dirty(mr, addr1, l);
> }
> } else {
> if (!memory_access_is_direct(mr, is_write)) {
> @@ -2437,7 +2441,7 @@ static inline void
> cpu_physical_memory_write_rom_internal(AddressSpace *as,
> switch (type) {
> case WRITE_DATA:
> memcpy(ptr, buf, l);
> -invalidate_and_set_dirty(addr1, l);
> +invalidate_and_set_dirty(mr, addr1, l);
> break;
> case FLUSH_CACHE:
> flush_icache_range((uintptr_t)ptr, (uintptr_t)ptr + l);
> @@ -2622,7 +2626,7 @@ void address_space_unmap(AddressSpace *as, void
> *buffer, hwaddr len,
> mr = qemu_ram_addr_from_host(buffer, &addr1);
> assert(mr != NULL);
> if (is_write) {
> -invalidate_and_set_dirty(addr1, access_len);
> +invalidate_and_set_dirty(mr, addr1, access_len);
> }
> if (xen_enabled()) {
> xen_invalidate_map_cache_entry(buffer);
> @@ -2904,7 +2908,7 @@ static inline void stl_phys_internal(AddressSpace *as,
> stl_p(ptr, val);
> break;
> }
> -invalidate_and_set_dirty(addr1, 4);
> +invalidate_and_set_dirty(mr, addr1, 4);
> }
> }
>
> @@ -2967,7 +2971,7 @@ static inline void stw_phys_internal(AddressSpace *as,
> stw_p(ptr, val);
> break;
> }
> -invalidate_and_set_dirty(addr1, 2);
> +invalidate_and_set_dirty(mr, addr1, 2);
> }
> }
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 8eb0db3910e86..9bf74e791f357 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -106,6 +106,43 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>hwaddr paddr, int prot,
>int mmu_idx, target_ulong size);
> void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr);
> +#if defined(__aarch64__)
> +static inline void clear_cache_to_poc(char *begin, char *end)
> +{
> +/* Unfortunately __builtin___clear_cache only flushes
> + * to PoU, we need to implement this for PoC.
> + */
> +static unsigned long line_sz = 0;
> +unsigned long start, stop, addr;
> +
> +if (!line_sz) {
> +unsigned int ctr_el0;
> +asm volatile("mrs %0, ctr_el0" : "=&r" (ctr_el0));
> +line_sz = (1UL << ((ctr_el0 >> 16) & 0xf)) * sizeof(int);
> +}
> +
> +start = (unsigned long)begin & ~(line_sz - 1);
> +stop = ((unsigned long)(end + line_sz) & ~(line_sz - 1));
> +
> +for (addr = start; addr < stop; addr += line_sz) {
> +asm volatile("dc cvac, %0" : : "r" (addr));
> +}
> +
> +/* FIXME: Ideally, we'd also flush the icache now, just in
> + * c
Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
On 11/03/15 17:57, Feng Kan wrote: > On Wed, Mar 11, 2015 at 10:31 AM, Marc Zyngier wrote: >> On 11/03/15 17:19, Feng Kan wrote: >>> On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier wrote: On 27/01/15 07:03, Pranavkumar Sawargaonkar wrote: > In APM X-Gene, GIC register space is 64K aligned while the sizes mentioned > in the dt are 4K aligned. This breaks KVM when kernel is built with 64K > page > size due to size alignment checking in vgic driver for VCPU Control and > VCPU register. > > This patch corrects the sizes to be inline with the hardware spec. This patch may be correct, but it is useless. The firmware on my APM system (some version of u-boot) repaints the DT at boot time, negating the effect of this patch. >>> We have updated u-boot to reflect this change. I can supply you with a >>> updated >>> image if you wish. >> >> That would be useful, thanks. >> >> But more importantly, why bother upstreaming your DT into the kernel >> tree if your firmware is going to overwrite whatever we provide? > We did tried to submit a version upstream but was rejected. > >> >> Either the firmware let the user provide its own DT (and doesn't touch >> it other than to change the CPU enable method, insert a /memreserve/ or >> similar things), or the firmware always provide its own DT, and doesn't >> let the user provide its own. Corrupting the user DT is a disaster, as >> we just found. > Yes, the intent of the change is listed in the link below. It is not a > justification by any means, > just the effects of things appearing in layers. > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/288574.html Yeah. This is as wrong as it can possibly be. Oh well... Thanks, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
On 11/03/15 17:19, Feng Kan wrote: > On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier wrote: >> On 27/01/15 07:03, Pranavkumar Sawargaonkar wrote: >>> In APM X-Gene, GIC register space is 64K aligned while the sizes mentioned >>> in the dt are 4K aligned. This breaks KVM when kernel is built with 64K page >>> size due to size alignment checking in vgic driver for VCPU Control and >>> VCPU register. >>> >>> This patch corrects the sizes to be inline with the hardware spec. >> >> This patch may be correct, but it is useless. The firmware on my APM >> system (some version of u-boot) repaints the DT at boot time, negating >> the effect of this patch. > We have updated u-boot to reflect this change. I can supply you with a updated > image if you wish. That would be useful, thanks. But more importantly, why bother upstreaming your DT into the kernel tree if your firmware is going to overwrite whatever we provide? Either the firmware let the user provide its own DT (and doesn't touch it other than to change the CPU enable method, insert a /memreserve/ or similar things), or the firmware always provide its own DT, and doesn't let the user provide its own. Corrupting the user DT is a disaster, as we just found. Thanks, M. >> >> Another system I can remove from my 64k-capable list. >> >> M. >> >>> CC: linux-arm-ker...@lists.infradead.org >>> CC: kvmarm@lists.cs.columbia.edu >>> CC: a...@arndb.de >>> CC: marc.zyng...@arm.com >>> CC: christoffer.d...@linaro.org >>> CC: j...@redhat.com >>> Signed-off-by: Pranavkumar Sawargaonkar >>> Signed-off-by: Tushar Jagad >>> Signed-off-by: Feng Kan >>> --- >>> arch/arm64/boot/dts/apm/apm-storm.dtsi |8 >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi >>> b/arch/arm64/boot/dts/apm/apm-storm.dtsi >>> index f1ad9c2..65f0e6d 100644 >>> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi >>> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi >>> @@ -81,10 +81,10 @@ >>> compatible = "arm,cortex-a15-gic"; >>> #interrupt-cells = <3>; >>> interrupt-controller; >>> - reg = <0x0 0x7801 0x0 0x1000>, /* GIC Dist */ >>> - <0x0 0x7802 0x0 0x1000>, /* GIC CPU */ >>> - <0x0 0x7804 0x0 0x2000>, /* GIC VCPU Control */ >>> - <0x0 0x7806 0x0 0x2000>; /* GIC VCPU */ >>> + reg = <0x0 0x7801 0x0 0x1>, /* GIC Dist */ >>> + <0x0 0x7802 0x0 0x2>, /* GIC CPU */ >>> + <0x0 0x7804 0x0 0x1>, /* GIC VCPU Control */ >>> + <0x0 0x7806 0x0 0x2>; /* GIC VCPU */ >>> interrupts = <1 9 0xf04>; /* GIC Maintenence IRQ */ >>> }; >>> >>> -- >>> 1.7.9.5 >>> >>> >> >> >> -- >> Jazz is not dead. It just smells funny... > -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
On Wed, Mar 11, 2015 at 7:53 AM, Marc Zyngier wrote: > On 27/01/15 07:03, Pranavkumar Sawargaonkar wrote: >> In APM X-Gene, GIC register space is 64K aligned while the sizes mentioned >> in the dt are 4K aligned. This breaks KVM when kernel is built with 64K page >> size due to size alignment checking in vgic driver for VCPU Control and >> VCPU register. >> >> This patch corrects the sizes to be inline with the hardware spec. > > This patch may be correct, but it is useless. The firmware on my APM > system (some version of u-boot) repaints the DT at boot time, negating > the effect of this patch. We have updated u-boot to reflect this change. I can supply you with a updated image if you wish. > > Another system I can remove from my 64k-capable list. > > M. > >> CC: linux-arm-ker...@lists.infradead.org >> CC: kvmarm@lists.cs.columbia.edu >> CC: a...@arndb.de >> CC: marc.zyng...@arm.com >> CC: christoffer.d...@linaro.org >> CC: j...@redhat.com >> Signed-off-by: Pranavkumar Sawargaonkar >> Signed-off-by: Tushar Jagad >> Signed-off-by: Feng Kan >> --- >> arch/arm64/boot/dts/apm/apm-storm.dtsi |8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi >> b/arch/arm64/boot/dts/apm/apm-storm.dtsi >> index f1ad9c2..65f0e6d 100644 >> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi >> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi >> @@ -81,10 +81,10 @@ >> compatible = "arm,cortex-a15-gic"; >> #interrupt-cells = <3>; >> interrupt-controller; >> - reg = <0x0 0x7801 0x0 0x1000>, /* GIC Dist */ >> - <0x0 0x7802 0x0 0x1000>, /* GIC CPU */ >> - <0x0 0x7804 0x0 0x2000>, /* GIC VCPU Control */ >> - <0x0 0x7806 0x0 0x2000>; /* GIC VCPU */ >> + reg = <0x0 0x7801 0x0 0x1>, /* GIC Dist */ >> + <0x0 0x7802 0x0 0x2>, /* GIC CPU */ >> + <0x0 0x7804 0x0 0x1>, /* GIC VCPU Control */ >> + <0x0 0x7806 0x0 0x2>; /* GIC VCPU */ >> interrupts = <1 9 0xf04>; /* GIC Maintenence IRQ */ >> }; >> >> -- >> 1.7.9.5 >> >> > > > -- > Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v5] hw/intc/arm_gic: Initialize the vgic in the realize function
This patch forces vgic initialization in the vgic realize function.
It uses a new group/attribute that allows such operation:
KVM_DEV_ARM_VGIC_GRP_CTRL/KVM_DEV_ARM_VGIC_CTRL_INIT
This earlier initialization allows, for example, to setup VFIO
signaling and irqfd after vgic initialization, on a reset notifier.
Signed-off-by: Eric Auger
---
v4 -> v5:
- add a comment to clarify what the new gic accesses aim at
- header sync removed. The series becomes a singleton.
v1 -> v2:
- The init is not mandated to be done in a machine init done notifier
anymore since only the number of vcpus and number of IRQs must be
known at init time.
---
hw/intc/arm_gic_kvm.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 1ad3eb0..0d20750 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -573,6 +573,13 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error
**errp)
kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0, &numirqs, 1);
}
+/* Tell the kernel to complete VGIC initialization now */
+if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
+ KVM_DEV_ARM_VGIC_CTRL_INIT)) {
+kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
+ KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
+}
+
/* Distributor */
memory_region_init_reservation(&s->iomem, OBJECT(s),
"kvm-gic_dist", 0x1000);
--
1.8.3.2
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v14 00/20] VFIO support for platform devices
Ok, from what I've seen : > - nit: interrupt.h not needed at early stage in vfio_platform_private.h, > until irq introduction This cannot be fixed by a follow-on patch as it is just the order in the patch series. > - version of the driver: 0.10? Let's just keep it like that, moreover I didn't add any functionnality since v10. Which leaves only the wait.h in vfio_pci_intrs.c as an issue. Do we really need a patch only to remove an include ? (And it seems that this is already included by eventfd.h, so we don't need to move it to vfio.h). Finally everything seems ready for upstream, thanks everyone. Best regards, Baptiste On Wed, Mar 11, 2015 at 4:52 PM, Eric Auger wrote: > On 03/11/2015 11:08 AM, Baptiste Reynal wrote: >> Thanks Eric and Alex for your reviews. >> >> I can confirm we can drop the dependency, as I re-tested without it. >> Do you need a fix for the headers issue underlined by Eric ? > > Hi Baptiste, > > As far as I am concerned I do not have such request ;-) > > Best Regards > > Eric >> >> Best regards, >> Baptiste >> >> On Wed, Mar 11, 2015 at 9:40 AM, Eric Auger wrote: >>> On 03/10/2015 07:04 PM, Alex Williamson wrote: On Tue, 2015-03-10 at 18:42 +0100, Eric Auger wrote: > Hi Baptiste, > > Please add: > Reviewed-by: Eric Auger on the whole series > Tested-by: Eric Auger on the whole series except > AMBA specific patches. > > I currently exercise the following functionalities on Calxeda Midway HW: > - MMIO read/write/mmap, > - automasked IRQ + mask/unmask (NONE flag), > - virqfd (unmask handler only). > > Despite a new thorough review I failed in pointing out any new > interesting issues in this series. > > just minor points below: > > - PIO regions: currently read/write/mmap is not supported. I can't > figure out how much this is a problem at this point? AIUI, nobody has any way to test this, which is why it's not implemented. I think we've done due diligence in accommodating PIO into the vfio-platform framework though and hopefully it will be a simple matter to add when someone has a use case for it. > - nit: vfio_pci_intrs.c: may not need wait.h anymore after removal of > virqfd code. may be added in vfio.h > - nit: interrupt.h not needed at early stage in vfio_platform_private.h, > until irq introduction > - version of the driver: 0.10? None of these seem like blockers, I'd be willing to accept header cleanups as follow-on and driver versions are mostly arbitrary anyway. > Since there is no real dependency on "[PATCH v4 0/6] vfio: type1: > support for ARM SMMUS with VFIO_IOMMU_TYPE1" I hope we can get this > upstreamed soon. Me too, I also re-reviewed the series yesterday. If Baptiste agrees that there's no dependency on the other series, I can add your R-b/T-b, do some additional testing, and queue the series for linux-next. Thanks, >>> >>> Hi Alex, >>> >>> I let Baptiste answer. If he agrees please do as proposed. I am looking >>> forward to seeing the series in linux-next! >>> >>> Best Regards >>> >>> Eric Alex >>> > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v9 5/5] KVM: arm/arm64: add irqfd support
On 03/11/2015 03:24 PM, Christoffer Dall wrote: > On Thu, Mar 05, 2015 at 12:27:42PM +0100, Paolo Bonzini wrote: >> >> >> On 05/03/2015 11:53, Marc Zyngier wrote: +#ifdef CONFIG_HAVE_KVM_IRQFD + case KVM_CAP_IRQFD: + r = vgic_present; + break; +#endif >>> >>> Nitpick: we have "select HAVE_KVM_IRQFD", so we can lose the #ifdef-ery. >> >> Alternatively, I've just posted a patch to move the KVM_CAP_IRQFD case >> to common code. >> >> vgic_present probably should be replaced by >> IS_ENABLED(CONFIG_KVM_ARM_VGIC). I've sent a patch for this. >> > I have applied all of this together with my kill-the-vgic-config-option > patch here: > > git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git queue > > Take a look please. Hi Christoffer, This looks good to me. Thanks for the integration. - Eric > > Thanks, > -Christoffer > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v14 00/20] VFIO support for platform devices
On Wed, 2015-03-11 at 11:08 +0100, Baptiste Reynal wrote: > Thanks Eric and Alex for your reviews. > > I can confirm we can drop the dependency, as I re-tested without it. > Do you need a fix for the headers issue underlined by Eric ? Sure, send it as a follow-on patch though, don't respin the series for that. Thanks, Alex > On Wed, Mar 11, 2015 at 9:40 AM, Eric Auger wrote: > > On 03/10/2015 07:04 PM, Alex Williamson wrote: > >> On Tue, 2015-03-10 at 18:42 +0100, Eric Auger wrote: > >>> Hi Baptiste, > >>> > >>> Please add: > >>> Reviewed-by: Eric Auger on the whole series > >>> Tested-by: Eric Auger on the whole series except > >>> AMBA specific patches. > >>> > >>> I currently exercise the following functionalities on Calxeda Midway HW: > >>> - MMIO read/write/mmap, > >>> - automasked IRQ + mask/unmask (NONE flag), > >>> - virqfd (unmask handler only). > >>> > >>> Despite a new thorough review I failed in pointing out any new > >>> interesting issues in this series. > >>> > >>> just minor points below: > >>> > >>> - PIO regions: currently read/write/mmap is not supported. I can't > >>> figure out how much this is a problem at this point? > >> > >> AIUI, nobody has any way to test this, which is why it's not > >> implemented. I think we've done due diligence in accommodating PIO into > >> the vfio-platform framework though and hopefully it will be a simple > >> matter to add when someone has a use case for it. > >> > >>> - nit: vfio_pci_intrs.c: may not need wait.h anymore after removal of > >>> virqfd code. may be added in vfio.h > >>> - nit: interrupt.h not needed at early stage in vfio_platform_private.h, > >>> until irq introduction > >>> - version of the driver: 0.10? > >> > >> None of these seem like blockers, I'd be willing to accept header > >> cleanups as follow-on and driver versions are mostly arbitrary anyway. > >> > >>> Since there is no real dependency on "[PATCH v4 0/6] vfio: type1: > >>> support for ARM SMMUS with VFIO_IOMMU_TYPE1" I hope we can get this > >>> upstreamed soon. > >> > >> Me too, I also re-reviewed the series yesterday. If Baptiste agrees > >> that there's no dependency on the other series, I can add your R-b/T-b, > >> do some additional testing, and queue the series for linux-next. > >> Thanks, > > > > Hi Alex, > > > > I let Baptiste answer. If he agrees please do as proposed. I am looking > > forward to seeing the series in linux-next! > > > > Best Regards > > > > Eric > >> > >> Alex > >> > > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 6/6] target-arm/cpu.h: document why env->spsr exists
On 25 February 2015 at 16:02, Alex Bennée wrote:
> I was getting very confused about the duplication of state. Perhaps we
> should just get rid of env->spsr and use helpers that understand the
> banking?
I've already disagreed with this. I would suggest putting
tentative questions about future direction of the codebase
below the '---' rather than in the commit log :-)
> Signed-off-by: Alex Bennée
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 11845a6..d7fd13f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -155,6 +155,11 @@ typedef struct CPUARMState {
> This contains all the other bits. Use cpsr_{read,write} to access
> the whole CPSR. */
> uint32_t uncached_cpsr;
> +/* The spsr is a alias for spsr_elN where N is the current
> + * exception level.
This isn't true for AArch32 (which has multiple different SPSRs
any of which might be the one in env->spsr when we're at EL1).
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [PATCH 6/6] target-arm/cpu.h: document why env->spsr exists
On Wed, Feb 25, 2015 at 10:02 AM, Alex Bennée wrote:
> I was getting very confused about the duplication of state. Perhaps we
> should just get rid of env->spsr and use helpers that understand the
> banking?
>
> Signed-off-by: Alex Bennée
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 11845a6..d7fd13f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -155,6 +155,11 @@ typedef struct CPUARMState {
> This contains all the other bits. Use cpsr_{read,write} to access
> the whole CPSR. */
> uint32_t uncached_cpsr;
> +/* The spsr is a alias for spsr_elN where N is the current
"an alias"
> + * exception level. It is provided for here so the TCG msr/mrs
remove "for here"
> + * implementation can access one register. Care needs to be taken
> + * to ensure the banked_spsr[] is also updated.
> + */
> uint32_t spsr;
>
> /* Banked registers. */
> --
> 2.3.0
>
>
Otherwise...
Reviewed-by: Greg Bellows
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [PATCH v2 4/6] target-arm: kvm64 sync FP register state
On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée wrote:
> For migration to work we need to sync all of the register state. This is
> especially noticeable when GCC starts using FP registers as spill
> registers even with integer programs.
>
> Signed-off-by: Alex Bennée
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 8cf3a62..c60e989 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -126,9 +126,17 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
> #define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
> KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>
> +/* The linux headers don't define a 128 bit wide SIMD macro for us */
> +#define AARCH64_SIMD_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U128 | \
> + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
> +
> +#define AARCH64_SIMD_CTRL_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
> + KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
> +
> int kvm_arch_put_registers(CPUState *cs, int level)
> {
> struct kvm_one_reg reg;
> +uint32_t fpr;
> uint64_t val;
> int i;
> int ret;
> @@ -207,13 +215,36 @@ int kvm_arch_put_registers(CPUState *cs, int level)
> }
> }
>
> +/* Advanced SIMD and FP registers */
> +for (i = 0; i < 32; i++) {
> +reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> +reg.addr = (uintptr_t)(&env->vfp.regs[i]);
> +ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
> +if (ret) {
> +return ret;
> +}
> +reg.id++;
What does this increment do? It appears that it just gets thrown away
on the next iteration of the loop unless ioctl(SET) return something,
but maybe I am missing something.
> +}
> +
> +reg.addr = (uintptr_t)(&fpr);
> +fpr = vfp_get_fpsr(env);
> +reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
> +ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
> +if (ret) {
> +return ret;
> +}
> +
> +fpr = vfp_get_fpcr(env);
> +reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
> +ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, ®);
> +if (ret) {
> +return ret;
> +}
> +
> if (!write_list_to_kvmstate(cpu)) {
> return EINVAL;
> }
>
> -/* TODO:
> - * FP state
> - */
> return ret;
> }
>
> @@ -221,6 +252,7 @@ int kvm_arch_get_registers(CPUState *cs)
> {
> struct kvm_one_reg reg;
> uint64_t val;
> +uint32_t fpr;
> int i;
> int ret;
>
> @@ -302,9 +334,36 @@ int kvm_arch_get_registers(CPUState *cs)
> }
> }
>
> +/* Advanced SIMD and FP registers */
> +for (i = 0; i < 32; i++) {
> +reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> +reg.addr = (uintptr_t)(&env->vfp.regs[i]);
> +ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
> +if (ret) {
> +return ret;
> +}
> +reg.id++;
> +}
> +
> +reg.addr = (uintptr_t)(&fpr);
> +reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
> +ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
> +if (ret) {
> +return ret;
> +}
> +vfp_set_fpsr(env, fpr);
> +
> +reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
> +ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, ®);
> +if (ret) {
> +return ret;
> +}
> +vfp_set_fpcr(env, fpr);
> +
> if (!write_kvmstate_to_list(cpu)) {
> return EINVAL;
> }
> +
> /* Note that it's OK to have registers which aren't in CPUState,
> * so we can ignore a failure return here.
> */
> --
> 2.3.1
>
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] arm/arm64: KVM: Kill CONFIG_KVM_ARM_{VGIC,TIMER}
On 11/03/15 14:23, Christoffer Dall wrote: > We can definitely decide at run-time whether to use the GIC and timers > or not, and the extra code and data structures that we allocate space > for is really negligable with this config option, so I don't think it's > worth the extra complexity of always having to define stub static > inlines. The !CONFIG_KVM_ARM_VGIC/TIMER case is pretty much an untested > code path anyway, so we're better off just getting rid of it. > > Signed-off-by: Christoffer Dall Acked-by: Marc Zyngier Thanks for putting this together. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v4 3/3] hw/intc/arm_gic: Initialize the vgic in the realize function
On 4 March 2015 at 14:16, Eric Auger wrote:
> This patch forces vgic initialization in the vgic realize function.
> It uses a new group/attribute that allows such operation:
> KVM_DEV_ARM_VGIC_GRP_CTRL/KVM_DEV_ARM_VGIC_CTRL_INIT
>
> This earlier initialization allows, for example, to setup VFIO
> signaling and irqfd after vgic initialization, on a reset notifier.
>
> Signed-off-by: Eric Auger
> ---
> hw/intc/arm_gic_kvm.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 1ad3eb0..1395c9e 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -573,6 +573,12 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error
> **errp)
> kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_NR_IRQS, 0, 0, &numirqs, 1);
> }
>
> +if (kvm_gic_supports_attr(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> + KVM_DEV_ARM_VGIC_CTRL_INIT)) {
> +kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CTRL,
> + KVM_DEV_ARM_VGIC_CTRL_INIT, 0, 0, 1);
> +}
This is opaque enough that I think a comment in front of it
would be nice; something like
/* Tell the kernel to complete initialization of the VGIC now */
Otherwise
Reviewed-by: Peter Maydell
thanks
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] arm64: dts: Fix GIC reg sizes for APM X-Gene
On 27/01/15 07:03, Pranavkumar Sawargaonkar wrote: > In APM X-Gene, GIC register space is 64K aligned while the sizes mentioned > in the dt are 4K aligned. This breaks KVM when kernel is built with 64K page > size due to size alignment checking in vgic driver for VCPU Control and > VCPU register. > > This patch corrects the sizes to be inline with the hardware spec. This patch may be correct, but it is useless. The firmware on my APM system (some version of u-boot) repaints the DT at boot time, negating the effect of this patch. Another system I can remove from my 64k-capable list. M. > CC: linux-arm-ker...@lists.infradead.org > CC: kvmarm@lists.cs.columbia.edu > CC: a...@arndb.de > CC: marc.zyng...@arm.com > CC: christoffer.d...@linaro.org > CC: j...@redhat.com > Signed-off-by: Pranavkumar Sawargaonkar > Signed-off-by: Tushar Jagad > Signed-off-by: Feng Kan > --- > arch/arm64/boot/dts/apm/apm-storm.dtsi |8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi > b/arch/arm64/boot/dts/apm/apm-storm.dtsi > index f1ad9c2..65f0e6d 100644 > --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi > +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi > @@ -81,10 +81,10 @@ > compatible = "arm,cortex-a15-gic"; > #interrupt-cells = <3>; > interrupt-controller; > - reg = <0x0 0x7801 0x0 0x1000>, /* GIC Dist */ > - <0x0 0x7802 0x0 0x1000>, /* GIC CPU */ > - <0x0 0x7804 0x0 0x2000>, /* GIC VCPU Control */ > - <0x0 0x7806 0x0 0x2000>; /* GIC VCPU */ > + reg = <0x0 0x7801 0x0 0x1>, /* GIC Dist */ > + <0x0 0x7802 0x0 0x2>, /* GIC CPU */ > + <0x0 0x7804 0x0 0x1>, /* GIC VCPU Control */ > + <0x0 0x7806 0x0 0x2>; /* GIC VCPU */ > interrupts = <1 9 0xf04>; /* GIC Maintenence IRQ */ > }; > > -- > 1.7.9.5 > > -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [PATCH v2 3/6] hw/char: pl011 don't keep setting the IRQ if nothing changed
On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée wrote:
> While observing KVM traces I can see additional IRQ calls on pretty much
> every MMIO access which is just plain inefficient. Only update the QEMU
> IRQ level if something has actually changed from last time. Otherwise we
> may be papering over other failure modes.
>
> Signed-off-by: Alex Bennée
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 0a45115..bb554bc 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -36,6 +36,9 @@ typedef struct PL011State {
> CharDriverState *chr;
> qemu_irq irq;
> const unsigned char *id;
> +
> +/* not serialised, prevents pl011_update doing extra set_irqs */
> +uint32_t current_irq;
> } PL011State;
>
> #define PL011_INT_TX 0x20
> @@ -53,10 +56,11 @@ static const unsigned char pl011_id_luminary[8] =
>
> static void pl011_update(PL011State *s)
> {
> -uint32_t flags;
> -
> -flags = s->int_level & s->int_enabled;
> -qemu_set_irq(s->irq, flags != 0);
> +uint32_t flags = s->int_level & s->int_enabled;
> +if (flags != s->current_irq) {
> +s->current_irq = flags;
> +qemu_set_irq(s->irq, s->current_irq != 0);
> +}
> }
>
> static uint64_t pl011_read(void *opaque, hwaddr offset,
> --
> 2.3.1
>
>
Did you find the source of the additional IRQs? This seems like it
could be potentially be hiding an issue. From looking at the code, it
appears that one source of the additional updates is in pl011_read,
where we possibly do an update without a change in int_level. I
wonder if finding and fixing the updates is a better approach.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [PATCH v2 1/6] target-arm: kvm: save/restore mp state
On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée wrote:
> This adds the saving and restore of the current Multi-Processing state
> of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
> potential states for x86 we only use two for ARM. Either the process is
> running or not. We then save this state into the cpu_powered TCG state
> to avoid changing the serialisation format.
>
> Signed-off-by: Alex Bennée
>
> ---
> v2
> - make mpstate field runtime dependant (kvm_enabled())
> - drop initial KVM_CAP_MP_STATE requirement
> - re-use cpu_powered instead of new field
>
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 9446e5a..185f9a2 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -161,6 +161,7 @@ static const VMStateInfo vmstate_cpsr = {
> .put = put_cpsr,
> };
>
> +
>
remove extraneous space
> static void cpu_pre_save(void *opaque)
> {
> ARMCPU *cpu = opaque;
> @@ -170,6 +171,20 @@ static void cpu_pre_save(void *opaque)
> /* This should never fail */
> abort();
> }
> +#if defined CONFIG_KVM
>
The convention for ifdefing KVMatures appears to be more around the
feature rather than KVM_CONFIG. For instance loo fek at
kvm_check_extension in kvm-all.c. I may be missing something but if you
follow this convention this should be
#ifdef KVM_CAP_MP_STATE
> +if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
> +struct kvm_mp_state mp_state;
> +int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE,
> &mp_state);
> +if (ret) {
> +fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
> +__func__, ret, strerror(ret));
> +abort();
> +}
> +cpu->powered_off =
> +(mp_state.mp_state == KVM_MP_STATE_RUNNABLE)
> +? false : true;
> +}
> +#endif
> } else {
> if (!write_cpustate_to_list(cpu)) {
> /* This should never fail. */
> @@ -222,6 +237,20 @@ static int cpu_post_load(void *opaque, int version_id)
> * we're using it.
> */
> write_list_to_cpustate(cpu);
> +#if defined CONFIG_KVM
> +if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
> +struct kvm_mp_state mp_state = {
> +.mp_state =
> +cpu->powered_off ? KVM_MP_STATE_HALTED :
> KVM_MP_STATE_RUNNABLE
> +};
> +int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE,
> &mp_state);
> +if (ret) {
> +fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n",
> +__func__, ret, strerror(ret));
> +return -1;
> +}
> +}
> +#endif
> } else {
> if (!write_list_to_cpustate(cpu)) {
> return -1;
> --
> 2.3.1
>
>
> Besides these the above nits...
Reviewed-by: Greg Bellows
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [Qemu-devel] [PATCH v2 2/6] hw/intc: arm_gic_kvm.c restore config first
On Wed, Mar 4, 2015 at 8:35 AM, Alex Bennée wrote: > As there is logic to deal with the difference between edge and level > triggered interrupts in the kernel we must ensure it knows the > configuration of the IRQs before we restore the pending state. > > Signed-off-by: Alex Bennée > Acked-by: Christoffer Dall > > diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c > index 1ad3eb0..2f21ae7 100644 > --- a/hw/intc/arm_gic_kvm.c > +++ b/hw/intc/arm_gic_kvm.c > @@ -370,6 +370,11 @@ static void kvm_arm_gic_put(GICState *s) > * the appropriate CPU interfaces in the kernel) */ > kvm_dist_put(s, 0x800, 8, s->num_irq, translate_targets); > > +/* irq_state[n].trigger -> GICD_ICFGRn > + * (restore targets before pending IRQs so we treat level/edge > + * correctly */ > +kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger); > + > /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */ > kvm_dist_put(s, 0x280, 1, s->num_irq, translate_clear); > kvm_dist_put(s, 0x200, 1, s->num_irq, translate_pending); > @@ -378,8 +383,6 @@ static void kvm_arm_gic_put(GICState *s) > kvm_dist_put(s, 0x380, 1, s->num_irq, translate_clear); > kvm_dist_put(s, 0x300, 1, s->num_irq, translate_active); > > -/* irq_state[n].trigger -> GICD_ICFRn */ > -kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger); > > /* s->priorityX[irq] -> ICD_IPRIORITYRn */ > kvm_dist_put(s, 0x400, 8, s->num_irq, translate_priority); > -- > 2.3.1 > > Reviewed-by: Greg Bellows ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/3] arm64: KVM: Fix stage-2 PGD allocation to have per-page refcounting
On Wed, Mar 11, 2015 at 3:03 PM, Andrew Jones wrote:
> On Wed, Mar 11, 2015 at 02:20:56PM +0100, Christoffer Dall wrote:
>> On Wed, Mar 11, 2015 at 2:13 PM, Marc Zyngier wrote:
>> > Hi Christoffer,
>> >
>> > On 11/03/15 12:16, Christoffer Dall wrote:
>> >> Hi Marc,
>> >>
>> >> On Tue, Mar 10, 2015 at 07:06:59PM +, Marc Zyngier wrote:
>> >>> We're using __get_free_pages with to allocate the guest's stage-2
>> >>> PGD. The standard behaviour of this function is to return a set of
>> >>> pages where only the head page has a valid refcount.
>> >>>
>> >>> This behaviour gets us into trouble when we're trying to increment
>> >>> the refcount on a non-head page:
>> >>>
>> >>> page:7c00cfb693c0 count:0 mapcount:0 mapping: (null)
>> >>> index:0x0
>> >>> flags: 0x4000()
>> >>> page dumped because: VM_BUG_ON_PAGE((*({ __attribute__((unused))
>> >>> typeof((&page->_count)->counter) __var = (
>> >>> typeof((&page->_count)->counter)) 0; (volatile
>> >>> typeof((&page->_count)->counter) *)&((&page->_count)->counter); })) <= 0)
>> >>> BUG: failure at include/linux/mm.h:548/get_page()!
>> >>> Kernel panic - not syncing: BUG!
>> >>> CPU: 1 PID: 1695 Comm: kvm-vcpu-0 Not tainted 4.0.0-rc1+ #3825
>> >>> Hardware name: APM X-Gene Mustang board (DT)
>> >>> Call trace:
>> >>> [] dump_backtrace+0x0/0x13c
>> >>> [] show_stack+0x10/0x1c
>> >>> [] dump_stack+0x74/0x94
>> >>> [] panic+0x100/0x240
>> >>> [] stage2_get_pmd+0x17c/0x2bc
>> >>> [] kvm_handle_guest_abort+0x4b4/0x6b0
>> >>> [] handle_exit+0x58/0x180
>> >>> [] kvm_arch_vcpu_ioctl_run+0x114/0x45c
>> >>> [] kvm_vcpu_ioctl+0x2e0/0x754
>> >>> [] do_vfs_ioctl+0x424/0x5c8
>> >>> [] SyS_ioctl+0x40/0x78
>> >>> CPU0: stopping
>> >>>
>> >>> A possible approach for this is to split the compound page using
>> >>> split_page() at allocation time, and change the teardown path to
>> >>> free one page at a time.
>> >>>
>> >>> While we're at it, the PGD allocation code is reworked to reduce
>> >>> duplication.
>> >>>
>> >>> This has been tested on an X-Gene platform with a 4kB/48bit-VA host
>> >>> kernel, and kvmtool hacked to place memory in the second page of
>> >>> the hardware PGD (PUD for the host kernel). Also regression-tested
>> >>> on a Cubietruck (Cortex-A7).
>> >>>
>> >>> Reported-by: Mark Rutland
>> >>> Signed-off-by: Marc Zyngier
>> >>> ---
>> >>> arch/arm/include/asm/kvm_mmu.h | 9 ++---
>> >>> arch/arm/kvm/mmu.c | 77
>> >>> +++-
>> >>> arch/arm64/include/asm/kvm_mmu.h | 46 +++-
>> >>> 3 files changed, 66 insertions(+), 66 deletions(-)
>> >>>
>> >>> diff --git a/arch/arm/include/asm/kvm_mmu.h
>> >>> b/arch/arm/include/asm/kvm_mmu.h
>> >>> index 0187606..ff56f91 100644
>> >>> --- a/arch/arm/include/asm/kvm_mmu.h
>> >>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> >>> @@ -162,18 +162,13 @@ static inline bool kvm_page_empty(void *ptr)
>> >>>
>> >>> #define KVM_PREALLOC_LEVEL 0
>> >>>
>> >>> -static inline int kvm_prealloc_hwpgd(struct kvm *kvm, pgd_t *pgd)
>> >>> -{
>> >>> -return 0;
>> >>> -}
>> >>> -
>> >>> -static inline void kvm_free_hwpgd(struct kvm *kvm) { }
>> >>> -
>> >>> static inline void *kvm_get_hwpgd(struct kvm *kvm)
>> >>> {
>> >>> return kvm->arch.pgd;
>> >>> }
>> >>>
>> >>> +static inline unsigned int kvm_get_hwpgd_order(void) { return
>> >>> S2_PGD_ORDER; }
>> >>> +
>> >>> struct kvm;
>> >>>
>> >>> #define kvm_flush_dcache_to_poc(a,l)
>> >>> __cpuc_flush_dcache_area((a), (l))
>> >>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> >>> index 69c2b4c..0a5457c 100644
>> >>> --- a/arch/arm/kvm/mmu.c
>> >>> +++ b/arch/arm/kvm/mmu.c
>> >>> @@ -634,6 +634,31 @@ int create_hyp_io_mappings(void *from, void *to,
>> >>> phys_addr_t phys_addr)
>> >>> __phys_to_pfn(phys_addr),
>> >>> PAGE_HYP_DEVICE);
>> >>> }
>> >>>
>> >>> +/* Free the HW pgd, one page at a time */
>> >>> +static void kvm_free_hwpgd(unsigned long hwpgd)
>> >>> +{
>> >>> +int i;
>> >>> +
>> >>> +for (i = 0; i < (1 << kvm_get_hwpgd_order()); i += PAGE_SIZE)
>> >>> +free_page(hwpgd + i);
>> >>> +}
>> >>> +
>> >>> +/* Allocate the HW PGD, making sure that each page gets its own
>> >>> refcount */
>> >>> +static int kvm_alloc_hwpgd(unsigned long *hwpgdp)
>> >>
>> >> I think this can be simplified somewhat by just returning an unsigned
>> >> long that can be 0 in the error case which will make the caller look
>> >> a little nicer too.
>> >
>> > Sure.
>> >
>> >>> +{
>> >>> +unsigned long hwpgd;
>> >>> +unsigned int order = kvm_get_hwpgd_order();
>> >>> +
>> >>> +hwpgd = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
>> >>> +if (!hwpgd)
>> >>> +return -ENOMEM;
>> >>> +
>> >>> +split_page(virt_to_page((void *)hwpgd), order);
>> >>
>> >> nit: alloc_pages_exact() and free_pages_exact() seem to do this for us,
>> >> is it worth using those instead?
>> >
>> > Ah, I didn't know about these.
>
Re: [PATCH v9 5/5] KVM: arm/arm64: add irqfd support
On Thu, Mar 05, 2015 at 12:27:42PM +0100, Paolo Bonzini wrote: > > > On 05/03/2015 11:53, Marc Zyngier wrote: > > > +#ifdef CONFIG_HAVE_KVM_IRQFD > > > + case KVM_CAP_IRQFD: > > > + r = vgic_present; > > > + break; > > > +#endif > > > > Nitpick: we have "select HAVE_KVM_IRQFD", so we can lose the #ifdef-ery. > > Alternatively, I've just posted a patch to move the KVM_CAP_IRQFD case > to common code. > > vgic_present probably should be replaced by > IS_ENABLED(CONFIG_KVM_ARM_VGIC). I've sent a patch for this. > I have applied all of this together with my kill-the-vgic-config-option patch here: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git queue Take a look please. Thanks, -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH] arm/arm64: KVM: Kill CONFIG_KVM_ARM_{VGIC,TIMER}
We can definitely decide at run-time whether to use the GIC and timers or not, and the extra code and data structures that we allocate space for is really negligable with this config option, so I don't think it's worth the extra complexity of always having to define stub static inlines. The !CONFIG_KVM_ARM_VGIC/TIMER case is pretty much an untested code path anyway, so we're better off just getting rid of it. Signed-off-by: Christoffer Dall --- This depends on Paolo's patch: KVM: arm/arm64: prefer IS_ENABLED to a static variable I've applied it to queue together with Eric's IRQFD series. arch/arm/kernel/asm-offsets.c | 4 -- arch/arm/kvm/Kconfig | 29 +++--- arch/arm/kvm/Makefile | 8 ++-- arch/arm/kvm/arm.c | 6 --- arch/arm/kvm/guest.c | 18 - arch/arm/kvm/interrupts_head.S | 8 arch/arm64/kvm/Kconfig | 17 + arch/arm64/kvm/Makefile| 16 include/kvm/arm_arch_timer.h | 31 --- include/kvm/arm_vgic.h | 85 -- 10 files changed, 20 insertions(+), 202 deletions(-) diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c index 2d2d608..488eaac 100644 --- a/arch/arm/kernel/asm-offsets.c +++ b/arch/arm/kernel/asm-offsets.c @@ -190,7 +190,6 @@ int main(void) DEFINE(VCPU_HxFAR, offsetof(struct kvm_vcpu, arch.fault.hxfar)); DEFINE(VCPU_HPFAR, offsetof(struct kvm_vcpu, arch.fault.hpfar)); DEFINE(VCPU_HYP_PC, offsetof(struct kvm_vcpu, arch.fault.hyp_pc)); -#ifdef CONFIG_KVM_ARM_VGIC DEFINE(VCPU_VGIC_CPU,offsetof(struct kvm_vcpu, arch.vgic_cpu)); DEFINE(VGIC_V2_CPU_HCR, offsetof(struct vgic_cpu, vgic_v2.vgic_hcr)); DEFINE(VGIC_V2_CPU_VMCR, offsetof(struct vgic_cpu, vgic_v2.vgic_vmcr)); @@ -200,14 +199,11 @@ int main(void) DEFINE(VGIC_V2_CPU_APR, offsetof(struct vgic_cpu, vgic_v2.vgic_apr)); DEFINE(VGIC_V2_CPU_LR, offsetof(struct vgic_cpu, vgic_v2.vgic_lr)); DEFINE(VGIC_CPU_NR_LR, offsetof(struct vgic_cpu, nr_lr)); -#ifdef CONFIG_KVM_ARM_TIMER DEFINE(VCPU_TIMER_CNTV_CTL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_ctl)); DEFINE(VCPU_TIMER_CNTV_CVAL, offsetof(struct kvm_vcpu, arch.timer_cpu.cntv_cval)); DEFINE(KVM_TIMER_CNTVOFF,offsetof(struct kvm, arch.timer.cntvoff)); DEFINE(KVM_TIMER_ENABLED,offsetof(struct kvm, arch.timer.enabled)); -#endif DEFINE(KVM_VGIC_VCTRL, offsetof(struct kvm, arch.vgic.vctrl_base)); -#endif DEFINE(KVM_VTTBR,offsetof(struct kvm, arch.vttbr)); #endif return 0; diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig index 338ace7..7b6347b 100644 --- a/arch/arm/kvm/Kconfig +++ b/arch/arm/kvm/Kconfig @@ -18,6 +18,7 @@ if VIRTUALIZATION config KVM bool "Kernel-based Virtual Machine (KVM) support" + depends on MMU && OF select PREEMPT_NOTIFIERS select ANON_INODES select HAVE_KVM_CPU_RELAX_INTERCEPT @@ -26,10 +27,11 @@ config KVM select KVM_ARM_HOST select KVM_GENERIC_DIRTYLOG_READ_PROTECT select SRCU - depends on ARM_VIRT_EXT && ARM_LPAE + select MMU_NOTIFIER + select HAVE_KVM_IRQCHIP + depends on ARM_VIRT_EXT && ARM_LPAE && ARM_ARCH_TIMER ---help--- - Support hosting virtualized guest machines. You will also - need to select one or more of the processor modules below. + Support hosting virtualized guest machines. This module provides access to the hardware capabilities through a character device node named /dev/kvm. @@ -37,10 +39,7 @@ config KVM If unsure, say N. config KVM_ARM_HOST - bool "KVM host support for ARM cpus." - depends on KVM - depends on MMU - select MMU_NOTIFIER + bool ---help--- Provides host support for ARM processors. @@ -55,20 +54,4 @@ config KVM_ARM_MAX_VCPUS large, so only choose a reasonable number that you expect to actually use. -config KVM_ARM_VGIC - bool "KVM support for Virtual GIC" - depends on KVM_ARM_HOST && OF - select HAVE_KVM_IRQCHIP - default y - ---help--- - Adds support for a hardware assisted, in-kernel GIC emulation. - -config KVM_ARM_TIMER - bool "KVM support for Architected Timers" - depends on KVM_ARM_VGIC && ARM_ARCH_TIMER - select HAVE_KVM_IRQCHIP - default y - ---help--- - Adds support for the Architected Timers in virtual machines - endif # VIRTUALIZATION diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile index 443b8be..60be7be 100644 --- a/arch/arm/kvm/Makefile +++ b/arch/arm/kvm/Makefile @@ -20,7 +20,7 @@ kvm-arm-y = $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o obj-y += kvm-arm.o init.o interrupts.o obj-y += arm.o handle_exit.o guest.o mmu.o emulate.o reset.o obj-y += coproc.o coproc_a15.o coproc_a7.o mmio.o
Re: [PATCH v2 1/3] arm64: KVM: Fix stage-2 PGD allocation to have per-page refcounting
On 11/03/15 13:20, Christoffer Dall wrote: > On Wed, Mar 11, 2015 at 2:13 PM, Marc Zyngier wrote: >> Looks good to me. I'll fold that into the patch, amend the commit >> message and resend. >> > > I've done that here: > https://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git/shortlog/refs/heads/pgd-fixes-alt > > (except for the commit message, which I can also adjust). Looks great, thanks. > I gave this a quick spin on Juno and TC2 (but couldn't test 64K pages > because I don't have a platform for that handy this very second)... I can do that if you want. M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/3] arm64: KVM: Fix stage-2 PGD allocation to have per-page refcounting
On Wed, Mar 11, 2015 at 2:13 PM, Marc Zyngier wrote:
> Hi Christoffer,
>
> On 11/03/15 12:16, Christoffer Dall wrote:
>> Hi Marc,
>>
>> On Tue, Mar 10, 2015 at 07:06:59PM +, Marc Zyngier wrote:
>>> We're using __get_free_pages with to allocate the guest's stage-2
>>> PGD. The standard behaviour of this function is to return a set of
>>> pages where only the head page has a valid refcount.
>>>
>>> This behaviour gets us into trouble when we're trying to increment
>>> the refcount on a non-head page:
>>>
>>> page:7c00cfb693c0 count:0 mapcount:0 mapping: (null) index:0x0
>>> flags: 0x4000()
>>> page dumped because: VM_BUG_ON_PAGE((*({ __attribute__((unused))
>>> typeof((&page->_count)->counter) __var = (
>>> typeof((&page->_count)->counter)) 0; (volatile
>>> typeof((&page->_count)->counter) *)&((&page->_count)->counter); })) <= 0)
>>> BUG: failure at include/linux/mm.h:548/get_page()!
>>> Kernel panic - not syncing: BUG!
>>> CPU: 1 PID: 1695 Comm: kvm-vcpu-0 Not tainted 4.0.0-rc1+ #3825
>>> Hardware name: APM X-Gene Mustang board (DT)
>>> Call trace:
>>> [] dump_backtrace+0x0/0x13c
>>> [] show_stack+0x10/0x1c
>>> [] dump_stack+0x74/0x94
>>> [] panic+0x100/0x240
>>> [] stage2_get_pmd+0x17c/0x2bc
>>> [] kvm_handle_guest_abort+0x4b4/0x6b0
>>> [] handle_exit+0x58/0x180
>>> [] kvm_arch_vcpu_ioctl_run+0x114/0x45c
>>> [] kvm_vcpu_ioctl+0x2e0/0x754
>>> [] do_vfs_ioctl+0x424/0x5c8
>>> [] SyS_ioctl+0x40/0x78
>>> CPU0: stopping
>>>
>>> A possible approach for this is to split the compound page using
>>> split_page() at allocation time, and change the teardown path to
>>> free one page at a time.
>>>
>>> While we're at it, the PGD allocation code is reworked to reduce
>>> duplication.
>>>
>>> This has been tested on an X-Gene platform with a 4kB/48bit-VA host
>>> kernel, and kvmtool hacked to place memory in the second page of
>>> the hardware PGD (PUD for the host kernel). Also regression-tested
>>> on a Cubietruck (Cortex-A7).
>>>
>>> Reported-by: Mark Rutland
>>> Signed-off-by: Marc Zyngier
>>> ---
>>> arch/arm/include/asm/kvm_mmu.h | 9 ++---
>>> arch/arm/kvm/mmu.c | 77
>>> +++-
>>> arch/arm64/include/asm/kvm_mmu.h | 46 +++-
>>> 3 files changed, 66 insertions(+), 66 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>>> index 0187606..ff56f91 100644
>>> --- a/arch/arm/include/asm/kvm_mmu.h
>>> +++ b/arch/arm/include/asm/kvm_mmu.h
>>> @@ -162,18 +162,13 @@ static inline bool kvm_page_empty(void *ptr)
>>>
>>> #define KVM_PREALLOC_LEVEL 0
>>>
>>> -static inline int kvm_prealloc_hwpgd(struct kvm *kvm, pgd_t *pgd)
>>> -{
>>> -return 0;
>>> -}
>>> -
>>> -static inline void kvm_free_hwpgd(struct kvm *kvm) { }
>>> -
>>> static inline void *kvm_get_hwpgd(struct kvm *kvm)
>>> {
>>> return kvm->arch.pgd;
>>> }
>>>
>>> +static inline unsigned int kvm_get_hwpgd_order(void) { return
>>> S2_PGD_ORDER; }
>>> +
>>> struct kvm;
>>>
>>> #define kvm_flush_dcache_to_poc(a,l)__cpuc_flush_dcache_area((a),
>>> (l))
>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>> index 69c2b4c..0a5457c 100644
>>> --- a/arch/arm/kvm/mmu.c
>>> +++ b/arch/arm/kvm/mmu.c
>>> @@ -634,6 +634,31 @@ int create_hyp_io_mappings(void *from, void *to,
>>> phys_addr_t phys_addr)
>>> __phys_to_pfn(phys_addr),
>>> PAGE_HYP_DEVICE);
>>> }
>>>
>>> +/* Free the HW pgd, one page at a time */
>>> +static void kvm_free_hwpgd(unsigned long hwpgd)
>>> +{
>>> +int i;
>>> +
>>> +for (i = 0; i < (1 << kvm_get_hwpgd_order()); i += PAGE_SIZE)
>>> +free_page(hwpgd + i);
>>> +}
>>> +
>>> +/* Allocate the HW PGD, making sure that each page gets its own refcount */
>>> +static int kvm_alloc_hwpgd(unsigned long *hwpgdp)
>>
>> I think this can be simplified somewhat by just returning an unsigned
>> long that can be 0 in the error case which will make the caller look
>> a little nicer too.
>
> Sure.
>
>>> +{
>>> +unsigned long hwpgd;
>>> +unsigned int order = kvm_get_hwpgd_order();
>>> +
>>> +hwpgd = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
>>> +if (!hwpgd)
>>> +return -ENOMEM;
>>> +
>>> +split_page(virt_to_page((void *)hwpgd), order);
>>
>> nit: alloc_pages_exact() and free_pages_exact() seem to do this for us,
>> is it worth using those instead?
>
> Ah, I didn't know about these.
>
>> It would look something like this on top of your changes:
>>
>>
>> arch/arm/include/asm/kvm_mmu.h | 5 -
>> arch/arm/kvm/mmu.c | 32 ++--
>> arch/arm64/include/asm/kvm_mmu.h | 6 +++---
>> 3 files changed, 17 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> index fc05ba8..4cf48c3 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -168,7 +168,10 @@ static inline v
Re: [PATCH v2 1/3] arm64: KVM: Fix stage-2 PGD allocation to have per-page refcounting
Hi Christoffer,
On 11/03/15 12:16, Christoffer Dall wrote:
> Hi Marc,
>
> On Tue, Mar 10, 2015 at 07:06:59PM +, Marc Zyngier wrote:
>> We're using __get_free_pages with to allocate the guest's stage-2
>> PGD. The standard behaviour of this function is to return a set of
>> pages where only the head page has a valid refcount.
>>
>> This behaviour gets us into trouble when we're trying to increment
>> the refcount on a non-head page:
>>
>> page:7c00cfb693c0 count:0 mapcount:0 mapping: (null) index:0x0
>> flags: 0x4000()
>> page dumped because: VM_BUG_ON_PAGE((*({ __attribute__((unused))
>> typeof((&page->_count)->counter) __var = ( typeof((&page->_count)->counter))
>> 0; (volatile typeof((&page->_count)->counter) *)&((&page->_count)->counter);
>> })) <= 0)
>> BUG: failure at include/linux/mm.h:548/get_page()!
>> Kernel panic - not syncing: BUG!
>> CPU: 1 PID: 1695 Comm: kvm-vcpu-0 Not tainted 4.0.0-rc1+ #3825
>> Hardware name: APM X-Gene Mustang board (DT)
>> Call trace:
>> [] dump_backtrace+0x0/0x13c
>> [] show_stack+0x10/0x1c
>> [] dump_stack+0x74/0x94
>> [] panic+0x100/0x240
>> [] stage2_get_pmd+0x17c/0x2bc
>> [] kvm_handle_guest_abort+0x4b4/0x6b0
>> [] handle_exit+0x58/0x180
>> [] kvm_arch_vcpu_ioctl_run+0x114/0x45c
>> [] kvm_vcpu_ioctl+0x2e0/0x754
>> [] do_vfs_ioctl+0x424/0x5c8
>> [] SyS_ioctl+0x40/0x78
>> CPU0: stopping
>>
>> A possible approach for this is to split the compound page using
>> split_page() at allocation time, and change the teardown path to
>> free one page at a time.
>>
>> While we're at it, the PGD allocation code is reworked to reduce
>> duplication.
>>
>> This has been tested on an X-Gene platform with a 4kB/48bit-VA host
>> kernel, and kvmtool hacked to place memory in the second page of
>> the hardware PGD (PUD for the host kernel). Also regression-tested
>> on a Cubietruck (Cortex-A7).
>>
>> Reported-by: Mark Rutland
>> Signed-off-by: Marc Zyngier
>> ---
>> arch/arm/include/asm/kvm_mmu.h | 9 ++---
>> arch/arm/kvm/mmu.c | 77
>> +++-
>> arch/arm64/include/asm/kvm_mmu.h | 46 +++-
>> 3 files changed, 66 insertions(+), 66 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
>> index 0187606..ff56f91 100644
>> --- a/arch/arm/include/asm/kvm_mmu.h
>> +++ b/arch/arm/include/asm/kvm_mmu.h
>> @@ -162,18 +162,13 @@ static inline bool kvm_page_empty(void *ptr)
>>
>> #define KVM_PREALLOC_LEVEL 0
>>
>> -static inline int kvm_prealloc_hwpgd(struct kvm *kvm, pgd_t *pgd)
>> -{
>> -return 0;
>> -}
>> -
>> -static inline void kvm_free_hwpgd(struct kvm *kvm) { }
>> -
>> static inline void *kvm_get_hwpgd(struct kvm *kvm)
>> {
>> return kvm->arch.pgd;
>> }
>>
>> +static inline unsigned int kvm_get_hwpgd_order(void) { return S2_PGD_ORDER;
>> }
>> +
>> struct kvm;
>>
>> #define kvm_flush_dcache_to_poc(a,l)__cpuc_flush_dcache_area((a),
>> (l))
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 69c2b4c..0a5457c 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -634,6 +634,31 @@ int create_hyp_io_mappings(void *from, void *to,
>> phys_addr_t phys_addr)
>> __phys_to_pfn(phys_addr), PAGE_HYP_DEVICE);
>> }
>>
>> +/* Free the HW pgd, one page at a time */
>> +static void kvm_free_hwpgd(unsigned long hwpgd)
>> +{
>> +int i;
>> +
>> +for (i = 0; i < (1 << kvm_get_hwpgd_order()); i += PAGE_SIZE)
>> +free_page(hwpgd + i);
>> +}
>> +
>> +/* Allocate the HW PGD, making sure that each page gets its own refcount */
>> +static int kvm_alloc_hwpgd(unsigned long *hwpgdp)
>
> I think this can be simplified somewhat by just returning an unsigned
> long that can be 0 in the error case which will make the caller look
> a little nicer too.
Sure.
>> +{
>> +unsigned long hwpgd;
>> +unsigned int order = kvm_get_hwpgd_order();
>> +
>> +hwpgd = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
>> +if (!hwpgd)
>> +return -ENOMEM;
>> +
>> +split_page(virt_to_page((void *)hwpgd), order);
>
> nit: alloc_pages_exact() and free_pages_exact() seem to do this for us,
> is it worth using those instead?
Ah, I didn't know about these.
> It would look something like this on top of your changes:
>
>
> arch/arm/include/asm/kvm_mmu.h | 5 -
> arch/arm/kvm/mmu.c | 32 ++--
> arch/arm64/include/asm/kvm_mmu.h | 6 +++---
> 3 files changed, 17 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index fc05ba8..4cf48c3 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -168,7 +168,10 @@ static inline void *kvm_get_hwpgd(struct kvm *kvm)
> return kvm->arch.pgd;
> }
>
> -static inline unsigned int kvm_get_hwpgd_order(void) { return S2_PGD_ORDER; }
> +static inline unsigned int k
Re: [PATCH] KVM: vgic: add virt-capable compatible strings
On Thu, Mar 05, 2015 at 02:47:44PM +, Mark Rutland wrote: > Several dts only list "arm,cortex-a7-gic" or "arm,gic-400" in their GIC > compatible list, and while this is correct (and supported by the GIC > driver), KVM will fail to detect that it can support these cases. > > This patch adds the missing strings to the VGIC code. The of_device_id > entries are padded to keep the probe fucntion data aligned. > > Signed-off-by: Mark Rutland > Cc: Andre Przywara > Cc: Christoffer Dall > Cc: Marc Zyngier > Cc: Michal Simek Applied, thanks. -Christoffer ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/3] arm64: KVM: Fix stage-2 PGD allocation to have per-page refcounting
Hi Marc,
On Tue, Mar 10, 2015 at 07:06:59PM +, Marc Zyngier wrote:
> We're using __get_free_pages with to allocate the guest's stage-2
> PGD. The standard behaviour of this function is to return a set of
> pages where only the head page has a valid refcount.
>
> This behaviour gets us into trouble when we're trying to increment
> the refcount on a non-head page:
>
> page:7c00cfb693c0 count:0 mapcount:0 mapping: (null) index:0x0
> flags: 0x4000()
> page dumped because: VM_BUG_ON_PAGE((*({ __attribute__((unused))
> typeof((&page->_count)->counter) __var = ( typeof((&page->_count)->counter))
> 0; (volatile typeof((&page->_count)->counter) *)&((&page->_count)->counter);
> })) <= 0)
> BUG: failure at include/linux/mm.h:548/get_page()!
> Kernel panic - not syncing: BUG!
> CPU: 1 PID: 1695 Comm: kvm-vcpu-0 Not tainted 4.0.0-rc1+ #3825
> Hardware name: APM X-Gene Mustang board (DT)
> Call trace:
> [] dump_backtrace+0x0/0x13c
> [] show_stack+0x10/0x1c
> [] dump_stack+0x74/0x94
> [] panic+0x100/0x240
> [] stage2_get_pmd+0x17c/0x2bc
> [] kvm_handle_guest_abort+0x4b4/0x6b0
> [] handle_exit+0x58/0x180
> [] kvm_arch_vcpu_ioctl_run+0x114/0x45c
> [] kvm_vcpu_ioctl+0x2e0/0x754
> [] do_vfs_ioctl+0x424/0x5c8
> [] SyS_ioctl+0x40/0x78
> CPU0: stopping
>
> A possible approach for this is to split the compound page using
> split_page() at allocation time, and change the teardown path to
> free one page at a time.
>
> While we're at it, the PGD allocation code is reworked to reduce
> duplication.
>
> This has been tested on an X-Gene platform with a 4kB/48bit-VA host
> kernel, and kvmtool hacked to place memory in the second page of
> the hardware PGD (PUD for the host kernel). Also regression-tested
> on a Cubietruck (Cortex-A7).
>
> Reported-by: Mark Rutland
> Signed-off-by: Marc Zyngier
> ---
> arch/arm/include/asm/kvm_mmu.h | 9 ++---
> arch/arm/kvm/mmu.c | 77
> +++-
> arch/arm64/include/asm/kvm_mmu.h | 46 +++-
> 3 files changed, 66 insertions(+), 66 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
> index 0187606..ff56f91 100644
> --- a/arch/arm/include/asm/kvm_mmu.h
> +++ b/arch/arm/include/asm/kvm_mmu.h
> @@ -162,18 +162,13 @@ static inline bool kvm_page_empty(void *ptr)
>
> #define KVM_PREALLOC_LEVEL 0
>
> -static inline int kvm_prealloc_hwpgd(struct kvm *kvm, pgd_t *pgd)
> -{
> - return 0;
> -}
> -
> -static inline void kvm_free_hwpgd(struct kvm *kvm) { }
> -
> static inline void *kvm_get_hwpgd(struct kvm *kvm)
> {
> return kvm->arch.pgd;
> }
>
> +static inline unsigned int kvm_get_hwpgd_order(void) { return S2_PGD_ORDER; }
> +
> struct kvm;
>
> #define kvm_flush_dcache_to_poc(a,l) __cpuc_flush_dcache_area((a), (l))
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 69c2b4c..0a5457c 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -634,6 +634,31 @@ int create_hyp_io_mappings(void *from, void *to,
> phys_addr_t phys_addr)
>__phys_to_pfn(phys_addr), PAGE_HYP_DEVICE);
> }
>
> +/* Free the HW pgd, one page at a time */
> +static void kvm_free_hwpgd(unsigned long hwpgd)
> +{
> + int i;
> +
> + for (i = 0; i < (1 << kvm_get_hwpgd_order()); i += PAGE_SIZE)
> + free_page(hwpgd + i);
> +}
> +
> +/* Allocate the HW PGD, making sure that each page gets its own refcount */
> +static int kvm_alloc_hwpgd(unsigned long *hwpgdp)
I think this can be simplified somewhat by just returning an unsigned
long that can be 0 in the error case which will make the caller look
a little nicer too.
> +{
> + unsigned long hwpgd;
> + unsigned int order = kvm_get_hwpgd_order();
> +
> + hwpgd = __get_free_pages(GFP_KERNEL | __GFP_ZERO, order);
> + if (!hwpgd)
> + return -ENOMEM;
> +
> + split_page(virt_to_page((void *)hwpgd), order);
nit: alloc_pages_exact() and free_pages_exact() seem to do this for us,
is it worth using those instead?
It would look something like this on top of your changes:
arch/arm/include/asm/kvm_mmu.h | 5 -
arch/arm/kvm/mmu.c | 32 ++--
arch/arm64/include/asm/kvm_mmu.h | 6 +++---
3 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index fc05ba8..4cf48c3 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h
@@ -168,7 +168,10 @@ static inline void *kvm_get_hwpgd(struct kvm *kvm)
return kvm->arch.pgd;
}
-static inline unsigned int kvm_get_hwpgd_order(void) { return S2_PGD_ORDER; }
+static inline unsigned int kvm_get_hwpgd_size(void)
+{
+ return PTRS_PER_S2_PGD * sizeof(pgd_t);
+}
struct kvm;
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 8e91bea3..5656d79 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -633,28 +633,17 @@ int
Re: [PATCH v14 00/20] VFIO support for platform devices
Thanks Eric and Alex for your reviews. I can confirm we can drop the dependency, as I re-tested without it. Do you need a fix for the headers issue underlined by Eric ? Best regards, Baptiste On Wed, Mar 11, 2015 at 9:40 AM, Eric Auger wrote: > On 03/10/2015 07:04 PM, Alex Williamson wrote: >> On Tue, 2015-03-10 at 18:42 +0100, Eric Auger wrote: >>> Hi Baptiste, >>> >>> Please add: >>> Reviewed-by: Eric Auger on the whole series >>> Tested-by: Eric Auger on the whole series except >>> AMBA specific patches. >>> >>> I currently exercise the following functionalities on Calxeda Midway HW: >>> - MMIO read/write/mmap, >>> - automasked IRQ + mask/unmask (NONE flag), >>> - virqfd (unmask handler only). >>> >>> Despite a new thorough review I failed in pointing out any new >>> interesting issues in this series. >>> >>> just minor points below: >>> >>> - PIO regions: currently read/write/mmap is not supported. I can't >>> figure out how much this is a problem at this point? >> >> AIUI, nobody has any way to test this, which is why it's not >> implemented. I think we've done due diligence in accommodating PIO into >> the vfio-platform framework though and hopefully it will be a simple >> matter to add when someone has a use case for it. >> >>> - nit: vfio_pci_intrs.c: may not need wait.h anymore after removal of >>> virqfd code. may be added in vfio.h >>> - nit: interrupt.h not needed at early stage in vfio_platform_private.h, >>> until irq introduction >>> - version of the driver: 0.10? >> >> None of these seem like blockers, I'd be willing to accept header >> cleanups as follow-on and driver versions are mostly arbitrary anyway. >> >>> Since there is no real dependency on "[PATCH v4 0/6] vfio: type1: >>> support for ARM SMMUS with VFIO_IOMMU_TYPE1" I hope we can get this >>> upstreamed soon. >> >> Me too, I also re-reviewed the series yesterday. If Baptiste agrees >> that there's no dependency on the other series, I can add your R-b/T-b, >> do some additional testing, and queue the series for linux-next. >> Thanks, > > Hi Alex, > > I let Baptiste answer. If he agrees please do as proposed. I am looking > forward to seeing the series in linux-next! > > Best Regards > > Eric >> >> Alex >> > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v14 00/20] VFIO support for platform devices
On 03/10/2015 07:04 PM, Alex Williamson wrote: > On Tue, 2015-03-10 at 18:42 +0100, Eric Auger wrote: >> Hi Baptiste, >> >> Please add: >> Reviewed-by: Eric Auger on the whole series >> Tested-by: Eric Auger on the whole series except >> AMBA specific patches. >> >> I currently exercise the following functionalities on Calxeda Midway HW: >> - MMIO read/write/mmap, >> - automasked IRQ + mask/unmask (NONE flag), >> - virqfd (unmask handler only). >> >> Despite a new thorough review I failed in pointing out any new >> interesting issues in this series. >> >> just minor points below: >> >> - PIO regions: currently read/write/mmap is not supported. I can't >> figure out how much this is a problem at this point? > > AIUI, nobody has any way to test this, which is why it's not > implemented. I think we've done due diligence in accommodating PIO into > the vfio-platform framework though and hopefully it will be a simple > matter to add when someone has a use case for it. > >> - nit: vfio_pci_intrs.c: may not need wait.h anymore after removal of >> virqfd code. may be added in vfio.h >> - nit: interrupt.h not needed at early stage in vfio_platform_private.h, >> until irq introduction >> - version of the driver: 0.10? > > None of these seem like blockers, I'd be willing to accept header > cleanups as follow-on and driver versions are mostly arbitrary anyway. > >> Since there is no real dependency on "[PATCH v4 0/6] vfio: type1: >> support for ARM SMMUS with VFIO_IOMMU_TYPE1" I hope we can get this >> upstreamed soon. > > Me too, I also re-reviewed the series yesterday. If Baptiste agrees > that there's no dependency on the other series, I can add your R-b/T-b, > do some additional testing, and queue the series for linux-next. > Thanks, Hi Alex, I let Baptiste answer. If he agrees please do as proposed. I am looking forward to seeing the series in linux-next! Best Regards Eric > > Alex > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
