[Xen-devel,v2,00/45] New VGIC(-v2) implementation

tl;dr: Coarse changelog below, individual patches have changelogs as
well. git branch:
http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/vgic-new/v2
git://linux-arm.org/xen-ap.git branch vgic-new/v2

Another update, addressing the review comments. Nothing too outstanding this
time, the most interesting changes include:
- removing the split-out preparatory patches, which are already merged
- changing the setting and clearing of _IRQ_INPROGRESS
- including Julien's LR access rework series
- restricting new level IRQ handling to the new VGIC
- fix multiple SGI handling (mimicing the recent Linux/KVM patch)
- ASSERTing that h/w IRQs stay connected to their virtual IRQs
- directly update h/w affinity, without taking the desc lock
- restrict 8K struct vcpu to new VGIC and ARM64
- use separate Makefile for vgic/ directory
- many minor changes to address whitespace issues and usage of unsigned,
  also extending comments

A summarising changelog can be found below, each individual patch has
its own changelog as well.

There are some things that have (still) not been covered yet:
- struct VCPU still allocates two pages on ARM64 when using the new VGIC now.
We could try to look if we can allocate some parts of struct vcpu instead of
embedding sub-structures into it.
- vGICv3 support is not implemented, but should be fairly straight-forward to
add, as the design incorporated this already. Will look at this next.
- There is a possible DOS vector on the VCPU ap_list, which holds pending
vIRQs. A guest can make this list rather long, which forces the hypervisor
to hold the list lock when iterating the list. This should be bounded by
the number of emulated vIRQs though, and there are ideas how to mitigate
this issue. Those fixes would be posted on top as fixes later.
- There is no ITS support, though the VGIC code itself is more ready for that
than the old VGIC ever was. However due to differences between the Xen
and KVM architecture the ITS bits are not easy to port over to Xen.
- Do we need to call vgic_evtchn_irq_pending() in
local_event_needs_delivery_nomask()? The event channel IRQ should be covered
by the VGIC already.

Cheers,
Andre

=====================
During development of the Dom0 ITS MSI support last year we realised
that the existing GIC interrupt controller emulation has some shortcomings.
After some tries to fix those in the existing code, it was agreed upon
that the problems are fundamental and a new implementation based on the
"new VGIC" in KVM is the best choice.
This is this new VGIC implementation, based on the (heavily modified)
KVM version. It lives in the xen/arch/arm/vgic/ directory and is written
to be a compile time option, so people can choose whether to use the new
VGIC or the existing implementation. This is just for a transitional period,
the old VGIC is expected to be removed after confidence in the new
implementation has grown.

This series starts with some cleanup and refactoring patches for the
existing VGIC/GIC code, this includes preparations to properly support
level triggered interrupts. This is one of the biggest problems in the
existing VGIC, which only correctly emulates edge triggered IRQs. This
affects both arch code and some users like the timer and the event channel.

Starting with patch 14 we plumb in the new VGIC then. This is done in a
new directory, with all the files actually not wired into the build system
until the very last patch. The idea is to split the series into reviewable
chunks without resorting to nasty hacks to keep bisectability.
The code was forked from Linux' virt/kvm/arm/vgic/, as of 4.14-rc7, plus
some recent changes to improve support for level triggered and hardware
mapped interrupts, which is what we use heavily in Dom0. The code was
heavily adapted to fit into Xen, starting with using the Xen coding style
and using Xen structure and variable names (struct domain instead of
struct kvm, for instance). Where interfacing functions were similar enough,
they were changed over to the existing Xen name and prototypes (for instance
kvm_vgic_create() was renamed to domain_vgic_register()). As far as possible
the code layout and split was re-used from KVM, so patches in Linux should
be relatively easy to port into Xen. Due to the mentioned changes this can
not be done easily in an automatic way, but it should be not too complicated
to extract the gist of the patch and re-apply this to our code base.

The actual VGIC code splits into several parts:
- The core is the struct vgic_irq, which holds every information about a
virtual IRQ, including a per-IRQ lock. Also there is on (ordered) per-VCPU
list (ap_list), which links the interrupts to be considered by a VCPU.
There are functions to deal with queuing and removing IRQs from those lists
safely, obeying the locking order. (patches 14-18)
- There are functions to push vIRQs on a VCPU list to the list registers,
and handle their state changes. (patches 19-21)
- The distributor MMIO emulation is using separate functions per register,
also having read and write split. (patches 22-32)
- There are functions to deal with Xen specialities. (patches 33-39)
- The data structures and the wiring of the emulation into the hypervisor
  and the guests are done in vgic-init.c. (patches 40-43)
