Hi Christoffer, Thanks for your response. I will use proper email client next time . sorry for inconvenience . Regards, Bob. On 6/18/2013 8:00 AM, Christoffer Dall wrote: > On Fri, Jun 14, 2013 at 02:20:30PM +0000, Zhaobo (Bob, ERC) wrote: >> Hi all , > > Hi Bob, > > First, to increase your chances of getting a response on these forums to > your questions, please consider using a decent e-mail client and send > your e-mails in clear text. > > Your e-mail did not contain standard newline chars to wrap lines and > uses excessive indentation of the code. > > >> As introduced in gic arch spec , GICD_ICFGRn provide two bit(yet only >> bit[1] is used in gicv2) for each irq to indicate level or edge >> trigged irq type . One GICD_ICFGR can hold for 16 irqs. In order to >> save space , in software level , vgic_dist structure only use one bit >> for each irq type . so GICD_ICFGR[2n+1..2n] can be stored in one >> word(4 bytes). > > Not sure exactly what your notation indicates here, but yes, by using > one bit instead of two bits, we can store the same information in half > the registers, roughly. > >> In below function , offset is calculated with formula: >> offset = mmio->phys_addr - range->base - base . hereby offset of >> GICD_ICFGR1 is 4 and offset of GICD_ICFGR2 is 8. thus I think red >> parts in below function maybe is not correct. it would be changed to >> >> >> If ( (offset >> 2 )& 1) >> val = *reg >> 16; >> else >> val = *reg & 0xffff; > > Yeah, you are right, it's a bug. The insight is simply that the > accessor is based on the real mmio offset, not the compressed version. > > The patch below should fix it, thanks: > > --- > > 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. > > 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) > 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) { > *reg &= 0xffff; > *reg |= val << 16; > } else { > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm