| Message ID | 1488800074-21991-20-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Eric, just fast-forwarded to the end and noticed this one: On 06/03/17 11:34, Eric Auger wrote: > Save and restore the pending tables. > > Pending table restore obviously requires the pendbaser to be > already set. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v1 -> v2: > - do not care about the 1st KB which should be zeroed according to > the spec. > --- > virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 69 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 27ebabd..24824be 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz, > */ > static int vgic_its_flush_pending_tables(struct vgic_its *its) > { > - return -ENXIO; > + struct kvm *kvm = its->dev->kvm; > + struct vgic_dist *dist = &kvm->arch.vgic; > + struct vgic_irq *irq; > + int ret; > + > + /** > + * we do not take the dist->lpi_list_lock since we have a garantee > + * the LPI list is not touched while the its lock is held Can you elaborate on what gives us this guarantee? I see that we have a locking *order*, but that doesn't mean we can avoid taking the lock. So to me it looks like we need to take the lpi_list_lock spinlock here, which unfortunately breaks the kvm_read_guest() calls below. If you agree on this, you can take a look at the INVALL implementation, where I faced the same issue. The solution we came up with is vgic_copy_lpi_list(), which you can call under the lock to create a (private) copy of the LPI list, which you can later iterate without holding the lock - and thus are free to call sleeping functions. Cheers, Andre. > + */ > + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { > + struct kvm_vcpu *vcpu; > + gpa_t pendbase, ptr; > + bool stored; > + u8 val; > + > + vcpu = irq->target_vcpu; > + if (!vcpu) > + return -EINVAL; > + > + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); > + > + ptr = pendbase + (irq->intid / BITS_PER_BYTE); > + > + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); > + if (ret) > + return ret; > + > + stored = val & (irq->intid % BITS_PER_BYTE); > + if (stored == irq->pending_latch) > + continue; > + > + if (irq->pending_latch) > + val |= 1 << (irq->intid % BITS_PER_BYTE); > + else > + val &= ~(1 << (irq->intid % BITS_PER_BYTE)); > + > + ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1); > + if (ret) > + return ret; > + } > + > + return 0; > } > > /** > @@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its) > */ > static int vgic_its_restore_pending_tables(struct vgic_its *its) > { > - return -ENXIO; > + struct vgic_irq *irq; > + struct kvm *kvm = its->dev->kvm; > + struct vgic_dist *dist = &kvm->arch.vgic; > + int ret; > + > + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { > + struct kvm_vcpu *vcpu; > + gpa_t pendbase, ptr; > + u8 val; > + > + vcpu = irq->target_vcpu; > + if (!vcpu) > + return -EINVAL; > + > + if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ)) > + return 0; > + > + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); > + > + ptr = pendbase + (irq->intid / BITS_PER_BYTE); > + > + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); > + if (ret) > + return ret; > + irq->pending_latch = val & (1 << (irq->intid % BITS_PER_BYTE)); > + } > + return 0; > } > > static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev, >
Hi Eric, On 06/03/17 11:34, Eric Auger wrote: > Save and restore the pending tables. > > Pending table restore obviously requires the pendbaser to be > already set. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v1 -> v2: > - do not care about the 1st KB which should be zeroed according to > the spec. > --- > virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 69 insertions(+), 2 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 27ebabd..24824be 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz, > */ > static int vgic_its_flush_pending_tables(struct vgic_its *its) So as suspected before, I think passing the "its" pointer here is wrong. In fact you don't use that ITS except for getting the kvm pointer. Architecturally the pending tables are per redistributor, so you should pass a struct vcpu pointer. So the cleanest way would be to have a FLUSH/RESTORE_PENDING_TABLE ioctl on the *redistributor* kvm_device, which iterates through the list here and just dumps the pending bits for LPIs targeting that VCPU (skipping over others). Alternatively it would be enough to pass just a struct kvm pointer here and keep dumping all LPIs, but call this function only once per VM (not for each ITS). That sounds a bit dodgy from the architectural point of view, though. > { > - return -ENXIO; > + struct kvm *kvm = its->dev->kvm; > + struct vgic_dist *dist = &kvm->arch.vgic; > + struct vgic_irq *irq; > + int ret; > + > + /** > + * we do not take the dist->lpi_list_lock since we have a garantee > + * the LPI list is not touched while the its lock is held > + */ As mentioned before I think this has to be reworked to take the lock, copy the table, drop the lock again and then iterate over the (private) copy to handle each LPI. Or something completely different. But IIRC the lpi_list_lock is taken completely independent of any ITS emulation code in vgic.c. > + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { > + struct kvm_vcpu *vcpu; > + gpa_t pendbase, ptr; > + bool stored; > + u8 val; > + > + vcpu = irq->target_vcpu; > + if (!vcpu) > + return -EINVAL; Isn't target_vcpu == NULL a valid use case? So continue; instead of return? > + > + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); > + > + ptr = pendbase + (irq->intid / BITS_PER_BYTE); > + > + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); > + if (ret) > + return ret; > + > + stored = val & (irq->intid % BITS_PER_BYTE); > + if (stored == irq->pending_latch) > + continue; > + > + if (irq->pending_latch) > + val |= 1 << (irq->intid % BITS_PER_BYTE); > + else > + val &= ~(1 << (irq->intid % BITS_PER_BYTE)); > + > + ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1); > + if (ret) > + return ret; > + } > + > + return 0; > } > > /** > @@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its) > */ > static int vgic_its_restore_pending_tables(struct vgic_its *its) Could deserve the comment that it doesn't actually scan the table for set bits, but only checks the mapped LPIs (and thus should come last in the restore process). Also the same comment as above about using the "its" pointer applies here. > { > - return -ENXIO; > + struct vgic_irq *irq; > + struct kvm *kvm = its->dev->kvm; > + struct vgic_dist *dist = &kvm->arch.vgic; > + int ret; > + > + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { > + struct kvm_vcpu *vcpu; > + gpa_t pendbase, ptr; > + u8 val; > + > + vcpu = irq->target_vcpu; > + if (!vcpu) > + return -EINVAL; > + > + if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ)) > + return 0; I believe this bit is only set once by software to communicate that *initially* (upon enabling LPIs in the redistributor) the pending table is clear. It is never cleared by the redistributor, so you can't rely on it here. Cheers, Andre. > + > + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); > + > + ptr = pendbase + (irq->intid / BITS_PER_BYTE); > + > + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); > + if (ret) > + return ret; > + irq->pending_latch = val & (1 << (irq->intid % BITS_PER_BYTE)); > + } > + return 0; > } > > static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev, >
Hi Andre, On 20/03/2017 19:21, Andre Przywara wrote: > Hi Eric, > > just fast-forwarded to the end and noticed this one: > > > On 06/03/17 11:34, Eric Auger wrote: >> Save and restore the pending tables. >> >> Pending table restore obviously requires the pendbaser to be >> already set. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v1 -> v2: >> - do not care about the 1st KB which should be zeroed according to >> the spec. >> --- >> virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 69 insertions(+), 2 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 27ebabd..24824be 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz, >> */ >> static int vgic_its_flush_pending_tables(struct vgic_its *its) >> { >> - return -ENXIO; >> + struct kvm *kvm = its->dev->kvm; >> + struct vgic_dist *dist = &kvm->arch.vgic; >> + struct vgic_irq *irq; >> + int ret; >> + >> + /** >> + * we do not take the dist->lpi_list_lock since we have a garantee >> + * the LPI list is not touched while the its lock is held > > Can you elaborate on what gives us this guarantee? I see that we have a > locking *order*, but that doesn't mean we can avoid taking the lock. So > to me it looks like we need to take the lpi_list_lock spinlock here, > which unfortunately breaks the kvm_read_guest() calls below. > > If you agree on this, you can take a look at the INVALL implementation, > where I faced the same issue. The solution we came up with is > vgic_copy_lpi_list(), which you can call under the lock to create a > (private) copy of the LPI list, which you can later iterate without > holding the lock - and thus are free to call sleeping functions. Yes the comment is wrong and at least I need to fix it. The its_lock prevents new commands to be absorbed but does not protect from a change of the pending status which is our interest here. On the other hand, can't we simply consider the flush (and restore) cannot happen if the VM is in running state. In the current QEMU integration we wait for the VM to be paused before flushing the tables in guest RAM. Otherwise you will get some stall data anyway. So can't we simply document this requirement. I think the requirement is different from the INVALL's one. Does it make sense? Thanks Eric > > Cheers, > Andre. > >> + */ >> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >> + struct kvm_vcpu *vcpu; >> + gpa_t pendbase, ptr; >> + bool stored; >> + u8 val; >> + >> + vcpu = irq->target_vcpu; >> + if (!vcpu) >> + return -EINVAL; >> + >> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); >> + >> + ptr = pendbase + (irq->intid / BITS_PER_BYTE); >> + >> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); >> + if (ret) >> + return ret; >> + >> + stored = val & (irq->intid % BITS_PER_BYTE); >> + if (stored == irq->pending_latch) >> + continue; >> + >> + if (irq->pending_latch) >> + val |= 1 << (irq->intid % BITS_PER_BYTE); >> + else >> + val &= ~(1 << (irq->intid % BITS_PER_BYTE)); >> + >> + ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> } >> >> /** >> @@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its) >> */ >> static int vgic_its_restore_pending_tables(struct vgic_its *its) >> { >> - return -ENXIO; >> + struct vgic_irq *irq; >> + struct kvm *kvm = its->dev->kvm; >> + struct vgic_dist *dist = &kvm->arch.vgic; >> + int ret; >> + >> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >> + struct kvm_vcpu *vcpu; >> + gpa_t pendbase, ptr; >> + u8 val; >> + >> + vcpu = irq->target_vcpu; >> + if (!vcpu) >> + return -EINVAL; >> + >> + if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ)) >> + return 0; >> + >> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); >> + >> + ptr = pendbase + (irq->intid / BITS_PER_BYTE); >> + >> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); >> + if (ret) >> + return ret; >> + irq->pending_latch = val & (1 << (irq->intid % BITS_PER_BYTE)); >> + } >> + return 0; >> } >> >> static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev, >>
On 22/03/17 15:12, Auger Eric wrote: > Hi Andre, > > On 20/03/2017 19:21, Andre Przywara wrote: >> Hi Eric, >> >> just fast-forwarded to the end and noticed this one: >> >> >> On 06/03/17 11:34, Eric Auger wrote: >>> Save and restore the pending tables. >>> >>> Pending table restore obviously requires the pendbaser to be >>> already set. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> >>> --- >>> >>> v1 -> v2: >>> - do not care about the 1st KB which should be zeroed according to >>> the spec. >>> --- >>> virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 69 insertions(+), 2 deletions(-) >>> >>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >>> index 27ebabd..24824be 100644 >>> --- a/virt/kvm/arm/vgic/vgic-its.c >>> +++ b/virt/kvm/arm/vgic/vgic-its.c >>> @@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz, >>> */ >>> static int vgic_its_flush_pending_tables(struct vgic_its *its) >>> { >>> - return -ENXIO; >>> + struct kvm *kvm = its->dev->kvm; >>> + struct vgic_dist *dist = &kvm->arch.vgic; >>> + struct vgic_irq *irq; >>> + int ret; >>> + >>> + /** >>> + * we do not take the dist->lpi_list_lock since we have a garantee >>> + * the LPI list is not touched while the its lock is held >> >> Can you elaborate on what gives us this guarantee? I see that we have a >> locking *order*, but that doesn't mean we can avoid taking the lock. So >> to me it looks like we need to take the lpi_list_lock spinlock here, >> which unfortunately breaks the kvm_read_guest() calls below. >> >> If you agree on this, you can take a look at the INVALL implementation, >> where I faced the same issue. The solution we came up with is >> vgic_copy_lpi_list(), which you can call under the lock to create a >> (private) copy of the LPI list, which you can later iterate without >> holding the lock - and thus are free to call sleeping functions. > > Yes the comment is wrong and at least I need to fix it. The its_lock > prevents new commands to be absorbed but does not protect from a change > of the pending status which is our interest here. > > On the other hand, can't we simply consider the flush (and restore) > cannot happen if the VM is in running state. In the current QEMU > integration we wait for the VM to be paused before flushing the tables > in guest RAM. Otherwise you will get some stall data anyway. So can't we > simply document this requirement. I think the requirement is different > from the INVALL's one. Does it make sense? That's probably true, but then we should *enforce* this. Didn't we have something like this somewhere (in the old VGIC?), where we collected all VCPU locks to make sure nothing runs? This should then be checked upon the flush and restore kvm_device ioctls. And there should be comments on this, to not give people funny ideas. I am sure Marc would love to see some BUG_ONs ;-) Cheers, Andre. >> >> Cheers, >> Andre. >> >>> + */ >>> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >>> + struct kvm_vcpu *vcpu; >>> + gpa_t pendbase, ptr; >>> + bool stored; >>> + u8 val; >>> + >>> + vcpu = irq->target_vcpu; >>> + if (!vcpu) >>> + return -EINVAL; >>> + >>> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); >>> + >>> + ptr = pendbase + (irq->intid / BITS_PER_BYTE); >>> + >>> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); >>> + if (ret) >>> + return ret; >>> + >>> + stored = val & (irq->intid % BITS_PER_BYTE); >>> + if (stored == irq->pending_latch) >>> + continue; >>> + >>> + if (irq->pending_latch) >>> + val |= 1 << (irq->intid % BITS_PER_BYTE); >>> + else >>> + val &= ~(1 << (irq->intid % BITS_PER_BYTE)); >>> + >>> + ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + return 0; >>> } >>> >>> /** >>> @@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its) >>> */ >>> static int vgic_its_restore_pending_tables(struct vgic_its *its) >>> { >>> - return -ENXIO; >>> + struct vgic_irq *irq; >>> + struct kvm *kvm = its->dev->kvm; >>> + struct vgic_dist *dist = &kvm->arch.vgic; >>> + int ret; >>> + >>> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >>> + struct kvm_vcpu *vcpu; >>> + gpa_t pendbase, ptr; >>> + u8 val; >>> + >>> + vcpu = irq->target_vcpu; >>> + if (!vcpu) >>> + return -EINVAL; >>> + >>> + if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ)) >>> + return 0; >>> + >>> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); >>> + >>> + ptr = pendbase + (irq->intid / BITS_PER_BYTE); >>> + >>> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); >>> + if (ret) >>> + return ret; >>> + irq->pending_latch = val & (1 << (irq->intid % BITS_PER_BYTE)); >>> + } >>> + return 0; >>> } >>> >>> static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev, >>>
Hi Andre, On 22/03/2017 15:39, Andre Przywara wrote: > Hi Eric, > > On 06/03/17 11:34, Eric Auger wrote: >> Save and restore the pending tables. >> >> Pending table restore obviously requires the pendbaser to be >> already set. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v1 -> v2: >> - do not care about the 1st KB which should be zeroed according to >> the spec. >> --- >> virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 69 insertions(+), 2 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 27ebabd..24824be 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz, >> */ >> static int vgic_its_flush_pending_tables(struct vgic_its *its) > > So as suspected before, I think passing the "its" pointer here is wrong. > In fact you don't use that ITS except for getting the kvm pointer. > Architecturally the pending tables are per redistributor, so you should > pass a struct vcpu pointer. > So the cleanest way would be to have a FLUSH/RESTORE_PENDING_TABLE ioctl > on the *redistributor* kvm_device, which iterates through the list here > and just dumps the pending bits for LPIs targeting that VCPU (skipping > over others). > Alternatively it would be enough to pass just a struct kvm pointer here > and keep dumping all LPIs, but call this function only once per VM (not > for each ITS). That sounds a bit dodgy from the architectural point of > view, though. > >> { >> - return -ENXIO; >> + struct kvm *kvm = its->dev->kvm; >> + struct vgic_dist *dist = &kvm->arch.vgic; >> + struct vgic_irq *irq; >> + int ret; >> + >> + /** >> + * we do not take the dist->lpi_list_lock since we have a garantee >> + * the LPI list is not touched while the its lock is held >> + */ > > As mentioned before I think this has to be reworked to take the lock, > copy the table, drop the lock again and then iterate over the (private) > copy to handle each LPI. > Or something completely different. > But IIRC the lpi_list_lock is taken completely independent of any ITS > emulation code in vgic.c. I aligned the code with other user access: take the kvm lock and take all vcpu locks to make sure the vcpu are not running > >> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >> + struct kvm_vcpu *vcpu; >> + gpa_t pendbase, ptr; >> + bool stored; >> + u8 val; >> + >> + vcpu = irq->target_vcpu; >> + if (!vcpu) >> + return -EINVAL; > > Isn't target_vcpu == NULL a valid use case? So continue; instead of return? yes correct. > >> + >> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); >> + >> + ptr = pendbase + (irq->intid / BITS_PER_BYTE); >> + >> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); >> + if (ret) >> + return ret; >> + >> + stored = val & (irq->intid % BITS_PER_BYTE); >> + if (stored == irq->pending_latch) >> + continue; >> + >> + if (irq->pending_latch) >> + val |= 1 << (irq->intid % BITS_PER_BYTE); >> + else >> + val &= ~(1 << (irq->intid % BITS_PER_BYTE)); >> + >> + ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> } >> >> /** >> @@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its) >> */ >> static int vgic_its_restore_pending_tables(struct vgic_its *its) > > Could deserve the comment that it doesn't actually scan the table for > set bits, but only checks the mapped LPIs (and thus should come last in > the restore process). > Also the same comment as above about using the "its" pointer applies here. done > >> { >> - return -ENXIO; >> + struct vgic_irq *irq; >> + struct kvm *kvm = its->dev->kvm; >> + struct vgic_dist *dist = &kvm->arch.vgic; >> + int ret; >> + >> + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >> + struct kvm_vcpu *vcpu; >> + gpa_t pendbase, ptr; >> + u8 val; >> + >> + vcpu = irq->target_vcpu; >> + if (!vcpu) >> + return -EINVAL; >> + >> + if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ)) >> + return 0; > > I believe this bit is only set once by software to communicate that > *initially* (upon enabling LPIs in the redistributor) the pending table > is clear. It is never cleared by the redistributor, so you can't rely on > it here. you're right thanks Eric > > Cheers, > Andre. > >> + >> + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); >> + >> + ptr = pendbase + (irq->intid / BITS_PER_BYTE); >> + >> + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); >> + if (ret) >> + return ret; >> + irq->pending_latch = val & (1 << (irq->intid % BITS_PER_BYTE)); >> + } >> + return 0; >> } >> >> static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev, >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 27ebabd..24824be 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -1736,7 +1736,48 @@ static int lookup_table(struct vgic_its *its, gpa_t base, int size, int esz, */ static int vgic_its_flush_pending_tables(struct vgic_its *its) { - return -ENXIO; + struct kvm *kvm = its->dev->kvm; + struct vgic_dist *dist = &kvm->arch.vgic; + struct vgic_irq *irq; + int ret; + + /** + * we do not take the dist->lpi_list_lock since we have a garantee + * the LPI list is not touched while the its lock is held + */ + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { + struct kvm_vcpu *vcpu; + gpa_t pendbase, ptr; + bool stored; + u8 val; + + vcpu = irq->target_vcpu; + if (!vcpu) + return -EINVAL; + + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); + + ptr = pendbase + (irq->intid / BITS_PER_BYTE); + + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); + if (ret) + return ret; + + stored = val & (irq->intid % BITS_PER_BYTE); + if (stored == irq->pending_latch) + continue; + + if (irq->pending_latch) + val |= 1 << (irq->intid % BITS_PER_BYTE); + else + val &= ~(1 << (irq->intid % BITS_PER_BYTE)); + + ret = kvm_write_guest(kvm, (gpa_t)ptr, &val, 1); + if (ret) + return ret; + } + + return 0; } /** @@ -1745,7 +1786,33 @@ static int vgic_its_flush_pending_tables(struct vgic_its *its) */ static int vgic_its_restore_pending_tables(struct vgic_its *its) { - return -ENXIO; + struct vgic_irq *irq; + struct kvm *kvm = its->dev->kvm; + struct vgic_dist *dist = &kvm->arch.vgic; + int ret; + + list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { + struct kvm_vcpu *vcpu; + gpa_t pendbase, ptr; + u8 val; + + vcpu = irq->target_vcpu; + if (!vcpu) + return -EINVAL; + + if (!(vcpu->arch.vgic_cpu.pendbaser & GICR_PENDBASER_PTZ)) + return 0; + + pendbase = PENDBASER_ADDRESS(vcpu->arch.vgic_cpu.pendbaser); + + ptr = pendbase + (irq->intid / BITS_PER_BYTE); + + ret = kvm_read_guest(kvm, (gpa_t)ptr, &val, 1); + if (ret) + return ret; + irq->pending_latch = val & (1 << (irq->intid % BITS_PER_BYTE)); + } + return 0; } static int vgic_its_flush_ite(struct vgic_its *its, struct its_device *dev,
Save and restore the pending tables. Pending table restore obviously requires the pendbaser to be already set. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v1 -> v2: - do not care about the 1st KB which should be zeroed according to the spec. --- virt/kvm/arm/vgic/vgic-its.c | 71 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 2 deletions(-)