Re: [PATCH v1 01/21] ArmPkg: allow HYP timer interrupt to be omitted

[Laszlo Ersek <lersek@redhat.com>]

On 01/23/15 16:02, Ard Biesheuvel wrote:
> The DT binding for the ARM generic timer describes the secure,
> non-secure, virtual and hypervisor timer interrupts, respectively.
> However, under virtualization, only the virtual timer is usable, and
> the device tree may omit the hypervisor timer interrupt. (Other timer
> interrupts cannot be omitted simply due to the fact that the virtual
> timer is listed third)
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Drivers/TimerDxe/TimerDxe.c                         | 14 +++++++++++---
>  .../ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c           |  4 ++--
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> index d0a819fc2729..1169d426b255 100644
> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> @@ -369,7 +369,8 @@ TimerInitialize (
>  {
>    EFI_HANDLE  Handle = NULL;
>    EFI_STATUS  Status;
> -  UINTN TimerCtrlReg;
> +  UINTN       TimerCtrlReg;
> +  UINT32      TimerHypIntrNum;
>  
>    if (ArmIsArchTimerImplemented () == 0) {
>      DEBUG ((EFI_D_ERROR, "ARM Architectural Timer is not available in the CPU, hence cann't use this Driver \n"));
> @@ -395,8 +396,15 @@ TimerInitialize (
>    Status = gInterrupt->RegisterInterruptSource (gInterrupt, PcdGet32 (PcdArmArchTimerVirtIntrNum), TimerInterruptHandler);
>    ASSERT_EFI_ERROR (Status);
>  
> -  Status = gInterrupt->RegisterInterruptSource (gInterrupt, PcdGet32 (PcdArmArchTimerHypIntrNum), TimerInterruptHandler);
> -  ASSERT_EFI_ERROR (Status);
> +  //
> +  // The hypervisor timer interrupt may be omitted by implementations that
> +  // execute under virtualization.
> +  //
> +  TimerHypIntrNum = PcdGet32 (PcdArmArchTimerHypIntrNum);
> +  if (TimerHypIntrNum != 0) {
> +    Status = gInterrupt->RegisterInterruptSource (gInterrupt, TimerHypIntrNum, TimerInterruptHandler);
> +    ASSERT_EFI_ERROR (Status);
> +  }
>  
>    Status = gInterrupt->RegisterInterruptSource (gInterrupt, PcdGet32 (PcdArmArchTimerSecIntrNum), TimerInterruptHandler);
>    ASSERT_EFI_ERROR (Status);
> diff --git a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> index 751864d4db9c..4e4f608923d3 100644
> --- a/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> +++ b/ArmPlatformPkg/ArmVirtualizationPkg/VirtFdtDxe/VirtFdtDxe.c
> @@ -274,7 +274,7 @@ InitializeVirtFdtDxe (
>        //  hypervisor timers, in that order.
>        //
>        InterruptProp = fdt_getprop (DeviceTreeBase, Node, "interrupts", &Len);
> -      ASSERT (Len == 48);
> +      ASSERT (Len == 36 || Len == 48);
>  
>        SecIntrNum = fdt32_to_cpu (InterruptProp[0].Number)
>                     + (InterruptProp[0].Type ? 16 : 0);
> @@ -282,7 +282,7 @@ InitializeVirtFdtDxe (
>                  + (InterruptProp[1].Type ? 16 : 0);
>        VirtIntrNum = fdt32_to_cpu (InterruptProp[2].Number)
>                      + (InterruptProp[2].Type ? 16 : 0);
> -      HypIntrNum = fdt32_to_cpu (InterruptProp[3].Number)
> +      HypIntrNum = Len < 48 ? 0 : fdt32_to_cpu (InterruptProp[3].Number)
>                     + (InterruptProp[3].Type ? 16 : 0);
>  
>        DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",
> 

Please align the plus sign on the second line with the first addend. The
layout should reflect operator binding if possible.

Thanks
Laszlo