- Finally patch 45 enables the build of the new VGIC. This requires to
  increase the size limit for struct vcpu in patch 44.

Andre

Changelog v1 ... v2:
- add VCPU parameter to renamed gic_event_needs_delivery()
- use vcpu_kick, using existing x86 prototype
- include Julien's struct gic_lr rework series
- extend setting of _IRQ_INPROGRESS when tweaking active/pending state
- restrict level IRQ device handling to new VGIC
- cleanup vgic.h
- make vgic_inject_irq() and sync_{to,from}_lr() functions return void
- add dropped code to properly handle 
- split off introduction of Linux' list_sort() into separate patch
- fix handling of multiple-source-SGIs, as done in Linux recently
- use KVM IIDR identifier, but use different variant for Xen
- ASSERT that association between hardware and virtual IRQs do not change
- print warning on every IRQ failing to set/clear active bit
- avoid unneeded calls to vgic_sync_hardware_irq(), avoiding desc lock
- fixup wrong number of SPIs (not a multiple of 32)
- move vgic_v2_enable patch around
- confine two 4K pages for struct vcpu to new VGIC and ARM64
- use separate Makefile for new VGIC
- enhance Kconfig help text
- many whitespace and indentation fixes
- using more unsigned ints
- adding and extending comments

Changelog RFC ... v1:
- observe review comments on GICv3 redistributor patches
- implement physical-follows-virtual IRQ affinity
- actually implement arch_move_irq()
- move max_domain_vcpus() into vgic.c, to make it VGIC specific
- improved many commit messages
- add ACKs so far
- added and extended many comments
- use C99 data types (uint32_t)
- use unsigned data types
- use symbolic names for constants
- white space fixes (indentation mostly)
- adapt later patches to changes earlier in the series (renames etc.)
- use 32 bit data types where sufficient
- add helper functions as requested (for instance gicv2/3_peek/poke_irq)
- use struct irq_desc * in interface of hardware facing functions
- rename some existing Xen function names to be more readable
- rename new header file from arm_vgic.h to new_vgic.h
- drop code or variables dealing with unimplemented features (ITS, CPU i/f)
- reorder struct vgic_irq and use bitfield to shrink data structure size
- remove not needed functions (gic_clear_lrs(), save/restore_state())
- add ASSERTS as requested
- add locking where missing (dump_vgic_info, read pending state, enabling GIC)
- keep Linux coding style for list_sort.c
- add set_pending_state() GIC abstraction function
- factor out and use kick_vcpu()
- use frame number instead of physical address
- use existing LR accessor functions, drop GICH_ accesses from vgic-v2.c
- skip already disabled/enabled IRQs and setting enabled state
- use PRODUCT_ID_XEN
- simplify and clarify on ACTIVE bit MMIO accesses
- use interface for HCR bit changes
- iterate over set CPU bits in SGI injection handler

Andre Przywara (39):
  ARM: VGIC: rename gic_event_needs_delivery()
  ARM: Implement vcpu_kick()
  ARM: GIC: Allow tweaking the active and pending state of an IRQ
  ARM: GIC: Allow reading pending state of a hardware IRQ
  ARM: timer: Handle level triggered IRQs correctly
  ARM: evtchn: Handle level triggered IRQs correctly
  ARM: vPL011: Use the VGIC's level triggered IRQs handling if available
  ARM: new VGIC: Add data structure definitions
  ARM: new VGIC: Add acccessor to new struct vgic_irq instance
  ARM: new VGIC: Implement virtual IRQ injection
  Add list_sort() routine from Linux
  ARM: new VGIC: Add IRQ sorting
  ARM: new VGIC: Add IRQ sync/flush framework
  ARM: new VGIC: Add GICv2 world switch backend
  ARM: new VGIC: Implement vgic_vcpu_pending_irq
  ARM: new VGIC: Add MMIO handling framework
  ARM: new VGIC: Add GICv2 MMIO handling framework
  ARM: new VGIC: Add CTLR, TYPER and IIDR handlers
  ARM: new VGIC: Add ENABLE registers handlers
  ARM: new VGIC: Add PENDING registers handlers
  ARM: new VGIC: Add ACTIVE registers handlers
  ARM: new VGIC: Add PRIORITY registers handlers
  ARM: new VGIC: Add CONFIG registers handlers
  ARM: new VGIC: Add TARGET registers handlers
  ARM: new VGIC: Add SGIR register handler
  ARM: new VGIC: Add SGIPENDR register handlers
  ARM: new VGIC: Handle hardware mapped IRQs
  ARM: new VGIC: Add event channel IRQ handling
  ARM: new VGIC: Handle virtual IRQ allocation/reservation
  ARM: new VGIC: Dump virtual IRQ info
  ARM: new VGIC: Provide system register emulation stub
  ARM: new VGIC: Implement arch_move_irqs()
  ARM: new VGIC: Add preliminary stub implementation
  ARM: new VGIC: vgic-init: register VGIC
  ARM: new VGIC: Add vgic_v2_enable
  ARM: new VGIC: vgic-init: implement vgic_init
  ARM: new VGIC: vgic-init: implement map_resources
  ARM: new VGIC: Allocate two pages for struct vcpu
  ARM: VGIC: wire new VGIC(-v2) files into Xen build system

