[PATCH v5 04/12] ARM: KVM: Initial VGIC infrastructure code
From: Marc Zyngier <hidden>
Date: 2013-01-15 10:33:30
Also in:
kvm
On 14/01/13 21:08, Christoffer Dall wrote:
On Mon, Jan 14, 2013 at 10:31 AM, Will Deacon [off-list ref] wrote:
quoted
As I mentioned previously, I suspect that this doesn't work with big-endian systems. Whilst that's reasonable for the moment, a comment would be useful for the unlucky soul that decides to do that work in future (or add accessors for mmio->data as I suggested before).admittedly this really hurts my brain, but I think there's actually no problem with endianness: whatever comes in mmio->data will have native endianness and the vgic is always little-endian, so a guest would have to make sure to do its own endianness conversion before writing data, or did I get this backwards? (some nasty feeling about if the OS is compiled in another endianness than the hardware everything may break). Anyhow, I think there's another bug in this code though. Please take a look and see if you agree: commit 3cab2b93a6f6acd3c043e584f23b94ab8f1bbd66 Author: Christoffer Dall [off-list ref] Date: Mon Jan 14 15:55:18 2013 -0500 KVM: ARM: Limit vgic read/writes to load/store length The vgic read/write operations did not consider ldrb/strb masks, and would therefore unintentionally overwrite parts of a register. Consider for example a store of a single byte to a word-aligned address of one of the priority registers, that would cause the 3 most significant bytes to be overwritten with zeros. Cc: Marc Zyniger [off-list ref] Cc: Will Deacon [off-list ref] Signed-off-by: Christoffer Dall [off-list ref]
Acked-by: Marc Zyngier <redacted>
quoted hunk ↗ jump to hunk
diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c index 25daa07..5c1bcf5 100644 --- a/arch/arm/kvm/vgic.c +++ b/arch/arm/kvm/vgic.c@@ -233,6 +233,16 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu*vcpu, int irq) vcpu->arch.vgic_cpu.pending_shared); } +static u32 mmio_data_read(struct kvm_exit_mmio *mmio, u32 mask) +{ + return *((u32 *)mmio->data) & mask; +} + +static void mmio_data_write(struct kvm_exit_mmio *mmio, u32 mask, u32 value) +{ + *((u32 *)mmio->data) = value & mask; +} + /** * vgic_reg_access - access vgic register * @mmio: pointer to the data describing the mmio access@@ -247,8 +257,8 @@ static void vgic_cpu_irq_clear(struct kvm_vcpu*vcpu, int irq) static void vgic_reg_access(struct kvm_exit_mmio *mmio, u32 *reg, phys_addr_t offset, int mode) { - int shift = (offset & 3) * 8; - u32 mask; + int word_offset = (offset & 3) * 8; + u32 mask = (1UL << (mmio->len * 8)) - 1; u32 regval; /*@@ -256,7 +266,6 @@ static void vgic_reg_access(struct kvm_exit_mmio*mmio, u32 *reg, * directly (ARM ARM B3.12.7 "Prioritization of aborts"). */ - mask = (~0U) >> shift; if (reg) { regval = *reg; } else {@@ -265,7 +274,7 @@ static void vgic_reg_access(struct kvm_exit_mmio*mmio, u32 *reg, } if (mmio->is_write) { - u32 data = (*((u32 *)mmio->data) & mask) << shift; + u32 data = mmio_data_read(mmio, mask) << word_offset; switch (ACCESS_WRITE_MASK(mode)) { case ACCESS_WRITE_IGNORED: return;@@ -279,7 +288,7 @@ static void vgic_reg_access(struct kvm_exit_mmio*mmio, u32 *reg, break; case ACCESS_WRITE_VALUE: - regval = (regval & ~(mask << shift)) | data; + regval = (regval & ~(mask << word_offset)) | data; break; } *reg = regval;@@ -290,7 +299,7 @@ static void vgic_reg_access(struct kvm_exit_mmio*mmio, u32 *reg, /* fall through */ case ACCESS_READ_VALUE: - *((u32 *)mmio->data) = (regval >> shift) & mask; + mmio_data_write(mmio, mask, regval >> word_offset); } } }@@ -702,6 +711,12 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu,struct kvm_run *run, (mmio->phys_addr + mmio->len) > (base + KVM_VGIC_V2_DIST_SIZE)) return false; + /* We don't support ldrd / strd or ldm / stm to the emulated vgic */ + if (mmio->len > 4) { + kvm_inject_dabt(vcpu, mmio->phys_addr); + return true; + } + range = find_matching_range(vgic_ranges, mmio, base); if (unlikely(!range || !range->handle_mmio)) { pr_warn("Unhandled access %d %08llx %d\n", -- Thanks, -Christoffer
-- Jazz is not dead. It just smells funny...