| Message ID | 1492164934-988-22-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 14, 2017 at 3:45 PM, Eric Auger <eric.auger@redhat.com> wrote: > In its_sync_lpi_pending_table() we currently ignore the > target_vcpu of the LPIs. We sync the pending bit found in > the vcpu pending table even if the LPI is not targeting it. > > Also in vgic_its_cmd_handle_invall() we are supposed to > read the config table data for the LPIs associated to the > collection ID. At the moment we refresh all LPI config > information. > > This patch passes a vpcu to vgic_copy_lpi_list() so that > this latter returns a snapshot of the LPIs targeting this > CPU and only those. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> Tested-by: Prakash, Brahmajyosyula <Brahmajyosyula.Prakash@cavium.com>
On Fri, Apr 14, 2017 at 12:15:33PM +0200, Eric Auger wrote: > In its_sync_lpi_pending_table() we currently ignore the > target_vcpu of the LPIs. We sync the pending bit found in > the vcpu pending table even if the LPI is not targeting it. > > Also in vgic_its_cmd_handle_invall() we are supposed to > read the config table data for the LPIs associated to the > collection ID. At the moment we refresh all LPI config > information. > > This patch passes a vpcu to vgic_copy_lpi_list() so that > this latter returns a snapshot of the LPIs targeting this > CPU and only those. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > virt/kvm/arm/vgic/vgic-its.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > index 86dfc6c..be848be 100644 > --- a/virt/kvm/arm/vgic/vgic-its.c > +++ b/virt/kvm/arm/vgic/vgic-its.c > @@ -252,13 +252,13 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq, > } > > /* > - * Create a snapshot of the current LPI list, so that we can enumerate all > - * LPIs without holding any lock. > - * Returns the array length and puts the kmalloc'ed array into intid_ptr. > + * Create a snapshot of the current LPIs targeting @vcpu, so that we can > + * enumerate those LPIs without holding any lock. > + * Returns their number and puts the kmalloc'ed array into intid_ptr. > */ > -static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) > +static int vgic_copy_lpi_list(struct kvm_vcpu *vcpu, u32 **intid_ptr) > { > - struct vgic_dist *dist = &kvm->arch.vgic; > + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > struct vgic_irq *irq; > u32 *intids; > int irq_count = dist->lpi_list_count, i = 0; > @@ -277,14 +277,14 @@ static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) > spin_lock(&dist->lpi_list_lock); > list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { > /* We don't need to "get" the IRQ, as we hold the list lock. */ > - intids[i] = irq->intid; > - if (++i == irq_count) > - break; > + if (irq->target_vcpu != vcpu) > + continue; > + intids[i++] = irq->intid; were we checking the ++i == irq_count condition for no good reason before since we can just drop it now? > } > spin_unlock(&dist->lpi_list_lock); > > *intid_ptr = intids; > - return irq_count; > + return i; > } > > /* > @@ -333,7 +333,7 @@ static u32 max_lpis_propbaser(u64 propbaser) > } > > /* > - * Scan the whole LPI pending table and sync the pending bit in there > + * Sync the pending table pending bit of LPIs targeting @vcpu > * with our own data structures. This relies on the LPI being > * mapped before. > */ > @@ -346,7 +346,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) > u32 *intids; > int nr_irqs, i; > > - nr_irqs = vgic_copy_lpi_list(vcpu->kvm, &intids); > + nr_irqs = vgic_copy_lpi_list(vcpu, &intids); > if (nr_irqs < 0) > return nr_irqs; > > @@ -1027,7 +1027,7 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its, > > vcpu = kvm_get_vcpu(kvm, collection->target_addr); > > - irq_count = vgic_copy_lpi_list(kvm, &intids); > + irq_count = vgic_copy_lpi_list(vcpu, &intids); > if (irq_count < 0) > return irq_count; > > -- > 2.5.5 > Assuming that it's ok to remove the irq_count check above, the rest of this patch looks good to me. Thanks, -Christoffer
Hi, On 30/04/2017 23:10, Christoffer Dall wrote: > On Fri, Apr 14, 2017 at 12:15:33PM +0200, Eric Auger wrote: >> In its_sync_lpi_pending_table() we currently ignore the >> target_vcpu of the LPIs. We sync the pending bit found in >> the vcpu pending table even if the LPI is not targeting it. >> >> Also in vgic_its_cmd_handle_invall() we are supposed to >> read the config table data for the LPIs associated to the >> collection ID. At the moment we refresh all LPI config >> information. >> >> This patch passes a vpcu to vgic_copy_lpi_list() so that >> this latter returns a snapshot of the LPIs targeting this >> CPU and only those. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> virt/kvm/arm/vgic/vgic-its.c | 24 ++++++++++++------------ >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c >> index 86dfc6c..be848be 100644 >> --- a/virt/kvm/arm/vgic/vgic-its.c >> +++ b/virt/kvm/arm/vgic/vgic-its.c >> @@ -252,13 +252,13 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq, >> } >> >> /* >> - * Create a snapshot of the current LPI list, so that we can enumerate all >> - * LPIs without holding any lock. >> - * Returns the array length and puts the kmalloc'ed array into intid_ptr. >> + * Create a snapshot of the current LPIs targeting @vcpu, so that we can >> + * enumerate those LPIs without holding any lock. >> + * Returns their number and puts the kmalloc'ed array into intid_ptr. >> */ >> -static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) >> +static int vgic_copy_lpi_list(struct kvm_vcpu *vcpu, u32 **intid_ptr) >> { >> - struct vgic_dist *dist = &kvm->arch.vgic; >> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; >> struct vgic_irq *irq; >> u32 *intids; >> int irq_count = dist->lpi_list_count, i = 0; >> @@ -277,14 +277,14 @@ static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) >> spin_lock(&dist->lpi_list_lock); >> list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >> /* We don't need to "get" the IRQ, as we hold the list lock. */ >> - intids[i] = irq->intid; >> - if (++i == irq_count) >> - break; >> + if (irq->target_vcpu != vcpu) >> + continue; >> + intids[i++] = irq->intid; > > were we checking the ++i == irq_count condition for no good reason > before since we can just drop it now? I didn't get why we had that check. Thanks Eric > >> } >> spin_unlock(&dist->lpi_list_lock); >> >> *intid_ptr = intids; >> - return irq_count; >> + return i; >> } >> >> /* >> @@ -333,7 +333,7 @@ static u32 max_lpis_propbaser(u64 propbaser) >> } >> >> /* >> - * Scan the whole LPI pending table and sync the pending bit in there >> + * Sync the pending table pending bit of LPIs targeting @vcpu >> * with our own data structures. This relies on the LPI being >> * mapped before. >> */ >> @@ -346,7 +346,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) >> u32 *intids; >> int nr_irqs, i; >> >> - nr_irqs = vgic_copy_lpi_list(vcpu->kvm, &intids); >> + nr_irqs = vgic_copy_lpi_list(vcpu, &intids); >> if (nr_irqs < 0) >> return nr_irqs; >> >> @@ -1027,7 +1027,7 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its, >> >> vcpu = kvm_get_vcpu(kvm, collection->target_addr); >> >> - irq_count = vgic_copy_lpi_list(kvm, &intids); >> + irq_count = vgic_copy_lpi_list(vcpu, &intids); >> if (irq_count < 0) >> return irq_count; >> >> -- >> 2.5.5 >> > > Assuming that it's ok to remove the irq_count check above, the rest of > this patch looks good to me. > > Thanks, > -Christoffer >
On Thu, May 04, 2017 at 12:20:13AM +0200, Auger Eric wrote: > Hi, > > On 30/04/2017 23:10, Christoffer Dall wrote: > > On Fri, Apr 14, 2017 at 12:15:33PM +0200, Eric Auger wrote: > >> In its_sync_lpi_pending_table() we currently ignore the > >> target_vcpu of the LPIs. We sync the pending bit found in > >> the vcpu pending table even if the LPI is not targeting it. > >> > >> Also in vgic_its_cmd_handle_invall() we are supposed to > >> read the config table data for the LPIs associated to the > >> collection ID. At the moment we refresh all LPI config > >> information. > >> > >> This patch passes a vpcu to vgic_copy_lpi_list() so that > >> this latter returns a snapshot of the LPIs targeting this > >> CPU and only those. > >> > >> Signed-off-by: Eric Auger <eric.auger@redhat.com> > >> --- > >> virt/kvm/arm/vgic/vgic-its.c | 24 ++++++++++++------------ > >> 1 file changed, 12 insertions(+), 12 deletions(-) > >> > >> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c > >> index 86dfc6c..be848be 100644 > >> --- a/virt/kvm/arm/vgic/vgic-its.c > >> +++ b/virt/kvm/arm/vgic/vgic-its.c > >> @@ -252,13 +252,13 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq, > >> } > >> > >> /* > >> - * Create a snapshot of the current LPI list, so that we can enumerate all > >> - * LPIs without holding any lock. > >> - * Returns the array length and puts the kmalloc'ed array into intid_ptr. > >> + * Create a snapshot of the current LPIs targeting @vcpu, so that we can > >> + * enumerate those LPIs without holding any lock. > >> + * Returns their number and puts the kmalloc'ed array into intid_ptr. > >> */ > >> -static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) > >> +static int vgic_copy_lpi_list(struct kvm_vcpu *vcpu, u32 **intid_ptr) > >> { > >> - struct vgic_dist *dist = &kvm->arch.vgic; > >> + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; > >> struct vgic_irq *irq; > >> u32 *intids; > >> int irq_count = dist->lpi_list_count, i = 0; > >> @@ -277,14 +277,14 @@ static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) > >> spin_lock(&dist->lpi_list_lock); > >> list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { > >> /* We don't need to "get" the IRQ, as we hold the list lock. */ > >> - intids[i] = irq->intid; > >> - if (++i == irq_count) > >> - break; > >> + if (irq->target_vcpu != vcpu) > >> + continue; > >> + intids[i++] = irq->intid; > > > > were we checking the ++i == irq_count condition for no good reason > > before since we can just drop it now? > I didn't get why we had that check. > ok, Andre is cc'ed so unless he complains let's just get rid of it. Thanks, -Christoffer
diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c index 86dfc6c..be848be 100644 --- a/virt/kvm/arm/vgic/vgic-its.c +++ b/virt/kvm/arm/vgic/vgic-its.c @@ -252,13 +252,13 @@ static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq, } /* - * Create a snapshot of the current LPI list, so that we can enumerate all - * LPIs without holding any lock. - * Returns the array length and puts the kmalloc'ed array into intid_ptr. + * Create a snapshot of the current LPIs targeting @vcpu, so that we can + * enumerate those LPIs without holding any lock. + * Returns their number and puts the kmalloc'ed array into intid_ptr. */ -static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) +static int vgic_copy_lpi_list(struct kvm_vcpu *vcpu, u32 **intid_ptr) { - struct vgic_dist *dist = &kvm->arch.vgic; + struct vgic_dist *dist = &vcpu->kvm->arch.vgic; struct vgic_irq *irq; u32 *intids; int irq_count = dist->lpi_list_count, i = 0; @@ -277,14 +277,14 @@ static int vgic_copy_lpi_list(struct kvm *kvm, u32 **intid_ptr) spin_lock(&dist->lpi_list_lock); list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { /* We don't need to "get" the IRQ, as we hold the list lock. */ - intids[i] = irq->intid; - if (++i == irq_count) - break; + if (irq->target_vcpu != vcpu) + continue; + intids[i++] = irq->intid; } spin_unlock(&dist->lpi_list_lock); *intid_ptr = intids; - return irq_count; + return i; } /* @@ -333,7 +333,7 @@ static u32 max_lpis_propbaser(u64 propbaser) } /* - * Scan the whole LPI pending table and sync the pending bit in there + * Sync the pending table pending bit of LPIs targeting @vcpu * with our own data structures. This relies on the LPI being * mapped before. */ @@ -346,7 +346,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu) u32 *intids; int nr_irqs, i; - nr_irqs = vgic_copy_lpi_list(vcpu->kvm, &intids); + nr_irqs = vgic_copy_lpi_list(vcpu, &intids); if (nr_irqs < 0) return nr_irqs; @@ -1027,7 +1027,7 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its, vcpu = kvm_get_vcpu(kvm, collection->target_addr); - irq_count = vgic_copy_lpi_list(kvm, &intids); + irq_count = vgic_copy_lpi_list(vcpu, &intids); if (irq_count < 0) return irq_count;
In its_sync_lpi_pending_table() we currently ignore the target_vcpu of the LPIs. We sync the pending bit found in the vcpu pending table even if the LPI is not targeting it. Also in vgic_its_cmd_handle_invall() we are supposed to read the config table data for the LPIs associated to the collection ID. At the moment we refresh all LPI config information. This patch passes a vpcu to vgic_copy_lpi_list() so that this latter returns a snapshot of the LPIs targeting this CPU and only those. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- virt/kvm/arm/vgic/vgic-its.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)