On 2021-02-08 14:53, Hector Martin wrote:

On 08/02/2021 21.27, Marc Zyngier wrote:

quoted

quoted

+	timer {
+		compatible = "arm,armv8-timer";
+		interrupt-parent = <&aic>;
+		interrupts = <AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_FIQ 1 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>;

This unfortunately doesn't match the binding, which doesn't cater
for systems without a secure physical timer, nor allows the 
description
of the EL2 virtual timer.

You should also have *different* interrupts for EL1 and EL2 timers,
although this is all a lie...

Well, we do - now that I confirmed all 4 timers work properly, the AIC
driver should provide all 4. And ideally I find those EL1 timer mask
bits and implement them in the aic driver too (for only the virt
timers that have them and of course need them).

I just found the code in arm_arch_timer that forwards all this stuff
to the kvm code, so it all makes sense now; if I can wire that up
properly, heck, KVM might even just work here.

There is a bunch of other things to do to enable KVM, specially the
GICv3 emulation, but I've now started refactoring that part of the
code not to rely on a full blown CPU interface. Hopefully I'll have
something for the 5.13 time frame.

quoted

Looking at the only similar case, XGene lies about the secure timer
(it doesn't have any), and of course doesn't have an EL2 virtual
timer (ARMv8.0 only).

A sensible course of action could be to update the binding to at 
least:

- tell the kernel that there is no secure physical timer (and that
    the interrupt should be ignored)
- introduce a 5th possible interrupt for the EL2 virtual timer.

Sounds like I should be introducing interrupt-names support into this
driver and using that, so we can just not specify IRQs that don't
exist, instead of the hack with dummies. Falling back to indexes of
course, to keep DT compat. i.e.

const char *names = {"phys-secure", "phys", "virt", "hyp-phys", 
"hyp-virt"};

bool has_names = of_property_read_bool(..., "interrupt-names");

for (each irq)
	if (has_names) foo = of_irq_get_byname(..., names[i])
	else foo = of_irq_get(..., i)

Yup, that definitely looks like a good thing to introduce.

That said, is there a use case for the EL2 virtual timer? The driver
always uses the EL2 physical timer with VHE right now. I guess it's
worth describing it in the binding and dts, even if the driver never
selects it...?

Linux doesn't have a use for the EL2 virtual timer yet. It was only
introduced for symmetry with EL1 (except for CNTVOFF of course).
But it definitely is worth describing it. Who knows, we may find a use
for it at some point, and other OSes are using the same DT binding 
anway.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel