Re: Smoke test failure on Arm (was Re: [PATCH v4 0/8] xen/arm: Emulate ID registers)

[André Przywara]

On 05/01/2021 16:06, Julien Grall wrote:
> (+ Ian and Andre)
> 
> Hi Bertrand,
> 
> On IRC, Ian pointed out that the smoke test was failing on Cubietruck.
> The only patches because the last success and the failure are your series.
> 
> This seems to be a very early failure as there is no output from Xen [1].
> 
> I originally thought the problem was because some of the ID registers
> (such as ID_PFR2) introduced in patch #2 doesn't exist in Armv7.
> 
> But per B7.2.1 in ARM DDI 0406C.d, unallocated ID registers should be
> RAZ. So it would result to a crash. Andre confirmed that PFR2 can be
> accessed by writing a small baremetal code and booted on Cortex-A7 and
> Cortex-A20.
> 
> So I am not entirely sure what's the problem. Andre kindly accepted to
> try to boot Xen on his board. Hopefully, this will give us a clue on the
> problem.
> 
> If not, I will borrow a Cubietruck in OssTest and see if I can reproduce
> it and debug it.


So I just compiled master and staging and ran just that on an Allwinner
H3 (Cortex-A7 r0p5). Master boots fine (till it complains about the
missing Dom0, as expected). However staging indeed fails:

(XEN) Xen version 4.15-unstable (andpr...@slackpad.lan)
(arm-slackware-linux-gnueabihf-gcc (GCC) 8.2.0) debug=y  Tue Jan  5
16:09:40 GMT 2021
(XEN) Latest ChangeSet: Sun Nov 8 15:59:42 2020 +0100 git:c992efd06a
(XEN) build-id: 85d361b8565b90d4e0defe2beb2419e191fd76b4
(XEN) CPU0: Unexpected Trap: Undefined Instruction
(XEN) [ Xen-4.15-unstable  arm32  debug=y   Not tainted ]
(XEN) CPU:0
(XEN) PC: 0026b8c8 identify_cpu+0xc0/0xd4
(XEN) CPSR:   61da MODE:Hypervisor
(XEN)  R0: 002acb20 R1:  R2:  R3: 
(XEN)  R4: 002acb1c R5: 002acb20 R6: 4e00 R7: 
(XEN)  R8: 0002 R9: 002d8200 R10:8000 R11:002f7e6c R12:0080
(XEN) HYP: SP: 002f7e68 LR: 002c419c
(XEN)
(XEN)   VTCR_EL2: 80002646
(XEN)  VTTBR_EL2: 0018e628bb80
(XEN)
(XEN)  SCTLR_EL2: 30cd187f
(XEN)HCR_EL2: 0038
(XEN)  TTBR0_EL2: 4013a000
(XEN)
(XEN)ESR_EL2: 
(XEN)  HPFAR_EL2: 0003fff0
(XEN)  HDFAR: 9d11
(XEN)  HIFAR: a04a
(XEN)
(XEN) Xen stack trace from sp=002f7e68:
(XEN) 002f7f54 8000  2000 002a4584 

(XEN) 8000 49ff5000 002d81f0 4000  2000
0001
(XEN) 5000 49ffd000  5000  
5000
(XEN)4c00  4e00    5000

(XEN)5000  5000    

(XEN) 0003    0040 

(XEN)   002a7000 40008050 001a 
49ff5000
(XEN)40008000 3fe08000 0004 0020006c   

(XEN)      

(XEN)      

(XEN)      

(XEN)      

(XEN)     
(XEN) Xen call trace:
(XEN)[<0026b8c8>] identify_cpu+0xc0/0xd4 (PC)
(XEN)[<002c419c>] start_xen+0x778/0xe50 (LR)
(XEN)[<002f7f54>] 002f7f54
(XEN)
(XEN)
(XEN) 
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Undefined Instruction
(XEN) 
(XEN)
(XEN) Reboot in five seconds...


