From mboxrd@z Thu Jan 1 00:00:00 1970 From: andre.przywara@arm.com (Andre Przywara) Date: Fri, 12 Jun 2015 18:03:32 +0100 Subject: [PATCH 05/13] KVM: arm64: handle ITS related GICv3 redistributor registers In-Reply-To: <5576A943.80803@linaro.org> References: <1432893209-27313-1-git-send-email-andre.przywara@arm.com> <1432893209-27313-6-git-send-email-andre.przywara@arm.com> <5576A943.80803@linaro.org> Message-ID: <557B10E4.1060305@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Eric, On 06/09/2015 09:52 AM, Eric Auger wrote: > On 05/29/2015 11:53 AM, Andre Przywara wrote: >> In the GICv3 redistributor there are the PENDBASER and PROPBASER >> registers which we did not emulate so far, as they only make sense >> when having an ITS. In preparation for that emulate those MMIO >> accesses by storing the 64-bit data written into it into a variable >> which we later read in the ITS emulation. >> >> Signed-off-by: Andre Przywara >> --- >> include/kvm/arm_vgic.h | 4 ++++ >> virt/kvm/arm/vgic-v3-emul.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic.c | 35 +++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic.h | 4 ++++ >> 4 files changed, 86 insertions(+) >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h >> index 37725bb..9ea0b3b 100644 >> --- a/include/kvm/arm_vgic.h >> +++ b/include/kvm/arm_vgic.h >> @@ -256,6 +256,10 @@ struct vgic_dist { >> struct vgic_vm_ops vm_ops; >> struct vgic_io_device dist_iodev; >> struct vgic_io_device *redist_iodevs; >> + > add some comments? > /* LPI config table shared by all distributors */ >> + u64 propbaser; > /* LPI pending table per distributors */ >> + u64 *pendbaser; >> + bool lpis_enabled; >> }; >> >> struct vgic_v2_cpu_if { >> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c >> index 16c6d8a..04f3aed 100644 >> --- a/virt/kvm/arm/vgic-v3-emul.c >> +++ b/virt/kvm/arm/vgic-v3-emul.c >> @@ -607,6 +607,37 @@ static bool handle_mmio_cfg_reg_redist(struct kvm_vcpu *vcpu, >> return vgic_handle_cfg_reg(reg, mmio, offset); >> } >> >> +/* We don't trigger any actions here, just store the register value */ >> +static bool handle_mmio_propbaser_redist(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset) >> +{ >> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >> + int mode = ACCESS_READ_VALUE; >> + > you may add the same comment as below? >> + mode |= dist->lpis_enabled ? ACCESS_WRITE_IGNORED : ACCESS_WRITE_VALUE; >> + vgic_handle_base_register(vcpu, mmio, offset, &dist->propbaser, mode); > having the PROPBASER programmed to different values on different redist > with EnableLPIs==1 also is unpredictable. Do we plan to check that > somewhere? Allow a single write? Well, we are safe if the spec says it's unpredictable, aren't we? I refrained from checking too many corner cases (same for the ITS commands, btw), since we lack a good way of communicating errors. SErrors into a guest do not work AFAIK, and spamming dmesg with guest-triggerable messages is also bad. After all this is an emulator, not a validator. So as long as this doesn't affect the host and violates the spec, I think we get away with ignoring stupid requests from the guest. I am happy to revisit this shall the need arise. >> + >> + return false; >> +} >> + >> +/* We don't trigger any actions here, just store the register value */ >> +static bool handle_mmio_pendbaser_redist(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset) >> +{ >> + struct kvm_vcpu *rdvcpu = mmio->private; >> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >> + int mode = ACCESS_READ_VALUE; >> + >> + /* Storing a value with LPIs already enabled is undefined */ >> + mode |= dist->lpis_enabled ? ACCESS_WRITE_IGNORED : ACCESS_WRITE_VALUE; >> + vgic_handle_base_register(vcpu, mmio, offset, >> + &dist->pendbaser[rdvcpu->vcpu_id], mode); > pendbaser is not yet allocated. Wouldn't it make sense to introduce that > patch later on? I am quite glad having found a patch order which compiles ;-) But well, I guess we have to address this as this strictly isn't safe if pendbaser is NULL (though it works with how GCC compiles this). Thanks for looking! Cheers, Andre. > Eric >> + >> + return false; >> +} >> + >> #define SGI_base(x) ((x) + SZ_64K) >> >> static const struct vgic_io_range vgic_redist_ranges[] = { >> @@ -635,6 +666,18 @@ static const struct vgic_io_range vgic_redist_ranges[] = { >> .handle_mmio = handle_mmio_raz_wi, >> }, >> { >> + .base = GICR_PENDBASER, >> + .len = 0x08, >> + .bits_per_irq = 0, >> + .handle_mmio = handle_mmio_pendbaser_redist, >> + }, >> + { >> + .base = GICR_PROPBASER, >> + .len = 0x08, >> + .bits_per_irq = 0, >> + .handle_mmio = handle_mmio_propbaser_redist, >> + }, >> + { >> .base = GICR_IDREGS, >> .len = 0x30, >> .bits_per_irq = 0, >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >> index 2e9723aa..0a9236d 100644 >> --- a/virt/kvm/arm/vgic.c >> +++ b/virt/kvm/arm/vgic.c >> @@ -448,6 +448,41 @@ void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, >> } >> } >> >> +/* handle a 64-bit register access */ >> +void vgic_handle_base_register(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset, u64 *basereg, >> + int mode) >> +{ >> + u32 reg; >> + u64 breg; >> + >> + switch (offset & ~3) { >> + case 0x00: >> + breg = *basereg; >> + reg = lower_32_bits(breg); >> + vgic_reg_access(mmio, ®, offset & 3, mode); >> + if (mmio->is_write && (mode & ACCESS_WRITE_VALUE)) { >> + breg &= GENMASK_ULL(63, 32); >> + breg |= reg; >> + *basereg = breg; >> + } >> + break; >> + case 0x04: >> + breg = *basereg; >> + reg = upper_32_bits(breg); >> + vgic_reg_access(mmio, ®, offset & 3, mode); >> + if (mmio->is_write && (mode & ACCESS_WRITE_VALUE)) { >> + breg = lower_32_bits(breg); >> + breg |= (u64)reg << 32; >> + *basereg = breg; >> + } >> + break; >> + } >> +} >> + >> + >> + >> bool handle_mmio_raz_wi(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, >> phys_addr_t offset) >> { >> diff --git a/virt/kvm/arm/vgic.h b/virt/kvm/arm/vgic.h >> index a093f5c..b2d791c 100644 >> --- a/virt/kvm/arm/vgic.h >> +++ b/virt/kvm/arm/vgic.h >> @@ -71,6 +71,10 @@ void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, >> phys_addr_t offset, int mode); >> bool handle_mmio_raz_wi(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, >> phys_addr_t offset); >> +void vgic_handle_base_register(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset, u64 *basereg, >> + int mode); >> >> static inline >> u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask) >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH 05/13] KVM: arm64: handle ITS related GICv3 redistributor registers Date: Fri, 12 Jun 2015 18:03:32 +0100 Message-ID: <557B10E4.1060305@arm.com> References: <1432893209-27313-1-git-send-email-andre.przywara@arm.com> <1432893209-27313-6-git-send-email-andre.przywara@arm.com> <5576A943.80803@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 86F0354F0C for ; Fri, 12 Jun 2015 12:52:54 -0400 (EDT) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4mP7v5qT8KJy for ; Fri, 12 Jun 2015 12:52:53 -0400 (EDT) Received: from cam-admin0.cambridge.arm.com (cam-admin0.cambridge.arm.com [217.140.96.50]) by mm01.cs.columbia.edu (Postfix) with ESMTP id E346754F0B for ; Fri, 12 Jun 2015 12:52:52 -0400 (EDT) In-Reply-To: <5576A943.80803@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Eric Auger Cc: Marc Zyngier , "kvm@vger.kernel.org" , "kvmarm@lists.cs.columbia.edu" , "linux-arm-kernel@lists.infradead.org" List-Id: kvmarm@lists.cs.columbia.edu Hi Eric, On 06/09/2015 09:52 AM, Eric Auger wrote: > On 05/29/2015 11:53 AM, Andre Przywara wrote: >> In the GICv3 redistributor there are the PENDBASER and PROPBASER >> registers which we did not emulate so far, as they only make sense >> when having an ITS. In preparation for that emulate those MMIO >> accesses by storing the 64-bit data written into it into a variable >> which we later read in the ITS emulation. >> >> Signed-off-by: Andre Przywara >> --- >> include/kvm/arm_vgic.h | 4 ++++ >> virt/kvm/arm/vgic-v3-emul.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic.c | 35 +++++++++++++++++++++++++++++++++++ >> virt/kvm/arm/vgic.h | 4 ++++ >> 4 files changed, 86 insertions(+) >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h >> index 37725bb..9ea0b3b 100644 >> --- a/include/kvm/arm_vgic.h >> +++ b/include/kvm/arm_vgic.h >> @@ -256,6 +256,10 @@ struct vgic_dist { >> struct vgic_vm_ops vm_ops; >> struct vgic_io_device dist_iodev; >> struct vgic_io_device *redist_iodevs; >> + > add some comments? > /* LPI config table shared by all distributors */ >> + u64 propbaser; > /* LPI pending table per distributors */ >> + u64 *pendbaser; >> + bool lpis_enabled; >> }; >> >> struct vgic_v2_cpu_if { >> diff --git a/virt/kvm/arm/vgic-v3-emul.c b/virt/kvm/arm/vgic-v3-emul.c >> index 16c6d8a..04f3aed 100644 >> --- a/virt/kvm/arm/vgic-v3-emul.c >> +++ b/virt/kvm/arm/vgic-v3-emul.c >> @@ -607,6 +607,37 @@ static bool handle_mmio_cfg_reg_redist(struct kvm_vcpu *vcpu, >> return vgic_handle_cfg_reg(reg, mmio, offset); >> } >> >> +/* We don't trigger any actions here, just store the register value */ >> +static bool handle_mmio_propbaser_redist(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset) >> +{ >> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >> + int mode = ACCESS_READ_VALUE; >> + > you may add the same comment as below? >> + mode |= dist->lpis_enabled ? ACCESS_WRITE_IGNORED : ACCESS_WRITE_VALUE; >> + vgic_handle_base_register(vcpu, mmio, offset, &dist->propbaser, mode); > having the PROPBASER programmed to different values on different redist > with EnableLPIs==1 also is unpredictable. Do we plan to check that > somewhere? Allow a single write? Well, we are safe if the spec says it's unpredictable, aren't we? I refrained from checking too many corner cases (same for the ITS commands, btw), since we lack a good way of communicating errors. SErrors into a guest do not work AFAIK, and spamming dmesg with guest-triggerable messages is also bad. After all this is an emulator, not a validator. So as long as this doesn't affect the host and violates the spec, I think we get away with ignoring stupid requests from the guest. I am happy to revisit this shall the need arise. >> + >> + return false; >> +} >> + >> +/* We don't trigger any actions here, just store the register value */ >> +static bool handle_mmio_pendbaser_redist(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset) >> +{ >> + struct kvm_vcpu *rdvcpu = mmio->private; >> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >> + int mode = ACCESS_READ_VALUE; >> + >> + /* Storing a value with LPIs already enabled is undefined */ >> + mode |= dist->lpis_enabled ? ACCESS_WRITE_IGNORED : ACCESS_WRITE_VALUE; >> + vgic_handle_base_register(vcpu, mmio, offset, >> + &dist->pendbaser[rdvcpu->vcpu_id], mode); > pendbaser is not yet allocated. Wouldn't it make sense to introduce that > patch later on? I am quite glad having found a patch order which compiles ;-) But well, I guess we have to address this as this strictly isn't safe if pendbaser is NULL (though it works with how GCC compiles this). Thanks for looking! Cheers, Andre. > Eric >> + >> + return false; >> +} >> + >> #define SGI_base(x) ((x) + SZ_64K) >> >> static const struct vgic_io_range vgic_redist_ranges[] = { >> @@ -635,6 +666,18 @@ static const struct vgic_io_range vgic_redist_ranges[] = { >> .handle_mmio = handle_mmio_raz_wi, >> }, >> { >> + .base = GICR_PENDBASER, >> + .len = 0x08, >> + .bits_per_irq = 0, >> + .handle_mmio = handle_mmio_pendbaser_redist, >> + }, >> + { >> + .base = GICR_PROPBASER, >> + .len = 0x08, >> + .bits_per_irq = 0, >> + .handle_mmio = handle_mmio_propbaser_redist, >> + }, >> + { >> .base = GICR_IDREGS, >> .len = 0x30, >> .bits_per_irq = 0, >> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c >> index 2e9723aa..0a9236d 100644 >> --- a/virt/kvm/arm/vgic.c >> +++ b/virt/kvm/arm/vgic.c >> @@ -448,6 +448,41 @@ void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, >> } >> } >> >> +/* handle a 64-bit register access */ >> +void vgic_handle_base_register(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset, u64 *basereg, >> + int mode) >> +{ >> + u32 reg; >> + u64 breg; >> + >> + switch (offset & ~3) { >> + case 0x00: >> + breg = *basereg; >> + reg = lower_32_bits(breg); >> + vgic_reg_access(mmio, ®, offset & 3, mode); >> + if (mmio->is_write && (mode & ACCESS_WRITE_VALUE)) { >> + breg &= GENMASK_ULL(63, 32); >> + breg |= reg; >> + *basereg = breg; >> + } >> + break; >> + case 0x04: >> + breg = *basereg; >> + reg = upper_32_bits(breg); >> + vgic_reg_access(mmio, ®, offset & 3, mode); >> + if (mmio->is_write && (mode & ACCESS_WRITE_VALUE)) { >> + breg = lower_32_bits(breg); >> + breg |= (u64)reg << 32; >> + *basereg = breg; >> + } >> + break; >> + } >> +} >> + >> + >> + >> bool handle_mmio_raz_wi(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, >> phys_addr_t offset) >> { >> diff --git a/virt/kvm/arm/vgic.h b/virt/kvm/arm/vgic.h >> index a093f5c..b2d791c 100644 >> --- a/virt/kvm/arm/vgic.h >> +++ b/virt/kvm/arm/vgic.h >> @@ -71,6 +71,10 @@ void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, >> phys_addr_t offset, int mode); >> bool handle_mmio_raz_wi(struct kvm_vcpu *vcpu, struct kvm_exit_mmio *mmio, >> phys_addr_t offset); >> +void vgic_handle_base_register(struct kvm_vcpu *vcpu, >> + struct kvm_exit_mmio *mmio, >> + phys_addr_t offset, u64 *basereg, >> + int mode); >> >> static inline >> u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask) >> >