| Message ID | 20200304203330.4967-17-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/gic-v4: GICv4.1 architecture support | expand |
On 2020/3/5 4:33, Marc Zyngier wrote: > Now that we have HW-accelerated SGIs being delivered to VPEs, it > becomes required to map the VPEs on all ITSs instead of relying > on the lazy approach that we would use when using the ITS-list > mechanism. > > Signed-off-by: Marc Zyngier <maz@kernel.org> Reviewed-by: Zenghui Yu <yuzenghui@huawei.com> Thanks
Hi Marc, On 2020/3/5 4:33, Marc Zyngier wrote: > Now that we have HW-accelerated SGIs being delivered to VPEs, it > becomes required to map the VPEs on all ITSs instead of relying > on the lazy approach that we would use when using the ITS-list > mechanism. > > Signed-off-by: Marc Zyngier <maz@kernel.org> Before GICv4.1, we use vlpi_count to evaluate whether the vPE has been mapped on the specified ITS, and use this refcount to only issue VMOVP to those involved ITSes. It's broken after this patch. We may need to re-evaluate "whether the vPE is mapped" now that we're at GICv4.1, otherwise *no* VMOVP will be issued on the v4.1 capable machine (I mean those without single VMOVP support). What I'm saying is something like below (only an idea, it even can't compile), any thoughts? diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index 2e12bc52e3a2..3450b5e847ca 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -198,7 +198,8 @@ static u16 get_its_list(struct its_vm *vm) if (!is_v4(its)) continue; - if (vm->vlpi_count[its->list_nr]) + if (vm->vlpi_count[its->list_nr] || + gic_requires_eager_mapping()) __set_bit(its->list_nr, &its_list); } @@ -1295,7 +1296,8 @@ static void its_send_vmovp(struct its_vpe *vpe) if (!is_v4(its)) continue; - if (!vpe->its_vm->vlpi_count[its->list_nr]) + if (!vpe->its_vm->vlpi_count[its->list_nr] && + !gic_requires_eager_mapping()) continue; desc.its_vmovp_cmd.col = &its->collections[col_id]; Thanks, Zenghui > --- > drivers/irqchip/irq-gic-v3-its.c | 39 +++++++++++++++++++++++++------- > 1 file changed, 31 insertions(+), 8 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index b65fba67bd85..6277b3e3731f 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -1586,12 +1586,31 @@ static int its_irq_set_irqchip_state(struct irq_data *d, > return 0; > } > > +/* > + * Two favourable cases: > + * > + * (a) Either we have a GICv4.1, and all vPEs have to be mapped at all times > + * for vSGI delivery > + * > + * (b) Or the ITSs do not use a list map, meaning that VMOVP is cheap enough > + * and we're better off mapping all VPEs always > + * > + * If neither (a) nor (b) is true, then we map vPEs on demand. > + * > + */ > +static bool gic_requires_eager_mapping(void) > +{ > + if (!its_list_map || gic_rdists->has_rvpeid) > + return true; > + > + return false; > +} > + > static void its_map_vm(struct its_node *its, struct its_vm *vm) > { > unsigned long flags; > > - /* Not using the ITS list? Everything is always mapped. */ > - if (!its_list_map) > + if (gic_requires_eager_mapping()) > return; > > raw_spin_lock_irqsave(&vmovp_lock, flags); > @@ -1625,7 +1644,7 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm) > unsigned long flags; > > /* Not using the ITS list? Everything is always mapped. */ > - if (!its_list_map) > + if (gic_requires_eager_mapping()) > return; > > raw_spin_lock_irqsave(&vmovp_lock, flags); > @@ -4260,8 +4279,12 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain, > struct its_vpe *vpe = irq_data_get_irq_chip_data(d); > struct its_node *its; > > - /* If we use the list map, we issue VMAPP on demand... */ > - if (its_list_map) > + /* > + * If we use the list map, we issue VMAPP on demand... Unless > + * we're on a GICv4.1 and we eagerly map the VPE on all ITSs > + * so that VSGIs can work. > + */ > + if (!gic_requires_eager_mapping()) > return 0; > > /* Map the VPE to the first possible CPU */ > @@ -4287,10 +4310,10 @@ static void its_vpe_irq_domain_deactivate(struct irq_domain *domain, > struct its_node *its; > > /* > - * If we use the list map, we unmap the VPE once no VLPIs are > - * associated with the VM. > + * If we use the list map on GICv4.0, we unmap the VPE once no > + * VLPIs are associated with the VM. > */ > - if (its_list_map) > + if (!gic_requires_eager_mapping()) > return; > > list_for_each_entry(its, &its_nodes, entry) { >
On 2020-03-17 02:49, Zenghui Yu wrote: > Hi Marc, > > On 2020/3/5 4:33, Marc Zyngier wrote: >> Now that we have HW-accelerated SGIs being delivered to VPEs, it >> becomes required to map the VPEs on all ITSs instead of relying >> on the lazy approach that we would use when using the ITS-list >> mechanism. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> > > Before GICv4.1, we use vlpi_count to evaluate whether the vPE has been > mapped on the specified ITS, and use this refcount to only issue VMOVP > to those involved ITSes. It's broken after this patch. > > We may need to re-evaluate "whether the vPE is mapped" now that we're > at > GICv4.1, otherwise *no* VMOVP will be issued on the v4.1 capable > machine > (I mean those without single VMOVP support). > > What I'm saying is something like below (only an idea, it even can't > compile), any thoughts? > > > diff --git a/drivers/irqchip/irq-gic-v3-its.c > b/drivers/irqchip/irq-gic-v3-its.c > index 2e12bc52e3a2..3450b5e847ca 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -198,7 +198,8 @@ static u16 get_its_list(struct its_vm *vm) > if (!is_v4(its)) > continue; > > - if (vm->vlpi_count[its->list_nr]) > + if (vm->vlpi_count[its->list_nr] || > + gic_requires_eager_mapping()) > __set_bit(its->list_nr, &its_list); > } > > @@ -1295,7 +1296,8 @@ static void its_send_vmovp(struct its_vpe *vpe) > if (!is_v4(its)) > continue; > > - if (!vpe->its_vm->vlpi_count[its->list_nr]) > + if (!vpe->its_vm->vlpi_count[its->list_nr] && > + !gic_requires_eager_mapping()) > continue; > > desc.its_vmovp_cmd.col = &its->collections[col_id]; It took me a while to wrap my head around that one, but you're as usual spot on. The use of gic_requires_eager_mapping() is a bit confusing here, as it isn't so much that the VPE is eagerly mapped, but the predicate on which we evaluate the need for a VMOVP. How about this: diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index cd6451e190c9..348f7a909a69 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -189,6 +189,15 @@ static DEFINE_IDA(its_vpeid_ida); #define gic_data_rdist_rd_base() (gic_data_rdist()->rd_base) #define gic_data_rdist_vlpi_base() (gic_data_rdist_rd_base() + SZ_128K) +/* + * Skip ITSs that have no vLPIs mapped, unless we're on GICv4.1, as we + * always have vSGIs mapped. + */ +static bool require_its_list_vmovp(struct its_vm *vm, struct its_node *its) +{ + return (gic_rdists->has_rvpeid || vm->vlpi_count[its->list_nr]); +} + static u16 get_its_list(struct its_vm *vm) { struct its_node *its; @@ -198,7 +207,7 @@ static u16 get_its_list(struct its_vm *vm) if (!is_v4(its)) continue; - if (vm->vlpi_count[its->list_nr]) + if (require_its_list_vmovp(vm, its)) __set_bit(its->list_nr, &its_list); } @@ -1295,7 +1304,7 @@ static void its_send_vmovp(struct its_vpe *vpe) if (!is_v4(its)) continue; - if (!vpe->its_vm->vlpi_count[its->list_nr]) + if (!require_its_list_vmovp(vpe->its_vm, its)) continue; desc.its_vmovp_cmd.col = &its->collections[col_id]; Thanks, M.
Hi Marc, On 2020/3/19 18:55, Marc Zyngier wrote: > On 2020-03-17 02:49, Zenghui Yu wrote: >> Hi Marc, >> >> On 2020/3/5 4:33, Marc Zyngier wrote: >>> Now that we have HW-accelerated SGIs being delivered to VPEs, it >>> becomes required to map the VPEs on all ITSs instead of relying >>> on the lazy approach that we would use when using the ITS-list >>> mechanism. >>> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >> >> Before GICv4.1, we use vlpi_count to evaluate whether the vPE has been >> mapped on the specified ITS, and use this refcount to only issue VMOVP >> to those involved ITSes. It's broken after this patch. >> >> We may need to re-evaluate "whether the vPE is mapped" now that we're at >> GICv4.1, otherwise *no* VMOVP will be issued on the v4.1 capable machine >> (I mean those without single VMOVP support). >> >> What I'm saying is something like below (only an idea, it even can't >> compile), any thoughts? >> >> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c >> b/drivers/irqchip/irq-gic-v3-its.c >> index 2e12bc52e3a2..3450b5e847ca 100644 >> --- a/drivers/irqchip/irq-gic-v3-its.c >> +++ b/drivers/irqchip/irq-gic-v3-its.c >> @@ -198,7 +198,8 @@ static u16 get_its_list(struct its_vm *vm) >> if (!is_v4(its)) >> continue; >> >> - if (vm->vlpi_count[its->list_nr]) >> + if (vm->vlpi_count[its->list_nr] || >> + gic_requires_eager_mapping()) >> __set_bit(its->list_nr, &its_list); >> } >> >> @@ -1295,7 +1296,8 @@ static void its_send_vmovp(struct its_vpe *vpe) >> if (!is_v4(its)) >> continue; >> >> - if (!vpe->its_vm->vlpi_count[its->list_nr]) >> + if (!vpe->its_vm->vlpi_count[its->list_nr] && >> + !gic_requires_eager_mapping()) >> continue; >> >> desc.its_vmovp_cmd.col = &its->collections[col_id]; > > It took me a while to wrap my head around that one, but you're as usual > spot on. > > The use of gic_requires_eager_mapping() is a bit confusing here, as it > isn't > so much that the VPE is eagerly mapped, but the predicate on which we > evaluate > the need for a VMOVP. How about this: > > diff --git a/drivers/irqchip/irq-gic-v3-its.c > b/drivers/irqchip/irq-gic-v3-its.c > index cd6451e190c9..348f7a909a69 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -189,6 +189,15 @@ static DEFINE_IDA(its_vpeid_ida); > #define gic_data_rdist_rd_base() (gic_data_rdist()->rd_base) > #define gic_data_rdist_vlpi_base() (gic_data_rdist_rd_base() + > SZ_128K) > > +/* > + * Skip ITSs that have no vLPIs mapped, unless we're on GICv4.1, as we > + * always have vSGIs mapped. > + */ > +static bool require_its_list_vmovp(struct its_vm *vm, struct its_node > *its) > +{ > + return (gic_rdists->has_rvpeid || vm->vlpi_count[its->list_nr]); > +} > + > static u16 get_its_list(struct its_vm *vm) > { > struct its_node *its; > @@ -198,7 +207,7 @@ static u16 get_its_list(struct its_vm *vm) > if (!is_v4(its)) > continue; > > - if (vm->vlpi_count[its->list_nr]) > + if (require_its_list_vmovp(vm, its)) > __set_bit(its->list_nr, &its_list); > } > > @@ -1295,7 +1304,7 @@ static void its_send_vmovp(struct its_vpe *vpe) > if (!is_v4(its)) > continue; > > - if (!vpe->its_vm->vlpi_count[its->list_nr]) > + if (!require_its_list_vmovp(vpe->its_vm, its)) > continue; > > desc.its_vmovp_cmd.col = &its->collections[col_id]; Indeed this looks much clearer. We're actually evaluating the need for issuing VMOVP to a specified ITS, on system using its_list_map feature (though we evaluate it by checking whether the vPE is mapped on this ITS). Thanks, Zenghui
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c index b65fba67bd85..6277b3e3731f 100644 --- a/drivers/irqchip/irq-gic-v3-its.c +++ b/drivers/irqchip/irq-gic-v3-its.c @@ -1586,12 +1586,31 @@ static int its_irq_set_irqchip_state(struct irq_data *d, return 0; } +/* + * Two favourable cases: + * + * (a) Either we have a GICv4.1, and all vPEs have to be mapped at all times + * for vSGI delivery + * + * (b) Or the ITSs do not use a list map, meaning that VMOVP is cheap enough + * and we're better off mapping all VPEs always + * + * If neither (a) nor (b) is true, then we map vPEs on demand. + * + */ +static bool gic_requires_eager_mapping(void) +{ + if (!its_list_map || gic_rdists->has_rvpeid) + return true; + + return false; +} + static void its_map_vm(struct its_node *its, struct its_vm *vm) { unsigned long flags; - /* Not using the ITS list? Everything is always mapped. */ - if (!its_list_map) + if (gic_requires_eager_mapping()) return; raw_spin_lock_irqsave(&vmovp_lock, flags); @@ -1625,7 +1644,7 @@ static void its_unmap_vm(struct its_node *its, struct its_vm *vm) unsigned long flags; /* Not using the ITS list? Everything is always mapped. */ - if (!its_list_map) + if (gic_requires_eager_mapping()) return; raw_spin_lock_irqsave(&vmovp_lock, flags); @@ -4260,8 +4279,12 @@ static int its_vpe_irq_domain_activate(struct irq_domain *domain, struct its_vpe *vpe = irq_data_get_irq_chip_data(d); struct its_node *its; - /* If we use the list map, we issue VMAPP on demand... */ - if (its_list_map) + /* + * If we use the list map, we issue VMAPP on demand... Unless + * we're on a GICv4.1 and we eagerly map the VPE on all ITSs + * so that VSGIs can work. + */ + if (!gic_requires_eager_mapping()) return 0; /* Map the VPE to the first possible CPU */ @@ -4287,10 +4310,10 @@ static void its_vpe_irq_domain_deactivate(struct irq_domain *domain, struct its_node *its; /* - * If we use the list map, we unmap the VPE once no VLPIs are - * associated with the VM. + * If we use the list map on GICv4.0, we unmap the VPE once no + * VLPIs are associated with the VM. */ - if (its_list_map) + if (!gic_requires_eager_mapping()) return; list_for_each_entry(its, &its_nodes, entry) {
Now that we have HW-accelerated SGIs being delivered to VPEs, it becomes required to map the VPEs on all ITSs instead of relying on the lazy approach that we would use when using the ITS-list mechanism. Signed-off-by: Marc Zyngier <maz@kernel.org> --- drivers/irqchip/irq-gic-v3-its.c | 39 +++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-)