Am 14.01.2013 um 22:08 schrieb Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>:
> On Mon, Jan 14, 2013 at 10:31 AM, Will Deacon <will.deacon@xxxxxxx> wrote:
>> On Tue, Jan 08, 2013 at 06:41:51PM +0000, Christoffer Dall wrote:
>>> From: Marc Zyngier <marc.zyngier@xxxxxxx>
>>>
>>> Wire the basic framework code for VGIC support and the initial in-kernel
>>> MMIO support code for the VGIC, used for the distributor emulation.
>>
>> [...]
>>
>>> +/**
>>> + * vgic_reg_access - access vgic register
>>> + * @mmio: pointer to the data describing the mmio access
>>> + * @reg: pointer to the virtual backing of vgic distributor data
>>> + * @offset: least significant 2 bits used for word offset
>>> + * @mode: ACCESS_ mode (see defines above)
>>> + *
>>> + * Helper to make vgic register access easier using one of the access
>>> + * modes defined for vgic register access
>>> + * (read,raz,write-ignored,setbit,clearbit,write)
>>> + */
>>> +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;
>>> + u32 regval;
>>> +
>>> + /*
>>> + * Any alignment fault should have been delivered to the guest
>>> + * directly (ARM ARM B3.12.7 "Prioritization of aborts").
>>> + */
>>> +
>>> + mask = (~0U) >> shift;
>>> + if (reg) {
>>> + regval = *reg;
>>> + } else {
>>> + BUG_ON(mode != (ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED));
>>> + regval = 0;
>>> + }
>>> +
>>> + if (mmio->is_write) {
>>> + u32 data = (*((u32 *)mmio->data) & mask) << shift;
>>> + switch (ACCESS_WRITE_MASK(mode)) {
>>> + case ACCESS_WRITE_IGNORED:
>>> + return;
>>> +
>>> + case ACCESS_WRITE_SETBIT:
>>> + regval |= data;
>>> + break;
>>> +
>>> + case ACCESS_WRITE_CLEARBIT:
>>> + regval &= ~data;
>>> + break;
>>> +
>>> + case ACCESS_WRITE_VALUE:
>>> + regval = (regval & ~(mask << shift)) | data;
>>> + break;
>>> + }
>>> + *reg = regval;
>>> + } else {
>>> + switch (ACCESS_READ_MASK(mode)) {
>>> + case ACCESS_READ_RAZ:
>>> + regval = 0;
>>> + /* fall through */
>>> +
>>> + case ACCESS_READ_VALUE:
>>> + *((u32 *)mmio->data) = (regval >> shift) & mask;
>>> + }
>>> + }
>>> +}
>>
>> 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
IIRC we have a local endianness flag on ppc. Once you introduce big endian guests, you can just add one too and add a CAP for it. I wouldn't worry about it now though.
Alex
> 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 <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
> 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 <marc.zyngier@xxxxxxx>
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Signed-off-by: Christoffer Dall <c.dall@xxxxxxxxxxxxxxxxxxxxxx>
>
> 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
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
