Re: [PATCH 31/35] irqchip/gic-v4.1: Eagerly vmap vPEs
On 2019/9/24 2:26, 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
---
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 4aae9582182b..a1e8c4c2598a 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1417,12 +1417,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 VLPIs on demand.
^
vPEs
+ *
+ */
+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);
@@ -1456,7 +1475,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);
@@ -3957,8 +3976,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 */
@@ -3984,10 +4007,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) {
Thanks,
zenghui
Re: [PATCH 24/35] irqchip/gic-v4.1: Add initial SGI configuration
On 2019/9/28 10:20, Zenghui Yu wrote:
Hi Marc,
On 2019/9/24 2:25, Marc Zyngier wrote:
The GICv4.1 ITS has yet another new command (VSGI) which allows
a VPE-targeted SGI to be configured (or have its pending state
cleared). Add support for this command and plumb it into the
activate irqdomain callback so that it is ready to be used.
Signed-off-by: Marc Zyngier
---
drivers/irqchip/irq-gic-v3-its.c | 88 ++
include/linux/irqchip/arm-gic-v3.h | 3 +-
2 files changed, 90 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 69c26be709be..5234b9eef8ad 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
[...]
@@ -3574,6 +3628,38 @@ static struct irq_chip its_vpe_4_1_irq_chip = {
.irq_set_vcpu_affinity = its_vpe_4_1_set_vcpu_affinity,
};
+static struct its_node *find_4_1_its(void)
+{
+ static struct its_node *its = NULL;
+
+ if (!its) {
+ list_for_each_entry(its, &its_nodes, entry) {
+ if (is_v4_1(its))
+ return its;
+ }
+
+ /* Oops? */
+ its = NULL;
+ }
+
+ return its;
+}
+
+static void its_configure_sgi(struct irq_data *d, bool clear)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+ struct its_cmd_desc desc;
+
+ desc.its_vsgi_cmd.vpe = vpe;
+ desc.its_vsgi_cmd.sgi = d->hwirq;
+ desc.its_vsgi_cmd.priority = vpe->sgi_config[d->hwirq].priority;
+ desc.its_vsgi_cmd.enable = vpe->sgi_config[d->hwirq].enabled;
+ desc.its_vsgi_cmd.group = vpe->sgi_config[d->hwirq].group;
+ desc.its_vsgi_cmd.clear = clear;
+
+ its_send_single_vcommand(find_4_1_its(), its_build_vsgi_cmd, &desc);
I can't follow the logic in find_4_1_its(). We simply use the first ITS
with GICv4.1 support, but what if the vPE is not mapped on this ITS?
We will fail the valid_vpe() check when building this command and will
have no effect on HW in the end?
I guess I find the answer in patch#31 ("Eagerly vmap vPEs").
I missed the important point in GICv4.1 that we have to map vPEs at all
times for VSGI delivery, and we currently choose to map vPEs on all ITSs
when requesting per vPE irq (instead of mapping them on demand, before
mapping VLPI, which could happen at a pretty late stage).
So it's OK to use the first GICv4.1 ITS when configuring VSGI for this
specified vPE, given that it is already mapped on all ITSs.
Thanks,
zenghui
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 24/35] irqchip/gic-v4.1: Add initial SGI configuration
Hi Marc,
On 2019/9/24 2:25, Marc Zyngier wrote:
The GICv4.1 ITS has yet another new command (VSGI) which allows
a VPE-targeted SGI to be configured (or have its pending state
cleared). Add support for this command and plumb it into the
activate irqdomain callback so that it is ready to be used.
Signed-off-by: Marc Zyngier
---
drivers/irqchip/irq-gic-v3-its.c | 88 ++
include/linux/irqchip/arm-gic-v3.h | 3 +-
2 files changed, 90 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 69c26be709be..5234b9eef8ad 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
[...]
@@ -3574,6 +3628,38 @@ static struct irq_chip its_vpe_4_1_irq_chip = {
.irq_set_vcpu_affinity = its_vpe_4_1_set_vcpu_affinity,
};
+static struct its_node *find_4_1_its(void)
+{
+ static struct its_node *its = NULL;
+
+ if (!its) {
+ list_for_each_entry(its, &its_nodes, entry) {
+ if (is_v4_1(its))
+ return its;
+ }
+
+ /* Oops? */
+ its = NULL;
+ }
+
+ return its;
+}
+
+static void its_configure_sgi(struct irq_data *d, bool clear)
+{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+ struct its_cmd_desc desc;
+
+ desc.its_vsgi_cmd.vpe = vpe;
+ desc.its_vsgi_cmd.sgi = d->hwirq;
+ desc.its_vsgi_cmd.priority = vpe->sgi_config[d->hwirq].priority;
+ desc.its_vsgi_cmd.enable = vpe->sgi_config[d->hwirq].enabled;
+ desc.its_vsgi_cmd.group = vpe->sgi_config[d->hwirq].group;
+ desc.its_vsgi_cmd.clear = clear;
+
+ its_send_single_vcommand(find_4_1_its(), its_build_vsgi_cmd, &desc);
I can't follow the logic in find_4_1_its(). We simply use the first ITS
with GICv4.1 support, but what if the vPE is not mapped on this ITS?
We will fail the valid_vpe() check when building this command and will
have no effect on HW in the end?
Thanks,
zenghui
+}
+
static int its_sgi_set_affinity(struct irq_data *d,
const struct cpumask *mask_val,
bool force)
@@ -3619,6 +3705,8 @@ static void its_sgi_irq_domain_free(struct irq_domain
*domain,
static int its_sgi_irq_domain_activate(struct irq_domain *domain,
struct irq_data *d, bool reserve)
{
+ /* Write out the initial SGI configuration */
+ its_configure_sgi(d, false);
return 0;
}
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 5f3278cbf247..c73176d3ab2b 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -497,8 +497,9 @@
#define GITS_CMD_VMAPTI GITS_CMD_GICv4(GITS_CMD_MAPTI)
#define GITS_CMD_VMOVIGITS_CMD_GICv4(GITS_CMD_MOVI)
#define GITS_CMD_VSYNCGITS_CMD_GICv4(GITS_CMD_SYNC)
-/* VMOVP and INVDB are the odd ones, as they dont have a physical counterpart
*/
+/* VMOVP, VSGI and INVDB are the odd ones, as they dont have a physical
counterpart */
#define GITS_CMD_VMOVPGITS_CMD_GICv4(2)
+#define GITS_CMD_VSGI GITS_CMD_GICv4(3)
#define GITS_CMD_INVDBGITS_CMD_GICv4(0xe)
/*
Re: [PATCH 20/35] irqchip/gic-v4.1: Allow direct invalidation of VLPIs
On 2019/9/24 2:25, Marc Zyngier wrote:
Just like for INVALL, GICv4.1 has grown a VPE-aware INVLPI register.
Let's plumb it in and make use of the DirectLPI code in that case.
Signed-off-by: Marc Zyngier
---
drivers/irqchip/irq-gic-v3-its.c | 19 +--
include/linux/irqchip/arm-gic-v3.h | 1 +
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index b791c9beddf2..34595a7fcccb 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1200,13 +1200,27 @@ static void wait_for_syncr(void __iomem *rdbase)
static void direct_lpi_inv(struct irq_data *d)
{
+ struct its_vlpi_map *map = get_vlpi_map(d);
struct its_collection *col;
void __iomem *rdbase;
+ u64 val;
+
+ if (map) {
+ struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+
+ WARN_ON(!is_v4_1(its_dev->its));
+
+ val = GICR_INVLPIR_V;
+ val |= FIELD_PREP(GICR_INVLPIR_VPEID, map->vpe->vpe_id);
+ val |= FIELD_PREP(GICR_INVLPIR_INTID, map->vintid);
+ } else {
+ val = d->hwirq;
+ }
/* Target the redistributor this LPI is currently routed to */
col = irq_to_col(d);
I think irq_to_col() may not work when GICv4.1 VLPIs are involved in.
irq_to_col() gives us col_map[event] as the target redistributor,
but the correct one for VLPIs should be vlpi_maps[event]->vpe->col_idx.
These two are not always pointing to the same physical RD.
For example, if guest issues a MOVI against a VLPI, we will update the
corresponding vlpi_map->vpe and issue a VMOVI on ITS... but leave the
col_map[event] unchanged.
col_map[event] usually describes the physical LPI's CPU affinity, but
when this physical LPI serves as something which the VLPI is backed by,
we take really little care of it. Did I miss something here?
Thanks,
zenghui
rdbase = per_cpu_ptr(gic_rdists->rdist, col->col_id)->rd_base;
- gic_write_lpir(d->hwirq, rdbase + GICR_INVLPIR);
+ gic_write_lpir(val, rdbase + GICR_INVLPIR);
wait_for_syncr(rdbase);
}
@@ -1216,7 +1230,8 @@ static void lpi_update_config(struct irq_data *d, u8 clr,
u8 set)
struct its_device *its_dev = irq_data_get_irq_chip_data(d);
lpi_write_config(d, clr, set);
- if (gic_rdists->has_direct_lpi && !irqd_is_forwarded_to_vcpu(d))
+ if (gic_rdists->has_direct_lpi &&
+ (is_v4_1(its_dev->its) || !irqd_is_forwarded_to_vcpu(d)))
direct_lpi_inv(d);
else
its_send_inv(its_dev, its_get_event_id(d));
diff --git a/include/linux/irqchip/arm-gic-v3.h
b/include/linux/irqchip/arm-gic-v3.h
index b69f60792554..5f3278cbf247 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -247,6 +247,7 @@
#define GICR_TYPER_COMMON_LPI_AFF GENMASK_ULL(25, 24)
#define GICR_TYPER_AFFINITY GENMASK_ULL(63, 32)
+#define GICR_INVLPIR_INTID GENMASK_ULL(31, 0)
#define GICR_INVLPIR_VPEIDGENMASK_ULL(47, 32)
#define GICR_INVLPIR_VGENMASK_ULL(63, 63)
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 1/6] arm: gic: check_acked: add test description
On Fri, Sep 27, 2019 at 01:40:16PM +0100, Andre Przywara wrote:
> On Fri, 27 Sep 2019 14:18:45 +0200
> Andrew Jones wrote:
>
> Hi,
>
> > On Fri, Sep 27, 2019 at 11:42:22AM +0100, Andre Przywara wrote:
> > > At the moment the check_acked() IRQ helper function just prints a
> > > generic "Completed" or "Timed out" message, without given a more
> > > detailed test description.
> > >
> > > To be able to tell the different IRQ tests apart, and also to allow
> > > re-using it more easily, add a "description" parameter string,
> > > which is prefixing the output line. This gives more information on what
> > > exactly was tested.
> > >
> > > This also splits the variable output part of the line (duration of IRQ
> > > delivery) into a separate INFO: line, to not confuse testing frameworks.
> > >
> > > Signed-off-by: Andre Przywara
> > > ---
> > > arm/gic.c | 17 ++---
> > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arm/gic.c b/arm/gic.c
> > > index ed5642e..6fd5e5e 100644
> > > --- a/arm/gic.c
> > > +++ b/arm/gic.c
> > > @@ -60,7 +60,7 @@ static void stats_reset(void)
> > > smp_wmb();
> > > }
> > >
> > > -static void check_acked(cpumask_t *mask)
> > > +static void check_acked(const char *testname, cpumask_t *mask)
> > > {
> > > int missing = 0, extra = 0, unexpected = 0;
> > > int nr_pass, cpu, i;
> > > @@ -88,7 +88,9 @@ static void check_acked(cpumask_t *mask)
> > > }
> > > }
> > > if (nr_pass == nr_cpus) {
> > > - report("Completed in %d ms", !bad, ++i * 100);
> > > + report("%s", !bad, testname);
> > > + if (i)
> > > + report_info("took more than %d ms", i * 100);
> >
> > Any reason for dropping the '++'? Without it we don't account for the last
> > 100 ms.
>
> Actually we expect the interrupt to either fire immediately, or to not fire
> at all (timeout). So the previous message of "Completed in 100 ms" was
> somewhat misleading, because this was just due to the mdelay(100) above, and
> the IRQ was most probably delivered before this delay loop even started.
>
> I had "took less than ++i *100 ms" before (and can revert to that if you
> like), but then figured that filtering for the most common case (immediate
> delivery) is more useful.
OK, with that argument we can leave the '++' off.
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 1/6] arm: gic: check_acked: add test description
On Fri, 27 Sep 2019 14:18:45 +0200
Andrew Jones wrote:
Hi,
> On Fri, Sep 27, 2019 at 11:42:22AM +0100, Andre Przywara wrote:
> > At the moment the check_acked() IRQ helper function just prints a
> > generic "Completed" or "Timed out" message, without given a more
> > detailed test description.
> >
> > To be able to tell the different IRQ tests apart, and also to allow
> > re-using it more easily, add a "description" parameter string,
> > which is prefixing the output line. This gives more information on what
> > exactly was tested.
> >
> > This also splits the variable output part of the line (duration of IRQ
> > delivery) into a separate INFO: line, to not confuse testing frameworks.
> >
> > Signed-off-by: Andre Przywara
> > ---
> > arm/gic.c | 17 ++---
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/arm/gic.c b/arm/gic.c
> > index ed5642e..6fd5e5e 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -60,7 +60,7 @@ static void stats_reset(void)
> > smp_wmb();
> > }
> >
> > -static void check_acked(cpumask_t *mask)
> > +static void check_acked(const char *testname, cpumask_t *mask)
> > {
> > int missing = 0, extra = 0, unexpected = 0;
> > int nr_pass, cpu, i;
> > @@ -88,7 +88,9 @@ static void check_acked(cpumask_t *mask)
> > }
> > }
> > if (nr_pass == nr_cpus) {
> > - report("Completed in %d ms", !bad, ++i * 100);
> > + report("%s", !bad, testname);
> > + if (i)
> > + report_info("took more than %d ms", i * 100);
>
> Any reason for dropping the '++'? Without it we don't account for the last
> 100 ms.
Actually we expect the interrupt to either fire immediately, or to not fire at
all (timeout). So the previous message of "Completed in 100 ms" was somewhat
misleading, because this was just due to the mdelay(100) above, and the IRQ was
most probably delivered before this delay loop even started.
I had "took less than ++i *100 ms" before (and can revert to that if you like),
but then figured that filtering for the most common case (immediate delivery)
is more useful.
>
> > return;
> > }
> > }
> > @@ -105,8 +107,9 @@ static void check_acked(cpumask_t *mask)
> > }
> > }
> >
> > - report("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> > - false, missing, extra, unexpected);
> > + report("%s", false, testname);
> > + report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> > + missing, extra, unexpected);
> > }
> >
> > static void check_spurious(void)
> > @@ -185,7 +188,7 @@ static void ipi_test_self(void)
> > cpumask_clear(&mask);
> > cpumask_set_cpu(smp_processor_id(), &mask);
> > gic->ipi.send_self();
> > - check_acked(&mask);
> > + check_acked("IPI to self", &mask);
>
> Could even do "IPI: self"
> "IPI: directed"
> "IPI: broadcast"
>
> to improve parsibility
Indeed.
Thanks for having a look!
Cheers,
Andre.
>
> > report_prefix_pop();
> > }
> >
> > @@ -200,7 +203,7 @@ static void ipi_test_smp(void)
> > for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> > cpumask_clear_cpu(i, &mask);
> > gic_ipi_send_mask(IPI_IRQ, &mask);
> > - check_acked(&mask);
> > + check_acked("directed IPI", &mask);
> > report_prefix_pop();
> >
> > report_prefix_push("broadcast");
> > @@ -208,7 +211,7 @@ static void ipi_test_smp(void)
> > cpumask_copy(&mask, &cpu_present_mask);
> > cpumask_clear_cpu(smp_processor_id(), &mask);
> > gic->ipi.send_broadcast();
> > - check_acked(&mask);
> > + check_acked("IPI broadcast", &mask);
> > report_prefix_pop();
> > }
> >
> > --
> > 2.17.1
> >
>
> Thanks,
> drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 6/6] arm: Add missing test name prefix calls
On Fri, Sep 27, 2019 at 11:42:27AM +0100, Andre Przywara wrote:
> When running the unit tests in TAP mode (./run_tests.sh -t), every single
> test result is printed. This works fine for most tests which use the
> reporting prefix feature to indicate the actual test name.
> However psci and pci were missing those names, so the reporting left
> people scratching their head what was actually tested:
> ...
> ok 74 - invalid-function
> ok 75 - affinity-info-on
> ok 76 - affinity-info-off
> ok 77 - cpu-on
>
> Push a "psci" prefix before running those tests to make those report
> lines more descriptive.
> While at it, do the same for pci, even though it is less ambigious there.
> Also the GIC ITARGETSR test was missing a report_prefix_pop().
>
> Signed-off-by: Andre Przywara
> ---
> arm/gic.c | 2 ++
> arm/pci-test.c | 2 ++
> arm/psci.c | 2 ++
> 3 files changed, 6 insertions(+)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 66dcafe..ebb6ea2 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -480,6 +480,8 @@ static void test_targets(int nr_irqs)
> test_byte_access(targetsptr + GIC_FIRST_SPI, pattern, cpu_mask);
>
> writel(orig_targets, targetsptr + GIC_FIRST_SPI);
> +
> + report_prefix_pop();
> }
>
> static void gic_test_mmio(void)
> diff --git a/arm/pci-test.c b/arm/pci-test.c
> index cf128ac..7c3836e 100644
> --- a/arm/pci-test.c
> +++ b/arm/pci-test.c
> @@ -19,6 +19,8 @@ int main(void)
> return report_summary();
> }
>
> + report_prefix_push("pci");
> +
> pci_print();
>
> ret = pci_testdev();
> diff --git a/arm/psci.c b/arm/psci.c
> index 5cb4d5c..536c9b7 100644
> --- a/arm/psci.c
> +++ b/arm/psci.c
> @@ -126,6 +126,8 @@ int main(void)
> {
> int ver = psci_invoke(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>
> + report_prefix_push("psci");
> +
> if (nr_cpus < 2) {
> report_skip("At least 2 cpus required");
> goto done;
> --
> 2.17.1
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 5/6] arm: selftest: Make MPIDR output stable
On Fri, Sep 27, 2019 at 11:42:26AM +0100, Andre Przywara wrote:
> At the moment the smp selftest outputs one line for each vCPU, with the
> CPU number and its MPIDR printed in the same test result line.
> For automated test frameworks this has the problem of including variable
> output in the test name, also the number of tests varies, depending on the
> number of vCPUs.
>
> Fix this by only generating a single line of output for the SMP test,
> which summarises the result. We use two cpumasks, to let each vCPU report
> its result and completion of the test (code stolen from the GIC test).
>
> For informational purposes we keep the one line per CPU, but prefix it
> with an INFO: tag, so that frameworks can ignore it.
>
> Signed-off-by: Andre Przywara
> ---
> arm/selftest.c | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arm/selftest.c b/arm/selftest.c
> index a0c1ab8..e9dc5c0 100644
> --- a/arm/selftest.c
> +++ b/arm/selftest.c
> @@ -17,6 +17,8 @@
> #include
> #include
>
> +static cpumask_t ready, valid;
> +
> static void __user_psci_system_off(void)
> {
> psci_system_off();
> @@ -341,8 +343,11 @@ static void cpu_report(void *data __unused)
> uint64_t mpidr = get_mpidr();
> int cpu = smp_processor_id();
>
> - report("CPU(%3d) mpidr=%010" PRIx64,
> - mpidr_to_cpu(mpidr) == cpu, cpu, mpidr);
> + if (mpidr_to_cpu(mpidr) == cpu)
> + cpumask_set_cpu(smp_processor_id(), &valid);
> + smp_wmb(); /* Paired with rmb in main(). */
> + cpumask_set_cpu(smp_processor_id(), &ready);
> + report_info("CPU%3d: MPIDR=%010" PRIx64, cpu, mpidr);
> }
>
> int main(int argc, char **argv)
> @@ -371,6 +376,11 @@ int main(int argc, char **argv)
>
> report("PSCI version", psci_check());
> on_cpus(cpu_report, NULL);
> + while (!cpumask_full(&ready))
> + cpu_relax();
> + smp_rmb(); /* Paired with wmb in cpu_report(). */
> + report("MPIDR test on all CPUs", cpumask_full(&valid));
> + report_info("%d CPUs reported back", nr_cpus);
>
> } else {
> printf("Unknown subtest\n");
> --
> 2.17.1
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 4/6] arm: selftest: Split variable output data from test name
On Fri, Sep 27, 2019 at 11:42:25AM +0100, Andre Przywara wrote:
> For some tests we mix variable diagnostic output with the test name,
> which leads to variable test line, confusing some higher level
> frameworks.
>
> Split the output to always use the same test name for a certain test,
> and put diagnostic output on a separate line using the INFO: tag.
>
> Signed-off-by: Andre Przywara
> ---
> arm/selftest.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arm/selftest.c b/arm/selftest.c
> index 28a17f7..a0c1ab8 100644
> --- a/arm/selftest.c
> +++ b/arm/selftest.c
> @@ -43,13 +43,16 @@ static void check_setup(int argc, char **argv)
> phys_addr_t memsize = PHYS_END - PHYS_OFFSET;
> phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
>
> - report("size = %" PRIu64 " MB", memsize == expected,
> - memsize/1024/1024);
> + report("memory size matches expectation",
> +memsize == expected);
> + report_info("found %" PRIu64 " MB", memsize/1024/1024);
> ++nr_tests;
>
> } else if (strcmp(argv[i], "smp") == 0) {
>
> - report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
> + report("number of CPUs matches expectation",
> +nr_cpus == (int)val);
> + report_info("found %d CPUs", nr_cpus);
> ++nr_tests;
> }
>
> --
> 2.17.1
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 3/6] arm: timer: Split variable output data from test name
On Fri, Sep 27, 2019 at 11:42:24AM +0100, Andre Przywara wrote:
> For some tests we mix variable diagnostic output with the test name,
> which leads to variable test line, confusing some higher level
> frameworks.
>
> Split the output to always use the same test name for a certain test,
> and put diagnostic output on a separate line using the INFO: tag.
>
> Signed-off-by: Andre Przywara
> ---
> arm/timer.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index f2f6019..0b808d5 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -249,7 +249,8 @@ static void test_timer(struct timer_info *info)
> local_irq_enable();
> left = info->read_tval();
> report("interrupt received after TVAL/WFI", info->irq_received);
> - report("timer has expired (%d)", left < 0, left);
> + report("timer has expired", left < 0);
> + report_info("TVAL is %d ticks", left);
> }
>
> static void test_vtimer(void)
> --
> 2.17.1
>
Reviewed-by: Andrew Jones
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH 2/6] arm: gic: Split variable output data from test name
On Fri, Sep 27, 2019 at 11:42:23AM +0100, Andre Przywara wrote:
> For some tests we mix variable diagnostic output with the test name,
> which leads to variable test line, confusing some higher level
> frameworks.
>
> Split the output to always use the same test name for a certain test,
> and put diagnostic output on a separate line using the INFO: tag.
>
> Signed-off-by: Andre Przywara
> ---
> arm/gic.c | 45 ++---
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index 6fd5e5e..66dcafe 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -353,8 +353,8 @@ static void test_typer_v2(uint32_t reg)
> {
> int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
>
> - report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
> -nr_gic_cpus);
> + report("all CPUs have interrupts", nr_cpus == nr_gic_cpus);
> + report_info("having %d CPUs", nr_gic_cpus);
Maybe slightly better:
report_info("nr_cpus=%d", nr_cpus);
report("all CPUs have interrupts", nr_cpus == nr_gic_cpus);
> }
>
> #define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
> @@ -370,16 +370,21 @@ static void test_typer_v2(uint32_t reg)
> static void test_byte_access(void *base_addr, u32 pattern, u32 mask)
> {
> u32 reg = readb(base_addr + 1);
> + bool res;
>
> - report("byte reads successful (0x%08x => 0x%02x)",
> -reg == (BYTE(pattern, 1) & (mask >> 8)),
> -pattern & mask, reg);
> + res = (reg == (BYTE(pattern, 1) & (mask >> 8)));
> + report("byte reads successful", res);
> + if (!res)
> + report_info("byte 1 of 0x%08x => 0x%02x", pattern & mask, reg);
>
> pattern = REPLACE_BYTE(pattern, 2, 0x1f);
> writeb(BYTE(pattern, 2), base_addr + 2);
> reg = readl(base_addr);
> - report("byte writes successful (0x%02x => 0x%08x)",
> -reg == (pattern & mask), BYTE(pattern, 2), reg);
> + res = (reg == (pattern & mask));
> + report("byte writes successful", res);
> + if (!res)
> + report_info("writing 0x%02x into bytes 2 => 0x%08x",
> + BYTE(pattern, 2), reg);
> }
>
> static void test_priorities(int nr_irqs, void *priptr)
> @@ -399,15 +404,16 @@ static void test_priorities(int nr_irqs, void *priptr)
> pri_mask = readl(first_spi);
>
> reg = ~pri_mask;
> - report("consistent priority masking (0x%08x)",
> + report("consistent priority masking",
> (((reg >> 16) == (reg & 0x)) &&
> - ((reg & 0xff) == ((reg >> 8) & 0xff))), pri_mask);
> + ((reg & 0xff) == ((reg >> 8) & 0xff;
> + report_info("priority mask is 0x%08x", pri_mask);
>
> reg = reg & 0xff;
> for (pri_bits = 8; reg & 1; reg >>= 1, pri_bits--)
> ;
> - report("implements at least 4 priority bits (%d)",
> -pri_bits >= 4, pri_bits);
> + report("implements at least 4 priority bits", pri_bits >= 4);
> + report_info("%d priority bits implemented", pri_bits);
>
> pattern = 0;
> writel(pattern, first_spi);
> @@ -452,9 +458,9 @@ static void test_targets(int nr_irqs)
> /* Check that bits for non implemented CPUs are RAZ/WI. */
> if (nr_cpus < 8) {
> writel(0x, targetsptr + GIC_FIRST_SPI);
> - report("bits for %d non-existent CPUs masked",
> -!(readl(targetsptr + GIC_FIRST_SPI) & ~cpu_mask),
> -8 - nr_cpus);
> + report("bits for non-existent CPUs masked",
> +!(readl(targetsptr + GIC_FIRST_SPI) & ~cpu_mask));
> + report_info("%d non-existent CPUs", 8 - nr_cpus);
> } else {
> report_skip("CPU masking (all CPUs implemented)");
> }
> @@ -465,8 +471,10 @@ static void test_targets(int nr_irqs)
> pattern = 0x0103020f;
> writel(pattern, targetsptr + GIC_FIRST_SPI);
> reg = readl(targetsptr + GIC_FIRST_SPI);
> - report("register content preserved (%08x => %08x)",
> -reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
> + report("register content preserved", reg == (pattern & cpu_mask));
> + if (reg != (pattern & cpu_mask))
> + report_info("writing %08x reads back as %08x",
> + pattern & cpu_mask, reg);
>
> /* The TARGETS registers are byte accessible. */
> test_byte_access(targetsptr + GIC_FIRST_SPI, pattern, cpu_mask);
> @@ -505,9 +513,8 @@ static void gic_test_mmio(void)
> test_readonly_32(gic_dist_base + GICD_IIDR, false));
>
> reg = readl(idreg);
> - report("ICPIDR2 is read-only (0x%08x)",
> -test_readonly_32(idreg, false),
> -reg);
> + report("ICPIDR2 is read-only", test_readonly_32(idreg, false));
> + report_info("value of ICPIDR2: 0x%08x", reg);
>
> test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
>
> -
Re: [kvm-unit-tests PATCH 1/6] arm: gic: check_acked: add test description
On Fri, Sep 27, 2019 at 11:42:22AM +0100, Andre Przywara wrote:
> At the moment the check_acked() IRQ helper function just prints a
> generic "Completed" or "Timed out" message, without given a more
> detailed test description.
>
> To be able to tell the different IRQ tests apart, and also to allow
> re-using it more easily, add a "description" parameter string,
> which is prefixing the output line. This gives more information on what
> exactly was tested.
>
> This also splits the variable output part of the line (duration of IRQ
> delivery) into a separate INFO: line, to not confuse testing frameworks.
>
> Signed-off-by: Andre Przywara
> ---
> arm/gic.c | 17 ++---
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/arm/gic.c b/arm/gic.c
> index ed5642e..6fd5e5e 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -60,7 +60,7 @@ static void stats_reset(void)
> smp_wmb();
> }
>
> -static void check_acked(cpumask_t *mask)
> +static void check_acked(const char *testname, cpumask_t *mask)
> {
> int missing = 0, extra = 0, unexpected = 0;
> int nr_pass, cpu, i;
> @@ -88,7 +88,9 @@ static void check_acked(cpumask_t *mask)
> }
> }
> if (nr_pass == nr_cpus) {
> - report("Completed in %d ms", !bad, ++i * 100);
> + report("%s", !bad, testname);
> + if (i)
> + report_info("took more than %d ms", i * 100);
Any reason for dropping the '++'? Without it we don't account for the last
100 ms.
> return;
> }
> }
> @@ -105,8 +107,9 @@ static void check_acked(cpumask_t *mask)
> }
> }
>
> - report("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> -false, missing, extra, unexpected);
> + report("%s", false, testname);
> + report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> + missing, extra, unexpected);
> }
>
> static void check_spurious(void)
> @@ -185,7 +188,7 @@ static void ipi_test_self(void)
> cpumask_clear(&mask);
> cpumask_set_cpu(smp_processor_id(), &mask);
> gic->ipi.send_self();
> - check_acked(&mask);
> + check_acked("IPI to self", &mask);
Could even do "IPI: self"
"IPI: directed"
"IPI: broadcast"
to improve parsibility
> report_prefix_pop();
> }
>
> @@ -200,7 +203,7 @@ static void ipi_test_smp(void)
> for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> cpumask_clear_cpu(i, &mask);
> gic_ipi_send_mask(IPI_IRQ, &mask);
> - check_acked(&mask);
> + check_acked("directed IPI", &mask);
> report_prefix_pop();
>
> report_prefix_push("broadcast");
> @@ -208,7 +211,7 @@ static void ipi_test_smp(void)
> cpumask_copy(&mask, &cpu_present_mask);
> cpumask_clear_cpu(smp_processor_id(), &mask);
> gic->ipi.send_broadcast();
> - check_acked(&mask);
> + check_acked("IPI broadcast", &mask);
> report_prefix_pop();
> }
>
> --
> 2.17.1
>
Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests PATCH 4/6] arm: selftest: Split variable output data from test name
For some tests we mix variable diagnostic output with the test name,
which leads to variable test line, confusing some higher level
frameworks.
Split the output to always use the same test name for a certain test,
and put diagnostic output on a separate line using the INFO: tag.
Signed-off-by: Andre Przywara
---
arm/selftest.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/arm/selftest.c b/arm/selftest.c
index 28a17f7..a0c1ab8 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -43,13 +43,16 @@ static void check_setup(int argc, char **argv)
phys_addr_t memsize = PHYS_END - PHYS_OFFSET;
phys_addr_t expected = ((phys_addr_t)val)*1024*1024;
- report("size = %" PRIu64 " MB", memsize == expected,
- memsize/1024/1024);
+ report("memory size matches expectation",
+ memsize == expected);
+ report_info("found %" PRIu64 " MB", memsize/1024/1024);
++nr_tests;
} else if (strcmp(argv[i], "smp") == 0) {
- report("nr_cpus = %d", nr_cpus == (int)val, nr_cpus);
+ report("number of CPUs matches expectation",
+ nr_cpus == (int)val);
+ report_info("found %d CPUs", nr_cpus);
++nr_tests;
}
--
2.17.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests PATCH 1/6] arm: gic: check_acked: add test description
At the moment the check_acked() IRQ helper function just prints a
generic "Completed" or "Timed out" message, without given a more
detailed test description.
To be able to tell the different IRQ tests apart, and also to allow
re-using it more easily, add a "description" parameter string,
which is prefixing the output line. This gives more information on what
exactly was tested.
This also splits the variable output part of the line (duration of IRQ
delivery) into a separate INFO: line, to not confuse testing frameworks.
Signed-off-by: Andre Przywara
---
arm/gic.c | 17 ++---
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/arm/gic.c b/arm/gic.c
index ed5642e..6fd5e5e 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -60,7 +60,7 @@ static void stats_reset(void)
smp_wmb();
}
-static void check_acked(cpumask_t *mask)
+static void check_acked(const char *testname, cpumask_t *mask)
{
int missing = 0, extra = 0, unexpected = 0;
int nr_pass, cpu, i;
@@ -88,7 +88,9 @@ static void check_acked(cpumask_t *mask)
}
}
if (nr_pass == nr_cpus) {
- report("Completed in %d ms", !bad, ++i * 100);
+ report("%s", !bad, testname);
+ if (i)
+ report_info("took more than %d ms", i * 100);
return;
}
}
@@ -105,8 +107,9 @@ static void check_acked(cpumask_t *mask)
}
}
- report("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
- false, missing, extra, unexpected);
+ report("%s", false, testname);
+ report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
+ missing, extra, unexpected);
}
static void check_spurious(void)
@@ -185,7 +188,7 @@ static void ipi_test_self(void)
cpumask_clear(&mask);
cpumask_set_cpu(smp_processor_id(), &mask);
gic->ipi.send_self();
- check_acked(&mask);
+ check_acked("IPI to self", &mask);
report_prefix_pop();
}
@@ -200,7 +203,7 @@ static void ipi_test_smp(void)
for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
cpumask_clear_cpu(i, &mask);
gic_ipi_send_mask(IPI_IRQ, &mask);
- check_acked(&mask);
+ check_acked("directed IPI", &mask);
report_prefix_pop();
report_prefix_push("broadcast");
@@ -208,7 +211,7 @@ static void ipi_test_smp(void)
cpumask_copy(&mask, &cpu_present_mask);
cpumask_clear_cpu(smp_processor_id(), &mask);
gic->ipi.send_broadcast();
- check_acked(&mask);
+ check_acked("IPI broadcast", &mask);
report_prefix_pop();
}
--
2.17.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests PATCH 5/6] arm: selftest: Make MPIDR output stable
At the moment the smp selftest outputs one line for each vCPU, with the
CPU number and its MPIDR printed in the same test result line.
For automated test frameworks this has the problem of including variable
output in the test name, also the number of tests varies, depending on the
number of vCPUs.
Fix this by only generating a single line of output for the SMP test,
which summarises the result. We use two cpumasks, to let each vCPU report
its result and completion of the test (code stolen from the GIC test).
For informational purposes we keep the one line per CPU, but prefix it
with an INFO: tag, so that frameworks can ignore it.
Signed-off-by: Andre Przywara
---
arm/selftest.c | 14 --
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/arm/selftest.c b/arm/selftest.c
index a0c1ab8..e9dc5c0 100644
--- a/arm/selftest.c
+++ b/arm/selftest.c
@@ -17,6 +17,8 @@
#include
#include
+static cpumask_t ready, valid;
+
static void __user_psci_system_off(void)
{
psci_system_off();
@@ -341,8 +343,11 @@ static void cpu_report(void *data __unused)
uint64_t mpidr = get_mpidr();
int cpu = smp_processor_id();
- report("CPU(%3d) mpidr=%010" PRIx64,
- mpidr_to_cpu(mpidr) == cpu, cpu, mpidr);
+ if (mpidr_to_cpu(mpidr) == cpu)
+ cpumask_set_cpu(smp_processor_id(), &valid);
+ smp_wmb(); /* Paired with rmb in main(). */
+ cpumask_set_cpu(smp_processor_id(), &ready);
+ report_info("CPU%3d: MPIDR=%010" PRIx64, cpu, mpidr);
}
int main(int argc, char **argv)
@@ -371,6 +376,11 @@ int main(int argc, char **argv)
report("PSCI version", psci_check());
on_cpus(cpu_report, NULL);
+ while (!cpumask_full(&ready))
+ cpu_relax();
+ smp_rmb(); /* Paired with wmb in cpu_report(). */
+ report("MPIDR test on all CPUs", cpumask_full(&valid));
+ report_info("%d CPUs reported back", nr_cpus);
} else {
printf("Unknown subtest\n");
--
2.17.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests PATCH 0/6] arm: Use stable test output lines
When using kvm-unit-tests inside automated testing frameworks, variable test naming becomes a problem. Some frameworks recognise tests by their test output line and group the outputs from various runs for statistical and reporting purposes. Having variable output like timer values in there spoils this approach. Also the test name should be somewhat self-explanatory, which is not true for every test. Some examples highlighting the problem (TAP output from run-tests.sh -t): ok 1 - selftest: setup: smp: nr_cpus = 2 ok 2 - selftest: setup: mem: size = 256 MB ok 8 - selftest: smp: CPU( 1) mpidr=008001 ok 9 - selftest: smp: CPU( 2) mpidr=008002 ok 54 - gicv2: mmio: ITARGETSR: byte writes successful (0x1f => 0x01010001) ok 55 - gicv2: mmio: all 3 CPUs have interrupts ok 73 - invalid-function ok 76 - cpu-on ok 90 - ptimer-busy-loop: timer has expired (-8445) This series aims to fix most of the problems, by making the actual test report output line stable. I think this is best practises in the testing world, at least when using TAP. We still retain the full information, by moving every variable output into INFO: lines (which are still logged, but typically filtered for automated processing). The above lines now look like this: ok 1 - selftest: setup: smp: number of CPUs matches expectation ok 2 - selftest: setup: mem: memory size matches expectation ok 8 - selftest: smp: MPIDR test on all CPUs ok 49 - gicv2: mmio: ITARGETSR: byte writes successful ok 50 - gicv2: mmio: all CPUs have interrupts ok 68 - psci: invalid-function ok 71 - psci: cpu-on ok 85 - ptimer-busy-loop: timer has expired Looks a bit more boring, but it's nicer for automated processing and logging. I am open for a discussion about the general approach, thus this is dealing with ARM tests for now only. Looking forward to any feedback! Cheers, Andre Andre Przywara (6): arm: gic: check_acked: add test description arm: gic: Split variable output data from test name arm: timer: Split variable output data from test name arm: selftest: Split variable output data from test name arm: selftest: Make MPIDR output stable arm: Add missing test name prefix calls arm/gic.c | 64 ++ arm/pci-test.c | 2 ++ arm/psci.c | 2 ++ arm/selftest.c | 23 ++ arm/timer.c| 3 ++- 5 files changed, 62 insertions(+), 32 deletions(-) -- 2.17.1 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests PATCH 2/6] arm: gic: Split variable output data from test name
For some tests we mix variable diagnostic output with the test name,
which leads to variable test line, confusing some higher level
frameworks.
Split the output to always use the same test name for a certain test,
and put diagnostic output on a separate line using the INFO: tag.
Signed-off-by: Andre Przywara
---
arm/gic.c | 45 ++---
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/arm/gic.c b/arm/gic.c
index 6fd5e5e..66dcafe 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -353,8 +353,8 @@ static void test_typer_v2(uint32_t reg)
{
int nr_gic_cpus = ((reg >> 5) & 0x7) + 1;
- report("all %d CPUs have interrupts", nr_cpus == nr_gic_cpus,
- nr_gic_cpus);
+ report("all CPUs have interrupts", nr_cpus == nr_gic_cpus);
+ report_info("having %d CPUs", nr_gic_cpus);
}
#define BYTE(reg32, byte) (((reg32) >> ((byte) * 8)) & 0xff)
@@ -370,16 +370,21 @@ static void test_typer_v2(uint32_t reg)
static void test_byte_access(void *base_addr, u32 pattern, u32 mask)
{
u32 reg = readb(base_addr + 1);
+ bool res;
- report("byte reads successful (0x%08x => 0x%02x)",
- reg == (BYTE(pattern, 1) & (mask >> 8)),
- pattern & mask, reg);
+ res = (reg == (BYTE(pattern, 1) & (mask >> 8)));
+ report("byte reads successful", res);
+ if (!res)
+ report_info("byte 1 of 0x%08x => 0x%02x", pattern & mask, reg);
pattern = REPLACE_BYTE(pattern, 2, 0x1f);
writeb(BYTE(pattern, 2), base_addr + 2);
reg = readl(base_addr);
- report("byte writes successful (0x%02x => 0x%08x)",
- reg == (pattern & mask), BYTE(pattern, 2), reg);
+ res = (reg == (pattern & mask));
+ report("byte writes successful", res);
+ if (!res)
+ report_info("writing 0x%02x into bytes 2 => 0x%08x",
+ BYTE(pattern, 2), reg);
}
static void test_priorities(int nr_irqs, void *priptr)
@@ -399,15 +404,16 @@ static void test_priorities(int nr_irqs, void *priptr)
pri_mask = readl(first_spi);
reg = ~pri_mask;
- report("consistent priority masking (0x%08x)",
+ report("consistent priority masking",
(((reg >> 16) == (reg & 0x)) &&
- ((reg & 0xff) == ((reg >> 8) & 0xff))), pri_mask);
+ ((reg & 0xff) == ((reg >> 8) & 0xff;
+ report_info("priority mask is 0x%08x", pri_mask);
reg = reg & 0xff;
for (pri_bits = 8; reg & 1; reg >>= 1, pri_bits--)
;
- report("implements at least 4 priority bits (%d)",
- pri_bits >= 4, pri_bits);
+ report("implements at least 4 priority bits", pri_bits >= 4);
+ report_info("%d priority bits implemented", pri_bits);
pattern = 0;
writel(pattern, first_spi);
@@ -452,9 +458,9 @@ static void test_targets(int nr_irqs)
/* Check that bits for non implemented CPUs are RAZ/WI. */
if (nr_cpus < 8) {
writel(0x, targetsptr + GIC_FIRST_SPI);
- report("bits for %d non-existent CPUs masked",
- !(readl(targetsptr + GIC_FIRST_SPI) & ~cpu_mask),
- 8 - nr_cpus);
+ report("bits for non-existent CPUs masked",
+ !(readl(targetsptr + GIC_FIRST_SPI) & ~cpu_mask));
+ report_info("%d non-existent CPUs", 8 - nr_cpus);
} else {
report_skip("CPU masking (all CPUs implemented)");
}
@@ -465,8 +471,10 @@ static void test_targets(int nr_irqs)
pattern = 0x0103020f;
writel(pattern, targetsptr + GIC_FIRST_SPI);
reg = readl(targetsptr + GIC_FIRST_SPI);
- report("register content preserved (%08x => %08x)",
- reg == (pattern & cpu_mask), pattern & cpu_mask, reg);
+ report("register content preserved", reg == (pattern & cpu_mask));
+ if (reg != (pattern & cpu_mask))
+ report_info("writing %08x reads back as %08x",
+ pattern & cpu_mask, reg);
/* The TARGETS registers are byte accessible. */
test_byte_access(targetsptr + GIC_FIRST_SPI, pattern, cpu_mask);
@@ -505,9 +513,8 @@ static void gic_test_mmio(void)
test_readonly_32(gic_dist_base + GICD_IIDR, false));
reg = readl(idreg);
- report("ICPIDR2 is read-only (0x%08x)",
- test_readonly_32(idreg, false),
- reg);
+ report("ICPIDR2 is read-only", test_readonly_32(idreg, false));
+ report_info("value of ICPIDR2: 0x%08x", reg);
test_priorities(nr_irqs, gic_dist_base + GICD_IPRIORITYR);
--
2.17.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests PATCH 3/6] arm: timer: Split variable output data from test name
For some tests we mix variable diagnostic output with the test name,
which leads to variable test line, confusing some higher level
frameworks.
Split the output to always use the same test name for a certain test,
and put diagnostic output on a separate line using the INFO: tag.
Signed-off-by: Andre Przywara
---
arm/timer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arm/timer.c b/arm/timer.c
index f2f6019..0b808d5 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -249,7 +249,8 @@ static void test_timer(struct timer_info *info)
local_irq_enable();
left = info->read_tval();
report("interrupt received after TVAL/WFI", info->irq_received);
- report("timer has expired (%d)", left < 0, left);
+ report("timer has expired", left < 0);
+ report_info("TVAL is %d ticks", left);
}
static void test_vtimer(void)
--
2.17.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[kvm-unit-tests PATCH 6/6] arm: Add missing test name prefix calls
When running the unit tests in TAP mode (./run_tests.sh -t), every single
test result is printed. This works fine for most tests which use the
reporting prefix feature to indicate the actual test name.
However psci and pci were missing those names, so the reporting left
people scratching their head what was actually tested:
...
ok 74 - invalid-function
ok 75 - affinity-info-on
ok 76 - affinity-info-off
ok 77 - cpu-on
Push a "psci" prefix before running those tests to make those report
lines more descriptive.
While at it, do the same for pci, even though it is less ambigious there.
Also the GIC ITARGETSR test was missing a report_prefix_pop().
Signed-off-by: Andre Przywara
---
arm/gic.c | 2 ++
arm/pci-test.c | 2 ++
arm/psci.c | 2 ++
3 files changed, 6 insertions(+)
diff --git a/arm/gic.c b/arm/gic.c
index 66dcafe..ebb6ea2 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -480,6 +480,8 @@ static void test_targets(int nr_irqs)
test_byte_access(targetsptr + GIC_FIRST_SPI, pattern, cpu_mask);
writel(orig_targets, targetsptr + GIC_FIRST_SPI);
+
+ report_prefix_pop();
}
static void gic_test_mmio(void)
diff --git a/arm/pci-test.c b/arm/pci-test.c
index cf128ac..7c3836e 100644
--- a/arm/pci-test.c
+++ b/arm/pci-test.c
@@ -19,6 +19,8 @@ int main(void)
return report_summary();
}
+ report_prefix_push("pci");
+
pci_print();
ret = pci_testdev();
diff --git a/arm/psci.c b/arm/psci.c
index 5cb4d5c..536c9b7 100644
--- a/arm/psci.c
+++ b/arm/psci.c
@@ -126,6 +126,8 @@ int main(void)
{
int ver = psci_invoke(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
+ report_prefix_push("psci");
+
if (nr_cpus < 2) {
report_skip("At least 2 cpus required");
goto done;
--
2.17.1
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
RE: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
Hi Suzuki, > -Original Message- > From: Suzuki K Poulose > Sent: Friday, September 27, 2019 6:23 PM > To: Jianyong Wu (Arm Technology China) ; > net...@vger.kernel.org; yangbo...@nxp.com; john.stu...@linaro.org; > t...@linutronix.de; pbonz...@redhat.com; sean.j.christopher...@intel.com; > m...@kernel.org; richardcoch...@gmail.com; Mark Rutland > ; Will Deacon > Cc: linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org; > kvmarm@lists.cs.columbia.edu; k...@vger.kernel.org; Steve Capper > ; Kaly Xin (Arm Technology China) > ; Justin He (Arm Technology China) > ; nd > Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it > arch-independent. > > > > On 26/09/2019 12:42, Jianyong Wu wrote: > > Currently, ptp_kvm modules implementation is only for x86 which > > includs large part of arch-specific code. This patch move all of > > those code into new arch related file in the same directory. > > > > Signed-off-by: Jianyong Wu > > ... > > > +int kvm_arch_ptp_get_clock_fn(unsigned long *cycle, struct timespec64 > *tspec, > > + struct clocksource **cs) > > > > diff --git a/include/asm-generic/ptp_kvm.h > > b/include/asm-generic/ptp_kvm.h new file mode 100644 index > > ..208e842bfa64 > > --- /dev/null > > +++ b/include/asm-generic/ptp_kvm.h > > > +int kvm_arch_ptp_get_clock_fn(long *cycle, > > + struct timespec64 *tspec, void *cs); > > > > Conflicting types for kvm_arch_ptp_get_clock_fn() ? > Yeah, need fix. Thanks Jianyong Wu > Suzuki
Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
On 26/09/2019 12:42, Jianyong Wu wrote: Currently, ptp_kvm modules implementation is only for x86 which includs large part of arch-specific code. This patch move all of those code into new arch related file in the same directory. Signed-off-by: Jianyong Wu ... +int kvm_arch_ptp_get_clock_fn(unsigned long *cycle, struct timespec64 *tspec, + struct clocksource **cs) diff --git a/include/asm-generic/ptp_kvm.h b/include/asm-generic/ptp_kvm.h new file mode 100644 index ..208e842bfa64 --- /dev/null +++ b/include/asm-generic/ptp_kvm.h +int kvm_arch_ptp_get_clock_fn(long *cycle, + struct timespec64 *tspec, void *cs); Conflicting types for kvm_arch_ptp_get_clock_fn() ? Suzuki
RE: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
Hi Suzuki,
> -Original Message-
> From: Suzuki K Poulose
> Sent: Friday, September 27, 2019 6:12 PM
> To: Jianyong Wu (Arm Technology China) ;
> net...@vger.kernel.org; yangbo...@nxp.com; john.stu...@linaro.org;
> t...@linutronix.de; pbonz...@redhat.com; sean.j.christopher...@intel.com;
> m...@kernel.org; richardcoch...@gmail.com; Mark Rutland
> ; Will Deacon
> Cc: linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> kvmarm@lists.cs.columbia.edu; k...@vger.kernel.org; Steve Capper
> ; Kaly Xin (Arm Technology China)
> ; Justin He (Arm Technology China)
> ; nd
> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it
> arch-independent.
>
>
>
> On 27/09/2019 11:10, Jianyong Wu (Arm Technology China) wrote:
> > Hi Suzuki,
> >
> >> -Original Message-
> >> From: Suzuki K Poulose
> >> Sent: Thursday, September 26, 2019 9:06 PM
> >> To: Jianyong Wu (Arm Technology China) ;
> >> net...@vger.kernel.org; yangbo...@nxp.com; john.stu...@linaro.org;
> >> t...@linutronix.de; pbonz...@redhat.com;
> >> sean.j.christopher...@intel.com; m...@kernel.org;
> >> richardcoch...@gmail.com; Mark Rutland ;
> Will
> >> Deacon
> >> Cc: linux-ker...@vger.kernel.org;
> >> linux-arm-ker...@lists.infradead.org;
> >> kvmarm@lists.cs.columbia.edu; k...@vger.kernel.org; Steve Capper
> >> ; Kaly Xin (Arm Technology China)
> >> ; Justin He (Arm Technology China)
> >> ; nd
> >> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to
> >> make it arch-independent.
> >>
> >> Hi Jianyong,
> >>
> >> On 26/09/2019 12:42, Jianyong Wu wrote:
> >>> Currently, ptp_kvm modules implementation is only for x86 which
> >>> includs large part of arch-specific code. This patch move all of
> >>> those code into new arch related file in the same directory.
> >>>
> >>> Signed-off-by: Jianyong Wu
> >>> ---
> >>>drivers/ptp/Makefile | 1 +
> >>>drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++--
> >>>drivers/ptp/ptp_kvm_x86.c| 87
> >>
> >>>include/asm-generic/ptp_kvm.h| 12
> >>>4 files changed, 118 insertions(+), 59 deletions(-)
> >>>rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
> >>
> >> minor nit: Could we not skip renaming the file ? Given you are
> >> following the
> >> ptp_kvm_* for the arch specific files and the header files, wouldn't
> >> it be good to keep ptp_kvm.c ?
> >>
> > If the module name ptp_kvm.ko is the same with its dependent object
> > file ptp_kvm.o, warning will be given by compiler, So I change the
> ptp_kvm.c to kvm_ptp.c to avoid that conflict.
>
> Hmm, ok. How about ptp_kvm_common.c instead of kvm_ptp.c ?
Yeah, it's a better name, I will change it next version.
Thanks
Jianyong Wu
>
> Suzuki
Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
On 27/09/2019 11:10, Jianyong Wu (Arm Technology China) wrote:
Hi Suzuki,
-Original Message-
From: Suzuki K Poulose
Sent: Thursday, September 26, 2019 9:06 PM
To: Jianyong Wu (Arm Technology China) ;
net...@vger.kernel.org; yangbo...@nxp.com; john.stu...@linaro.org;
t...@linutronix.de; pbonz...@redhat.com; sean.j.christopher...@intel.com;
m...@kernel.org; richardcoch...@gmail.com; Mark Rutland
; Will Deacon
Cc: linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
kvmarm@lists.cs.columbia.edu; k...@vger.kernel.org; Steve Capper
; Kaly Xin (Arm Technology China)
; Justin He (Arm Technology China)
; nd
Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it
arch-independent.
Hi Jianyong,
On 26/09/2019 12:42, Jianyong Wu wrote:
Currently, ptp_kvm modules implementation is only for x86 which
includs large part of arch-specific code. This patch move all of
those code into new arch related file in the same directory.
Signed-off-by: Jianyong Wu
---
drivers/ptp/Makefile | 1 +
drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++--
drivers/ptp/ptp_kvm_x86.c| 87
include/asm-generic/ptp_kvm.h| 12
4 files changed, 118 insertions(+), 59 deletions(-)
rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
minor nit: Could we not skip renaming the file ? Given you are following the
ptp_kvm_* for the arch specific files and the header files, wouldn't it be
good to keep ptp_kvm.c ?
If the module name ptp_kvm.ko is the same with its dependent object file
ptp_kvm.o, warning will be given by compiler,
So I change the ptp_kvm.c to kvm_ptp.c to avoid that conflict.
Hmm, ok. How about ptp_kvm_common.c instead of kvm_ptp.c ?
Suzuki
RE: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
Hi Suzuki,
> -Original Message-
> From: Suzuki K Poulose
> Sent: Thursday, September 26, 2019 9:06 PM
> To: Jianyong Wu (Arm Technology China) ;
> net...@vger.kernel.org; yangbo...@nxp.com; john.stu...@linaro.org;
> t...@linutronix.de; pbonz...@redhat.com; sean.j.christopher...@intel.com;
> m...@kernel.org; richardcoch...@gmail.com; Mark Rutland
> ; Will Deacon
> Cc: linux-ker...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> kvmarm@lists.cs.columbia.edu; k...@vger.kernel.org; Steve Capper
> ; Kaly Xin (Arm Technology China)
> ; Justin He (Arm Technology China)
> ; nd
> Subject: Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it
> arch-independent.
>
> Hi Jianyong,
>
> On 26/09/2019 12:42, Jianyong Wu wrote:
> > Currently, ptp_kvm modules implementation is only for x86 which
> > includs large part of arch-specific code. This patch move all of
> > those code into new arch related file in the same directory.
> >
> > Signed-off-by: Jianyong Wu
> > ---
> > drivers/ptp/Makefile | 1 +
> > drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++--
> > drivers/ptp/ptp_kvm_x86.c| 87
>
> > include/asm-generic/ptp_kvm.h| 12
> > 4 files changed, 118 insertions(+), 59 deletions(-)
> > rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
>
> minor nit: Could we not skip renaming the file ? Given you are following the
> ptp_kvm_* for the arch specific files and the header files, wouldn't it be
> good to keep ptp_kvm.c ?
>
If the module name ptp_kvm.ko is the same with its dependent object file
ptp_kvm.o, warning will be given by compiler,
So I change the ptp_kvm.c to kvm_ptp.c to avoid that conflict.
Thanks
Jianyong Wu
> Rest looks fine.
>
> Cheers
> Suzuki
Re: [RFC PATCH v4 3/5] psci: Add hvc call service for ptp_kvm.
Hello! On 26.09.2019 14:42, Jianyong Wu wrote: This patch is the base of ptp_kvm for arm64. ptp_kvm modules will call hvc to get this service. The service offers real time and counter cycle of host for guest. Signed-off-by: Jianyong Wu --- include/linux/arm-smccc.h | 12 virt/kvm/arm/psci.c | 18 ++ 2 files changed, 30 insertions(+) [...] diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c index 0debf49bf259..3f30fc42a5ca 100644 --- a/virt/kvm/arm/psci.c +++ b/virt/kvm/arm/psci.c [...] @@ -431,6 +433,22 @@ int kvm_hvc_call_handler(struct kvm_vcpu *vcpu) case ARM_SMCCC_VENDOR_HYP_KVM_FEATURES_FUNC_ID: val[0] = BIT(ARM_SMCCC_KVM_FUNC_FEATURES); break; + /* +* This will used for virtual ptp kvm clock. three ^ be?^ T [...] MBR, Sergei
Re: [PATCH 5/5] arm64: Enable and document ARM errata 1319367 and 1319537
On Wed, Sep 25, 2019 at 12:19:41PM +0100, Marc Zyngier wrote: > Now that everything is in place, let's get the ball rolling > by allowing the corresponding config option to be selected. > Also add the required information to silicon_arrata.rst. > > Signed-off-by: Marc Zyngier Acked-by: Catalin Marinas ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/5] arm64: Add ARM64_WORKAROUND_1319367 for all A57 and A72 versions
On Wed, Sep 25, 2019 at 12:19:37PM +0100, Marc Zyngier wrote: > Rework the EL2 vector hardening that is only selected for A57 and A72 > so that the table can also be used for ARM64_WORKAROUND_1319367. > > Signed-off-by: Marc Zyngier Acked-by: Catalin Marinas -- Catalin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/5] arm64: Add ARM64_WORKAROUND_1319367 for all A57 and A72 versions
On 25/09/2019 12:19, Marc Zyngier wrote: Rework the EL2 vector hardening that is only selected for A57 and A72 so that the table can also be used for ARM64_WORKAROUND_1319367. Signed-off-by: Marc Zyngier Reviewed-by: Suzuki K Poulose ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 03/35] irqchip/gic-v3-its: Allow LPI invalidation via the DirectLPI interface
On 2019/9/27 0:17, Marc Zyngier wrote:
On 26/09/2019 15:57, Zenghui Yu wrote:
Hi Marc,
I get one kernel panic with this patch on D05.
Can you please try this (completely untested for now) on top of the
whole series? I'll otherwise give it a go next week.
Yes, it helps. At least I don't see panic any more. Without this change,
the host would get crashed as long as the VM is started.
Thanks,
zenghui
Thanks,
M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index bc55327406b7..9d24236d1257 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3461,15 +3461,16 @@ static void its_vpe_send_cmd(struct its_vpe *vpe,
static void its_vpe_send_inv(struct irq_data *d)
{
+ struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+
if (gic_rdists->has_direct_lpi) {
- /*
-* Don't mess about. Generating the invalidation is easily
-* done by using the parent irq_data, just like below.
-*/
- direct_lpi_inv(d->parent_data);
- } else {
- struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
+ void __iomem *rdbase;
+ /* Target the redistributor this VPE is currently known on */
+ rdbase = per_cpu_ptr(gic_rdists->rdist, vpe->col_idx)->rd_base;
+ gic_write_lpir(d->parent_data->hwirq, rdbase + GICR_INVLPIR);
+ wait_for_syncr(rdbase);
+ } else {
its_vpe_send_cmd(vpe, its_send_inv);
}
}
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