The code in question:
  26b8c0:   eef63a10vmrsr3, mvfr1
  26b8c4:   e5803058str r3, [r0, #88]   ; 0x58
> 26b8c8:   eef53a10vmrsr3, mvfr2
  26b8cc:   e580305cstr r3, [r0, #92]   ; 0x5c
  26b8d0:   e28bd000add sp, fp, #0
  26b8d4:   e49db004pop {fp}   ; (ldr fp, [sp], #4)
  26b8d8:   e12fff1ebx  lr

And indeed MVFR2 is not mentioned in the ARMv7 ARM, and in contrast to
the CP15 CPUID registers this is using the VMRS instruction, so it's not
protected by future-proof CPUID register scheme.

Not sure what to do about this, maybe #ifdef'ing this register access
with arm64?
I guess this comes from the slightly too optimistic code-sharing between
arm32 and arm64?

Cheers,
Andre

> On 17/12/2020 15:38, Bertrand Marquis wrote:
>> The goal of this serie is to emulate coprocessor ID registers so that
>> Xen only publish to guest features that are supported by Xen and can
>> actually be used by guests.
>> One practical example where this is required are SVE support which is
>> forbidden by Xen as it is not supported, but if Linux is compiled with
>> it, it will crash on boot. An other one is AMU which is also forbidden
>> by Xen but one Linux compiled with it would crash if the platform
>> supports it.
>>
>> To be able to emulate the coprocessor registers defining what featu

Re: [PATCH 0/4] xen/arm: Unbreak ACPI

[André Przywara]

On 29/09/2020 22:11, Alex Bennée wrote:

Hi,

> Julien Grall  writes:
> 
>> Hi Alex,
>>
>> On 29/09/2020 16:29, Alex Bennée wrote:
>>>
>>> Julien Grall  writes:
>>>
 From: Julien Grall 

 Hi all,

 Xen on ARM has been broken for quite a while on ACPI systems. This
 series aims to fix it.

 Unfortunately I don't have a system with ACPI v6.0 or later (QEMU seems
 to only support 5.1).

Does QEMU provide ACPI tables? Or is this actually EDK2 generating them?

> So I did only some light testing.

So about that v6.0 or later: it seems like the requirement comes from:
commit 1c9bd43019cd "arm/acpi: Parse FADT table and get PSCI flags":
"Since STAO table and the GIC version are introduced by ACPI 6.0, we
will check the version and only parse FADT table with version >= 6.0. If
firmware provides ACPI tables with ACPI version less than 6.0, OS will
be messed up with those information, so disable ACPI if we get an FADT
table with version less than 6.0."

STAO (and XENV) have indeed been introduced by ACPI v6.0, but those
tables are supposed to be *generated* by Xen and handed down to Dom0,
they will never be provided by firmware (or parsed) by the Xen
hypervisor. Checking the Linux code it seems to actually not (yet?)
support the STAO named list, and currently finds and uses the STAO (UART
block bit) regardless of the FADT version number.

I can't find anything GIC related in the FADT, the whole GIC information
is described in MADT.

Linux/arm64 seems to be happy with ACPI >= v5.1:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/kernel/acpi.c#n148

So can we relax the v6.0 requirement, to be actually >= v5.1? That is
among the first to support ARM anyway, IIRC.

I have only a smattering about ACPI, so happy to stand corrected.

Cheers,
Andre

>>>
>>> I was hoping to get more diagnostics out to get it working under QEMU
>>> TCG so I think must of missed a step:
>>>
>>>Loading Xen 4.15-unstable ...
>>>Loading Linux 4.19.0-11-arm64 ...
>>>Loading initial ramdisk ...
>>>Using modules provided by bootloader in FDT
>>>Xen 4.15-unstable (c/s Sat Sep 26 21:55:42 2020 +0100 git:72f3d495d0) 
>>> EFI loader
>>>...silence...
>>>
>>> I have a grub installed from testing on a buster base:
>>>
>>>dpkg --status grub-arm64-efi
>>>Version: 2.04-8
>>>
>>> With:
>>>
>>>GRUB_CMDLINE_LINUX_DEFAULT=""
>>>GRUB_CMDLINE_LINUX="console=ttyAMA0"
>>>GRUB_CMDLINE_LINUX_XEN_REPLACE="console=hvc0 earlyprintk=xen"
>>>GRUB_CMDLINE_XEN="loglvl=all guest_loglvl=all 
>>> com1=115200,8n1,0x3e8,5console=com1,vg"
>>>
>>> And I built Xen with --enable-systemd and tweaked the hypervisor .config:
>>>
>>>CONFIG_EXPERT=y
>>>CONFIG_ACPI=y
>>>
>>> So any pointers to make it more verbose would be helpful.
>>
>> The error is hapenning before Xen setup the console. You can get early 
>> output on QEMU if you rebuild Xen with the following .config options:
>>
>> CONFIG_DEBUG=y
>> CONFIG_EARLY_UART_CHOICE_PL011=y
>> CONFIG_EARLY_UART_PL011=y
>> CONFIG_EARLY_PRINTK=y
>> CONFIG_EARLY_UART_BASE_ADDRESS=0x0900
>> CONFIG_EARLY_UART_PL011_BAUD_RATE=0
>> CONFIG_EARLY_PRINTK_INC="debug-pl011.inc"
> 
> OK I can see it fails on the ACPI and then tries to fall back to FDT and
> then fails to find the GIC:
> 
>   (XEN) CMDLINE[f7bbe000]:chosen placeholder 
> root=UUID=cf00cd3a-066b-4146-bedf-f811d3343077 ro console=hvc0 earlyprintk=xen
>   (XEN)
>   (XEN) Command line: placeholder loglvl=all guest_loglvl=all 
> com1=115200,8n1,0x3e8,5console=com1,vg no-real-mode edd=off
>   (XEN) parameter "placeholder" unknown!
>   (XEN) parameter "no-real-mode" unknown!
>   (XEN) parameter "edd" unknown!
>   (XEN) ACPI: RSDP 13856, 0024 (r2 BOCHS )
>   (XEN) ACPI: XSDT 13855, 004C (r1 BOCHS  BXPCFACP1   113)
>   (XEN) ACPI: FACP 13851, 010C (r5 BOCHS  BXPCFACP1 BXPC1)
>   (XEN) ACPI: DSDT 13852, 14A6 (r2 BOCHS  BXPCDSDT1 BXPC1)
>   (XEN) ACPI: APIC 13850, 018C (r3 BOCHS  BXPCAPIC1 BXPC1)
>   (XEN) ACPI: GTDT 1384F, 0060 (r2 BOCHS  BXPCGTDT1 BXPC1)
>   (XEN) ACPI: MCFG 1384E, 003C (r1 BOCHS  BXPCMCFG1 BXPC1)
>   (XEN) ACPI: SPCR 1384D, 0050 (r2 BOCHS  BXPCSPCR1 BXPC1)
>   (XEN) Unsupported FADT revision 5.1, should be 6.0+, will disable ACPI
>   (XEN) acpi_boot_table_init: FADT not found (-22)
>   (XEN) Domain heap initialised
>   (XEN) Booting using Device Tree
>   (XEN) Platform: Generic System
>   (XEN)
>   (XEN) 
>   (XEN) Panic on CPU 0:
>   (XEN) Unable to find compatible GIC in the device tree
>   (XEN) 
>   (XEN)
>   (XEN) Reboot in five seconds...
> 
> Despite saying it is going to reboot it never manages to. Any idea how
> it is trying to reset the system?
> 

Re: DT with memory bank of size 0 (WAS: Re: AW: AW: Colibri imx8qxp: Missing kernel boot module)

[André Przywara]

On 17/09/2020 01:31, Stefano Stabellini wrote:

Hi,

> On Wed, 16 Sep 2020, Julien Grall wrote:
>> On 14/09/2020 15:26, Daniel Wagner2 wrote:
>>> Hi Julien,
>>
>> Hi Daniel,
>>
>> I am moving the thread to xen-devel and adding a couple of more folks.
>>

>
> this is the full version of the fdt that threw the error:
> https://pastebin.com/63TZ9z3k
> The problematic memory node appears in line 126

 Thanks! The output looks corrupted as some of the lines are not valid DTB:

 fsl,pins = * 0x9300184c [0x0048];

 Although, I am not sure if it is just U-boot dumping the DTB differently.

 Anyway, after removing the "corrupted" line, I managed to get a compile
>>> the
 DTB. I don't have a Colibri IMX8QXP. However, given this is an early
>>> parsing
 error, I have just embed the DTB in Xen binary via CONFIG_DTB_FILE.

 Unfortunately I couldn't reproduce your error. This either suggests the
>>> DTB gets
 corrupted or Xen doesn't access the DTB with the correct memory attribute.

 Do you have the DTB in hand?
>>>
>>> Sorry for the corrupted version, I've uploaded the DTB
>>> (fsl-imx8qxp-colibri-eval-v3.dtb) to
>>> https://drive.google.com/drive/folders/1jbpnz35sC0NbCyEjrkLqelBsKBztW1S6?usp
>>> =sharing
>>>
>>> I have also uploaded my modified xen source files.
>>> 1. arch/arm/bootfdt.c
>>> where I have added the additional printk's seen in the log and
>>> 2. arch/arm/setup.c
>>> where I rerun the devicetree parser in line 935 to get the logs, since the
>>> console is not yet initialised when the function is called for the first
>>> time and I
>>> didn't manage to enable earlyprintk.
>>>
>>> I think the breaking point is the second memory bank which appears in the
>>> logs (see the output line marked with "!!")  with start=0x8 8000  and
>>> size=0.
>>> It isn't specified in the DTB, so I am not sure where this comes from.
>>> It has size=0 so
>>> if ( !size )
>>>  {
>>>  printk("invalid size, bank %d\n",i);
>>>  return -EINVAL;
>>>  }
>>> In bootfdt.c makes the function stop.
>>>
>>> Log:
>>> (XEN) arch/arm/bootfdt.c: early_scan_node
>>> (XEN) -> fdt: node `memory@8000': parsing
>>> (XEN) -> process_memory_node
>>> (XEN)
>>> (XEN) arch/arm/bootfdt.c: process_memory_node
>>> (XEN) ->found memory:reg
>>> (XEN) ->cell=
>>> (XEN) ->banks=2
>>> (XEN) ->mem->nr_banks=1
>>> (XEN) ->NR_MEM_BANKS=128
>>> (XEN) ->start=0x8020 size=0x7fe0
>>> !! (XEN) ->start=0x88000 size=0
>>> (XEN) invalid size, bank 1
>>> (XEN) END of arch/arm/bootfdt.c: process_memory_node
>>
>> When I tried to run it on the model I get:
>>
>> (XEN) device_tree_for_each_node: memory@8000
>> (XEN)
>> (XEN) arch/arm/bootfdt.c: early_scan_node
>> (XEN) -> fdt: node `memory@8000': parsing
>> (XEN) -> process_memory_node
>> (XEN)
>> (XEN) arch/arm/bootfdt.c: process_memory_node
>> (XEN) ->found memory:reg
>> (XEN) ->cell=
>> (XEN) ->banks=1
>> (XEN) ->mem->nr_banks=0
>> (XEN) ->NR_MEM_BANKS=128
>> (XEN) ->start=0x8000 size=0x4000
>> (XEN) END of arch/arm/bootfdt.c: process_memory_node
>>
>>>
>>> Btw 8_8000_ is the start address of this systems DDR Main memory,
>>> according to the Reference Manual of the i.MX8QXP.
>> I couldn't find this value in the DT. It is possible that U-boot is modifying
>> the memory node before jumping to Xen (or Linux).

U-Boot almost always rewrites the memory node, filling in its own view
of DRAM. It's actually not that easy to avoid that.
This is in U-Boot's colibri-imx8x.h:
#define CONFIG_SYS_SDRAM_BASE   0x8000
#define PHYS_SDRAM_10x8000
#define PHYS_SDRAM_20x88000
#define PHYS_SDRAM_1_SIZE   SZ_2G   /* 2 GB */
#define PHYS_SDRAM_2_SIZE   0x  /* 0 GB */

The Colibri iMX8X SoM seems to support "Up to 2GB LPDDR4", so not sure
why we have the upper memory here at all. It could just set
CONFIG_NR_DRAM_BANKS to 1 and be done. But anyway ...

>> Looking at Linux, they seem to ignore any bank with size == 0. I am starting
>> to wonder whether your DT is (ab)using it.
>>
>> Do you have Linux running on baremetal on this board? If so would you mind to
>> dump the DT from the userspace (via /proc/device-tree) this time?
>>
>> In any case, we may want to relax the check in Xen. Any opinions?
> 
> Yeah, ignoring a bank with size=0 is fine. I checked the epapr and it

Not sure you meant this, but the official DT spec replaced ePAPR for a
while now: https://www.devicetree.org/specifications/
It has been heavily updated and even got some bug fixes.

> doesn't specify that size=0 is invalid, so I think it is actually better
> to ignore it and continue even from a spec perspective.

Yeah, I don't see any harm as well with ignoring 0 sized banks.

Just not sure if that has any implication with the number of memory
banks that Xen wants to deal with (it if counts two, but there is only
one valid

Re: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU context switch

[André Przywara]

On 18/08/2020 11:13, Bertrand Marquis wrote:

Hi,

>> On 18 Aug 2020, at 10:42, André Przywara  wrote:
>>
>> On 18/08/2020 10:25, Bertrand Marquis wrote:
>>
>> Hi,
>>
>>>> On 18 Aug 2020, at 10:14, André Przywara  wrote:
>>>>
>>>> On 18/08/2020 04:11, Wei Chen wrote:
>>>>
>>>> Hi Wei,
>>>>
>>>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
>>>>> FP/SIMD or not. But currently, this two MACROs only consider value 0
>>>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
>>>>> that support FP/SIMD and half-precision floating-point features, the
>>>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
>>>>> no FP/SIMD support. In this case, the vfp_save/restore_state will not
>>>>> take effect.
>>>>>
>>>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
>>>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
>>>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
>>>>> platforms, Xen will always miss the float pointer registers save/restore.
>>>>> If different vCPUs are running on the same pCPU, the float pointer
>>>>> registers will be corrupted randomly.
>>>>
>>>> That's a good catch, thanks for working this out!
>>>>
>>>> One thing below...
>>>>
>>>>> This patch fixes Xen on these new cores.
>>>>>
>>>>> Signed-off-by: Wei Chen 
>>>>> ---
>>>>> xen/include/asm-arm/cpufeature.h | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/include/asm-arm/cpufeature.h 
>>>>> b/xen/include/asm-arm/cpufeature.h
>>>>> index 674beb0353..588089e5ae 100644
>>>>> --- a/xen/include/asm-arm/cpufeature.h
>>>>> +++ b/xen/include/asm-arm/cpufeature.h
>>>>> @@ -13,8 +13,8 @@
>>>>> #define cpu_has_el2_64(boot_cpu_feature64(el2) >= 1)
>>>>> #define cpu_has_el3_32(boot_cpu_feature64(el3) == 2)
>>>>> #define cpu_has_el3_64(boot_cpu_feature64(el3) >= 1)
>>>>> -#define cpu_has_fp(boot_cpu_feature64(fp) == 0)
>>>>> -#define cpu_has_simd  (boot_cpu_feature64(simd) == 0)
>>>>> +#define cpu_has_fp(boot_cpu_feature64(fp) <= 1)
>>>>> +#define cpu_has_simd  (boot_cpu_feature64(simd) <= 1)
>>>>
>>>> But this is only good until the next feature bump. I think we should be
>>>> more future-proof here. The architecture describes those two fields as
>>>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
>>>> feature. Which means we are good as long as the sign bit (bit 3) is
>>>> clear, which translates into:
>>>> #define cpu_has_fp(boot_cpu_feature64(fp) < 8)
>>>> Same for simd.
>>>>
>>>
>>> We cannot really be sure that a new version introduced will require the
>>> same context save/restore so it might dangerous to claim we support
>>> something we have no idea about.
>>
>> I am pretty sure we can, because this is what the FP feature describes.
>> If a feature bump would introduce a larger state to be saved and
>> restored, that would be covered by a new field, look at AdvSIMD and SVE
>> for examples.
>> The feature number would only be bumped if it's compatible:
>> 
>> · The field holds a signed value.
>> · The field value 0xF indicates that the feature is not implemented.
>> · The field value 0x0 indicates that the feature is implemented.
>> · Software that depends on the feature can use the test:
>>  if value >= 0 {  // Software features that depend on the presence
>> of the hardware feature }
>> 
>> (ARMv8 ARM D13.1.3)
>>
>> And this is how Linux handles this.
> 
> Then changing the code to use <8 should be ok.

Thanks. Another angle to look at this:
Using "< 8" will never be worse than "<= 1", since we only derive the
existence of the floating point registers from it. The moment we see a 2
in this register field, the "<= 1" would definitely fail to save/restore
the FP registers again. But the ARM ARM guarantees that those registers
are still around (since "value >= 0" hits, so the feature is present, as
shown above).
The theoretical worst case with "< 8" would be that it would not cover
*enough* state, but as described above this will never happen, with this
particular FP/SIMD field.

Cheers,
Andre

>>> I agree though about the analysis on the fact that values under 8 should
>>> be valid but only 0 and 1 currently exist [1], other values are reserved.
>>>
>>> So I would vote to keep the 1 for now there.
>>>
>>> Cheers
>>> Bertrand
>>>
>>> [1] 
>>> https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/id_aa64pfr0_el1
> 

Re: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU context switch

[André Przywara]

On 18/08/2020 10:25, Bertrand Marquis wrote:

Hi,

>> On 18 Aug 2020, at 10:14, André Przywara  wrote:
>>
>> On 18/08/2020 04:11, Wei Chen wrote:
>>
>> Hi Wei,
>>
>>> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
>>> FP/SIMD or not. But currently, this two MACROs only consider value 0
>>> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
>>> that support FP/SIMD and half-precision floating-point features, the
>>> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
>>> no FP/SIMD support. In this case, the vfp_save/restore_state will not
>>> take effect.
>>>
>>> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
>>> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
>>> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
>>> platforms, Xen will always miss the float pointer registers save/restore.
>>> If different vCPUs are running on the same pCPU, the float pointer
>>> registers will be corrupted randomly.
>>
>> That's a good catch, thanks for working this out!
>>
>> One thing below...
>>
>>> This patch fixes Xen on these new cores.
>>>
>>> Signed-off-by: Wei Chen 
>>> ---
>>> xen/include/asm-arm/cpufeature.h | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/include/asm-arm/cpufeature.h 
>>> b/xen/include/asm-arm/cpufeature.h
>>> index 674beb0353..588089e5ae 100644
>>> --- a/xen/include/asm-arm/cpufeature.h
>>> +++ b/xen/include/asm-arm/cpufeature.h
>>> @@ -13,8 +13,8 @@
>>> #define cpu_has_el2_64(boot_cpu_feature64(el2) >= 1)
>>> #define cpu_has_el3_32(boot_cpu_feature64(el3) == 2)
>>> #define cpu_has_el3_64(boot_cpu_feature64(el3) >= 1)
>>> -#define cpu_has_fp(boot_cpu_feature64(fp) == 0)
>>> -#define cpu_has_simd  (boot_cpu_feature64(simd) == 0)
>>> +#define cpu_has_fp(boot_cpu_feature64(fp) <= 1)
>>> +#define cpu_has_simd  (boot_cpu_feature64(simd) <= 1)
>>
>> But this is only good until the next feature bump. I think we should be
>> more future-proof here. The architecture describes those two fields as
>> "signed"[1], and guarantees that "if value >= 0" is a valid test for the
>> feature. Which means we are good as long as the sign bit (bit 3) is
>> clear, which translates into:
>> #define cpu_has_fp(boot_cpu_feature64(fp) < 8)
>> Same for simd.
>>
> 
> We cannot really be sure that a new version introduced will require the
> same context save/restore so it might dangerous to claim we support
> something we have no idea about.

I am pretty sure we can, because this is what the FP feature describes.
If a feature bump would introduce a larger state to be saved and
restored, that would be covered by a new field, look at AdvSIMD and SVE
for examples.
The feature number would only be bumped if it's compatible:

· The field holds a signed value.
· The field value 0xF indicates that the feature is not implemented.
· The field value 0x0 indicates that the feature is implemented.
· Software that depends on the feature can use the test:
  if value >= 0 {  // Software features that depend on the presence
of the hardware feature }

(ARMv8 ARM D13.1.3)

And this is how Linux handles this.

Cheers,
Andre

> I agree though about the analysis on the fact that values under 8 should
> be valid but only 0 and 1 currently exist [1], other values are reserved.
> 
> So I would vote to keep the 1 for now there.
> 
> Cheers
> Bertrand
> 
> [1] 
> https://developer.arm.com/docs/ddi0595/h/aarch64-system-registers/id_aa64pfr0_el1
> 

Re: [PATCH] xen/arm: Missing N1/A76/A75 FP registers in vCPU context switch

[André Przywara]

On 18/08/2020 04:11, Wei Chen wrote:

Hi Wei,

> Xen has cpu_has_fp/cpu_has_simd to detect whether the CPU supports
> FP/SIMD or not. But currently, this two MACROs only consider value 0
> of ID_AA64PFR0_EL1.FP/SIMD as FP/SIMD features enabled. But for CPUs
> that support FP/SIMD and half-precision floating-point features, the
> ID_AA64PFR0_EL1.FP/SIMD are 1. For these CPUs, xen will treat them as
> no FP/SIMD support. In this case, the vfp_save/restore_state will not
> take effect.
> 
> Unfortunately, Cortex-N1/A76/A75 are the CPUs support FP/SIMD and
> half-precision floatiing-point. Their ID_AA64PFR0_EL1.FP/SMID are 1
> (see Arm ARM DDI0487F.b, D13.2.64). In this case, on N1/A76/A75
> platforms, Xen will always miss the float pointer registers save/restore.
> If different vCPUs are running on the same pCPU, the float pointer
> registers will be corrupted randomly.

That's a good catch, thanks for working this out!

One thing below...

> This patch fixes Xen on these new cores.
> 
> Signed-off-by: Wei Chen 
> ---
>  xen/include/asm-arm/cpufeature.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/cpufeature.h 
> b/xen/include/asm-arm/cpufeature.h
> index 674beb0353..588089e5ae 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -13,8 +13,8 @@
>  #define cpu_has_el2_64(boot_cpu_feature64(el2) >= 1)
>  #define cpu_has_el3_32(boot_cpu_feature64(el3) == 2)
>  #define cpu_has_el3_64(boot_cpu_feature64(el3) >= 1)
> -#define cpu_has_fp(boot_cpu_feature64(fp) == 0)
> -#define cpu_has_simd  (boot_cpu_feature64(simd) == 0)
> +#define cpu_has_fp(boot_cpu_feature64(fp) <= 1)
> +#define cpu_has_simd  (boot_cpu_feature64(simd) <= 1)

But this is only good until the next feature bump. I think we should be
more future-proof here. The architecture describes those two fields as
"signed"[1], and guarantees that "if value >= 0" is a valid test for the
feature. Which means we are good as long as the sign bit (bit 3) is
clear, which translates into:
#define cpu_has_fp(boot_cpu_feature64(fp) < 8)
Same for simd.

Cheers,
Andre.

[1] ARMv8 ARM (ARM DDI 0487F.b), section D13.1.3

>  #define cpu_has_gicv3 (boot_cpu_feature64(gic) == 1)
>  #endif
>  
> 

Re: dom0 LInux 5.8-rc5 kernel failing to initialize cooling maps for Allwinner H6 SoC

[André Przywara]

On 28/07/2020 19:52, Christopher Clark wrote:

Hi Christopher,

wow, this quickly got out of hand. I never meant to downplay anyone's
work here, but on this particular platform some things might look a bit
different than normal. See below.

> On Tue, Jul 28, 2020 at 11:16 AM Stefano Stabellini
>  wrote:
>>
>> On Tue, 28 Jul 2020, André Przywara wrote:
>>> On 28/07/2020 11:39, Alejandro wrote:
>>>> Hello,
>>>>
>>>> El dom., 26 jul. 2020 a las 22:25, André Przywara
>>>> () escribió:
>>>>> So this was actually my first thought: The firmware (U-Boot SPL) sets up
>>>>> some basic CPU frequency (888 MHz for H6 [1]), which is known to never
>>>>> overheat the chip, even under full load. So any concern from your side
>>>>> about the board or SoC overheating could be dismissed, with the current
>>>>> mainline code, at least. However you lose the full speed, by quite a
>>>>> margin on the H6 (on the A64 it's only 816 vs 1200(ish) MHz).
>>>>> However, without the clock entries in the CPU node, the frequency would
>>>>> never be changed by Dom0 anyway (nor by Xen, which doesn't even know how
>>>>> to do this).
>>>>> So from a practical point of view: unless you hack Xen to pass on more
>>>>> cpu node properties, you are stuck at 888 MHz anyway, and don't need to
>>>>> worry about overheating.
>>>> Thank you. Knowing that at least it won't overheat is a relief. But
>>>> the performance definitely suffers from the current situation, and
>>>> quite a bit. I'm thinking about using KVM instead: even if it does
>>>> less paravirtualization of guests,
>>>
>>> What is this statement based on? I think on ARM this never really
>>> applied, and in general whether you do virtio or xen front-end/back-end
>>> does not really matter.
> 
> When you say "in general" here, this becomes a very broad statement
> about virtio and xen front-end/back-ends being equivalent and
> interchangable, and that could cause some misunderstanding for a
> newcomer.
> 
> There are important differences between the isolation properties of
> classic virtio and Xen's front-end/back-ends -- and also the Argo
> transport. It's particularly important for Xen because it has
> priortized support for stronger isolation between execution
> environments to a greater extent than some other hypervisors. It is a
> critical differentiator for it. The importance of isolation is why Xen
> 4.14's headline feature was support for Linux stubdomains, upstreamed
> to Xen after years of work by the Qubes and OpenXT communities.

He was talking about performance. My take on this was that this seems to
go back to the old days, when Xen was considered faster because of
paravirt (vs. trap&emulate h/w in QEMU). And this clearly does not apply
anymore, and never really applied to ARM.

>>> IMHO any reasoning about performance just based
>>> on software architecture is mostly flawed (because it's complex and
>>> reality might have missed some memos ;-)
> 
> That's another pretty strong statement. Measurement is great, but
> maybe performance analysis that is informed and directed by an
> understanding of the architecture under test could potentially be more
> rigorous and persuasive than work done without it?

You seem to draw quite a lot from my statement. All I was saying that
modern systems are far too complex to reason about actual performance
based on some architectural ideas.
Also my statement was in response to some generic statement, but of
course in this particular context. Please keep in mind that we are
talking about a 5 US$ TV-box SoC here, basically a toy platform. The
chip has severe architectural issues (secure devices not being secure,
critical devices not being isolated). I/O probably means SD card at
about 25MB/s, the fastest I have seen is 80MB/s on some better (but
optional!) eMMC modules. DRAM is via a single channel 32bit path. The
cores are using an almost 8 year old energy-efficient
micro-architecture. So whether any clever architecture really
contributes to performance on this system is somewhat questionable.

So I was suggesting that before jumping to conclusions based on broad
architectural design ideas an actual reality check of whether those
really apply to the platform might be warranted.
Also I haven't seen what kind of performance he is actually interested
in. Is the task at hand I/O bound, memory bound, CPU bound?
The discussion so far was about the CPU clock frequency only.

>>> So just measure your particular use case, then you know.
> 
&

Re: dom0 LInux 5.8-rc5 kernel failing to initialize cooling maps for Allwinner H6 SoC

[André Przywara]

On 28/07/2020 11:39, Alejandro wrote:
> Hello,
> 
> El dom., 26 jul. 2020 a las 22:25, André Przywara
> () escribió:
>> So this was actually my first thought: The firmware (U-Boot SPL) sets up
>> some basic CPU frequency (888 MHz for H6 [1]), which is known to never
>> overheat the chip, even under full load. So any concern from your side
>> about the board or SoC overheating could be dismissed, with the current
>> mainline code, at least. However you lose the full speed, by quite a
>> margin on the H6 (on the A64 it's only 816 vs 1200(ish) MHz).
>> However, without the clock entries in the CPU node, the frequency would
>> never be changed by Dom0 anyway (nor by Xen, which doesn't even know how
>> to do this).
>> So from a practical point of view: unless you hack Xen to pass on more
>> cpu node properties, you are stuck at 888 MHz anyway, and don't need to
>> worry about overheating.
> Thank you. Knowing that at least it won't overheat is a relief. But
> the performance definitely suffers from the current situation, and
> quite a bit. I'm thinking about using KVM instead: even if it does
> less paravirtualization of guests,

What is this statement based on? I think on ARM this never really
applied, and in general whether you do virtio or xen front-end/back-end
does not really matter. IMHO any reasoning about performance just based
on software architecture is mostly flawed (because it's complex and
reality might have missed some memos ;-) So just measure your particular
use case, then you know.

> I'm sure that the ability to use
> the maximum frequency of the CPU would offset the additional overhead,
> and in general offer better performance. But with KVM I lose the
> ability to have individual domU's dedicated to some device driver,
> which is a nice thing to have from a security standpoint.

I understand the theoretical merits, but a) does this really work on
your board and b) is this really more secure? What do you want to
protect against?

>> Now if you would pass on the CPU clock frequency control to Dom0, you
>> run into more issues: the Linux governors would probably try to setup
>> both frequency and voltage based on load, BUT this would be Dom0's bogus
>> perception of the actual system load. Even with pinned Dom0 vCPUs, a
>> busy system might spend most of its CPU time in DomU VCPUs, which
>> probably makes it look mostly idle in Dom0. Using a fixed governor
>> (performance) would avoid this, at the cost of running full speed all of
>> the time, probably needlessly.
>>
>> So fixing the CPU clocking issue is more complex and requires more
>> ground work in Xen first, probably involving some enlightenend Dom0
>> drivers as well. I didn't follow latest developments in this area, nor
>> do I remember x86's answer to this, but it's not something easy, I would
>> presume.
> I understand, thanks :). I know that recent Intel CPUs (from Sandy
> Bridge onwards) use P-states to manage frequencies, and even have a
> mode of operation that lets the CPU select the P-states by itself. On
> older processors, Xen can probably rely on ACPI data to do the
> frequency scaling. But the most similar "standard thing" that my board
> has, a AR100 coprocessor that with the (work in progress) Crust
> firmware can be used with SCMI, doesn't even seem to support the use
> case of changing CPU frequency... and SCMI is the most promising
> approach for adding DVFS support in Xen for ARM, according to this
> previous work: 
> https://www.slideshare.net/xen_com_mgr/xpdds18-cpufreq-in-xen-on-arm-oleksandr-tyshchenko-epam-systems

So architecturally you could run all cores at full speed, always, and
tell Crust to clock down / decrease voltage once a thermal condition
triggers. That's not power-saving, but at least should be relatively safe.
On Allwinner platforms this isn't really bullet-proof, though, since the
THS device is non-secure, so anyone with access to the MMIO region could
turn it off. Or Dom0 could just turn the THS clock off - which it
actually does, because it's not used.
In the end it's a much bigger discussion about doing those things in
firmware or in the OS. For those traditionally embedded platforms like
Allwinner there is a huge fraction that does not trust firmware,
unfortunately, so moving responsibility to firmware is not very popular
upstream (been there, done that).

>> Alejandro: can you try to measure the actual CPU frequency in Dom0?
>> Maybe some easy benchmark? "mhz" from lmbench does a great job in
>> telling you the actual frequency, just by clever measurement. But any
>> other CPU bound benchmark would do, if you compare bare metal Linux vs.
>

Re: dom0 LInux 5.8-rc5 kernel failing to initialize cooling maps for Allwinner H6 SoC

[André Przywara]

On 24/07/2020 12:20, Alejandro wrote:

Hi,

> El vie., 24 jul. 2020 a las 12:45, Julien Grall () escribió:
>>> I'm trying Xen 4.13.1 in a Allwinner H6 SoC (more precisely a Pine H64
>>> model B, with a ARM Cortex-A53 CPU).
>>> I managed to get a dom0 Linux 5.8-rc5 kernel running fine, unpatched,
>>> and I'm using the upstream device tree for
>>> my board. However, the dom0 kernel has trouble when reading some DT
>>> nodes that are related to the CPUs, and
>>> it can't initialize the thermal subsystem properly, which is a kind of
>>> showstopper for me, because I'm concerned
>>> that letting the CPU run at the maximum frequency without watching out
>>> its temperature may cause overheating.
>>
>> I understand this concern, I am aware of some efforts to get CPUFreq
>> working on Xen but I am not sure if there is anything available yet. I
>> have CCed a couple of more person that may be able to help here.
> 
> Thank you for the CCs. I hope they can bring on some insight about this :)
> 
>>> The relevant kernel messages are:
>>>
>>> [  +0.001959] sun50i-cpufreq-nvmem: probe of sun50i-cpufreq-nvmem
>>> failed with error -2
>>> ...
>>> [  +0.003053] hw perfevents: failed to parse interrupt-affinity[0] for pmu
>>> [  +0.43] hw perfevents: /pmu: failed to register PMU devices!
>>> [  +0.37] armv8-pmu: probe of pmu failed with error -22
>>
>> I am not sure the PMU failure is related to the thermal failure below.
> 
> I'm not sure either, but after comparing the kernel messages for a
> boot with and without Xen, those were the differences (excluding, of
> course, the messages that inform that the Xen hypervisor console is
> being used and such). For the sake of completeness, I decided to
> mention it anyway.
> 
>>> [  +0.000163] OF: /thermal-zones/cpu-thermal/cooling-maps/map0: could
>>> not find phandle
>>> [  +0.63] thermal_sys: failed to build thermal zone cpu-thermal: -22
>> Would it be possible to paste the device-tree node for
>> /thermal-zones/cpu-thermal/cooling-maps? I suspect the issue is because
>> we recreated /cpus from scratch.
>>
>> I don't know much about how the thermal subsystem works, but I suspect
>> this will not be enough to get it working properly on Xen. For a
>> workaround, you would need to create a dom0 with the same numbers of
>> vCPU as the numbers of pCPUs. They would also need to be pinned.
>>
>> I will leave the others to fill in more details.
> 
> I think I should mention that I've tried to hackily fix things by
> removing the make_cpus_node call on handle_node
> (https://github.com/xen-project/xen/blob/master/xen/arch/arm/domain_build.c#L1585),
> after removing the /cpus node from the skip_matches array. This way,
> the original /cpus node was passed through, without being recreated by
> Xen. Of course, I made sure that dom0 used the same number of vCPUs as
> pCPUs, because otherwise things would probably blow up, which luckily
> that was not a compromise for me. The end result was that the
> aforementioned kernel error messages were gone, and the thermal
> subsystem worked fine again. However, this time the cpufreq-dt probe
> failed, with what I think was an ENODEV error. This left the CPU
> locked at the boot frequency of less than 1 GHz, compared to the
> maximum 1.8 GHz frequency that the SoC supports, which has bad
> implications for performance.

So this was actually my first thought: The firmware (U-Boot SPL) sets up
some basic CPU frequency (888 MHz for H6 [1]), which is known to never
overheat the chip, even under full load. So any concern from your side
about the board or SoC overheating could be dismissed, with the current
mainline code, at least. However you lose the full speed, by quite a
margin on the H6 (on the A64 it's only 816 vs 1200(ish) MHz).
However, without the clock entries in the CPU node, the frequency would
never be changed by Dom0 anyway (nor by Xen, which doesn't even know how
to do this).
So from a practical point of view: unless you hack Xen to pass on more
cpu node properties, you are stuck at 888 MHz anyway, and don't need to
worry about overheating.

Now if you would pass on the CPU clock frequency control to Dom0, you
run into more issues: the Linux governors would probably try to setup
both frequency and voltage based on load, BUT this would be Dom0's bogus
perception of the actual system load. Even with pinned Dom0 vCPUs, a
busy system might spend most of its CPU time in DomU VCPUs, which
probably makes it look mostly idle in Dom0. Using a fixed governor
(performance) would avoid this, at the cost of running full speed all of
the time, probably needlessly.

So fixing the CPU clocking issue is more complex and requires more
ground work in Xen first, probably involving some enlightenend Dom0
drivers as well. I didn't follow latest developments in this area, nor
do I remember x86's answer to this, but it's not something easy, I would
presume.

Alejandro: can you try to measure the actual CPU frequency in Dom0?
Maybe some easy benchmark? "

Re: Virtio in Xen on Arm (based on IOREQ concept)

[André Przywara]

On 21/07/2020 17:09, Oleksandr wrote:
> 
> On 21.07.20 17:58, André Przywara wrote:
>> On 21/07/2020 15:52, Oleksandr wrote:
>>> On 21.07.20 17:32, André Przywara wrote:
>>>> On 21/07/2020 14:43, Julien Grall wrote:
>>> Hello Andre, Julien
>>>
>>>
>>>>> (+ Andre)
>>>>>
>>>>> Hi Oleksandr,
>>>>>
>>>>> On 21/07/2020 13:26, Oleksandr wrote:
>>>>>> On 20.07.20 23:38, Stefano Stabellini wrote:
>>>>>>> For instance, what's your take on notifications with virtio-mmio?
>>>>>>> How
>>>>>>> are they modelled today? Are they good enough or do we need MSIs?
>>>>>> Notifications are sent from device (backend) to the driver (frontend)
>>>>>> using interrupts. Additional DM function was introduced for that
>>>>>> purpose xendevicemodel_set_irq_level() which results in
>>>>>> vgic_inject_irq() call.
>>>>>>
>>>>>> Currently, if device wants to notify a driver it should trigger the
>>>>>> interrupt by calling that function twice (high level at first, then
>>>>>> low level).
>>>>> This doesn't look right to me. Assuming the interrupt is trigger when
>>>>> the line is high-level, the backend should only issue the hypercall
>>>>> once
>>>>> to set the level to high. Once the guest has finish to process all the
>>>>> notifications the backend would then call the hypercall to lower the
>>>>> interrupt line.
>>>>>
>>>>> This means the interrupts should keep firing as long as the interrupt
>>>>> line is high.
>>>>>
>>>>> It is quite possible that I took some shortcut when implementing the
>>>>> hypercall, so this should be corrected before anyone start to rely on
>>>>> it.
>>>> So I think the key question is: are virtio interrupts level or edge
>>>> triggered? Both QEMU and kvmtool advertise virtio-mmio interrupts as
>>>> edge-triggered.
>>>>   From skimming through the virtio spec I can't find any explicit
>>>> mentioning of the type of IRQ, but the usage of MSIs indeed hints at
>>>> using an edge property. Apparently reading the PCI ISR status register
>>>> clears it, which again sounds like edge. For virtio-mmio the driver
>>>> needs to explicitly clear the interrupt status register, which again
>>>> says: edge (as it's not the device clearing the status).
>>>>
>>>> So the device should just notify the driver once, which would cause one
>>>> vgic_inject_irq() call. It would be then up to the driver to clear up
>>>> that status, by reading PCI ISR status or writing to virtio-mmio's
>>>> interrupt-acknowledge register.
>>>>
>>>> Does that make sense?
>>> When implementing Xen backend, I didn't have an already working example
>>> so only guessed. I looked how kvmtool behaved when actually triggering
>>> the interrupt on Arm [1].
>>>
>>> Taking into the account that Xen PoC on Arm advertises [2] the same irq
>>> type (TYPE_EDGE_RISING) as kvmtool [3] I decided to follow the model of
>>> triggering an interrupt. Could you please explain, is this wrong?
>> Yes, kvmtool does a double call needlessly (on x86, ppc and arm, mips is
>> correct).
>> I just chased it down in the kernel, a KVM_IRQ_LINE ioctl with level=low
>> is ignored when the target IRQ is configured as edge (which it is,
>> because the DT says so), check vgic_validate_injection() in the kernel.
>>
>> So you should only ever need one call to set the line "high" (actually:
>> trigger the edge pulse).
> 
> Got it, thanks for the explanation. Have just removed an extra action
> (setting low level) and checked.
> 

Just for the records: the KVM API documentation explicitly mentions:
"Note that edge-triggered interrupts require the level to be set to 1
and then back to 0." So kvmtool is just following the book.

Setting it to 0 still does nothing *on ARM*, and the x86 IRQ code is far
to convoluted to easily judge what's really happening here. For MSIs at
least it's equally ignored.

So I guess a clean implementation in Xen does not need two calls, but
some folks with understanding of x86 IRQ handling in Xen should confirm.

Cheers,
Andre.

Re: Virtio in Xen on Arm (based on IOREQ concept)

[André Przywara]

On 21/07/2020 15:52, Oleksandr wrote:
> 
> On 21.07.20 17:32, André Przywara wrote:
>> On 21/07/2020 14:43, Julien Grall wrote:
> 
> Hello Andre, Julien
> 
> 
>>> (+ Andre)
>>>
>>> Hi Oleksandr,
>>>
>>> On 21/07/2020 13:26, Oleksandr wrote:
>>>> On 20.07.20 23:38, Stefano Stabellini wrote:
>>>>> For instance, what's your take on notifications with virtio-mmio? How
>>>>> are they modelled today? Are they good enough or do we need MSIs?
>>>> Notifications are sent from device (backend) to the driver (frontend)
>>>> using interrupts. Additional DM function was introduced for that
>>>> purpose xendevicemodel_set_irq_level() which results in
>>>> vgic_inject_irq() call.
>>>>
>>>> Currently, if device wants to notify a driver it should trigger the
>>>> interrupt by calling that function twice (high level at first, then
>>>> low level).
>>> This doesn't look right to me. Assuming the interrupt is trigger when
>>> the line is high-level, the backend should only issue the hypercall once
>>> to set the level to high. Once the guest has finish to process all the
>>> notifications the backend would then call the hypercall to lower the
>>> interrupt line.
>>>
>>> This means the interrupts should keep firing as long as the interrupt
>>> line is high.
>>>
>>> It is quite possible that I took some shortcut when implementing the
>>> hypercall, so this should be corrected before anyone start to rely on
>>> it.
>> So I think the key question is: are virtio interrupts level or edge
>> triggered? Both QEMU and kvmtool advertise virtio-mmio interrupts as
>> edge-triggered.
>>  From skimming through the virtio spec I can't find any explicit
>> mentioning of the type of IRQ, but the usage of MSIs indeed hints at
>> using an edge property. Apparently reading the PCI ISR status register
>> clears it, which again sounds like edge. For virtio-mmio the driver
>> needs to explicitly clear the interrupt status register, which again
>> says: edge (as it's not the device clearing the status).
>>
>> So the device should just notify the driver once, which would cause one
>> vgic_inject_irq() call. It would be then up to the driver to clear up
>> that status, by reading PCI ISR status or writing to virtio-mmio's
>> interrupt-acknowledge register.
>>
>> Does that make sense?
> When implementing Xen backend, I didn't have an already working example
> so only guessed. I looked how kvmtool behaved when actually triggering
> the interrupt on Arm [1].
> 
> Taking into the account that Xen PoC on Arm advertises [2] the same irq
> type (TYPE_EDGE_RISING) as kvmtool [3] I decided to follow the model of
> triggering an interrupt. Could you please explain, is this wrong?

Yes, kvmtool does a double call needlessly (on x86, ppc and arm, mips is
correct).
I just chased it down in the kernel, a KVM_IRQ_LINE ioctl with level=low
is ignored when the target IRQ is configured as edge (which it is,
because the DT says so), check vgic_validate_injection() in the kernel.

So you should only ever need one call to set the line "high" (actually:
trigger the edge pulse).

Cheers,
Andre.

> 
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git/tree/arm/gic.c#n418
> 
> 
> [2]
> https://github.com/xen-troops/xen/blob/ioreq_4.14_ml/tools/libxl/libxl_arm.c#L727
> 
> 
> [3]
> https://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git/tree/virtio/mmio.c#n270
> 
> 

Re: Virtio in Xen on Arm (based on IOREQ concept)

[André Przywara]

On 21/07/2020 14:43, Julien Grall wrote:
> (+ Andre)
> 
> Hi Oleksandr,
> 
> On 21/07/2020 13:26, Oleksandr wrote:
>> On 20.07.20 23:38, Stefano Stabellini wrote:
>>> For instance, what's your take on notifications with virtio-mmio? How
>>> are they modelled today? Are they good enough or do we need MSIs?
>>
>> Notifications are sent from device (backend) to the driver (frontend)
>> using interrupts. Additional DM function was introduced for that
>> purpose xendevicemodel_set_irq_level() which results in
>> vgic_inject_irq() call.
>>
>> Currently, if device wants to notify a driver it should trigger the
>> interrupt by calling that function twice (high level at first, then
>> low level).
> 
> This doesn't look right to me. Assuming the interrupt is trigger when
> the line is high-level, the backend should only issue the hypercall once
> to set the level to high. Once the guest has finish to process all the
> notifications the backend would then call the hypercall to lower the
> interrupt line.
> 
> This means the interrupts should keep firing as long as the interrupt
> line is high.
> 
> It is quite possible that I took some shortcut when implementing the
> hypercall, so this should be corrected before anyone start to rely on it.

So I think the key question is: are virtio interrupts level or edge
triggered? Both QEMU and kvmtool advertise virtio-mmio interrupts as
edge-triggered.
>From skimming through the virtio spec I can't find any explicit
mentioning of the type of IRQ, but the usage of MSIs indeed hints at
using an edge property. Apparently reading the PCI ISR status register
clears it, which again sounds like edge. For virtio-mmio the driver
needs to explicitly clear the interrupt status register, which again
says: edge (as it's not the device clearing the status).

So the device should just notify the driver once, which would cause one
vgic_inject_irq() call. It would be then up to the driver to clear up
that status, by reading PCI ISR status or writing to virtio-mmio's
interrupt-acknowledge register.

Does that make sense?

Cheers,
Andre

Re: [PATCH] xen/rpi4: implement watchdog-based reset

[André Przywara]

On 04/06/2020 17:46, Stefano Stabellini wrote:
> On Thu, 4 Jun 2020, André Przywara wrote:
>> On 04/06/2020 17:24, Stefano Stabellini wrote:
>>> On Thu, 4 Jun 2020, André Przywara wrote:
>>>> On 04/06/2020 09:48, Julien Grall wrote:
>>>>
>>>> Hi,
>>>>
>>>>> On 03/06/2020 23:31, Stefano Stabellini wrote:
>>>>>> Touching the watchdog is required to be able to reboot the board.
>>>>>
>>>>> In general the preferred method is PSCI. Does it mean RPI 4 doesn't
>>>>> support PSCI at all?
>>>>
>>>> There is mainline Trusted Firmware (TF-A) support for the RPi4 for a few
>>>> months now, which includes proper PSCI support (both for SMP bringup and
>>>> system reset/shutdown). At least that should work, if not, it's a bug.
>>>> An EDK-2 build for RPi4 bundles TF-A automatically, but you can use TF-A
>>>> without it, with or without U-Boot: It works as a drop-in replacement
>>>> for armstub.bin. Instruction for building it (one line!) are here:
>>>> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/plat/rpi4.rst
>>>>
>>>>>>
>>>>>> The implementation is based on
>>>>>> drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux.
>>>>>
>>>>> Can you give the baseline? This would allow us to track an issue and
>>>>> port them.
>>>>
>>>> Given the above I don't think it's a good idea to add extra platform
>>>> specific code to Xen.
>>>
>>> The RPi4, at least the one I have, doesn't come with any TF, and it
>>
>> My RPi4 didn't come with anything, actually ;-) It's just a matter of
>> what you put in the uSD card slot.
>>
>>> doesn't come with PSCI in device tree.
>>
>> TF-A patches the PSCI nodes in:
>> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/rpi/rpi4?id=f67fa69cb6937a7fc559bbec4a7acce5edefa888
>>
>>> As a user, I would rather have
>>> this patch (even downstream) than having to introduce TF in my build and
>>> deployment just to be able to reboot.
>>
>> I get your point, but would rather put more pressure on people using
>> TF-A. After all you run without CPU hotplug, A72 errata workarounds and
>> without Spectre/Meltdown fixes. What was the IP address of your board
>> again? ;-)
> 
> Please send a pull request to remove __bcm2835_restart from the Linux
> kernel, once it is removed from there I'd be happy to take it away from
> Xen too ;-)

The kernel needs to support all RPi models, so we definitely need this.
Also it's already in there, so removing it is more churn.

The reason I am bringing this up is that we should get away from those
platform specific files in Xen at all. The only reason we have it for
the RPi4 is the non-page-aligned MMIO regions and overlaps, which could
actually be determined much better at runtime ...

> I know I am being cheeky but we have enough battles to fight and enough
> problems with Xen -- I don't think we should use the hypervisor as a
> leverage to get people to use or upgrade TF. We just need to get good
> functionalities to our users with the less amount of friction possible.

As I said: it's not my call, just pointing that out. It's just sad that
people everywhere work around the limited firmware instead of doing it
properly.

> Everything you mentioned are good reason to use TF, and this patch does
> not take anything away from it. My suggestion would be to work with
> raspberrypi.org to have TF installed by default by the Raspberry Pi
> Imager.

As far as I know there are (were?) efforts underway. For years ;-)

Cheers,
Andre

Re: [PATCH] xen/rpi4: implement watchdog-based reset

[André Przywara]

On 04/06/2020 17:24, Stefano Stabellini wrote:
> On Thu, 4 Jun 2020, André Przywara wrote:
>> On 04/06/2020 09:48, Julien Grall wrote:
>>
>> Hi,
>>
>>> On 03/06/2020 23:31, Stefano Stabellini wrote:
>>>> Touching the watchdog is required to be able to reboot the board.
>>>
>>> In general the preferred method is PSCI. Does it mean RPI 4 doesn't
>>> support PSCI at all?
>>
>> There is mainline Trusted Firmware (TF-A) support for the RPi4 for a few
>> months now, which includes proper PSCI support (both for SMP bringup and
>> system reset/shutdown). At least that should work, if not, it's a bug.
>> An EDK-2 build for RPi4 bundles TF-A automatically, but you can use TF-A
>> without it, with or without U-Boot: It works as a drop-in replacement
>> for armstub.bin. Instruction for building it (one line!) are here:
>> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/plat/rpi4.rst
>>
>>>>
>>>> The implementation is based on
>>>> drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux.
>>>
>>> Can you give the baseline? This would allow us to track an issue and
>>> port them.
>>
>> Given the above I don't think it's a good idea to add extra platform
>> specific code to Xen.
> 
> The RPi4, at least the one I have, doesn't come with any TF, and it

My RPi4 didn't come with anything, actually ;-) It's just a matter of
what you put in the uSD card slot.

> doesn't come with PSCI in device tree.

TF-A patches the PSCI nodes in:
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/rpi/rpi4?id=f67fa69cb6937a7fc559bbec4a7acce5edefa888

> As a user, I would rather have
> this patch (even downstream) than having to introduce TF in my build and
> deployment just to be able to reboot.

I get your point, but would rather put more pressure on people using
TF-A. After all you run without CPU hotplug, A72 errata workarounds and
without Spectre/Meltdown fixes. What was the IP address of your board
again? ;-)

> 
> Do other RPi4 users on this thread agree?
> 
> 
> But fortunately this one of the few cases where we can have our cake and
> eat it too :-)
> 
> If PSCI is supported on the RPi4, Xen automatically uses the PSCI reboot
> method first. (We could even go one step further and check for PSCI
> support in rpi4_reset below.) See
> xen/arch/arm/shutdown.c:machine_restart:
> 
> /* This is mainly for PSCI-0.2, which does not return if success. */
> call_psci_system_reset();
> 
> /* Alternative reset procedure */
> while ( 1 )
> {
> platform_reset();
> mdelay(100);
> }
> 
> 
> In other words, this patch won't take anything away from the good work
> done in TF, and when/if available, Xen will use it.

