| Message ID | 20210404172243.504309-8-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/ARM: Some vgic fixes and init sequence KVM selftests | expand |
On Sun, 04 Apr 2021 18:22:42 +0100, Eric Auger <eric.auger@redhat.com> wrote: > > Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the > reporting of GICR_TYPER.Last for userspace") temporarily fixed > a bug identified when attempting to access the GICR_TYPER > register before the redistributor region setting, but dropped > the support of the LAST bit. > > Emulating the GICR_TYPER.Last bit still makes sense for > architecture compliance though. This patch restores its support > (if the redistributor region was set) while keeping the code safe. > > We introduce a new helper, vgic_mmio_vcpu_rdist_is_last() which > computes whether a redistributor is the highest one of a series > of redistributor contributor pages. > > With this new implementation we do not need to have a uaccess > read accessor anymore. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > v4 -> v5: > - redist region list now is sorted by @base > - change the implementation according to Marc's understanding of > the spec > --- > arch/arm64/kvm/vgic/vgic-mmio-v3.c | 58 +++++++++++++++++------------- > include/kvm/arm_vgic.h | 1 + > 2 files changed, 34 insertions(+), 25 deletions(-) > > diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > index e1ed0c5a8eaa..03a253785700 100644 > --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c > @@ -251,45 +251,52 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu, > vgic_enable_lpis(vcpu); > } > > +static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu) > +{ > + struct vgic_dist *vgic = &vcpu->kvm->arch.vgic; > + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > + struct vgic_redist_region *iter, *rdreg = vgic_cpu->rdreg; > + > + if (!rdreg) > + return false; > + > + if (vgic_cpu->rdreg_index < rdreg->free_index - 1) { > + return false; > + } else if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) { > + struct list_head *rd_regions = &vgic->rd_regions; > + gpa_t end = rdreg->base + rdreg->count * KVM_VGIC_V3_REDIST_SIZE; > + > + /* > + * the rdist is the last one of the redist region, > + * check whether there is no other contiguous rdist region > + */ > + list_for_each_entry(iter, rd_regions, list) { > + if (iter->base == end && iter->free_index > 0) > + return false; > + } In the above notes, you state that the region list is now sorted by base address, but I really can't see what sorts that list. And the lines above indicate that you are still iterating over the whole RD regions. It's not a big deal (the code is now simple enough), but that's just to confirm that I understand what is going on here. Thanks, M.
Hi Marc, On 4/5/21 12:10 PM, Marc Zyngier wrote: > On Sun, 04 Apr 2021 18:22:42 +0100, > Eric Auger <eric.auger@redhat.com> wrote: >> >> Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the >> reporting of GICR_TYPER.Last for userspace") temporarily fixed >> a bug identified when attempting to access the GICR_TYPER >> register before the redistributor region setting, but dropped >> the support of the LAST bit. >> >> Emulating the GICR_TYPER.Last bit still makes sense for >> architecture compliance though. This patch restores its support >> (if the redistributor region was set) while keeping the code safe. >> >> We introduce a new helper, vgic_mmio_vcpu_rdist_is_last() which >> computes whether a redistributor is the highest one of a series >> of redistributor contributor pages. >> >> With this new implementation we do not need to have a uaccess >> read accessor anymore. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> v4 -> v5: >> - redist region list now is sorted by @base >> - change the implementation according to Marc's understanding of >> the spec >> --- >> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 58 +++++++++++++++++------------- >> include/kvm/arm_vgic.h | 1 + >> 2 files changed, 34 insertions(+), 25 deletions(-) >> >> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c >> index e1ed0c5a8eaa..03a253785700 100644 >> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c >> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c >> @@ -251,45 +251,52 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu, >> vgic_enable_lpis(vcpu); >> } >> >> +static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu) >> +{ >> + struct vgic_dist *vgic = &vcpu->kvm->arch.vgic; >> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; >> + struct vgic_redist_region *iter, *rdreg = vgic_cpu->rdreg; >> + >> + if (!rdreg) >> + return false; >> + >> + if (vgic_cpu->rdreg_index < rdreg->free_index - 1) { >> + return false; >> + } else if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) { >> + struct list_head *rd_regions = &vgic->rd_regions; >> + gpa_t end = rdreg->base + rdreg->count * KVM_VGIC_V3_REDIST_SIZE; >> + >> + /* >> + * the rdist is the last one of the redist region, >> + * check whether there is no other contiguous rdist region >> + */ >> + list_for_each_entry(iter, rd_regions, list) { >> + if (iter->base == end && iter->free_index > 0) >> + return false; >> + } > > In the above notes, you state that the region list is now sorted by > base address, but I really can't see what sorts that list. And the > lines above indicate that you are still iterating over the whole RD > regions. > > It's not a big deal (the code is now simple enough), but that's just > to confirm that I understand what is going on here. Sorry I should have removed the notes. I made the change but then I noticed that the list was already sorted by redistributor region index as the API forbids to register rdist regions in non ascending index order. So sorting by base address was eventually causing more trouble than it helped. Thanks Eric > > Thanks, > > M. >
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c index e1ed0c5a8eaa..03a253785700 100644 --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c @@ -251,45 +251,52 @@ static void vgic_mmio_write_v3r_ctlr(struct kvm_vcpu *vcpu, vgic_enable_lpis(vcpu); } +static bool vgic_mmio_vcpu_rdist_is_last(struct kvm_vcpu *vcpu) +{ + struct vgic_dist *vgic = &vcpu->kvm->arch.vgic; + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; + struct vgic_redist_region *iter, *rdreg = vgic_cpu->rdreg; + + if (!rdreg) + return false; + + if (vgic_cpu->rdreg_index < rdreg->free_index - 1) { + return false; + } else if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) { + struct list_head *rd_regions = &vgic->rd_regions; + gpa_t end = rdreg->base + rdreg->count * KVM_VGIC_V3_REDIST_SIZE; + + /* + * the rdist is the last one of the redist region, + * check whether there is no other contiguous rdist region + */ + list_for_each_entry(iter, rd_regions, list) { + if (iter->base == end && iter->free_index > 0) + return false; + } + } + return true; +} + static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len) { unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu); - struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; - struct vgic_redist_region *rdreg = vgic_cpu->rdreg; int target_vcpu_id = vcpu->vcpu_id; - gpa_t last_rdist_typer = rdreg->base + GICR_TYPER + - (rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE; u64 value; value = (u64)(mpidr & GENMASK(23, 0)) << 32; value |= ((target_vcpu_id & 0xffff) << 8); - if (addr == last_rdist_typer) + if (vgic_has_its(vcpu->kvm)) + value |= GICR_TYPER_PLPIS; + + if (vgic_mmio_vcpu_rdist_is_last(vcpu)) value |= GICR_TYPER_LAST; - if (vgic_has_its(vcpu->kvm)) - value |= GICR_TYPER_PLPIS; return extract_bytes(value, addr & 7, len); } -static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu, - gpa_t addr, unsigned int len) -{ - unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu); - int target_vcpu_id = vcpu->vcpu_id; - u64 value; - - value = (u64)(mpidr & GENMASK(23, 0)) << 32; - value |= ((target_vcpu_id & 0xffff) << 8); - - if (vgic_has_its(vcpu->kvm)) - value |= GICR_TYPER_PLPIS; - - /* reporting of the Last bit is not supported for userspace */ - return extract_bytes(value, addr & 7, len); -} - static unsigned long vgic_mmio_read_v3r_iidr(struct kvm_vcpu *vcpu, gpa_t addr, unsigned int len) { @@ -612,7 +619,7 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = { VGIC_ACCESS_32bit), REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_TYPER, vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, - vgic_uaccess_read_v3r_typer, vgic_mmio_uaccess_write_wi, 8, + NULL, vgic_mmio_uaccess_write_wi, 8, VGIC_ACCESS_64bit | VGIC_ACCESS_32bit), REGISTER_DESC_WITH_LENGTH(GICR_WAKER, vgic_mmio_read_raz, vgic_mmio_write_wi, 4, @@ -714,6 +721,7 @@ int vgic_register_redist_iodev(struct kvm_vcpu *vcpu) return -EINVAL; vgic_cpu->rdreg = rdreg; + vgic_cpu->rdreg_index = rdreg->free_index; rd_base = rdreg->base + rdreg->free_index * KVM_VGIC_V3_REDIST_SIZE; diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index 3d74f1060bd1..ec621180ef09 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -322,6 +322,7 @@ struct vgic_cpu { */ struct vgic_io_device rd_iodev; struct vgic_redist_region *rdreg; + u32 rdreg_index; /* Contains the attributes and gpa of the LPI pending tables. */ u64 pendbaser;
Commit 23bde34771f1 ("KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace") temporarily fixed a bug identified when attempting to access the GICR_TYPER register before the redistributor region setting, but dropped the support of the LAST bit. Emulating the GICR_TYPER.Last bit still makes sense for architecture compliance though. This patch restores its support (if the redistributor region was set) while keeping the code safe. We introduce a new helper, vgic_mmio_vcpu_rdist_is_last() which computes whether a redistributor is the highest one of a series of redistributor contributor pages. With this new implementation we do not need to have a uaccess read accessor anymore. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v4 -> v5: - redist region list now is sorted by @base - change the implementation according to Marc's understanding of the spec --- arch/arm64/kvm/vgic/vgic-mmio-v3.c | 58 +++++++++++++++++------------- include/kvm/arm_vgic.h | 1 + 2 files changed, 34 insertions(+), 25 deletions(-)