Julien Grall (6):
  xen/arm: gic: Fix indentation in gic_update_one_lr
  xen/arm: vgic: Override the group in lr everytime
  xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr
  xen/arm: gic: Split the field state in gic_lr in 2 fields active and
    pending
  xen/arm: GIC: Only set pirq in the LR when hw_status is set
  ARM: GIC: extend LR read/write functions to cover EOI and source

 xen/arch/arm/Kconfig              |  17 +-
 xen/arch/arm/Makefile             |   5 +-
 xen/arch/arm/domain.c             |  34 ++
 xen/arch/arm/gic-v2.c             | 111 ++++-
 xen/arch/arm/gic-v3.c             | 123 ++++-
 xen/arch/arm/gic-vgic.c           |  24 +-
 xen/arch/arm/time.c               |  36 ++
 xen/arch/arm/traps.c              |  12 +
 xen/arch/arm/vgic.c               |  11 +-
 xen/arch/arm/vgic/Makefile        |   5 +
 xen/arch/arm/vgic/vgic-init.c     | 259 ++++++++++
 xen/arch/arm/vgic/vgic-mmio-v2.c  | 321 +++++++++++++
 xen/arch/arm/vgic/vgic-mmio.c     | 639 ++++++++++++++++++++++++
 xen/arch/arm/vgic/vgic-mmio.h     | 138 ++++++
 xen/arch/arm/vgic/vgic-v2.c       | 301 ++++++++++++
 xen/arch/arm/vgic/vgic.c          | 988 ++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/vgic/vgic.h          |  79 +++
 xen/arch/arm/vpl011.c             |   4 +
 xen/common/Makefile               |   1 +
 xen/common/list_sort.c            | 157 ++++++
 xen/include/asm-arm/event.h       |   3 +-
 xen/include/asm-arm/gic.h         |  59 ++-
 xen/include/asm-arm/gic_v3_defs.h |   4 +
 xen/include/asm-arm/new_vgic.h    | 198 ++++++++
 xen/include/asm-arm/perfc_defn.h  |   3 +-
 xen/include/asm-arm/vgic.h        |   6 +
 xen/include/xen/list_sort.h       |  11 +
 xen/include/xen/timer.h           |   2 +
 28 files changed, 3505 insertions(+), 46 deletions(-)
 create mode 100644 xen/arch/arm/vgic/Makefile
 create mode 100644 xen/arch/arm/vgic/vgic-init.c
 create mode 100644 xen/arch/arm/vgic/vgic-mmio-v2.c
 create mode 100644 xen/arch/arm/vgic/vgic-mmio.c
 create mode 100644 xen/arch/arm/vgic/vgic-mmio.h
 create mode 100644 xen/arch/arm/vgic/vgic-v2.c
 create mode 100644 xen/arch/arm/vgic/vgic.c
 create mode 100644 xen/arch/arm/vgic/vgic.h
 create mode 100644 xen/common/list_sort.c
 create mode 100644 xen/include/asm-arm/new_vgic.h
 create mode 100644 xen/include/xen/list_sort.h

Hi,

