On 13.05.19 12:19, Julien Grall wrote:
Hi,
Hi, Julien, Geert
On 5/10/19 5:22 PM, Oleksandr Tyshchenko wrote:
From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
Don't use hardcoded address, retrieve it from device-tree instead.
And besides, this patch fixes the memory error when running
on top of Xen hypervisor:
(XEN) traps.c:1999:d0v0 HSR=0x93830007 pc=0xc0b097f8 gva=0xf0805000
gpa=0x000000e6080000
Which shows that VCPU0 in Dom0 is trying to access an address in memory
it is not allowed to access (0x000000e6080000).
Put simply, Xen doesn't know that it is a device's register memory
since it wasn't described in a host device tree (which Xen parses)
and as the result this memory region wasn't assigned to Dom0 at
domain creation time.
Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
---
This patch is meant to get feedback from the community before
proceeding further. If we decide to go this direction, all Gen2
device-trees should be updated (add memory region) before
this patch going in.
e.g. r8a7790.dtsi:
...
timer {
compatible = "arm,armv7-timer";
interrupts-extended = <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) |
IRQ_TYPE_LEVEL_LOW)>,
<&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) |
IRQ_TYPE_LEVEL_LOW)>,
<&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) |
IRQ_TYPE_LEVEL_LOW)>,
<&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) |
IRQ_TYPE_LEVEL_LOW)>;
+ reg = <0 0xe6080000 0 0x1000>;
This looks incorrect, the "arm,armv7-timer" bindings doesn't offer you
the possibility to specify an MMIO region. This makes sense because it
is meant to describe the Arch timer that is only access via
co-processor registers.
Looking at the code, I think the MMIO region corresponds to the
coresight (based on the register name). So you may want to describe
the coresight in the Device-Tree.
Also, AFAICT, the code is configuring and turning on the timer if it
has not been done yet. If you are here running on Xen, then you have
probably done something wrong. Indeed, it means Xen would not be able
to use the timer until Dom0 has booted. But, shouldn't newer U-boot do
it for you?
Let me elaborate a bit more on this...
Indeed, my PSCI patch series for U-Boot includes a patch [1] for
configuring that "counter module". So, if PSCI is available
(psci_smp_available() == true), then most likely we are running on
PSCI-enabled
U-Boot which, we assume, has already taken care of configuring timer (as
well as resetting CNTVOFF). So, when running on Xen, the timer was
configured beforehand in U-Boot, and Xen is able to use it from the very
beginning, these is no need to wait for Dom0 to configure it.
(XEN) Generic Timer IRQ: phys=30 hyp=26 virt=27 Freq: 10000 KHz
So, the code in brackets won't be called when using PSCI/running Xen,
since the timer is already both enabled and configured:
if ((ioread32(base + CNTCR) & 1) == 0 ||
ioread32(base + CNTFID0) != freq) {
/* Update registers with correct frequency */
iowrite32(freq, base + CNTFID0);
asm volatile("mcr p15, 0, %0, c14, c0, 0" : : "r" (freq));
/* make sure arch timer is started by setting bit 0 of CNTCR */
iowrite32(1, base + CNTCR);
}
But, the problem here is the first read access from timer register (when
we check whether the timer requires enabling) results in hypervisor trap:
(XEN) traps.c:1999:d0v0 HSR=0x93830007 pc=0xc0b097f8 gva=0xf0805000
gpa=0x000000e6080000
So, if the DT bindings for the counter module is not an option (if I
correctly understood a discussion pointed by Geert in another letter),
we should probably prevent all timer code here from being executed if
PSCI is in use.
What I mean is to return to [2], but with the modification to use
psci_smp_available() helper as an indicator of PSCI usage.
Julien, Geert, what do you think?
[1] https://marc.info/?l=u-boot&m=154895714510154&w=2
[2] https://lkml.org/lkml/2019/4/17/810
Cheers,
--
Regards,
Oleksandr Tyshchenko