Re: [Qemu-devel] [PATCH v4 00/17] arm_gic: Add security and grouping support

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:26PM +0100, Peter Maydell wrote:
> This patch series adds support for GICv1 and GICv2 security
> extensions, as well as support for GIC interrupt grouping on GICv2.

A question. Once we enable the the security extensions on the GICs,
do you have any suggestions on howto best handle direct boots into
NS EL2/1 (Linux)?

The GIC resets to all interrupts configured for Group0 and Linux running
in NS mode cannot change that so we need some kind of boot-loader
code or magic to do what firmware would have been expected to do
at boot time (switch some irqs to NS).

Cheers,
Edgar


> 
> This is based on the work originally by Fabian and then by Greg.
> I've gone through and dealt with all the issues I raised in code
> review, and a few others I noticed as I was working on it.
> The general structure of the changes is still the same, though
> I've reordered one or two of the patches; I've touched most of
> the lines of code in the series, though, as well as deleting
> quite a few (the patches now add ~375 lines of code rather than
> over 475).
> 
> I think these patches are in a suitable state to apply (and
> they have no dependencies that aren't in master), assuming no
> further issues found in review.
> 
> With this patchset the security extensions are still disabled
> on all boards, so the actual functional change is that GICv2
> now correctly implements interrupt grouping. This is enabled
> always for GICv2, because the programming model is fully backwards
> compatible with treating the GIC like one which doesn't support
> groups (which is what Linux does).
> 
> The next part of this work is going to be actually enabling
> the security extensions. Here's a sketch of my plan for that:
>  * the a15mpcore and a9mpcore wrapper objects will default to
>    enabling the security extensions in the GIC they create
>    (unless the GIC is the KVM one). They also provide a
>    QOM property to override this
>  * for the set of legacy boards which are currently disabling
>    has_el3 on their CPUs, we also have them disable TZ in the GIC
>    (a non-TZ CPU and a TZ GIC is a bad combo because the CPU
>    has no way to put the interrupts into Group1 where it can
>    use them, so the whole system is busted)
>  * the virt board creates its GIC directly, so it should also
>    set the has-security-extensions property as needed
>  * if boot.c is starting the CPUs directly in NonSecure
>    mode (because we're booting a kernel directly rather than
>    starting firmware, and arm_boot_info::secure_boot is false)
>    then it must also manually configure the GIC distributor
>    to put all interrupts into Group1. This is boot.c having
>    to do a firmware configuration job since it's effectively
>    acting as lightweight builtin firmware.
> 
> I think we could reasonably review and commit this patchseries
> without waiting for that bit of board-wiring work; let me know
> if you disagree.
> 
> 
> Major changes since v3:
>  * renamed property to 'has-security-extensions', to be a bit
>    more in line with the CPU's 'has_el3'. I'm not wedded to this
>    name so if anybody wants to suggest something better (or
>    tell me our convention for prop names is underscores!) feel free
>  * error on realize if security extensions turned on for a GIC
>    which doesn't support them
>  * new patch: switch to read/write_with_attrs MMIO callbacks so
>    we can get at the Secure/NonSecure tx attribute
>  * make the GIC_*_GROUP macros work like the others, with a simple
>    SET/CLEAR/TEST semantic
>  * new patch: save and restore GICD_IGROUPRn state when using KVM
>    now we have the state struct fields to keep it in [the kernel
>    doesn't implement grouping, but if it ever does we will be ready]
>  * rather than having a 2-element array for storing the S and NS
>    banked versions of GICD_CTLR and GICC_CTLR, just store the S
>    version, since in both cases the NS view is just an alias of
>    a subset of bits from the S register. This allows us to nicely
>    simplify a lot of the logic that deals with these registers.
>  * fixed bug in handling of GICC_BPR for GICv2-without-TZ
>  * added missing masks in gic_set_priority_mask() and gic_set_priority()
>  * make AckCtl operate on GICv2-without-TZ
>  * handle an UNPREDICTABLE case (Secure EOI of a Group1 irq
>    with AckCtl == 0) in a way more convenient for the implementation
>  * reuse gic_get_current_pending_irq() in implementation of IAR writes,
>    rather than reimplementing equivalent logic
>  * new patch: support grouping in a single gic_update function (rather
>    than having split update functions for the two cases)
>  * new patch: wire FIQ up on highbank/midway; this means we're now
>    consistent in having FIQ wired up on all our boards with GICv2
>  * lots of minor formatting tweaks, etc; see individual commit messages
> 
> 
> Fabian Aggeler (12):
>   hw/intc/arm_gic: Create outbound FIQ lines
>   hw/intc/arm_gic: Add Security Extensions property
>   hw/intc/arm_gic: Add Interrupt Group Registers
>   hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked
>   hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked
>   hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked
>   hw/intc/arm_gic: Implement Non-secure view of RPR
>   hw/intc/arm_gic: Restrict priority view
>   hw/intc/arm_gic: Handle grouping for GICC_HPPIR
>   hw/intc/arm_gic: Change behavior of EOIR writes
>   hw/intc/arm_gic: Change behavior of IAR writes
>   hw/arm/vexpress.c: Wire FIQ between CPU <> GIC
> 
> Greg Bellows (1):
>   hw/arm/virt.c: Wire FIQ between CPU <> GIC
> 
> Peter Maydell (4):
>   hw/intc/arm_gic: Switch to read/write callbacks with tx attributes
>   hw/intc/arm_gic_kvm.c: Save and restore GICD_IGROUPRn state
>   hw/intc/arm_gic: Add grouping support to gic_update()
>   hw/arm/highbank.c: Wire FIQ between CPU <> GIC
> 
>  hw/arm/highbank.c                |   3 +
>  hw/arm/vexpress.c                |   2 +
>  hw/arm/virt.c                    |   2 +
>  hw/intc/arm_gic.c                | 469 
> ++++++++++++++++++++++++++++++++-------
>  hw/intc/arm_gic_common.c         |  22 +-
>  hw/intc/arm_gic_kvm.c            |  51 +++--
>  hw/intc/armv7m_nvic.c            |   8 +-
>  hw/intc/gic_internal.h           |  29 ++-
>  include/hw/intc/arm_gic_common.h |  24 +-
>  9 files changed, 492 insertions(+), 118 deletions(-)
> 
> -- 
> 1.9.1
>