Re: [PATCH v4 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
Hi Eric,
On Thu, 01 Apr 2021 20:16:53 +0100,
Auger Eric wrote:
>
> Hi Marc,
>
> On 4/1/21 7:30 PM, Marc Zyngier wrote:
> > On Thu, 01 Apr 2021 18:03:25 +0100,
> > Auger Eric wrote:
> >>
> >> Hi Marc,
> >>
> >> On 4/1/21 3:42 PM, Marc Zyngier wrote:
> >>> Hi Eric,
> >>>
> >>> On Thu, 01 Apr 2021 09:52:37 +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.
>
> The spec says "Indicates whether this Redistributor is the
> highest-numbered Redistributor in a series of contiguous
> Redistributor pages."
>
> The code is a bit convulated since there is no guarantee
> >>>
> >>> nit: convoluted
> >>>
> redistributors are added in a given reditributor region in
> ascending order. In that case the current implementation was
> wrong. Also redistributor regions can be contiguous
> and registered in non increasing base address order.
>
> So the index of redistributors are stored in an array within
> the redistributor region structure.
>
> With this new implementation we do not need to have a uaccess
> read accessor anymore.
>
> Signed-off-by: Eric Auger
> >>>
> >>> This patch also hurt my head, a lot more than the first one. See
> >>> below.
> >>>
> ---
> arch/arm64/kvm/vgic/vgic-init.c| 7 +--
> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 97 --
> arch/arm64/kvm/vgic/vgic.h | 1 +
> include/kvm/arm_vgic.h | 3 +
> 4 files changed, 73 insertions(+), 35 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c
> b/arch/arm64/kvm/vgic/vgic-init.c
> index cf6faa0aeddb2..61150c34c268c 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -190,6 +190,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> int i;
>
> vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
> +vgic_cpu->index = vcpu->vcpu_id;
> >>>
> >>> Is it so that vgic_cpu->index is always equal to vcpu_id? If so, why
> >>> do we need another field? We can always get to the vcpu using a
> >>> container_of().
> >>>
>
> INIT_LIST_HEAD(_cpu->ap_list_head);
> raw_spin_lock_init(_cpu->ap_list_lock);
> @@ -338,10 +339,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
> dist->vgic_dist_base = VGIC_ADDR_UNDEF;
>
> if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> -list_for_each_entry_safe(rdreg, next,
> >rd_regions, list) {
> -list_del(>list);
> -kfree(rdreg);
> -}
> +list_for_each_entry_safe(rdreg, next,
> >rd_regions, list)
> +vgic_v3_free_redist_region(rdreg);
> >>>
> >>> Consider moving the introduction of vgic_v3_free_redist_region() into
> >>> a separate patch. On its own, that's a good readability improvement.
> >>>
> INIT_LIST_HEAD(>rd_regions);
> } else {
> dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 987e366c80008..f6a7eed1d6adb 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -251,45 +251,57 @@ 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 *rdreg = vgic_cpu->rdreg;
> +
> +if (!rdreg)
> +return false;
> +
> +if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count -
> 1)) {
> +/* check whether there is no other contiguous rdist
> region */
> +struct list_head *rd_regions = >rd_regions;
> +struct vgic_redist_region *iter;
> +
> +
Re: [PATCH v4 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
Hi Marc,
On 4/1/21 7:30 PM, Marc Zyngier wrote:
> On Thu, 01 Apr 2021 18:03:25 +0100,
> Auger Eric wrote:
>>
>> Hi Marc,
>>
>> On 4/1/21 3:42 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On Thu, 01 Apr 2021 09:52:37 +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.
The spec says "Indicates whether this Redistributor is the
highest-numbered Redistributor in a series of contiguous
Redistributor pages."
The code is a bit convulated since there is no guarantee
>>>
>>> nit: convoluted
>>>
redistributors are added in a given reditributor region in
ascending order. In that case the current implementation was
wrong. Also redistributor regions can be contiguous
and registered in non increasing base address order.
So the index of redistributors are stored in an array within
the redistributor region structure.
With this new implementation we do not need to have a uaccess
read accessor anymore.
Signed-off-by: Eric Auger
>>>
>>> This patch also hurt my head, a lot more than the first one. See
>>> below.
>>>
---
arch/arm64/kvm/vgic/vgic-init.c| 7 +--
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 97 --
arch/arm64/kvm/vgic/vgic.h | 1 +
include/kvm/arm_vgic.h | 3 +
4 files changed, 73 insertions(+), 35 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-init.c
b/arch/arm64/kvm/vgic/vgic-init.c
index cf6faa0aeddb2..61150c34c268c 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -190,6 +190,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
int i;
vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
+ vgic_cpu->index = vcpu->vcpu_id;
>>>
>>> Is it so that vgic_cpu->index is always equal to vcpu_id? If so, why
>>> do we need another field? We can always get to the vcpu using a
>>> container_of().
>>>
INIT_LIST_HEAD(_cpu->ap_list_head);
raw_spin_lock_init(_cpu->ap_list_lock);
@@ -338,10 +339,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
dist->vgic_dist_base = VGIC_ADDR_UNDEF;
if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
- list_for_each_entry_safe(rdreg, next, >rd_regions, list) {
- list_del(>list);
- kfree(rdreg);
- }
+ list_for_each_entry_safe(rdreg, next, >rd_regions, list)
+ vgic_v3_free_redist_region(rdreg);
>>>
>>> Consider moving the introduction of vgic_v3_free_redist_region() into
>>> a separate patch. On its own, that's a good readability improvement.
>>>
INIT_LIST_HEAD(>rd_regions);
} else {
dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 987e366c80008..f6a7eed1d6adb 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -251,45 +251,57 @@ 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 *rdreg = vgic_cpu->rdreg;
+
+ if (!rdreg)
+ return false;
+
+ if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) {
+ /* check whether there is no other contiguous rdist region */
+ struct list_head *rd_regions = >rd_regions;
+ struct vgic_redist_region *iter;
+
+ list_for_each_entry(iter, rd_regions, list) {
+ if (iter->base == rdreg->base + rdreg->count *
KVM_VGIC_V3_REDIST_SIZE &&
+ iter->free_index > 0) {
+ /* check the first rdist index of this region, if any */
+ if (vgic_cpu->index < iter->rdist_indices[0])
+ return false;
>>>
>>> rdist_indices[] contains the vcpu_id of the vcpu associated with a
>>> given RD in the
Re: [PATCH v4 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
On Thu, 01 Apr 2021 18:03:25 +0100,
Auger Eric wrote:
>
> Hi Marc,
>
> On 4/1/21 3:42 PM, Marc Zyngier wrote:
> > Hi Eric,
> >
> > On Thu, 01 Apr 2021 09:52:37 +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.
> >>
> >> The spec says "Indicates whether this Redistributor is the
> >> highest-numbered Redistributor in a series of contiguous
> >> Redistributor pages."
> >>
> >> The code is a bit convulated since there is no guarantee
> >
> > nit: convoluted
> >
> >> redistributors are added in a given reditributor region in
> >> ascending order. In that case the current implementation was
> >> wrong. Also redistributor regions can be contiguous
> >> and registered in non increasing base address order.
> >>
> >> So the index of redistributors are stored in an array within
> >> the redistributor region structure.
> >>
> >> With this new implementation we do not need to have a uaccess
> >> read accessor anymore.
> >>
> >> Signed-off-by: Eric Auger
> >
> > This patch also hurt my head, a lot more than the first one. See
> > below.
> >
> >> ---
> >> arch/arm64/kvm/vgic/vgic-init.c| 7 +--
> >> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 97 --
> >> arch/arm64/kvm/vgic/vgic.h | 1 +
> >> include/kvm/arm_vgic.h | 3 +
> >> 4 files changed, 73 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/vgic/vgic-init.c
> >> b/arch/arm64/kvm/vgic/vgic-init.c
> >> index cf6faa0aeddb2..61150c34c268c 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-init.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> >> @@ -190,6 +190,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> >>int i;
> >>
> >>vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
> >> + vgic_cpu->index = vcpu->vcpu_id;
> >
> > Is it so that vgic_cpu->index is always equal to vcpu_id? If so, why
> > do we need another field? We can always get to the vcpu using a
> > container_of().
> >
> >>
> >>INIT_LIST_HEAD(_cpu->ap_list_head);
> >>raw_spin_lock_init(_cpu->ap_list_lock);
> >> @@ -338,10 +339,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
> >>dist->vgic_dist_base = VGIC_ADDR_UNDEF;
> >>
> >>if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> >> - list_for_each_entry_safe(rdreg, next, >rd_regions, list) {
> >> - list_del(>list);
> >> - kfree(rdreg);
> >> - }
> >> + list_for_each_entry_safe(rdreg, next, >rd_regions, list)
> >> + vgic_v3_free_redist_region(rdreg);
> >
> > Consider moving the introduction of vgic_v3_free_redist_region() into
> > a separate patch. On its own, that's a good readability improvement.
> >
> >>INIT_LIST_HEAD(>rd_regions);
> >>} else {
> >>dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
> >> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> index 987e366c80008..f6a7eed1d6adb 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> >> @@ -251,45 +251,57 @@ 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 *rdreg = vgic_cpu->rdreg;
> >> +
> >> + if (!rdreg)
> >> + return false;
> >> +
> >> + if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) {
> >> + /* check whether there is no other contiguous rdist region */
> >> + struct list_head *rd_regions = >rd_regions;
> >> + struct vgic_redist_region *iter;
> >> +
> >> + list_for_each_entry(iter, rd_regions, list) {
> >> + if (iter->base == rdreg->base + rdreg->count *
> >> KVM_VGIC_V3_REDIST_SIZE &&
> >> + iter->free_index > 0) {
> >> + /* check the first rdist index of this region, if any */
> >> + if (vgic_cpu->index < iter->rdist_indices[0])
> >> + return false;
> >
> > rdist_indices[] contains the vcpu_id of the vcpu associated with a
> > given RD in the region. At this stage, you have established
Re: [PATCH v4 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
Hi Marc,
On 4/1/21 3:42 PM, Marc Zyngier wrote:
> Hi Eric,
>
> On Thu, 01 Apr 2021 09:52:37 +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.
>>
>> The spec says "Indicates whether this Redistributor is the
>> highest-numbered Redistributor in a series of contiguous
>> Redistributor pages."
>>
>> The code is a bit convulated since there is no guarantee
>
> nit: convoluted
>
>> redistributors are added in a given reditributor region in
>> ascending order. In that case the current implementation was
>> wrong. Also redistributor regions can be contiguous
>> and registered in non increasing base address order.
>>
>> So the index of redistributors are stored in an array within
>> the redistributor region structure.
>>
>> With this new implementation we do not need to have a uaccess
>> read accessor anymore.
>>
>> Signed-off-by: Eric Auger
>
> This patch also hurt my head, a lot more than the first one. See
> below.
>
>> ---
>> arch/arm64/kvm/vgic/vgic-init.c| 7 +--
>> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 97 --
>> arch/arm64/kvm/vgic/vgic.h | 1 +
>> include/kvm/arm_vgic.h | 3 +
>> 4 files changed, 73 insertions(+), 35 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-init.c
>> b/arch/arm64/kvm/vgic/vgic-init.c
>> index cf6faa0aeddb2..61150c34c268c 100644
>> --- a/arch/arm64/kvm/vgic/vgic-init.c
>> +++ b/arch/arm64/kvm/vgic/vgic-init.c
>> @@ -190,6 +190,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>> int i;
>>
>> vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
>> +vgic_cpu->index = vcpu->vcpu_id;
>
> Is it so that vgic_cpu->index is always equal to vcpu_id? If so, why
> do we need another field? We can always get to the vcpu using a
> container_of().
>
>>
>> INIT_LIST_HEAD(_cpu->ap_list_head);
>> raw_spin_lock_init(_cpu->ap_list_lock);
>> @@ -338,10 +339,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
>> dist->vgic_dist_base = VGIC_ADDR_UNDEF;
>>
>> if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
>> -list_for_each_entry_safe(rdreg, next, >rd_regions, list) {
>> -list_del(>list);
>> -kfree(rdreg);
>> -}
>> +list_for_each_entry_safe(rdreg, next, >rd_regions, list)
>> +vgic_v3_free_redist_region(rdreg);
>
> Consider moving the introduction of vgic_v3_free_redist_region() into
> a separate patch. On its own, that's a good readability improvement.
>
>> INIT_LIST_HEAD(>rd_regions);
>> } else {
>> dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> index 987e366c80008..f6a7eed1d6adb 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
>> @@ -251,45 +251,57 @@ 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 *rdreg = vgic_cpu->rdreg;
>> +
>> +if (!rdreg)
>> +return false;
>> +
>> +if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) {
>> +/* check whether there is no other contiguous rdist region */
>> +struct list_head *rd_regions = >rd_regions;
>> +struct vgic_redist_region *iter;
>> +
>> +list_for_each_entry(iter, rd_regions, list) {
>> +if (iter->base == rdreg->base + rdreg->count *
>> KVM_VGIC_V3_REDIST_SIZE &&
>> +iter->free_index > 0) {
>> +/* check the first rdist index of this region, if any */
>> +if (vgic_cpu->index < iter->rdist_indices[0])
>> +return false;
>
> rdist_indices[] contains the vcpu_id of the vcpu associated with a
> given RD in the region. At this stage, you have established that there
> is another region that is contiguous with the one associated with our
> vcpu. You also know that this adjacent region has a vcpu mapped in
> (free_index isn't 0). Isn't that enough to declare that our vcpu isn't
> last? I
Re: [PATCH v4 7/8] KVM: arm64: vgic-v3: Expose GICR_TYPER.Last for userspace
Hi Eric,
On Thu, 01 Apr 2021 09:52:37 +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.
>
> The spec says "Indicates whether this Redistributor is the
> highest-numbered Redistributor in a series of contiguous
> Redistributor pages."
>
> The code is a bit convulated since there is no guarantee
nit: convoluted
> redistributors are added in a given reditributor region in
> ascending order. In that case the current implementation was
> wrong. Also redistributor regions can be contiguous
> and registered in non increasing base address order.
>
> So the index of redistributors are stored in an array within
> the redistributor region structure.
>
> With this new implementation we do not need to have a uaccess
> read accessor anymore.
>
> Signed-off-by: Eric Auger
This patch also hurt my head, a lot more than the first one. See
below.
> ---
> arch/arm64/kvm/vgic/vgic-init.c| 7 +--
> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 97 --
> arch/arm64/kvm/vgic/vgic.h | 1 +
> include/kvm/arm_vgic.h | 3 +
> 4 files changed, 73 insertions(+), 35 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index cf6faa0aeddb2..61150c34c268c 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -190,6 +190,7 @@ int kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> int i;
>
> vgic_cpu->rd_iodev.base_addr = VGIC_ADDR_UNDEF;
> + vgic_cpu->index = vcpu->vcpu_id;
Is it so that vgic_cpu->index is always equal to vcpu_id? If so, why
do we need another field? We can always get to the vcpu using a
container_of().
>
> INIT_LIST_HEAD(_cpu->ap_list_head);
> raw_spin_lock_init(_cpu->ap_list_lock);
> @@ -338,10 +339,8 @@ static void kvm_vgic_dist_destroy(struct kvm *kvm)
> dist->vgic_dist_base = VGIC_ADDR_UNDEF;
>
> if (dist->vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3) {
> - list_for_each_entry_safe(rdreg, next, >rd_regions, list) {
> - list_del(>list);
> - kfree(rdreg);
> - }
> + list_for_each_entry_safe(rdreg, next, >rd_regions, list)
> + vgic_v3_free_redist_region(rdreg);
Consider moving the introduction of vgic_v3_free_redist_region() into
a separate patch. On its own, that's a good readability improvement.
> INIT_LIST_HEAD(>rd_regions);
> } else {
> dist->vgic_cpu_base = VGIC_ADDR_UNDEF;
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 987e366c80008..f6a7eed1d6adb 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -251,45 +251,57 @@ 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 *rdreg = vgic_cpu->rdreg;
> +
> + if (!rdreg)
> + return false;
> +
> + if (rdreg->count && vgic_cpu->rdreg_index == (rdreg->count - 1)) {
> + /* check whether there is no other contiguous rdist region */
> + struct list_head *rd_regions = >rd_regions;
> + struct vgic_redist_region *iter;
> +
> + list_for_each_entry(iter, rd_regions, list) {
> + if (iter->base == rdreg->base + rdreg->count *
> KVM_VGIC_V3_REDIST_SIZE &&
> + iter->free_index > 0) {
> + /* check the first rdist index of this region, if any */
> + if (vgic_cpu->index < iter->rdist_indices[0])
> + return false;
rdist_indices[] contains the vcpu_id of the vcpu associated with a
given RD in the region. At this stage, you have established that there
is another region that is contiguous with the one associated with our
vcpu. You also know that this adjacent region has a vcpu mapped in
(free_index isn't 0). Isn't that enough to declare that our vcpu isn't
last? I definitely don't understand what the index comparison does
here.
It also seem to me that some of the complexity could be eliminated if
the regions were kept
