Re: [PATCH v5 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
Hi Marc,
On 4/5/21 12:10 PM, Marc Zyngier wrote:
> On Sun, 04 Apr 2021 18:22:42 +0100,
> Eric Auger 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
>>
>> ---
>>
>> 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 = >kvm->arch.vgic;
>> +struct vgic_cpu *vgic_cpu = >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 = >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.
>
Re: [PATCH v5 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
On Sun, 04 Apr 2021 18:22:42 +0100,
Eric Auger 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
>
> ---
>
> 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 = >kvm->arch.vgic;
> + struct vgic_cpu *vgic_cpu = >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 = >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.
--
Without deviation from the norm, progress is not possible.
[PATCH v5 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
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
---
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 = >kvm->arch.vgic;
+ struct vgic_cpu *vgic_cpu = >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 = >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 = >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 & 0x) << 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 & 0x) << 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