Sure, it doesn't block anything. I won't be in your way, after all I
don't have much of a say anyway ;-)

But how do you actually run Xen on the board? I guess this involves
quite some hacks on the firmware side to get it running (bootloader?
EFI? grub? hack the DTB?). I wonder if adding bl31.bin is really your
biggest problem, then.

Cheers,
Andre

>>>>
>>>> Signed-off-by: Stefano Stabellini 
>>>> ---
>>>>   xen/arch/arm/platforms/brcm-raspberry-pi.c | 60 ++
>>>>   1 file changed, 60 insertions(+)
>>>>
>>>> diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> b/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> index f5ae58a7d5..0214ae2b3c 100644
>>>> --- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> +++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
>>>> @@ -18,6 +18,10 @@
>>>>    */
>>>>     #include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>> +#include 
>>>
>>> We are trying to keep the headers ordered alphabetically within each
>>> directory and then 'xen/' first following by 'asm/'.
>>>
>>>>     static const char *const rpi4_dt_compat[] __initconst =
>>>>   {
>>>> @@ -37,12 +41,68 @@ static const struct dt_device_match
>>>> rpi4_blacklist_dev[] __initconst =
>>>>    * The aux peripheral also shares a page with the aux UART.
>>>>    */
>>>>   DT_MATCH_COMPATIBLE("brcm,bcm2835-aux"),
>>>> +    /* Special device used for rebooting */
>>>> +    DT_MATCH_COMPATIBLE("brcm,bcm2835-pm"),
>>>>   { /* sentinel */ },
>>>>

Re: [PATCH] xen/rpi4: implement watchdog-based reset

[André Przywara]

On 04/06/2020 09:48, Julien Grall wrote:

Hi,

> On 03/06/2020 23:31, Stefano Stabellini wrote:
>> Touching the watchdog is required to be able to reboot the board.
> 
> In general the preferred method is PSCI. Does it mean RPI 4 doesn't
> support PSCI at all?

There is mainline Trusted Firmware (TF-A) support for the RPi4 for a few
months now, which includes proper PSCI support (both for SMP bringup and
system reset/shutdown). At least that should work, if not, it's a bug.
An EDK-2 build for RPi4 bundles TF-A automatically, but you can use TF-A
without it, with or without U-Boot: It works as a drop-in replacement
for armstub.bin. Instruction for building it (one line!) are here:
https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/docs/plat/rpi4.rst

>>
>> The implementation is based on
>> drivers/watchdog/bcm2835_wdt.c:__bcm2835_restart in Linux.
> 
> Can you give the baseline? This would allow us to track an issue and
> port them.

Given the above I don't think it's a good idea to add extra platform
specific code to Xen.

Cheers,
Andre


> 
>>
>> Signed-off-by: Stefano Stabellini 
>> ---
>>   xen/arch/arm/platforms/brcm-raspberry-pi.c | 60 ++
>>   1 file changed, 60 insertions(+)
>>
>> diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c
>> b/xen/arch/arm/platforms/brcm-raspberry-pi.c
>> index f5ae58a7d5..0214ae2b3c 100644
>> --- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
>> +++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
>> @@ -18,6 +18,10 @@
>>    */
>>     #include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> We are trying to keep the headers ordered alphabetically within each
> directory and then 'xen/' first following by 'asm/'.
> 
>>     static const char *const rpi4_dt_compat[] __initconst =
>>   {
>> @@ -37,12 +41,68 @@ static const struct dt_device_match
>> rpi4_blacklist_dev[] __initconst =
>>    * The aux peripheral also shares a page with the aux UART.
>>    */
>>   DT_MATCH_COMPATIBLE("brcm,bcm2835-aux"),
>> +    /* Special device used for rebooting */
>> +    DT_MATCH_COMPATIBLE("brcm,bcm2835-pm"),
>>   { /* sentinel */ },
>>   };
>>   +
>> +#define PM_PASSWORD 0x5a00
>> +#define PM_RSTC 0x1c
>> +#define PM_WDOG 0x24
>> +#define PM_RSTC_WRCFG_FULL_RESET    0x0020
>> +#define PM_RSTC_WRCFG_CLR   0xffcf
> 
> NIT: It is a bit odd you introduce the 5 define together but the first 3
> have a different indentation compare to the last 2.
> 
>> +
>> +static void __iomem *rpi4_map_watchdog(void)
>> +{
>> +    void __iomem *base;
>> +    struct dt_device_node *node;
>> +    paddr_t start, len;
>> +    int ret;
>> +
>> +    node = dt_find_compatible_node(NULL, NULL, "brcm,bcm2835-pm");
>> +    if ( !node )
>> +    return NULL;
>> +
>> +    ret = dt_device_get_address(node, 0, &start, &len);
>> +    if ( ret )
>> +    {
>> +    dprintk(XENLOG_ERR, "Cannot read watchdog register address\n");
> 
> I would suggest to use printk() rather than dprintk. It would be useful
> for a normal user to know that we didn't manage to reset the platform
> and why.
> 
> 
>> +    return NULL;
>> +    }
>> +
>> +    base = ioremap_nocache(start & PAGE_MASK, PAGE_SIZE);
>> +    if ( !base )
>> +    {
>> +    dprintk(XENLOG_ERR, "Unable to map watchdog register!\n");
>> +    return NULL;
>> +    }
>> +
>> +    return base;
>> +}
>> +
>> +static void rpi4_reset(void)
>> +{
>> +    u32 val;
> 
> We are trying to get rid of any use of u32 in Xen code (the coding style
> used in this file). Please use uint32_t instead.
> 
>> +    void __iomem *base = rpi4_map_watchdog();
> 
> Newline here please.
> 
>> +    if ( !base )
>> +    return;
>> +
>> +    /* use a timeout of 10 ticks (~150us) */
>> +    writel(10 | PM_PASSWORD, base + PM_WDOG);
>> +    val = readl(base + PM_RSTC);
>> +    val &= PM_RSTC_WRCFG_CLR;
>> +    val |= PM_PASSWORD | PM_RSTC_WRCFG_FULL_RESET;
>> +    writel(val, base + PM_RSTC);
>> +
>> +    /* No sleeping, possibly atomic. */
>> +    mdelay(1);
>> +}
>> +
>>   PLATFORM_START(rpi4, "Raspberry Pi 4")
>>   .compatible = rpi4_dt_compat,
>>   .blacklist_dev  = rpi4_blacklist_dev,
>> +    .reset = rpi4_reset,
>>   .dma_bitsize    = 30,
>>   PLATFORM_END
>>  
> 
> Cheers,
> 

Re: [Xen-devel] [PATCH] arch: arm: vgic-v3: fix GICD_ISACTIVER range

[André Przywara]

On 13/11/2019 01:08, Julien Grall wrote:

Hi,

> On Tue, 12 Nov 2019, 04:01 Stefano Stabellini,  > wrote:
> 
> On Sat, 9 Nov 2019, Julien Grall wrote:
> > On Sat, 9 Nov 2019, 04:27 Stefano Stabellini,
> mailto:sstabell...@kernel.org>> wrote:
> >       On Thu, 7 Nov 2019, Peng Fan wrote:
> >       > The end should be GICD_ISACTIVERN not GICD_ISACTIVER.
> >       >
> >       > Signed-off-by: Peng Fan  >
> >
> >       Reviewed-by: Stefano Stabellini  >
> >
> >
> > To be honest, I am not sure the code is correct. A read to those
> registers should tell you the list of interrupts active. As we always
> > return 0, this will not return the correct state of the GIC.
> >
> > I know that returning the list of actives interrupts is
> complicated with the old vGIC, but I don't think silently ignoring
> it is a good
> > idea.
> > The question here is why the guest accessed those registers? What
> is it trying to figure out?
> 
> We are not going to solve the general problem at this stage. At the
> moment the code:
> 
> - ignore the first register only
> - print an error and return an IO_ABORT error for the other regs
> 
> For the inconsistency alone the second option is undesirable. Also it
> doesn't match the write implementation, which does the same thing for
> all the GICD_ISACTIVER* regs instead of having a special treatment for
> the first one only. It looks like a typo in the original patch to me.
> 
> The proposed patch switches the behavior to:
> 
> - silently ignore all the GICD_ISACTIVER* regs (as proposed)
> 
> 
> is an improvement.
> 
> 
> Peng mentioned that Linux is accessing it, so the worst thing we can do
> is lying to the guest (as you suggest here). I would definitely not call
> that an improvement.

The ISACTIVER range is wrong in the description, it covers only one
register, not multiple. This is obviously a typo, since it's correct in
both GICv2 and in the high level switch/case in GICv3. Reading from
outside of any range will inject an abort into the guest, which runs in
kernel space. This will probably result in a guest crash. I would
consider not crashing an improvement.

About "lying" to the guest: Typically an IRQ is just active for a very
short time, so 0 is a very good answer, actually. The old VGIC in KVM
did exactly the same:
vgic_reg_access(mmio, NULL, offset,
ACCESS_READ_RAZ | ACCESS_WRITE_IGNORED);

The proper solution would be:
1) Track the state of the active bit when we can observe it, so when the
guest exits with an active IRQ. The new VGIC does that.
2) Kick out all VCPUs that have IRQs in that given rank, and sample the
active bit from the LRs. Sounds pretty horrible, and chances are very
high you will get all 0s there.

