Enable IPIs on CPU 0 on arm and arm64

[andrew]

Summary

We seem to have relied on IPIs being enabled before entering the kernel
on arm and arm64. This appears to not be the case on VMWare ESXi-Arm.
Fix this by enabling them after we have configured them.

[emaste]

Test result: https://twitter.com/DarkainMX/status/1321124223805386753

[mmel]

I'm afraid it's not that simple.

• the GICv2 architecture manual states for GICD_ISENABLE[0] this: "for SGIs the behavior of the bit on reads and writes is IMPLEMENTATION DEFINED".
• for all ARM GICs up to GIC-500 all SGIs are always enabled.
• GICD_ISENABLE[0] is declared as RO for older versions of GICs (i.e. PL390) and attempt to write to it may cause abort.
• GIC-600 have this slightly cryptic note for GICD_ISENABLE register block: "The first one of these registers does not exist when affinity routing is enabled."

So I think that access to GICD_ISENABLE[0] should be moved to particular GIC driver initialization routine and only for exact version of GIC.
Moreover, even though I have no idea which version of GIC is used in ESXi experiment, I'm afraid that something is wrong with its GIC emulation/virtualization code (well, GIC-600 may be exception).

[emaste]

@mmel twitter feedback: https://twitter.com/cypou/status/1321428823490088964

[andrew]

We already enable the SGIs on the non-boot CPUs in arm_gic_init_secondary and PIC_ENABLE_INTR will only be called when mp_ncpus != 1 so will be in a multicore system.

[mmel]

In D26975#601910, @andrew wrote:

We already enable the SGIs on the non-boot CPUs in arm_gic_init_secondary and PIC_ENABLE_INTR will only be called when mp_ncpus != 1 so will be in a multicore system.