On 03/15/2018 08:30 PM, Andre Przywara wrote:
> tl;dr: Coarse changelog below, individual patches have changelogs as
> well. git branch:
> http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/vgic-new/v2
> git://linux-arm.org/xen-ap.git branch vgic-new/v2
> 
> Another update, addressing the review comments. Nothing too outstanding this
> time, the most interesting changes include:
> - removing the split-out preparatory patches, which are already merged
> - changing the setting and clearing of _IRQ_INPROGRESS
> - including Julien's LR access rework series
> - restricting new level IRQ handling to the new VGIC
> - fix multiple SGI handling (mimicing the recent Linux/KVM patch)
> - ASSERTing that h/w IRQs stay connected to their virtual IRQs
> - directly update h/w affinity, without taking the desc lock
> - restrict 8K struct vcpu to new VGIC and ARM64
> - use separate Makefile for vgic/ directory
> - many minor changes to address whitespace issues and usage of unsigned,
>    also extending comments
> 
> A summarising changelog can be found below, each individual patch has
> its own changelog as well.
> 
> There are some things that have (still) not been covered yet:
> - struct VCPU still allocates two pages on ARM64 when using the new VGIC now.
> We could try to look if we can allocate some parts of struct vcpu instead of
> embedding sub-structures into it.
> - vGICv3 support is not implemented, but should be fairly straight-forward to
> add, as the design incorporated this already. Will look at this next.
> - There is a possible DOS vector on the VCPU ap_list, which holds pending
> vIRQs. A guest can make this list rather long, which forces the hypervisor
> to hold the list lock when iterating the list. This should be bounded by
> the number of emulated vIRQs though, and there are ideas how to mitigate
> this issue. Those fixes would be posted on top as fixes later.
> - There is no ITS support, though the VGIC code itself is more ready for that
> than the old VGIC ever was. However due to differences between the Xen
> and KVM architecture the ITS bits are not easy to port over to Xen.
> - Do we need to call vgic_evtchn_irq_pending() in
> local_event_needs_delivery_nomask()? The event channel IRQ should be covered
> by the VGIC already.

Thank you for summarising the open questions! I will log them on jira 
once the series is merged.

Meanwhile, I have committed patches #1 - #8.

Cheers,

Hi,

On 20/03/18 08:30, Julien Grall wrote:
> Hi,
> 
> On 03/15/2018 08:30 PM, Andre Przywara wrote:
>> tl;dr: Coarse changelog below, individual patches have changelogs as
>> well. git branch:
>> http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/vgic-new/v2
>>
>> git://linux-arm.org/xen-ap.git branch vgic-new/v2
>>
>> Another update, addressing the review comments. Nothing too
>> outstanding this
>> time, the most interesting changes include:
>> - removing the split-out preparatory patches, which are already merged
>> - changing the setting and clearing of _IRQ_INPROGRESS
>> - including Julien's LR access rework series
>> - restricting new level IRQ handling to the new VGIC
>> - fix multiple SGI handling (mimicing the recent Linux/KVM patch)
>> - ASSERTing that h/w IRQs stay connected to their virtual IRQs
>> - directly update h/w affinity, without taking the desc lock
>> - restrict 8K struct vcpu to new VGIC and ARM64
>> - use separate Makefile for vgic/ directory
>> - many minor changes to address whitespace issues and usage of unsigned,
>>    also extending comments
>>
>> A summarising changelog can be found below, each individual patch has
>> its own changelog as well.
>>
>> There are some things that have (still) not been covered yet:
>> - struct VCPU still allocates two pages on ARM64 when using the new
>> VGIC now.
>> We could try to look if we can allocate some parts of struct vcpu
>> instead of
>> embedding sub-structures into it.
>> - vGICv3 support is not implemented, but should be fairly
>> straight-forward to
>> add, as the design incorporated this already. Will look at this next.
>> - There is a possible DOS vector on the VCPU ap_list, which holds pending
>> vIRQs. A guest can make this list rather long, which forces the
>> hypervisor
>> to hold the list lock when iterating the list. This should be bounded by
>> the number of emulated vIRQs though, and there are ideas how to mitigate
>> this issue. Those fixes would be posted on top as fixes later.
>> - There is no ITS support, though the VGIC code itself is more ready
>> for that
>> than the old VGIC ever was. However due to differences between the Xen
>> and KVM architecture the ITS bits are not easy to port over to Xen.
>> - Do we need to call vgic_evtchn_irq_pending() in
>> local_event_needs_delivery_nomask()? The event channel IRQ should be
>> covered
>> by the VGIC already.
> 
> Thank you for summarising the open questions! I will log them on jira
> once the series is merged.

Good idea, thanks!

> Meanwhile, I have committed patches #1 - #8.

Many thanks for that! Only 37 to go ;-)
Will try to send a v3 later today.

Cheers,
Andre.