So if I compare "fix those two typos and preserve the state that the Xen
VGIC has been in for years" to "create a lot of racy code for a rare
corner case", the first one surely wins.
That doesn't mean we should never try it, but surely this fix needs to
go in meanwhile.

> In the current state, it is a Nack. If there were a warning, then I
> would be more inclined to see this patch going through.

Do you mean a warning that we are about to lie to the guest? That sounds
pretty useless, since nobody can do anything about it. Plus we have
already those warnings on writing to these registers, and this always
confuses people and triggered pointless bug reports.

I think the old VGIC has bigger problems ;-)

Cheers,
Andre

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Blacklist PMU with "arm, cortex-a53-pmu"

[André Przywara]

On 14/04/2019 17:07, Julien Grall wrote:
> Hi,
> 
> On 4/14/19 3:33 PM, Amit Singh Tomar wrote:
>> At the moment, we hide PMU's from domain 0 and XEN boot fails on
>> platform[1]
>> where DTS contains "arm,cortex-a53-pmu" as compatible string for PMU
>> node.
> 
> "Domain 0 and Xen boot fails" is pretty vague. How does it fail?

After talking via IRC, the problem is PPIs, that this platform uses for
PMU interrupts. When Xen tries to setup the IRQ forwarding for Dom0 for
this device, it fails because it only supports forwarding SPIs.
So interestingly we erroneously forwarded A53 PMUs (those that are
described by that particular compatible string only) to Dom0 for quite a
while, but nobody noticed (or cared).

Amit, please adjust the commit message accordingly.

Cheers,
Andre.


> 
> In general, a commit message should give enough to the reviewer to
> understand what is the issue and be able to decide whether your patch is
> the correct fix.
> 
> Cheers,
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/3] MAINTAINERS: add ARM meson serial driver

[André Przywara]

On 21/03/2019 10:25, Amit Singh Tomar wrote:
> The meson-uart.c is an ARM specific UART driver for the Amlogic MESON
> SoC family.
> 
> Signed-off-by: Amit Singh Tomar 

Reviewed-by: Andre Przywara 

Cheers,
Andre

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ba7527c..aff7f81 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -182,6 +182,7 @@ F:xen/arch/arm/
>  F:   xen/drivers/char/arm-uart.c
>  F:   xen/drivers/char/cadence-uart.c
>  F:   xen/drivers/char/exynos4210-uart.c
> +F:   xen/drivers/char/meson-uart.c
>  F:   xen/drivers/char/mvebu-uart.c
>  F:   xen/drivers/char/omap-uart.c
>  F:   xen/drivers/char/pl011.c
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/3] xen/arm: Add MESON UART driver for Amlogic Meson SoCs

[André Przywara]

On 21/03/2019 10:25, Amit Singh Tomar wrote:
> This patch adds driver for UART controller present on Amlogic Meson
> SoCs and it has been tested on Nanopi K2 board based on S905 SoC.
> 
> Controller registers defination is taken from Linux 4.20.
> https://github.com/torvalds/linux/blob/v4.20-rc1/drivers/tty/serial/meson_uart.c
> 
> Signed-off-by: Amit Singh Tomar 

Thanks for the changes!

Reviewed-by: Andre Przywara 

Cheers,
Andre.