I think that calling gic_irq_mask() for SGIs is redundant and potentially dangerous, we should remove it (or eventually, make it specific only for exact GIC implementation .
The right place for doing same on boot CPU is arm_gic_attach() again only conditionally for specific version.

Don't take my badly, I only think that unconditional write to implementation defined register is not right code style, nothing else.

I just tried that clearing SGI enable bits (on boot and secondary cores) doesn't breaks nothing. But I have not board with GIC-500 or GIC-600.

[cyprien_cypou.net]

In D26975#602006, @mmel wrote:

I think that calling gic_irq_mask() for SGIs is redundant and potentially dangerous, we should remove it (or eventually, make it specific only for exact GIC implementation .
The right place for doing same on boot CPU is arm_gic_attach() again only conditionally for specific version.

I think from GICv2 it would be assumed that the OS could write to the SGI bits. Software cannot know all existing implementations.

I just tried that clearing SGI enable bits (on boot and secondary cores) doesn't breaks nothing. But I have not board with GIC-500 or GIC-600.

For GIC-600, "when affinity routing is enabled" means that it's GICv3 not in GICv2 compat mode. The GIC code won't be the same.

[andrew]

The GIC-500 TRM has the following note against GICD_ISENABLER0: "Writes to bits corresponding to the SGIs are ignored."

However the GICv2 spec allows for the behaviour VMWare implements: "Whether implemented SGIs are permanently enabled, or can be enabled and disabled by writes to GICD_ISENABLER0 and GICD_ICENABLER0, is IMPLEMENTATION DEFINED."

I've tested against a GIC-600 using the GICv3 driver & saw no problems. That driver uses the GICR_ISENABLER0 register which is defined in the GICv3 spec as read/write with an implementation defined initial value.

[mmel]

Andrew, I'm sorry but I don't quite understand why we crossing.
I will try to repeat my objections in a more compact/consistent form.
I have 3 objections:

1. most important for me. The SGI enable should be initiated by driver itself. Proposed solution, where SGI enable register for boot CPU is initialized from common machine code but enable bits for secondary CPUs are initialized from driver code is clear layering violation. Please, move the initialization for boot CPU also to driver code. I already suggested arm_gic_attach() as optimal place(at least for me).
2. is minor (because it's clear that it cannot affect all current systems). I still thinking that we should limit writes to implementation defined registers only to given known implementation. Nothing more, nothing less.
3. is not related to FreeBSD or first two objections. I fully agree with "However the GICv2 spec allows for the behavior VMWare implements" but with limitation that VMWare also changed ICDIIDR. If not, then VMWare implementations clearly violates given specific GIC-400 (or GIC-500) TRM. (Please take also to account that we already have systems where GIC is not a root interrupt controller and these irregularities may made their implementations more complicated - but of course, I have not a real example of this)

[mmel]

In D26975#602036, @cyprien_cypou.net wrote:

I think from GICv2 it would be assumed that the OS could write to the SGI bits. Software cannot know all existing implementations.

I'm sorry but my interpretation of this fact is exactly opposite. Because software cannot know all existing implementations (thus it cannot predict exact functionality of these bits) we are not allowed to blindly change it. Mainly if TRM for given parts exactly specifies this register as RO...

For GIC-600, "when affinity routing is enabled" means that it's GICv3 not in GICv2 compat mode. The GIC code won't be the same.

OK, many thanks for clarification.

But, please, can you post full verbose bool log from this system? Thanks.

[cyprien_cypou.net]

In D26975#602470, @mmel wrote:

In D26975#602036, @cyprien_cypou.net wrote:

I think from GICv2 it would be assumed that the OS could write to the SGI bits. Software cannot know all existing implementations.

I'm sorry but my interpretation of this fact is exactly opposite. Because software cannot know all existing implementations (thus it cannot predict exact functionality of these bits) we are not allowed to blindly change it. Mainly if TRM for given parts exactly specifies this register as RO...

IHI0048B_b for the GICv2 Architecture Specification, Chapter 3.1.2:

If an interrupt is:
• not supported, the Set-enable bit corresponding to its interrupt ID is RAZ/WI
• supported and permanently enabled, the Set-enable bit corresponding to its interrupt ID is RAO/WI.

The document then advises to write 0xFFFFFFFF to GICD_ISENABLER0 and GICD_ICENABLER0 to detect supported interrupts and those who are permanently enabled.

[cyprien_cypou.net]

In D26975#602470, @mmel wrote:

But, please, can you post full verbose bool log from this system? Thanks.

I'm new to FreeBSD so I don't know if the "full verbose boot log" is anything different than dmesg?
and if there is a better way to attach a file to a review.

In the meantime, here it is: http://cypou.free.fr/dmesg_esxi_arm_pi4.txt

Note: there are IIDR registers in GICD, v2m, GICR, GICC and for GICv3 MSI frame. The one shown in dmesg is only the GICC one, which is the Arm virtual GIC Cpu Interface, not VMware's. Right now ESXi would return 0 for all of those.

[cyprien_cypou.net]

IHI0048A for the GICv1 Architecture Specification, Chapter 4.3.6 on the Interrupt Clear-Enable Registers (ICDICERn):

A register bit corresponding to an unimplemented interrupt is RAZ/WI.
[...]
Support of Clear-enable bits for SGIs is IMPLEMENTATION DEFINED .

What I understand is that writes to the SGI bits when not implemented must be ignored.

[mmel]

In D26975#602614, @cyprien_cypou.net wrote:

IHI0048B_b for the GICv2 Architecture Specification, Chapter 3.1.2:

If an interrupt is:
• not supported, the Set-enable bit corresponding to its interrupt ID is RAZ/WI
• supported and permanently enabled, the Set-enable bit corresponding to its interrupt ID is RAO/WI.

The document then advises to write 0xFFFFFFFF to GICD_ISENABLER0 and GICD_ICENABLER0 to detect supported interrupts and those who are permanently enabled.

Yes, that is true. I missed the "Identifying the supported interrupts" chapter.

If memory serves me correctly then PL390 was first member of GIC family (and used in many armv7 SOCs)
and PL390 TRM declares SGI part of ICDISER register block as RO ( see page 3-9 of ARM DDI 0416B) .
But, as I already wrote above in point 2, I taking this as minor problem

In D26975#602615, @cyprien_cypou.net wrote:

In the meantime, here it is: http://cypou.free.fr/dmesg_esxi_arm_pi4.txt

Note: there are IIDR registers in GICD, v2m, GICR, GICC and for GICv3 MSI frame. The one shown in dmesg is only the GICC one, which is the Arm virtual GIC Cpu Interface, not VMware's. Right now ESXi would return 0 for all of those.

I thought boot -v but even this listing is sufficient, thanks.

So yes, the emulation provided by ESXi is clearly incorrect. This behavior is against the GIC-400 specification. In addition, it also raises a big question (if they want to have writable SGI bits). Which compatibility string should be specified in device tree?
None of https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/arm%2Cgic.yaml
can be used (because all of these always have SGI enabled).
But this is of course VMWare problem.

And again, addressing of point 1 from above list is more that sufficient for me.

[freebsd_darkain.com]

This two-line patch fixes SMP under Parallels on the m1 MacBook Air (and probably all other Apple Silicon systems). With testing to ensure it doesn't break any other platforms (I dont think it will), this patch should then be included in the main kernel in my opinion.

https://twitter.com/DarkainMX/status/1384724684902600706

[tuexen]

I can confirm that this patch allows FreeBSD to use more than one CPU on Parallels Desktop. I can't comment if this patch is architecturally correct, but I would like to see a patch allowing to use more than one CPU on Parallels Desktop to be committed. I'm more than happy to test any patch.