On Tue, Jun 18, 2013 at 09:43:27AM +0200, Marc Zyngier wrote:
> On Mon, 17 Jun 2013 23:00:59 -0700, Christoffer Dall
> <christoffer.dall@xxxxxxxxxx> wrote:
> > On Fri, Jun 14, 2013 at 02:20:30PM +0000, Zhaobo (Bob, ERC) wrote:
>
> [...]
> >
> > From: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> > Subject: [PATCH] KVM: ARM: vgic: Fix yet another cfg register bug
> >
> > This code is so unnecessarily complicated, but it does serve the purpose
> > of being able to and all the bitmaps together in the end. Oh well, we
> > fix another issue.
>
> If it is "so unnecessary complicated", how about submitting a patch that
> will make it both less complicated and bug-free? I'll definitely welcome
> such an improvement. And once it is correct, I'll Ack it.
Unnecessary complicated because we're directly setting getting values,
with the exception of ignoring writes to the lower 8 bytes, and we've
bugs in the compressing/uncompressing code already.
But, as I wrote, it has the benefit of allowing us to just and all the
bitmaps together in the end, and it makes the init code of this field
look so symmetrical, so my conclusion was that it's worth the
complication.
>
> In the meantime...
>
> > The registers are accessed by the guests view of the real HW, not the
> > compressed backing view, so the offsets are one-off.
> >
> > Reported-by: Zhaobo (Bob, ERC) <zhaobo@xxxxxxxxxx>
> > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
> > ---
> > arch/arm/kvm/vgic.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
> > index 17c5ac7..981d448 100644
> > --- a/arch/arm/kvm/vgic.c
> > +++ b/arch/arm/kvm/vgic.c
> > @@ -549,7 +549,7 @@ static bool handle_mmio_cfg_reg(struct kvm_vcpu
> *vcpu,
> > u32 val;
> > u32 *reg = vgic_bitmap_get_reg(&vcpu->kvm->arch.vgic.irq_cfg,
> > vcpu->vcpu_id, offset >> 1);
> > - if (offset & 2)
> > + if (offset & 4)
>
> This is wrong. Bob's suggestion was ugly, but correct.
>
Explain to me the difference between:
if ((x >> 2) & 1)
foo();
and
if (x & 4)
foo();
> > val = *reg >> 16;
> > else
> > val = *reg & 0xffff;
> > @@ -558,13 +558,13 @@ static bool handle_mmio_cfg_reg(struct kvm_vcpu
> > *vcpu,
> > vgic_reg_access(mmio, &val, offset,
> > ACCESS_READ_VALUE | ACCESS_WRITE_VALUE);
> > if (mmio->is_write) {
> > - if (offset < 4) {
> > + if (offset < 8) {
> > *reg = ~0U; /* Force PPIs/SGIs to 1 */
> > return false;
> > }
> >
> > val = vgic_cfg_compress(val);
> > - if (offset & 2) {
> > + if (offset & 4) {
>
> Same here.
>
> > *reg &= 0xffff;
> > *reg |= val << 16;
> > } else {
>
> M.
> --
> Fast, cheap, reliable. Pick two.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