> ---
> Changes since v1:
> 
> * Removed clrsetbits macro.
> * Fixed coding style issue.
> * Updated MAINTAINERS file.
> 
> Changes since RFC:
> 
> * Removed S905 reference as other Amlogic SoCs
>   have this uart controller.
> * Replaced meson_s905_read/write helper
>   with clrsetbit and friends helper.
> * Followed proper UART reset sequence.
> * List all UART compatible strings same as Linux
>   driver.
> ---
>  xen/drivers/char/Kconfig  |   8 ++
>  xen/drivers/char/Makefile |   1 +
>  xen/drivers/char/meson-uart.c | 276 
> ++
>  3 files changed, 285 insertions(+)
>  create mode 100644 xen/drivers/char/meson-uart.c
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b1f07f8..b572305 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -20,6 +20,14 @@ config HAS_MVEBU
> This selects the Marvell MVEBU UART. If you have a ARMADA 3700
> based board, say Y.
>  
> +config HAS_MESON
> + bool "Amlogic MESON UART driver"
> + default y
> + depends on ARM_64
> + help
> +   This selects the Amlogic MESON UART. If you have a Amlogic based
> +   board, say Y.
> +
>  config HAS_PL011
>   bool "ARM PL011 UART driver"
>   default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index b68c330..7c646d7 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAS_NS16550) += ns16550.o
>  obj-$(CONFIG_HAS_CADENCE_UART) += cadence-uart.o
>  obj-$(CONFIG_HAS_PL011) += pl011.o
>  obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(CONFIG_HAS_MESON) += meson-uart.o
>  obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> diff --git a/xen/drivers/char/meson-uart.c b/xen/drivers/char/meson-uart.c
> new file mode 100644
> index 000..c16c188
> --- /dev/null
> +++ b/xen/drivers/char/meson-uart.c
> @@ -0,0 +1,276 @@
> +/*
> + * xen/drivers/char/meson-uart.c
> + *
> + * Driver for Amlogic MESON UART
> + *
> + * Copyright (c) 2019, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* Register offsets */
> +#define AML_UART_WFIFO_REG  0x00
> +#define AML_UART_RFIFO_REG  0x04
> +#define AML_UART_CONTROL_REG0x08
> +#define AML_UART_STATUS_REG 0x0c
> +#define AML_UART_MISC_REG   0x10
> +
> +/* UART_CONTROL bits */
> +#define AML_UART_TX_RST BIT(22)
> +#define AML_UART_RX_RST BIT(23)
> +#define AML_UART_CLEAR_ERR  BIT(24)
> +#define AML_UART_RX_INT_EN  BIT(27)
> +#define AML_UART_TX_INT_EN  BIT(28)
> +
> +/* UART_STATUS bits */
> +#define AML_UART_RX_FIFO_EMPTY  BIT(20)
> +#define AML_UART_TX_FIFO_FULL   BIT(21)
> +#define AML_UART_TX_FIFO_EMPTY  BIT(22)
> +#define AML_UART_TX_CNT_MASKGENMASK(14, 8)
> +
> +/* AML_UART_MISC bits */
> +#define AML_UART_XMIT_IRQ(c)(((c) & 0xff) << 8)
> +#define AML_UART_RECV_IRQ(c)((c) & 0xff)
> +
> +#define TX_FIFO_SIZE64
> +
> +#define setbits(addr, set)  writel((readl(addr) | (set)), (addr))
> +#define clrbits(addr, clear)writel((readl(addr) & ~(clear)), 
> (addr))
> +
> +static struct meson_uart {
> +unsigned int irq;
> +void __iomem *regs;
> +struct irqaction irqaction;
> +struct vuart_info vuart;
> +} meson_com;
> +
> +static void meson_uart_interrupt(int irq, void *data,
> + struct cpu_user_regs *regs)
> +{
> +struct serial_port *port = data;
> +struct meson_uart *uart = port->uart;
> +uint32_t st = readl(uart->regs + AML_UART_STATUS_REG);
> +
> +

Re: [Xen-devel] [PATCH v2 1/3] xen/arm: Add Amlogic Meson SoCs earlyprintk support

[André Przywara]

On 21/03/2019 10:25, Amit Singh Tomar wrote:
> Signed-off-by: Amit Singh Tomar 

Apart from the missing commit message:

Reviewed-by: Andre Przywara 
Tested-by: Andre Przywara 

Cheers,
Andre.

> ---
> TODO:
> * Capture XEN boot info on WIKI.
> 
> Changes since v1:
> 
> * Fixed coding style issue.
> * Undone changes in early-printk.txt.
> 
> Changes since RFC:
> 
> * Replaced LDRH with LDR, with this there
>   is no scattered output on console now.
> * Used tbnz instead of tst and b.ne.
> * Used AML_ prefix against register names.
> ---
>  xen/arch/arm/arm64/debug-meson.inc | 51 
> ++
>  1 file changed, 51 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/debug-meson.inc
> 
> diff --git a/xen/arch/arm/arm64/debug-meson.inc 
> b/xen/arch/arm/arm64/debug-meson.inc
> new file mode 100644
> index 000..01b70f0
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-meson.inc
> @@ -0,0 +1,51 @@
> +/*
> + * xen/arch/arm/arm64/debug-meson.inc
> + *
> + * MESON specific debug code.
> + *
> + * Copyright (c) 2019, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> .
> + */
> +
> +#define AML_UART_WFIFO_REG  0x00
> +#define AML_UART_STATUS_REG 0x0c
> +
> +#define AML_UART_TX_FIFO_FULL   21
> +
> +/*
> + * MESON UART wait UART to be ready to transmit
> + * xb: register which contains the UART base address
> + * c: scratch register
> + */
> +.macro early_uart_ready xb c
> +1:
> +ldr w\c, [\xb, #AML_UART_STATUS_REG]/* status register */
> +tbnzw\c, #AML_UART_TX_FIFO_FULL, 1b /* Check TXFIFO FULL 
> bit */
> +.endm
> +
> +/*
> + * MESON UART transmit character
> + * xb: register which contains the UART base address
> + * wt: register which contains the character to transmit
> + */
> +.macro early_uart_transmit xb wt
> +str\wt, [\xb, #AML_UART_WFIFO_REG]
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1 2/2] xen/arm: Add MESON UART driver for Amlogic Meson SoCs

[André Przywara]

On 26/01/2019 08:53, Amit Singh Tomar wrote:
> This patch adds driver for UART controller present on Amlogic Meson
> SoCs and it has been tested on Nanopi K2 board based on S905 SoC.
> 
> Controller registers defination is taken from Linux 4.20.
> 
> Signed-off-by: Amit Singh Tomar 
> ---
> Changes since RFC:
> 
> * Removed S905 reference as other Amlogic SoCs
>   have this uart controller.
> * Replaced meson_s905_read/write helper
>   with clrsetbit and friends helper.
> * Followed proper UART reset sequence.
> * List all UART compatible strings same as Linux
>   driver.
> ---
>  xen/drivers/char/Kconfig  |   8 ++
>  xen/drivers/char/Makefile |   1 +
>  xen/drivers/char/meson-uart.c | 282 
> ++
>  3 files changed, 291 insertions(+)
>  create mode 100644 xen/drivers/char/meson-uart.c
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b1f07f8..d4add7f 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -20,6 +20,14 @@ config HAS_MVEBU
> This selects the Marvell MVEBU UART. If you have a ARMADA 3700
> based board, say Y.
>  
> +config HAS_MESON
> +bool "Amlogic MESON UART driver"
> +default y
> +depends on ARM_64
> +help
> +  This selects the Amlogic MESON UART. If you have a Amlogic based
> +  board, say Y.
> +
>  config HAS_PL011
>   bool "ARM PL011 UART driver"
>   default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index b68c330..7c646d7 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAS_NS16550) += ns16550.o
>  obj-$(CONFIG_HAS_CADENCE_UART) += cadence-uart.o
>  obj-$(CONFIG_HAS_PL011) += pl011.o
>  obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(CONFIG_HAS_MESON) += meson-uart.o
>  obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> diff --git a/xen/drivers/char/meson-uart.c b/xen/drivers/char/meson-uart.c
> new file mode 100644
> index 000..990ae95
> --- /dev/null
> +++ b/xen/drivers/char/meson-uart.c
> @@ -0,0 +1,282 @@
> +/*
> + * xen/drivers/char/meson-uart.c
> + *
> + * Driver for Amlogic MESON UART
> + *
> + * Copyright (c) 2019, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* Register offsets */
> +#define AML_UART_WFIFO_REG  0x00
> +#define AML_UART_RFIFO_REG  0x04
> +#define AML_UART_CONTROL_REG0x08
> +#define AML_UART_STATUS_REG 0x0c
> +#define AML_UART_MISC_REG   0x10
> +
> +/* UART_CONTROL bits */
> +#define AML_UART_TX_RST BIT(22)
> +#define AML_UART_RX_RST BIT(23)
> +#define AML_UART_CLEAR_ERR  BIT(24)
> +#define AML_UART_RX_INT_EN  BIT(27)
> +#define AML_UART_TX_INT_EN  BIT(28)
> +
> +/* UART_STATUS bits */
> +#define AML_UART_RX_FIFO_EMPTY  BIT(20)
> +#define AML_UART_TX_FIFO_FULL   BIT(21)
> +#define AML_UART_TX_FIFO_EMPTY  BIT(22)
> +#define AML_UART_TX_CNT_MASKGENMASK(14, 8)
> +
> +/* AML_UART_MISC bits */
> +#define AML_UART_XMIT_IRQ(c)(((c) & 0xff) << 8)
> +#define AML_UART_RECV_IRQ(c)((c) & 0xff)
> +
> +#define TX_FIFO_SIZE64
> +
> +#define setbits(addr, set)  writel((readl(addr) | (set)), (addr))
> +#define clrbits(addr, clear)writel((readl(addr) & ~(clear)), 
> (addr))
> +#define clrsetbits(addr, clear, set)writel(((readl(addr) & ~(clear)) \
> +| (set)), (addr))

You don't seem to need this last one. Every user of it below sets "set"
to 0, so it's effectively just clrbits().

And keeping the other two as macros is probably fine then.

> +
> +static struct meson_uart {
> +unsigned int irq;
> +void __iomem *regs;
> +struct irqaction irqaction;
> +struct vuart_info vuart;
> +} meson_com;
> +
> +static void meson_uart_interrupt(int irq, void *data,
> +  struct cpu_user_regs *regs)

w/s

