| Message ID | 20200331031245.1562-1-yuzenghui@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic-v3: Clear pending bit in guest memory after synchronization | expand |
Hi Zenghui, On Tue, 31 Mar 2020 11:12:45 +0800 Zenghui Yu <yuzenghui@huawei.com> wrote: > When LPI support is enabled at redistributor level, VGIC will potentially > load the correspond LPI penging table and sync it into the pending_latch. > To avoid keeping the 'consumed' pending bits lying around in guest memory > (though they're not used), let's clear them after synchronization. > > The similar work had been done in vgic_v3_lpi_sync_pending_status(). > > Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> > --- > virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index d53d34a33e35..905760bfa404 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -435,6 +435,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) > > for (i = 0; i < nr_irqs; i++) { > int byte_offset, bit_nr; > + bool status; > > byte_offset = intids[i] / BITS_PER_BYTE; > bit_nr = intids[i] % BITS_PER_BYTE; > @@ -447,22 +448,32 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) > ret = kvm_read_guest_lock(vcpu->kvm, > pendbase + byte_offset, > &pendmask, 1); > - if (ret) { > - kfree(intids); > - return ret; > - } > + if (ret) > + goto out; > last_byte_offset = byte_offset; > } > > + status = pendmask & (1 << bit_nr); > + > irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]); > raw_spin_lock_irqsave(&irq->irq_lock, flags); > - irq->pending_latch = pendmask & (1U << bit_nr); > + irq->pending_latch = status; > vgic_queue_irq_unlock(vcpu->kvm, irq, flags); > vgic_put_irq(vcpu->kvm, irq); > + > + if (status) { > + /* clear consumed data */ > + pendmask &= ~(1 << bit_nr); > + ret = kvm_write_guest_lock(vcpu->kvm, > + pendbase + byte_offset, > + &pendmask, 1); > + if (ret) > + goto out; > + } > } > > +out: > kfree(intids); > - > return ret; > } > I've been thinking about this, and I wonder why we don't simply clear the whole pending table instead of carefully wiping it one bit at a time. My reasoning is that if a LPI isn't mapped, then it cannot be made pending the first place. And I think there is a similar issue in vgic_v3_lpi_sync_pending_status(). Why sync something back from the pending table when the LPI wasn't mapped yet? This seems pretty bizarre, as the GITS_TRANSLATER spec says that the write to this register is ignored when: "- The EventID is mapped to an Interrupt Translation Table and the EventID is within the range specified by MAPD on page 5-107, but the EventID is unmapped." (with the added bonus in the form of a type: the first instance of "EventID" here should obviously be "DeviceID"). What do you think? M.
Hi Marc, On 2020/3/31 16:07, Marc Zyngier wrote: > Hi Zenghui, > > On Tue, 31 Mar 2020 11:12:45 +0800 > Zenghui Yu <yuzenghui@huawei.com> wrote: > >> When LPI support is enabled at redistributor level, VGIC will potentially >> load the correspond LPI penging table and sync it into the pending_latch. >> To avoid keeping the 'consumed' pending bits lying around in guest memory >> (though they're not used), let's clear them after synchronization. >> >> The similar work had been done in vgic_v3_lpi_sync_pending_status(). >> >> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> >> --- >> virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index d53d34a33e35..905760bfa404 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -435,6 +435,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) >> >> for (i = 0; i < nr_irqs; i++) { >> int byte_offset, bit_nr; >> + bool status; >> >> byte_offset = intids[i] / BITS_PER_BYTE; >> bit_nr = intids[i] % BITS_PER_BYTE; >> @@ -447,22 +448,32 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) >> ret = kvm_read_guest_lock(vcpu->kvm, >> pendbase + byte_offset, >> &pendmask, 1); >> - if (ret) { >> - kfree(intids); >> - return ret; >> - } >> + if (ret) >> + goto out; >> last_byte_offset = byte_offset; >> } >> >> + status = pendmask & (1 << bit_nr); >> + >> irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]); >> raw_spin_lock_irqsave(&irq->irq_lock, flags); >> - irq->pending_latch = pendmask & (1U << bit_nr); >> + irq->pending_latch = status; >> vgic_queue_irq_unlock(vcpu->kvm, irq, flags); >> vgic_put_irq(vcpu->kvm, irq); >> + >> + if (status) { >> + /* clear consumed data */ >> + pendmask &= ~(1 << bit_nr); >> + ret = kvm_write_guest_lock(vcpu->kvm, >> + pendbase + byte_offset, >> + &pendmask, 1); >> + if (ret) >> + goto out; >> + } >> } >> >> +out: >> kfree(intids); >> - >> return ret; >> } >> > > I've been thinking about this, and I wonder why we don't simply clear > the whole pending table instead of carefully wiping it one bit at a > time. My reasoning is that if a LPI isn't mapped, then it cannot be made > pending the first place. A writing to GICR_CTLR.EnableLPIs can happen in parallel with MAPTI/INT command sequence, where the new LPI is mapped to *this* vcpu and made pending, wrong? I think commit 7d8b44c54e0c had described it in detail. But thinking that we cache the pending bit in pending_latch (instead of writing the corresponding bit in guest memory) when a LPI is made pending, it seems to be safe to clear the whole pending table here. > > And I think there is a similar issue in vgic_v3_lpi_sync_pending_status(). > Why sync something back from the pending table when the LPI wasn't > mapped yet? vgic_v3_lpi_sync_pending_status() can be called on the ITE restore path: vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status. We should rely on it to sync the pending bit from guest memory (which was saved on the source side). > This seems pretty bizarre, as the GITS_TRANSLATER spec says > that the write to this register is ignored when: > > "- The EventID is mapped to an Interrupt Translation Table and the > EventID is within the range specified by MAPD on page 5-107, but the > EventID is unmapped." > > (with the added bonus in the form of a type: the first instance of > "EventID" here should obviously be "DeviceID"). ;-) Thanks, Zenghui
On Tue, 31 Mar 2020 17:11:37 +0800 Zenghui Yu <yuzenghui@huawei.com> wrote: Hi Zenghui, > Hi Marc, > > On 2020/3/31 16:07, Marc Zyngier wrote: > > Hi Zenghui, [...] > >> > > I've been thinking about this, and I wonder why we don't simply clear > > the whole pending table instead of carefully wiping it one bit at a > > time. My reasoning is that if a LPI isn't mapped, then it cannot be made > > pending the first place. > > A writing to GICR_CTLR.EnableLPIs can happen in parallel with MAPTI/INT > command sequence, where the new LPI is mapped to *this* vcpu and made > pending, wrong? I think commit 7d8b44c54e0c had described it in detail. I'm not sure this commit is relevant here. It describes how the configuration is picked up by MAPTI, not how the pending bit got there the first place. > But thinking that we cache the pending bit in pending_latch (instead of > writing the corresponding bit in guest memory) when a LPI is made > pending, it seems to be safe to clear the whole pending table here. Yes, and this is my worry. The spec is pretty vague about what the behaviour of the redistributor is when something is set in the pending table. At the moment, we treat these bits as if they had been generated by a translation, but we do so inconsistently: we only pick these bits up and generate a LPI if there is a mapping at the ITS level. If these bits are relevant, we should forward a LPI to the CPU. It feels we're in UNPREDICTIBLE land... > > > > > And I think there is a similar issue in vgic_v3_lpi_sync_pending_status(). > > Why sync something back from the pending table when the LPI wasn't > > mapped yet? > > vgic_v3_lpi_sync_pending_status() can be called on the ITE restore path: > vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status. > We should rely on it to sync the pending bit from guest memory (which > was saved on the source side). The fact that we have *two* paths to restore pending bits is pretty annoying. There is certainly some scope for simplification here. Thanks, M.
On 2020/4/1 18:27, Marc Zyngier wrote: >>> And I think there is a similar issue in vgic_v3_lpi_sync_pending_status(). >>> Why sync something back from the pending table when the LPI wasn't >>> mapped yet? >> >> vgic_v3_lpi_sync_pending_status() can be called on the ITE restore path: >> vgic_its_restore_ite/vgic_add_lpi/vgic_v3_lpi_sync_pending_status. >> We should rely on it to sync the pending bit from guest memory (which >> was saved on the source side). > > The fact that we have *two* paths to restore pending bits is pretty > annoying. There is certainly some scope for simplification here. One thing need to be clarified first (if we're going to do some simplification here) is that if we follow the "ITS Restore Sequence" rule (in Documentation/virt/kvm/devices/arm-vgic-its.rst, which says that all redistributors are restored *before* ITS table data), then the pending bits will *only* be restored on the ITE restore path. When we're restoring the GICR_CTLR, we invoke vgic_enable_lpis()-> its_sync_lpi_pending_table(). But since no LPI has been restored yet, we will get an empty lpi_list snapshot from vgic_copy_lpi_list(). No pending table synchronization will happen on this path. I think this is what we have in the current code. Thanks, Zenghui
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index d53d34a33e35..905760bfa404 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -435,6 +435,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) for (i = 0; i < nr_irqs; i++) { int byte_offset, bit_nr; + bool status; byte_offset = intids[i] / BITS_PER_BYTE; bit_nr = intids[i] % BITS_PER_BYTE; @@ -447,22 +448,32 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) ret = kvm_read_guest_lock(vcpu->kvm, pendbase + byte_offset, &pendmask, 1); - if (ret) { - kfree(intids); - return ret; - } + if (ret) + goto out; last_byte_offset = byte_offset; } + status = pendmask & (1 << bit_nr); + irq = vgic_get_irq(vcpu->kvm, NULL, intids[i]); raw_spin_lock_irqsave(&irq->irq_lock, flags); - irq->pending_latch = pendmask & (1U << bit_nr); + irq->pending_latch = status; vgic_queue_irq_unlock(vcpu->kvm, irq, flags); vgic_put_irq(vcpu->kvm, irq); + + if (status) { + /* clear consumed data */ + pendmask &= ~(1 << bit_nr); + ret = kvm_write_guest_lock(vcpu->kvm, + pendbase + byte_offset, + &pendmask, 1); + if (ret) + goto out; + } } +out: kfree(intids); - return ret; }
When LPI support is enabled at redistributor level, VGIC will potentially load the correspond LPI penging table and sync it into the pending_latch. To avoid keeping the 'consumed' pending bits lying around in guest memory (though they're not used), let's clear them after synchronization. The similar work had been done in vgic_v3_lpi_sync_pending_status(). Signed-off-by: Zenghui Yu <yuzenghui@huawei.com> --- virt/kvm/arm/vgic/vgic-its.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)