> +{
> +struct serial_port *port = data;
> +struct meson_uart *uart = port->uart;
> +uint32_t st =

Re: [Xen-devel] [PATCH v1 1/2] xen/arm: Add Amlogic Meson SoCs earlyprintk support

[André Przywara]

On 26/01/2019 08:53, Amit Singh Tomar wrote:
> Signed-off-by: Amit Singh Tomar 
> ---
> TODO:
> * Capture XEN boot info on WIKI(just forgot my
>   XEN Wiki credentials). 
> 
> Changes since RFC:
> 
> * Replaced LDRH with LDR, with this there
>   is no scattered output on console now.
> * Used tbnz instead of tst and b.ne.
> * Used AML_ prefix against register names.
> ---
>  docs/misc/arm/early-printk.txt |  1 +
>  xen/arch/arm/arm64/debug-meson.inc | 51 
> ++
>  2 files changed, 52 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/debug-meson.inc
> 
> diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
> index f765f59..2aa9528 100644
> --- a/docs/misc/arm/early-printk.txt
> +++ b/docs/misc/arm/early-printk.txt
> @@ -41,6 +41,7 @@ the name of the machine:
>- juno: printk with pl011 on Juno platform
>- lager: printk with SCIF0 on Renesas R-Car H2 processors
>- midway: printk with the pl011 on Calxeda Midway processors
> +  - meson: printk with the MESON for Amlogic S905 SoCs
>- mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
>- omap5432: printk with UART3 on TI OMAP5432 processors
>- rcar3: printk with SCIF2 on Renesas R-Car Gen3 processors
> diff --git a/xen/arch/arm/arm64/debug-meson.inc 
> b/xen/arch/arm/arm64/debug-meson.inc
> new file mode 100644
> index 000..164bcdf
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-meson.inc
> @@ -0,0 +1,51 @@
> +/*
> + * xen/arch/arm/arm64/debug-meson.inc
> + *
> + * MESON specific debug code.
> + *
> + * Copyright (c) 2019, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> .
> + */
> +
> +#define AML_UART_WFIFO_REG  0x00
> +#define AML_UART_STATUS_REG 0x0c
> +
> +#define AML_UART_TX_FIFO_FULL   21
> +
> +/*
> + * MESON UART wait UART to be ready to transmit
> + * xb: register which contains the UART base address
> + * c: scratch register
> + */
> +.macro early_uart_ready xb c
> +1:
> +ldr w\c, [\xb, #AML_UART_STATUS_REG]/* status register */
> +tbnzw\c, #AML_UART_TX_FIFO_FULL, 1b /* Check TXFIFO FULL 
> bit */
> +.endm
> +
> +/*
> + * MESON UART transmit character
> + * xb: register which contains the UART base address
> + * wt: register which contains the character to transmit
> + */
> +.macro early_uart_transmit xb wt
> + str\wt, [\xb, #AML_UART_WFIFO_REG]

That's a hard tab here.

Apart from this and the unneeded addition to early-printk.txt (which
Julien mentioned already) this looks now correct to me.

Cheers,
Andre

> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH for-4.12] gic-vgic: fix assert condition

[André Przywara]

On 24/01/2019 18:47, Nuernberger, Stefan wrote:
> On Thu, 2019-01-24 at 20:33 +0200, Andrii Anisov wrote:
>> From: Andrii Anisov 
>>
>> Signed-off-by: Andrii Anisov 
>> ---
>>  xen/arch/arm/gic-vgic.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index 48922f5..684f2d1 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -443,7 +443,7 @@ int vgic_connect_hw_irq(struct domain *d, struct
>> vcpu *v, unsigned int virq,
>>  int ret = 0;
>>  
>>  /* "desc" is optional when we disconnect an IRQ. */
>> -ASSERT(connect && desc);
>> +ASSERT(connect || desc);
> 
> I assume it should be ASSERT(!connect || desc);

Drawing the truth table and throwing all my CS text book boolean
knowledge at it :-)  Yes, Stefan's version matches the comment.

Plus what Julien said about the commit message.

Cheers,
Andre

>>  
>>  /* We are taking to rank lock to prevent parallel connections.
>> */
>>  vgic_lock_rank(v_target, rank, flags);


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC v2 00/16] Old GIC (gic-vgic) optimizations for GICV2

[André Przywara]

On 26/12/2018 11:20, Andrii Anisov wrote:
> From: Andrii Anisov 
> 
> This patch series is an attempt to reduce IRQ latency with the
> old GIC implementation (gic-vgic). These patches originally based
> on XEN 4.10 release. The motivation was to improve benchmark
> results of a system given to a customer for evaluation.
> This patch series is tailored for GICv2 on RCAR H3. Several
> of the most controversial patches (i.e. LRs shadowing) were
> not shared to the customer, and here are for comments and discussion.
> I hope several patches from here could be upstreamed. Some as is,
> others with modifications.
> 
> There are several simple ideas behind these changes:
> - reduce an excessive code (condition checks)
> - drop an excessive peripheral register accesses
> - if not reduce, then move an excessive code out of spinlocks
> - if not drop, then move an excessive register
>   accesses out of spinlocks
> 
> This is a v2 of the original RFC series [1]. From that series, patches
> [2] and [3] have already reached mainline. Here few patches are reworked
> with addressing some comments or separating them into more clear pieces,
> more patches are taken from the RFC v1 as is.
> 
> The main intention of this version of RFC series is to reveal
> patch-by-patch IRQ latency impact.
> The measurement is performed with TBM [4], so the use-case is trivial -
> passing a single IRQ twice in a second. Thus no lock contentions nor
> even passing more than one interrupt to a guest at the time use-cases
> are hit.
> 
> The series is based on the current xenbits/staging, commit 7f28661f6a7.
> XEN is build with no DEBUG and no GICv3 support for the staging HEAD and
> each commit. Four runtime configurations are evaluated for each commit:
> - sched=credit2 vwfi=trap
> - sched=credit2 vwfi=native
> - sched=credit vwfi=trap
> - sched=credit vwfi=native
> 
> Each commit is incrementally cherry-picked for the latency evaluation in
> an order they appear in the table. The table also can be found shared
> as a Google spreadsheet here [5].

Many thanks for generating these numbers, this is very useful.

But: could you make any sense out them? I plotted them, but they don't
seem to be very conclusive. I am scratching my head about the following
issues:
- The behaviour seems to be somewhat different between the four cases.
Which one do you care about, in particular? Is vwfi=native an option for
your use case? Which scheduler do you need or want to use?
- Which of the four columns (max, warm_max, min, avg) are you actually
interested in? For a hard realtime system I would expect max, or maybe
warm_max?
- Some of the patches seem to *increase* the latency. Patch 08/16 for
instance sticks out here. Most of the times the latency decreases again
afterwards, with a later patch, but I wonder if you could just pick the
patches that help or somehow explain those outliers.
- Can you give some more background on how you generated the numbers? I
haven't looked with too much detail into the benchmark, but I wonder about:
 * What is the runtime of your test? It seems to run forever and updates
   the stats twice a second, if I get this correctly. So for how long
   did you let it run?
 * Do we have any idea what the reliability of the values are? Can we
   somehow calculate the standard deviation, for instance? That would
   help to get an idea about the error range we are looking at.
 * Is this still the same system as in [4]? The resolution in there is
   only 120ns, some of the values for two patches differ exactly by that
   amount. So is this actually within the error margin?

Also it seems that this test is the only thing running on the system,
beside the idle VCPU. Is that a reasonable way to assess real world
interrupt latency? For instance that means that the IRQ handler mostly
hits even in the L1 cache, which I wouldn't expect in actual systems.
The method to separate warm_max from max seems to support this. Do you
have some numbers from that 3D benchmark you mentioned earlier, to put
this into perspective and show that improvements in one benefit the
other as well?

Also I looked at some code and started to cook up something myself[6].
The first two patches there should replace your patch 01/16 in a much
cleaner and easier way, along the lines I mentioned before in a reply to
a former post of yours.

Then I looked at the IRQ handler and stumbled upon the function pointers
we are using. I was eyeing them before, because my hunch is they are
costly, especially on big cores, as it might be hard for the CPU to
speculate correctly. Basically something like a call to
gic_hw_ops->gic_read_irq() translates into:
ldr x0, [x20]
ldr x0, [x0, #72]
blr x0
That contains two dependency on loads. If one of them misses in the
cache, the whole pipeline is stalled, if the CPU doesn't speculate both
loads correctly (which it might, but we don't know).
This is extra annoying since those fu

[Xen-devel] [RFC PATCH 1/2] xen/arm: Add Amlogic S905 SoC early printk support

[André Przywara]

On 8/7/18 6:07 PM, Amit Singh Tomar wrote:

Hi,

commit message?

> Signed-off-by: Amit Singh Tomar 
> ---
>  docs/misc/arm/early-printk.txt |  1 +
>  xen/arch/arm/Rules.mk  |  1 +
>  xen/arch/arm/arm64/debug-meson.inc | 50
> ++ 3 files changed, 52
> insertions(+) create mode 100644 xen/arch/arm/arm64/debug-meson.inc
> 
> diff --git a/docs/misc/arm/early-printk.txt
> b/docs/misc/arm/early-printk.txt index f765f59..2aa9528 100644
> --- a/docs/misc/arm/early-printk.txt
> +++ b/docs/misc/arm/early-printk.txt
> @@ -41,6 +41,7 @@ the name of the machine:
>- juno: printk with pl011 on Juno platform
>- lager: printk with SCIF0 on Renesas R-Car H2 processors
>- midway: printk with the pl011 on Calxeda Midway processors
> +  - meson: printk with the MESON for Amlogic S905 SoCs
>- mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
>- omap5432: printk with UART3 on TI OMAP5432 processors
>- rcar3: printk with SCIF2 on Renesas R-Car Gen3 processors
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index f264592..d4fabdc 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -36,6 +36,7 @@ EARLY_PRINTK_hikey960   := pl011,0xfff32000
>  EARLY_PRINTK_juno   := pl011,0x7ff8
>  EARLY_PRINTK_lager  := scif,0xe6e6
>  EARLY_PRINTK_midway := pl011,0xfff36000
> +EARLY_PRINTK_meson  := meson,0xc81004c0
>  EARLY_PRINTK_mvebu  := mvebu,0xd0012000
>  EARLY_PRINTK_omap5432   := 8250,0x4802,2
>  EARLY_PRINTK_rcar3  := scif,0xe6e88000
> diff --git a/xen/arch/arm/arm64/debug-meson.inc
> b/xen/arch/arm/arm64/debug-meson.inc new file mode 100644
> index 000..d5507d3
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-meson.inc
> @@ -0,0 +1,50 @@
> +/*
> + * xen/arch/arm/arm64/debug-meson.inc
> + *
> + * MESON specific debug code.
> + *
> + * Copyright (c) 2018, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see
> .
> + */
> +
> +#define UART_STATUS_REG 0x0c
> +#define UART_TX_REG 0x00

As Julien mentioned, please stick to the manual names and be consistent
with the proper Xen driver. Also, please sort by offset.

> +
> +/*
> + * MESON UART wait UART to be ready to transmit
> + * xb: register which contains the UART base address
> + * c: scratch register
> + */
> +.macro early_uart_ready xb c
> +1:
> +ldrh   w\c, [\xb, #UART_STATUS_REG] /* status register */

Why ldrh? This is a 32-bit register, actually you can't be sure that the
device supports a 16-bit access. Besides: the bit you are after is in
the upper half, so you actually will never see the bit set. I wonder if
you are loosing characters here.

> +tstw\c, #(1 << 21)  /* Check TXFIFO FULL bit
> */
> +b.ne   1b   /* Wait for the UART to
> be ready */

You can use "tbnz" to replace those two instructions.

> +.endm
> +
> +/*
> + * MESON UART transmit character
> + * xb: register which contains the UART base address
> + * wt: register which contains the character to transmit
> + */
> +.macro early_uart_transmit xb wt
> + strb  \wt, [\xb, #UART_TX_REG]

TX_WFIFO is a 32-bit register, so you should rather use a 32-bit
accessor.

Cheers,
Andre.


> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [RFC PATCH 2/2] xen/arm: Add MESON UART driver for Amlogic S905 SoC

[André Przywara]

On 8/7/18 6:07 PM, Amit Singh Tomar wrote:

Hi,

> This patch adds driver for UART controller present on Amlogic S905
> SoC.
> https://dn.odroid.com/S905/DataSheet/S905_Public_Datasheet_V1.1.4.pdf
> 
> Signed-off-by: Amit Singh Tomar 
> ---
>  xen/drivers/char/Kconfig  |   8 ++
>  xen/drivers/char/Makefile |   1 +
>  xen/drivers/char/meson-uart.c | 290
> ++ 3 files changed, 299
> insertions(+) create mode 100644 xen/drivers/char/meson-uart.c
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index cc78ec3..b9fee4e 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -20,6 +20,14 @@ config HAS_MVEBU
> This selects the Marvell MVEBU UART. If you have a ARMADA
> 3700 based board, say Y.
>  
> +config HAS_MESON
> +bool
> +default y
> +depends on ARM_64
> +help
> +  This selects the Marvell MESON UART. If you have a Amlogic S905
> +  based board, say Y.

It seems this UART driver supports all Amlogic SoCs (905X, 805X, 912,
...), not just the S905. So please either remove the number or make it
clear that it's just an example.
And it's not a Marvell UART ;-)

> +
>  config HAS_PL011
>   bool
>   default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index b68c330..7c646d7 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAS_NS16550) += ns16550.o
>  obj-$(CONFIG_HAS_CADENCE_UART) += cadence-uart.o
>  obj-$(CONFIG_HAS_PL011) += pl011.o
>  obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(CONFIG_HAS_MESON) += meson-uart.o
>  obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
> diff --git a/xen/drivers/char/meson-uart.c
> b/xen/drivers/char/meson-uart.c new file mode 100644
> index 000..8fe7e62
> --- /dev/null
> +++ b/xen/drivers/char/meson-uart.c
> @@ -0,0 +1,290 @@
> +/*
> + * xen/drivers/char/meson-uart.c
> + *
> + * Driver for Amlogic MESON UART
> + *
> + * Copyright (c) 2018, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see
> .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* Register offsets */
> +#define UART_WFIFO  0x00
> +#define UART_RFIFO  0x04
> +#define UART_CONTROL0x08
> +#define UART_STATUS 0x0c
> +#define UART_MISC   0x10
> +#define UART_REG5   0x14
> +
> +/* UART_CONTROL bits */
> +#define UART_TX_EN  BIT(12)
> +#define UART_RX_EN  BIT(13)

You don't seem to use them in the code? This seems somewhat wrong, you
shouldn't rely on those bits being set by previous boot stages.

> +#define UART_TX_RST BIT(22)
> +#define UART_RX_RST BIT(23)
> +#define UART_CLEAR_ERR  BIT(24)
> +#define UART_RX_INT_EN  BIT(27)
> +#define UART_TX_INT_EN  BIT(28)
> +
> +/* UART_STATUS bits */
> +#define UART_PARITY_ERR BIT(16)
> +#define UART_FRAME_ERR  BIT(17)
> +#define UART_TX_FIFO_WERR   BIT(18)

You don't use those, so I don't see a need to define them.

> +#define UART_RX_EMPTY   BIT(20)
> +#define UART_TX_FULLBIT(21)
> +#define UART_TX_EMPTY   BIT(22)

Might be worth to add FIFO_ in those names.

> +#define UART_TX_CNT_MASKGENMASK(14, 8)
> +
> +
> +#define UART_XMIT_IRQ_CNT_MASK  GENMASK(15, 8)
> +#define UART_RECV_IRQ_CNT_MASK  GENMASK(7, 0)
> +
> +#define TX_FIFO_SIZE64
> +
> +static struct meson_s905_uart {
> +unsigned int irq;
> +void __iomem *regs;
> +struct irqaction irqaction;
> +struct vuart_info vuart;
> +} meson_s905_com = {0};
> +
> +#define meson_s905_read(uart, off)  readl((uart)->regs + off)
> +#define meson_s905_write(uart, off, val) writel(val, (uart->regs) + off)

I was wondering whether a clrsetbit helper would be more useful than
these very thin wrappers.

> +static void meson_s905_uart_interrupt(int irq, void *data,
> +struct cpu_user_regs *regs)
> +{
> +struct serial_port *port = data;
> +struct meson_s905_uart *uart = port->uart;
> +uint32_t st = meson_s905_read(uart, UART_STATUS);
> +
> +if ( !(st & UART_RX_EMPTY) )
> +{
> +serial_rx_in

Re: [Xen-devel] [PATCH] xen:arm: Populate arm64 image header

[André Przywara]

On 09/01/2018 02:01 PM, Amit Tomer wrote:
> Hello,
> 
>> Yes, and in fact it seems one can work around this by cleverly
>> constructing the load addresses,
> 
> But we are really dealing a corner case here. No matter where
> we load the image, it would be relocated to 0x8( since dram
> starts at 0x...) and unfortunately first 16MiB is reserved for
> ROM Firmware.
> 
 This unwanted situation can be fixed by updating image_size field
 along side kernel flags so that image wouldn't relocate from initial
 load address.
>>>
>>> I think the first step is to fix your U-boot and rethink where you load
>>> your binaries.
>>
>> I think U-Boot perfectly complies with the kernel document. Xen not so
>> much. The kernel image format was deliberately updated to become more
>> flexible with certain memory layout situations as we have here.
>> There is for instance a problem if there is something precious at 512KB
>> into DRAM (secure memory owned by firmware), as regardless of the load
>> addresses the user chooses U-Boot will (rightfully!) revert to the
>> original "512KB into DRAM" address to keep compatibility with older
>> kernels - and it believes Xen is such a one because of the ancient
>> header format.
>>
>> But ...
>>
>>> Regarding the patch in itself, I think this is a good addition as it
>>> allow Xen to be loaded in more places. But please rewrite the commit
>>> message accordingly, this is an update to a new version.
>>
>> I totally agree with that, the commit message should be reworded to
>> stress that we want to comply with a newer version of the kernel image
>> header (which is around for four years by now!), and just mention that
>> it fixes problems with non-ancient U-Boots on certain platforms as an
>> additional reason.
>>
 [1]:https://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/lib/image.c;h=699bf44e702f7a7084997406203fd7d2aaaf87fa;hb=HEAD#l50


 These changes are derived from kernel v4.18 files

 Signed-off-by: Amit Singh Tomar 
 ---
   xen/arch/arm/arm64/head.S |  5 ++-
   xen/arch/arm/arm64/lib/assembler.h| 11 +
   xen/arch/arm/xen.lds.S|  3 ++
   xen/include/asm-arm/arm64/linux_header_vars.h | 62
 +++
   4 files changed, 79 insertions(+), 2 deletions(-)
   create mode 100644 xen/include/asm-arm/arm64/linux_header_vars.h

 diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
 index d63734f..ce72c95 100644
 --- a/xen/arch/arm/arm64/head.S
 +++ b/xen/arch/arm/arm64/head.S
 @@ -25,6 +25,7 @@
   #include 
   #include 
   #include 
 +#include "lib/assembler.h"
 #define PT_PT 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1
 P=1 */
   #define PT_MEM0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0
 P=1 */
 @@ -120,8 +121,8 @@ efi_head:
   add x13, x18, #0x16
   b   real_start   /* branch to kernel start */
   .quad   0/* Image load offset from start
 of RAM */
 -.quad   0/* reserved */
 -.quad   0/* reserved */
 +le64sym _kernel_size_le  /* Effective size of kernel
 image, little-endian */
 +le64sym _kernel_flags_le /* Informative flags,
 little-endian */
>>>
>>> All the dance for to convert to little endian is not necessary on Xen.
>>> Please rework your series to avoid such code, this would also reduce
>>> quite significantly the series.
>>
>> Indeed!
> 
> How about this change?

The diff below doesn't make sense. Please just send a new version
without *any* endianess code and with the changed commit message -
focusing on the overdue image format update as the main rationale and
just mentioning the platform as an example.

Cheers,
Andre.

> 
> index ce72c95..ec29e01 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -121,8 +121,8 @@ efi_head:
>  add x13, x18, #0x16
>  b   real_start   /* branch to kernel start */
>  .quad   0/* Image load offset from start of RAM 
> */
> -le64sym _kernel_size_le  /* Effective size of kernel
> image, little-endian */
> -le64sym _kernel_flags_le /* Informative flags, little-endian */
> +.quad   _end - start  /* Effective size of kernel image,
> little-endian */
> +.quad   __HEAD_FLAGS /* Informative flags, little-endian */
>  .quad   0/* reserved */
>  .quad   0/* reserved */
>  .quad   0/* reserved */
> diff --git a/xen/arch/arm/arm64/lib/assembler.h
> b/xen/arch/arm/arm64/lib/assembler.h
> index c0ef758..05861b8 100644
> --- a/xen/arch/arm/arm64/lib/assembler.h
> +++ b/xen/arch/arm/arm64/lib/assembler.h
> @@ -9,15 +9,11 @@
>  #define

[Xen-devel] [PATCH] xen:arm: Populate arm64 image header

[André Przywara]

On 08/31/2018 09:39 PM, Julien Grall wrote:

Hi,

tl;dr: I think we don't disagree on the usefulness of this change in
general, so let's just change the commit towards something like:

Update the Xen image header to match the newest Linux kernel definition.
This allows to specify some relaxed properties of Xen and allows a
bootloader to place the Xen kernel more flexibly.
This fixes the boot on some systems which have other data or code close
to the beginning of DRAM.

More explanation why I don't think this is a U-Boot bug:

>>> On 08/31/2018 06:01 PM, Amit Singh Tomar wrote:
 While porting XEN on Amlogic SoC we found that in absence of
 image_size field of XEN image header, bootloader(U-BOOT)
 relocates[1] the XEN image to an address(not appropriate
 for Amlogic) derived from text_offset.
>>>
>>> IHMO, this is a bug in U-Boot rather than Xen. The current format, while
>>> old, is still valid.
>>
>> As tempting at is it to blame the "other guy", I think it's really Xen
>> not up to par here. The kernel doc [2] says: ...
>> "Where image_size is zero, text_offset can be assumed to be 0x8."
>>
>> This is what U-Boot implements.
> 
> And Xen comply to that... Xen is actually able to boot at any address
> that is 4KB aligned.

Yes, Xen is much more relaxed then even the newest kernel (ignoring
TEXT_OFFSET), but it doesn't tell the boot loader. So that assumes the
worst. Fair enough, works mostly, but not in every case, like this one
here. The kernel image header got improved to cope with those cases, and
Xen should just be updated to do as well. There is no regression
expected, it's just improving the situation in some cases.
Actually regardless of that we could just update the image header.

>> So you load Xen, say, to 16MB into DRAM, and U-Boot moves it to 512 KB,
>> again complying with the kernel doc:
>> "NOTE: versions prior to v4.6 cannot make use of memory below the
>> physical offset of the Image so it is recommended that the Image be
>> placed as close as possible to the start of system RAM."
>> U-Boot derives this property from bit 3 of the flags being clear.
> 
> What's wrong with clearing bit 3? This is a valid things to do per the
> new format.
> 
> So that does not explain why the bug lie in Xen...

There is no bug in Xen, it's just not very precise in advertising its
needs and properties and this bites us in corner cases like this one.

>>> In the case of U-Boot, you usually need to be really careful on the
>>> position of all the blobs (e.g Xen, DTB, Kernel, Initramfs) in the
>>> memory.
>>
>> Yes, and in fact it seems one can work around this by cleverly
>> constructing the load addresses, but it's really time to bring Xen into
>> the 21st century (or so) when it comes to the kernel image header ;-)
> 
> Even with the new format, you will have to cleverly construct the load
> addresses because nothing prevents U-boot to move the binaries around...

U-Boot will *try* to not move the image around, to avoid overwriting
other images (Dom0, DT, ramdisk, firmware...). But if the information it
gets from the kernel image give it no choice, it goes the extra mile to
make the kernel happy (as early as possible, 512KB offset). Xen doesn't
need all that, so we should tell that to the bootloader.

 This unwanted situation can be fixed by updating image_size field
 along side kernel flags so that image wouldn't relocate from initial
 load address.
>>>
>>> I think the first step is to fix your U-boot and rethink where you load
>>> your binaries.
>>
>> I think U-Boot perfectly complies with the kernel document. Xen not so
>> much. The kernel image format was deliberately updated to become more
>> flexible with certain memory layout situations as we have here.
>> There is for instance a problem if there is something precious at 512KB
>> into DRAM (secure memory owned by firmware), as regardless of the load
>> addresses the user chooses U-Boot will (rightfully!) revert to the
>> original "512KB into DRAM" address to keep compatibility with older
>> kernels - and it believes Xen is such a one because of the ancient
>> header format.
> 
> You could construct the same behavior with the new format (text-offset =
> 0x8 and flag 3 cleared). So how come this does not work?

The old kernel had some requirements, which got relaxed over time. The
image format now reflects this. You might run into similar issues with
an old kernel, but people don't care so much.
So at the moment it works somehow and mostly, but is unnecessarily
complicated. You load Xen to address X, but then have to know that
U-Boot relocates Xen (and just Xen!) to some other address and adjust
your load addresses accordingly. Not very user friendly.

> This is why the spec says: "As close as possible to start of System RAM"
> not "At the start of System RAM". U-boot is probably too pedantic here.

As a human I tend to agree, but what should U-Boot do? "As close as
possible" means 0, unless you have other information 

Re: [Xen-devel] [PATCH] xen:arm: Populate arm64 image header

[André Przywara]

On 08/31/2018 06:14 PM, Julien Grall wrote:

Hi,

> On 08/31/2018 06:01 PM, Amit Singh Tomar wrote:
>> While porting XEN on Amlogic SoC we found that in absence of
>> image_size field of XEN image header, bootloader(U-BOOT)
>> relocates[1] the XEN image to an address(not appropriate
>> for Amlogic) derived from text_offset.
> 
> IHMO, this is a bug in U-Boot rather than Xen. The current format, while
> old, is still valid.

As tempting at is it to blame the "other guy", I think it's really Xen
not up to par here. The kernel doc [2] says: ...
"Where image_size is zero, text_offset can be assumed to be 0x8."

This is what U-Boot implements.

So you load Xen, say, to 16MB into DRAM, and U-Boot moves it to 512 KB,
again complying with the kernel doc:
"NOTE: versions prior to v4.6 cannot make use of memory below the
physical offset of the Image so it is recommended that the Image be
placed as close as possible to the start of system RAM."
U-Boot derives this property from bit 3 of the flags being clear.

> In the case of U-Boot, you usually need to be really careful on the
> position of all the blobs (e.g Xen, DTB, Kernel, Initramfs) in the memory.

Yes, and in fact it seems one can work around this by cleverly
constructing the load addresses, but it's really time to bring Xen into
the 21st century (or so) when it comes to the kernel image header ;-)

>> This unwanted situation can be fixed by updating image_size field
>> along side kernel flags so that image wouldn't relocate from initial
>> load address.
> 
> I think the first step is to fix your U-boot and rethink where you load
> your binaries.

I think U-Boot perfectly complies with the kernel document. Xen not so
much. The kernel image format was deliberately updated to become more
flexible with certain memory layout situations as we have here.
There is for instance a problem if there is something precious at 512KB
into DRAM (secure memory owned by firmware), as regardless of the load
addresses the user chooses U-Boot will (rightfully!) revert to the
original "512KB into DRAM" address to keep compatibility with older
kernels - and it believes Xen is such a one because of the ancient
header format.

But ...

> Regarding the patch in itself, I think this is a good addition as it
> allow Xen to be loaded in more places. But please rewrite the commit
> message accordingly, this is an update to a new version.

I totally agree with that, the commit message should be reworded to
stress that we want to comply with a newer version of the kernel image
header (which is around for four years by now!), and just mention that
it fixes problems with non-ancient U-Boots on certain platforms as an
additional reason.

>> [1]:https://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/lib/image.c;h=699bf44e702f7a7084997406203fd7d2aaaf87fa;hb=HEAD#l50
>>
>>
>> These changes are derived from kernel v4.18 files
>>
>> Signed-off-by: Amit Singh Tomar 
>> ---
>>   xen/arch/arm/arm64/head.S |  5 ++-
>>   xen/arch/arm/arm64/lib/assembler.h    | 11 +
>>   xen/arch/arm/xen.lds.S    |  3 ++
>>   xen/include/asm-arm/arm64/linux_header_vars.h | 62
>> +++
>>   4 files changed, 79 insertions(+), 2 deletions(-)
>>   create mode 100644 xen/include/asm-arm/arm64/linux_header_vars.h
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index d63734f..ce72c95 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -25,6 +25,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include "lib/assembler.h"
>>     #define PT_PT 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1
>> P=1 */
>>   #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0
>> P=1 */
>> @@ -120,8 +121,8 @@ efi_head:
>>   add x13, x18, #0x16
>>   b   real_start   /* branch to kernel start */
>>   .quad   0    /* Image load offset from start
>> of RAM */
>> -    .quad   0    /* reserved */
>> -    .quad   0    /* reserved */
>> +    le64sym _kernel_size_le  /* Effective size of kernel
>> image, little-endian */
>> +    le64sym _kernel_flags_le /* Informative flags,
>> little-endian */
> 
> All the dance for to convert to little endian is not necessary on Xen.
> Please rework your series to avoid such code, this would also reduce
> quite significantly the series.

Indeed!

Cheers,
Andre.

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.txt#n98

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/2] xen/arm: Add Marvell ARMADA 3700 early printk support

[André Przywara]

On 05/04/18 11:16, Amit Singh Tomar wrote:
> Signed-off-by: Amit Singh Tomar 

Compiled with CONFIG_EARLY_PRINTK=mvebu, I see Xen's pre-DT boot
messages. In addition to my Reviewed-by:

Tested-by: Andre Przywara 

Cheers,
Andre.

> ---
> Changes since v2:
> * Addressed Andre's comments.
> Changes since v1:
> * Removed header file dependency.
> ---
>  docs/misc/arm/early-printk.txt |  1 +
>  xen/arch/arm/Rules.mk  |  1 +
>  xen/arch/arm/arm64/debug-mvebu.inc | 50 
> ++
>  3 files changed, 52 insertions(+)
>  create mode 100644 xen/arch/arm/arm64/debug-mvebu.inc
> 
> diff --git a/docs/misc/arm/early-printk.txt b/docs/misc/arm/early-printk.txt
> index 20a8af8..f765f59 100644
> --- a/docs/misc/arm/early-printk.txt
> +++ b/docs/misc/arm/early-printk.txt
> @@ -41,6 +41,7 @@ the name of the machine:
>- juno: printk with pl011 on Juno platform
>- lager: printk with SCIF0 on Renesas R-Car H2 processors
>- midway: printk with the pl011 on Calxeda Midway processors
> +  - mvebu: printk with the MVEBU for Marvell Armada 3700 SoCs
>- omap5432: printk with UART3 on TI OMAP5432 processors
>- rcar3: printk with SCIF2 on Renesas R-Car Gen3 processors
>- seattle: printk with pl011 for AMD Seattle processor
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index b66c19f..f264592 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -36,6 +36,7 @@ EARLY_PRINTK_hikey960   := pl011,0xfff32000
>  EARLY_PRINTK_juno   := pl011,0x7ff8
>  EARLY_PRINTK_lager  := scif,0xe6e6
>  EARLY_PRINTK_midway := pl011,0xfff36000
> +EARLY_PRINTK_mvebu  := mvebu,0xd0012000
>  EARLY_PRINTK_omap5432   := 8250,0x4802,2
>  EARLY_PRINTK_rcar3  := scif,0xe6e88000
>  EARLY_PRINTK_seattle:= pl011,0xe101
> diff --git a/xen/arch/arm/arm64/debug-mvebu.inc 
> b/xen/arch/arm/arm64/debug-mvebu.inc
> new file mode 100644
> index 000..3579ea6
> --- /dev/null
> +++ b/xen/arch/arm/arm64/debug-mvebu.inc
> @@ -0,0 +1,50 @@
> +/*
> + * xen/arch/arm/arm64/debug-mvebu.inc
> + *
> + * MVEBU specific debug code.
> + *
> + * Copyright (c) 2018, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> .
> + */
> +
> +#define UART_STATUS_REG 0x0c
> +#define UART_TX_REG 0x04
> +
> +/*
> + * MVEBU UART wait UART to be ready to transmit
> + * xb: register which contains the UART base address
> + * c: scratch register
> + */
> +.macro early_uart_ready xb c
> +1:
> +ldrh   w\c, [\xb, #UART_STATUS_REG]  /* status register */
> +tstw\c, #(1 << 11)/* Check TXFIFO FULL 
> bit */
> +b.ne   1b/* Wait for the UART to be 
> ready */
> +.endm
> +
> +/*
> + * MVEBU UART transmit character
> + * xb: register which contains the UART base address
> + * wt: register which contains the character to transmit
> + */
> +.macro early_uart_transmit xb wt
> + strb  \wt, [\xb, #UART_TX_REG]
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 1.9.1
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/2] xen/arm: Add MVEBU UART driver for Marvell Armada 3700 SoC

[André Przywara]

On 05/04/18 11:16, Amit Singh Tomar wrote:
> This patch adds driver for UART controller found on Armada 3700 SoC.
> 
> There is no reference manuals available for 3700 SoC in public and it
> is derived by looking at Linux driver[1].
> 
> [1]https://github.com/torvalds/linux/blob/master/drivers/tty/serial/mvebu-uart.c
> 
> Signed-off-by: Amit Singh Tomar 

Works like a charm, can log into Dom0, Xen console works as well. So in
addition to my Reviewed-by:

Tested-by: Andre Przywara 

Cheers,
Andre.

> ---
> Changes since v2:
> * Addressed Andre's comments.
> Changes since v1:
> * Addressed Wei Liu's comments
> * Addressed Andre's comments.
> Changes since RFC:
> * Addressed Julien's comments.
> ---
>  xen/drivers/char/Kconfig  |   8 ++
>  xen/drivers/char/Makefile |   1 +
>  xen/drivers/char/mvebu-uart.c | 296 
> ++
>  3 files changed, 305 insertions(+)
>  create mode 100644 xen/drivers/char/mvebu-uart.c
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index fb53dd8..04a4087 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -12,6 +12,14 @@ config HAS_CADENCE_UART
> This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
> based board, say Y.
> 
> +config HAS_MVEBU
> +bool
> +default y
> +depends on ARM_64
> +help
> +  This selects the Marvell MVEBU UART. If you have a ARMADA 3700
> +  based board, say Y.
> +
>  config HAS_PL011
>   bool
>   default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index 0d48b16..b68c330 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAS_NS16550) += ns16550.o
>  obj-$(CONFIG_HAS_CADENCE_UART) += cadence-uart.o
>  obj-$(CONFIG_HAS_PL011) += pl011.o
>  obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
>  obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
> diff --git a/xen/drivers/char/mvebu-uart.c b/xen/drivers/char/mvebu-uart.c
> new file mode 100644
> index 000..1561a50
> --- /dev/null
> +++ b/xen/drivers/char/mvebu-uart.c
> @@ -0,0 +1,296 @@
> +/*
> + * xen/drivers/char/mvebu3700-uart.c
> + *
> + * Driver for Marvell MVEBU UART.
> + *
> + * Copyright (c) 2018, Amit Singh Tomar .
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions of the GNU General Public
> + * License, version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; If not, see 
> .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* Register offsets */
> +#define UART_RX_REG 0x00
> +
> +#define UART_TX_REG 0x04
> +
> +#define UART_CTRL_REG   0x08
> +#define CTRL_TXFIFO_RST BIT(15)
> +#define CTRL_RXFIFO_RST BIT(14)
> +#define CTRL_TX_RDY_INT BIT(5)
> +#define CTRL_RX_RDY_INT BIT(4)
> +#define CTRL_BRK_DET_INTBIT(3)
> +#define CTRL_FRM_ERR_INTBIT(2)
> +#define CTRL_PAR_ERR_INTBIT(1)
> +#define CTRL_OVR_ERR_INTBIT(0)
> +#define CTRL_ERR_INT(CTRL_BRK_DET_INT | CTRL_FRM_ERR_INT | \
> + CTRL_PAR_ERR_INT | CTRL_OVR_ERR_INT)
> +
> +#define UART_STATUS_REG 0x0c
> +#define STATUS_TXFIFO_EMP   BIT(13)
> +#define STAT_TX_FIFO_FULBIT(11)
> +#define STAT_TX_FIFO_HFLBIT(10)
> +#define STATUS_TX_RDY   BIT(5)
> +#define STATUS_RX_RDY   BIT(4)
> +#define STATUS_BRK_DET  BIT(3)
> +#define STATUS_FRM_ERR  BIT(2)
> +#define STATUS_PAR_ERR  BIT(1)
> +#define STATUS_OVR_ERR  BIT(0)
> +#define STATUS_BRK_ERR  (STATUS_BRK_DET | STATUS_FRM_ERR | \
> + STATUS_PAR_ERR | STATUS_OVR_ERR)
> +
> +#define TX_FIFO_SIZE32
> +
> +static struct mvebu3700_uart {
> +unsigned int irq;
> +void __iomem *regs;
> +struct irqaction irqaction;
> +struct vuart_info vuart;
> +} mvebu3700_com = {0};
> +
> +#define mvebu3700_read(uart, off)   readl((uart)->regs + off)
> +#define mvebu3700_write(uart, off, val) writel(val, (uart->regs) + off)
> +
> +static void mvebu3700_uart_interrupt(int irq, void *data,
> + struct cpu_user_regs *regs)
> +{
> +struct serial_port *port = data;
> +struct mvebu3700_uart *uart = port->uart;
> +uint32_t st = mvebu3700_read(uart, UART_STATUS_REG

Re: [Xen-devel] [PATCH v2 1/2] xen/arm: Add MVEBU UART driver for Marvell Armada 3700 SoC

[André Przywara]

On 05/04/18 00:17, Amit Tomer wrote:
> Hi,
> 
>> Ah yes, if that works, we should. That bit is from the Linux BSP, isn't it?
> 
> Yes, it's from Linux BSP code only. Hope following is fine.
> 
> if ( reg & STATUS_TXFIFO_EMP )
> return TX_FIFO_SIZE;
> else if ( reg & STAT_TX_FIFO_HFL )

no else needed.
I'd write it like:
if (empty)
return FIFO_SIZE;

if (full)
return 0;

if (half)
return FIFO_SIZE / 2;

/* comment ... */
return 1;

Cheers,
Andre.

> return TX_FIFO_SIZE / 2;
> else if ( !(reg & STAT_TX_FIFO_FUL) )
> return 1;
> 
>return 0;
> 
> Thanks
> -Amit
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] xen/arm: Add MVEBU UART driver for Marvell Armada 3700 SoC

[André Przywara]

On 04/04/18 23:25, Amit Tomer wrote:
> Hi,
> 
>> Just a nit, but it might be smarter to first check for the receive IRQ,
>> because not handling this might loose information. TX is less critical.
> 
> Ok, actually tried it ealier when  I was debugging the Rx path issue and
> that time it didn't work. May be I should try it again.
> 
> 
>> Similar to the earlyprintk behaviour we are a bit pessimistic here. I
>> don't know if there is a way to determine the number of free characters
>> in the FIFO, but we could at least return 1 if the FIFO is not full:
>>
>> if (fifo_empty)
>> return FIFO_SIZE;
>> if (!fifo_full)
>> return 1;
>> return 0;
> 
> Yeah, I thought of it initially like should we also return half the
> size of FIFO(16) when STAT_TX_FIFO_HFL bit is set ?

Ah yes, if that works, we should. That bit is from the Linux BSP, isn't it?

Cheers,
Andre.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1] xen/arm: Add MVEBU UART driver for Armada 3700 SoC

[André Przywara]

Hi,

I tested this on my board and it works like expected. I would very much
like to see this driver still in 4.11.

Some (minor) comments on the code below.

On 16/03/18 17:34, Amit Singh Tomar wrote:
> This patch adds driver for UART controller found on Armada 3700 SoC.

Can you please mention "Marvell" in the subject?

> There is no reference manuals available for 3700 SoC in public and this
> driver is derived by looking at Linux driver.
> https://github.com/torvalds/linux/blob/master/drivers/tty/serial/mvebu-uart.c
> 
> It allows XEN to boot on ESPRESSObin board based on Marvell's ARMADA 3700 SoC.
> 
> Signed-off-by: Amit Singh Tomar 
> ---
> Changes since RFC:
>   * Addressed Julien's Comments. 
> TODO:
>   * Wiki page to capture XEN boot info.
>   * earlyprintk support.

This has been done already. Thanks! Please provide a link to the Wiki page.

> ---
>  xen/drivers/char/Kconfig |   8 ++
>  xen/drivers/char/Makefile|   1 +
>  xen/drivers/char/mvebu-uart.c| 260 
> +++
>  xen/include/asm-arm/mvebu-uart.h |  60 +
>  4 files changed, 329 insertions(+)
>  create mode 100644 xen/drivers/char/mvebu-uart.c
>  create mode 100644 xen/include/asm-arm/mvebu-uart.h
> 
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index fb53dd8..690eda6 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -12,6 +12,14 @@ config HAS_CADENCE_UART
> This selects the Xilinx Zynq Cadence UART. If you have a Xilinx Zynq
> based board, say Y.
>  
> +config HAS_MVEBU
> +bool
> +default y
> +depends on ARM_64
> +help
> +  This selects the Marvell MVEBU UART. if you have an ARMADA 3700
> +  based board, say Y.
> +

These should be indented by one tab (plus two spaces for the help text).
It's not obvious - I got this wrong myself the other day ;-), but it's
how the rest of the file works.

>  config HAS_PL011
>   bool
>   default y
> diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
> index 0d48b16..b68c330 100644
> --- a/xen/drivers/char/Makefile
> +++ b/xen/drivers/char/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_HAS_NS16550) += ns16550.o
>  obj-$(CONFIG_HAS_CADENCE_UART) += cadence-uart.o
>  obj-$(CONFIG_HAS_PL011) += pl011.o
>  obj-$(CONFIG_HAS_EXYNOS4210) += exynos4210-uart.o
> +obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
>  obj-$(CONFIG_HAS_OMAP) += omap-uart.o
>  obj-$(CONFIG_HAS_SCIF) += scif-uart.o
>  obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
> diff --git a/xen/drivers/char/mvebu-uart.c b/xen/drivers/char/mvebu-uart.c
> new file mode 100644
> index 000..c88d5e7
> --- /dev/null
> +++ b/xen/drivers/char/mvebu-uart.c
> @@ -0,0 +1,260 @@
> +/*
> + * xen/drivers/char/mvebu3700-uart.c
> + *
> + * Driver for Marvell MVEBU UART.
> + *
> + * Amit Singh Tomar 
> + * Copyright (c) 2018.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static struct mvebu3700_uart {
> +unsigned int irq;
> +void __iomem *regs;
> +struct irqaction irqaction;
> +struct vuart_info vuart;
> +} mvebu3700_com = {0};
> +
> +#define mvebu3700_read(uart, off)   readl((uart)->regs + off)
> +#define mvebu3700_write(uart, off, val) writel(val, (uart->regs) + off)
> +
> +static void mvebu3700_uart_interrupt(int irq, void *data,
> + struct cpu_user_regs *regs)
> +{
> +struct serial_port *port = data;
> +struct mvebu3700_uart *uart = port->uart;
> +uint32_t st = mvebu3700_read(uart, UART_STATUS_REG);
> +
> +if ( st & STATUS_TX_RDY )
> +serial_tx_interrupt(port, regs);
> +
> +if ( st & (STATUS_RX_RDY | STATUS_OVR_ERR | STATUS_FRM_ERR |
> +   STATUS_BRK_DET) )
> +serial_rx_interrupt(port, regs);
> +}
> +
> +static void __init mvebu3700_uart_init_preirq(struct serial_port *port)
> +{
> +struct mvebu3700_uart *uart = port->uart;
> +uint32_t reg;
> +
> +reg = mvebu3700_read(uart, UART_CTRL_REG);
> +reg |= (CTRL_TXFIFO_RST | CTRL_RXFIFO_RST);
> +mvebu3700_write(uart, UART_CTRL_REG, reg);
> +
> +/* Before we make IRQ request, clear the error bits of state register. */
> +reg = mvebu3700_read(uart, UART_STATUS_REG);
> +reg |= STATUS_BRK_ERR;
> +mvebu3700_write(uart, UART_STATUS_REG, reg);
> +
> +/* Clear error i

Re: [Xen-devel] [PATCH v3a 14/39] ARM: new VGIC: Add GICv2 world switch backend

[André Przywara]

On 27/03/18 20:41, Stefano Stabellini wrote:
> On Tue, 27 Mar 2018, Andre Przywara wrote:
>> Hi,
>>
>> On 27/03/18 00:22, Stefano Stabellini wrote:
>>> On Thu, 22 Mar 2018, Andre Przywara wrote:
 Processing maintenance interrupts and accessing the list registers
 are dependent on the host's GIC version.
 Introduce vgic-v2.c to contain GICv2 specific functions.
 Implement the GICv2 specific code for syncing the emulation state
 into the VGIC registers.
 This also adds the hook to let Xen setup the host GIC addresses.

 This is based on Linux commit 140b086dd197, written by Marc Zyngier.

 Signed-off-by: Andre Przywara 
 ---
 Changelog v3 ... v3a:
 - take hardware IRQ lock in vgic_v2_fold_lr_state()
 - fix last remaining u32 usage
 - print message when using new VGIC
 - add TODO about racy _IRQ_INPROGRESS setting

 Changelog v2 ... v3:
 - remove no longer needed asm/io.h header
 - replace 0/1 with false/true for bool's
 - clear _IRQ_INPROGRESS bit when retiring hardware mapped IRQ
 - fix indentation and w/s issues

 Changelog v1 ... v2:
 - remove v2 specific underflow function (now generic)
 - re-add Linux code to properly handle acked level IRQs

  xen/arch/arm/vgic/vgic-v2.c | 259 
 
  xen/arch/arm/vgic/vgic.c|   6 +
  xen/arch/arm/vgic/vgic.h|   9 ++
  3 files changed, 274 insertions(+)
  create mode 100644 xen/arch/arm/vgic/vgic-v2.c

 diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
 new file mode 100644
 index 00..1773503cfb
 --- /dev/null
 +++ b/xen/arch/arm/vgic/vgic-v2.c
 @@ -0,0 +1,259 @@
 +/*
 + * Copyright (C) 2015, 2016 ARM Ltd.
 + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as
 + * published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope that it will be useful,
 + * but WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License
 + * along with this program.  If not, see .
 + */
 +
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +
 +#include "vgic.h"
 +
 +static struct {
 +bool enabled;
 +paddr_t dbase;  /* Distributor interface address */
 +paddr_t cbase;  /* CPU interface address & size */
 +paddr_t csize;
 +paddr_t vbase;  /* Virtual CPU interface address */
 +
 +/* Offset to add to get an 8kB contiguous region if GIC is aliased */
 +uint32_t aliased_offset;
 +} gic_v2_hw_data;
 +
 +void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
 +  paddr_t vbase, uint32_t aliased_offset)
 +{
 +gic_v2_hw_data.enabled = true;
 +gic_v2_hw_data.dbase = dbase;
 +gic_v2_hw_data.cbase = cbase;
 +gic_v2_hw_data.csize = csize;
 +gic_v2_hw_data.vbase = vbase;
 +gic_v2_hw_data.aliased_offset = aliased_offset;
 +
 +printk("Using the new VGIC implementation.\n");
 +}
 +
 +/*
 + * transfer the content of the LRs back into the corresponding ap_list:
 + * - active bit is transferred as is
 + * - pending bit is
 + *   - transferred as is in case of edge sensitive IRQs
 + *   - set to the line-level (resample time) for level sensitive IRQs
 + */
 +void vgic_v2_fold_lr_state(struct vcpu *vcpu)
 +{
 +struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
 +unsigned int used_lrs = vcpu->arch.vgic.used_lrs;
 +unsigned long flags;
 +unsigned int lr;
 +
 +if ( !used_lrs )/* No LRs used, so nothing to sync back here. */
 +return;
 +
 +gic_hw_ops->update_hcr_status(GICH_HCR_UIE, false);
 +
 +for ( lr = 0; lr < used_lrs; lr++ )
 +{
 +struct gic_lr lr_val;
 +uint32_t intid;
 +struct vgic_irq *irq;
 +struct irq_desc *desc = NULL;
 +bool have_desc_lock = false;
 +
 +gic_hw_ops->read_lr(lr, &lr_val);
 +
 +/*
 + * TODO: Possible optimization to avoid reading LRs:
 + * Read the ELRSR to find out which of our LRs have been cleared
 + * by the guest. We just need to know the IRQ number for those, 
 which
 + * we could save in an array when populating the LRs.
 + * This trades one MMIO access 

Re: [Xen-devel] [PATCH v3 13/39] ARM: new VGIC: Add IRQ sync/flush framework

[André Przywara]

On 27/03/18 20:20, Stefano Stabellini wrote:
> On Tue, 27 Mar 2018, Stefano Stabellini wrote:
>> On Tue, 27 Mar 2018, Andre Przywara wrote:
>>> Hi,
>>>
>>> On 26/03/18 22:30, Stefano Stabellini wrote:
 On Wed, 21 Mar 2018, Andre Przywara wrote:
> Implement the framework for syncing IRQs between our emulation and the
> list registers, which represent the guest's view of IRQs.
> This is done in vgic_sync_from_lrs() and vgic_sync_to_lrs(), which
> get called on guest entry and exit, respectively.
> The code talking to the actual GICv2/v3 hardware is added in the
> following patches.
>
> This is based on Linux commit 0919e84c0fc1, written by Marc Zyngier.
>
> Signed-off-by: Andre Przywara 

 Just one question below, but the code looks nice


> ---
> Changelog v2 ... v3:
> - replace "true" instead of "1" for the boolean parameter
>
> Changelog v1 ... v2:
> - make functions void
> - do underflow setting directly (no v2/v3 indirection)
> - fix multiple SGIs injections (as the late Linux bugfix)
>
>  xen/arch/arm/vgic/vgic.c | 232 
> +++
>  xen/arch/arm/vgic/vgic.h |   2 +
>  2 files changed, 234 insertions(+)
>
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index ee0de8d2e0..52e1669888 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -409,6 +409,238 @@ void vgic_inject_irq(struct domain *d, struct vcpu 
> *vcpu, unsigned int intid,
>>>
>>> 
>>>
> +/* Requires the ap_list_lock to be held. */
> +static int compute_ap_list_depth(struct vcpu *vcpu)
> +{
> +struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
> +struct vgic_irq *irq;
> +int count = 0;
> +
> +ASSERT(spin_is_locked(&vgic_cpu->ap_list_lock));
> +
> +list_for_each_entry(irq, &vgic_cpu->ap_list_head, ap_list)
> +{
> +spin_lock(&irq->irq_lock);
> +/* GICv2 SGIs can count for more than one... */
> +if ( vgic_irq_is_sgi(irq->intid) && irq->source )
> +count += hweight8(irq->source);

 Why is this done?
>>>
>>> GICv2 SGIs always have a source CPU ID connected to them. So if two CPUs
>>> signal another CPU at the same time, there are *two* distinct SGIs, with
>>> two different source IDs. This is an architectural feature of GICv2, so
>>> we have to properly emulate this.
>>> Despite them being edge triggered IRQs, we cannot coalesce them in this
>>> case.
>>
>> I went through the whole lifecycle of SGIs with the new vgic and it is
>> quite different from before, but it makes sense to me now.
>>
>> Acked-by: Stefano Stabellini 
> 
> Actually, I take it back, one more question :-)
> 
> I understand that every bit set in irq->source corresponds to a
> different interrupt that needs to be injected into the guest. They are
> distinct interrupts.
> 
> However, compute_ap_list_depth is called to figure out whether the
> entries in ap_list overflow the LR registers, and it is never the case
> that we write to more than one LR register for a given SGI, even if
> irq->source has multiple bit sets, right?

Yes, that was actually a recent change in KVM:
https://lists.cs.columbia.edu/pipermail/kvmarm/2018-March/030226.html

So I basically took this patch right into the series.

Now there are more subtleties about priorities (see the follow-ups on
this thread), which actually led to a different patch being merged:
https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git/commit/?id=16ca6a607
(what I only learned today).
So this is a bit more sophisticated, and needs some porting because of
the new "empty LR" interrupt. I would rather do this on top.

> In a concrete example, if we have 3 LR registers and 3 interrupts in
> ap_list, one of them is an SGI with multiple irq->source bits, there is
> still no need to sort the ap_list, correct?
> 
> I think we should remove the special if statement for sgis in
> compute_ap_list_depth.

Yes, I believe this is a good intermediate measure, until we get the
proper solution.
Let me test this tomorrow, then I can push a reworked version of this patch.

Cheers,
Andre.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen/arm: gic: Fix indentation in gic_update_one_lr

[André Przywara]

On 09/03/18 17:35, julien.gr...@arm.com wrote:
> From: Julien Grall 
> 
> Signed-off-by: Julien Grall 

Reviewed-by: Andre Przywara 

Cheers,
Andre.

> ---
>  xen/arch/arm/gic-vgic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 61f093db50..e3cb47e80e 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -197,8 +197,8 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>  {
>  if ( p->desc == NULL )
>  {
> - lr_val.state |= GICH_LR_PENDING;
> - gic_hw_ops->write_lr(i, &lr_val);
> +lr_val.state |= GICH_LR_PENDING;
> +gic_hw_ops->write_lr(i, &lr_val);
>  }
>  else
>  gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into 
> d%dv%d: already active in LR%d\n",
> 


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Xen on POWER

[André Przywara]

On 09/03/18 13:35, awokd wrote:
> On Fri, March 9, 2018 11:52 am, awokd wrote:
> 
>> Xen on ARM might be a more reasonable starting point but I'm not sure
>> that would provide enough horsepower to drive a workstation

That's indeed an issue. There are ARM64 SoCs which are very capable and
could easily match or even outperform commodity Intel desktop hardware,
but no-one offers them in a desktop or workstation package. I guess the
seemingly dwindling desktop market is not very attractive to vendors.

>> and have
>> (possibly
>> unfounded) concerns about the platform following in x86's footsteps with
>> TrustZone and end-user lock out.
> 
> Sorry, didn't realize I was replying to someone @arm.com! My possibly
> mistaken perception is that ARM requires proprietary blobs and TrustZone
> provides a way to lock out end-users so they can't provide their own boot
> binaries. In some applications that's a good thing, but not for personal
> use.

That's a common misconception. Trustzone is the marketing name of a
rather innocent, but indeed quite clever architectural feature, namely
to separate the potentially vulnerable "non-secure" OS from some other
trusted code, for instance firmware. The neat property is that this
extends to hardware devices, which can be sorted into one of those two
groups. But how this is actually used is entirely up to the particular
platform.
The boot binary protection is a concept popular in embedded systems and
mobile phones, which is what ARM is often associated with.
But many server like systems (including storage or network focused
boards) run Open Source implementations of the firmware, for instance
the BSD licensed "ARM Trusted Firmware", developed on Github. This makes
the whole firmware on those boards replaceable and visitable. *Some*
systems may ship binary blobs, but this is mostly for system
initialization (DRAM training, platform setup) and the code won't stay
around at OS runtime. Eventually those could be replaced as well.
In fact on those systems you could actually use TrustZone to your
advantage, for instance for storing private keys in a place inaccessible
by the normal (read: hackable) OS, and only offering software interfaces
to encrypt data. The Open Source OP-TEE framework for instance is
exploring this direction.

> It's entirely possible I'm unfairly tarring ARM with the same x86 brush so
> if I'm mistaken on any of these aspects please feel free to correct me
> publicly.

No worries about that, and I actually didn't want to point you to ARM
platforms primarily (though this might have been a interesting side
effect. ;-)
I was just wondering if coming up with a whole new platform port of Xen
is the right and reasonable answer to you security concerns on your
existing platform. I very much fancy the idea of alternative platforms
(even non-ARM ;-), but it might be more worthwhile to support those
people who work on fixing or mitigating the existing problems (x86
binary blob firmware). Or even better: Piggy-back on the already
existing ARM64 port of Xen and help improving that.
Given from our experience there are many very detailed and intricate
problems to solve in such a platform port, especially since Power
deviates from both x86 and ARM in some ways (for instance paging and
TLBs). Also things like memory model and ordering, asynchronous
exceptions and tons of races in various places need to be considered.
Starting from scratch here sounds like a mammoth task, judging from the
bugs that we still discover on a somewhat weekly base here in the
existing Xen ports.
I don't want to steer you away from the Power on Xen port, it might
actually be beneficial to the Xen ecosystem, but if you are hoping for
having something usable in a year or so, you might be disappointed ;-)
At least without having the proper resources, to somewhat support
George's suggestions.

Cheers,
Andre.


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] ARM: GICv3: copy Dom0 GICv3 reg property from host DT

[André Przywara]

On 30/01/18 19:04, Stefano Stabellini wrote:
> On Tue, 30 Jan 2018, Andre Przywara wrote:
>> At the moment we re-generate the Dom0 GICv3 DT node, by creating the
>> "reg" property from scratch using our previously parsed and
>> translated(!) host addresses. However we then write the *absolute*
>> addresses into the new node, not considering possible "range" mappings
>> in any of the GIC's parent nodes. So whenever one of the parents has a
>> non-empty ranges property, Dom0 will wrongly translate the addresses.
>> Properly incorporating the ranges properties sounds tedious, so let's
>> just copy the first part of the reg property instead (as we do for GICv2),
>> since the addresses for Dom0 are identical to those from the hardware.
>>
>> The mainline kernel DT for the Espressobin board with an Marvell 3720 SoC
>> has the GIC in such an translated bus, so this patch allows this board
>> to boot properly (after adding support for the SoC's UART).
>>
>> Signed-off-by: Andre Przywara 
> 
> There is one code style issue below, but I'll fix it on commit.

Argh, indeed, Julien found this as well.
Thanks for the smooth handling!

Cheers,
Andre.

> Reviewed-by: Stefano Stabellini 
> 
>> ---
>>  xen/arch/arm/gic-v3.c | 29 +++--
>>  1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
>> index a0d290b55c..6b17abd0a1 100644
>> --- a/xen/arch/arm/gic-v3.c
>> +++ b/xen/arch/arm/gic-v3.c
>> @@ -1147,10 +1147,9 @@ static int gicv3_make_hwdom_dt_node(const struct 
>> domain *d,
>>  const struct dt_device_node *gic,
>>  void *fdt)
>>  {
>> -const void *compatible = NULL;
>> -uint32_t len;
>> -__be32 *new_cells, *tmp;
>> -int i, res = 0;
>> +const void *compatible, *hw_reg;
>> +uint32_t len, new_len;
>> +int res;
>>  
>>  compatible = dt_get_property(gic, "compatible", &len);
>>  if ( !compatible )
>> @@ -1173,27 +1172,21 @@ static int gicv3_make_hwdom_dt_node(const struct 
>> domain *d,
>>  if ( res )
>>  return res;
>>  
>> -len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>> +new_len = dt_cells_to_size(dt_n_addr_cells(gic) + dt_n_size_cells(gic));
>>  /*
>>   * GIC has two memory regions: Distributor + rdist regions
>>   * CPU interface and virtual cpu interfaces accessesed as System 
>> registers
>>   * So cells are created only for Distributor and rdist regions
>>   */
>> -len = len * (d->arch.vgic.nr_regions + 1);
>> -new_cells = xzalloc_bytes(len);
>> -if ( new_cells == NULL )
>> -return -FDT_ERR_XEN(ENOMEM);
>> -
>> -tmp = new_cells;
>> -
>> -dt_set_range(&tmp, gic, d->arch.vgic.dbase, SZ_64K);
>> +new_len = new_len * (d->arch.vgic.nr_regions + 1);
>>  
>> -for ( i = 0; i < d->arch.vgic.nr_regions; i++ )
>> -dt_set_range(&tmp, gic, d->arch.vgic.rdist_regions[i].base,
>> - d->arch.vgic.rdist_regions[i].size);
>> +hw_reg = dt_get_property(gic, "reg", &len);
>> +if ( !hw_reg )
>> +return -FDT_ERR_XEN(ENOENT);
>> +if ( new_len > len )
>> +return -FDT_ERR_XEN(ERANGE);
>>  
>> -res = fdt_property(fdt, "reg", new_cells, len);
>> -xfree(new_cells);
>> +res = fdt_property(fdt, "reg", hw_reg, new_len);
>>  if ( res )
>>  return res;
>>  
>> -- 
>> 2.14.1
>>


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel