Re: [edk2] [PATCH v2 2/8] ArmPkg: allow dynamically discovered virtual timer interrupt

Subject: Re: [edk2] [PATCH v2 2/8] ArmPkg: allow dynamically discovered virtual timer interrupt

From: Laszlo Ersek <lersek@redhat.com>

To: Ard Biesheuvel <ard.biesheuvel@linaro.org>, olivier.martin@arm.com, edk2-devel@lists.sourceforge.net, peter.maydell@linaro.org, christoffer.dall@linaro.org, drjones@redhat.com, ilias.biris@linaro.org, leif.lindholm@linaro.org

Date: 2014-08-27 04:32:43

On 08/26/14 15:03, Ard Biesheuvel wrote:
> To support booting on virtual machines whose interrupt routing is
> discovered from the device tree, allow the interrupt numbers to
> be redeclared as PcdsDynamic by the platform .dsc
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  ArmPkg/ArmPkg.dec                  | 2 ++
>  ArmPkg/Drivers/TimerDxe/TimerDxe.c | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index c2551d7c3307..0392af52758f 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -140,6 +140,8 @@
>    # Maximum file size for TFTP servers that do not support 'tsize' extension
>    gArmTokenSpaceGuid.PcdMaxTftpFileSize|0x01000000|UINT32|0x00000000
>  
> +
> +[PcdsFixedAtBuild.common,PcdsDynamic.common]
>    #
>    # ARM Architectural Timer
>    #
> diff --git a/ArmPkg/Drivers/TimerDxe/TimerDxe.c b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> index 9227be8326b0..2787f05c62be 100644
> --- a/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> +++ b/ArmPkg/Drivers/TimerDxe/TimerDxe.c
> @@ -36,6 +36,8 @@ EFI_EVENT             EfiExitBootServicesEvent = (EFI_EVENT)NULL;
>  // The current period of the timer interrupt
>  UINT64 mTimerPeriod = 0;
>  
> +UINTN mArmArchTimerTimerFreq = 0;
> +
>  // Cached copy of the Hardware Interrupt protocol instance
>  EFI_HARDWARE_INTERRUPT_PROTOCOL *gInterrupt = NULL;
>  
> @@ -144,7 +146,7 @@ TimerDriverSetTimerPeriod (
>      // Convert TimerPeriod to micro sec units
>      TimerTicks = DivU64x32 (TimerPeriod, 10);
>  
> -    TimerTicks = MultU64x32 (TimerTicks, (PcdGet32(PcdArmArchTimerFreqInHz)/1000000));
> +    TimerTicks = MultU64x32 (TimerTicks, mArmArchTimerTimerFreq);
>  
>      ArmArchTimerSetTimerVal((UINTN)TimerTicks);
>  
> @@ -343,6 +345,8 @@ TimerInitialize (
>    Status = TimerDriverSetTimerPeriod (&gTimer, 0);
>    ASSERT_EFI_ERROR (Status);
>  
> +  mArmArchTimerTimerFreq = ArmArchTimerGetTimerFreq () / 1000000;
> +
>    // Install secure and Non-secure interrupt handlers
>    // Note: Because it is not possible to determine the security state of the
>    // CPU dynamically, we just install interrupt handler for both sec and non-sec
> 

May not be important in practice, but this patch could be cleaner.

ArmArchTimerGetTimerFreq() returns an UINTN, so the type of
mArmArchTimerTimerFreq is OK thus far.

But the second parameter of MultU64x32() should be UINT32, not UINTN.
I'd declare mArmArchTimerTimerFreq with type UINT32, use a UINTN
temporary with ArmArchTimerGetTimerFreq(), and do an explicit range
check & assignment between them.

Feel free to ignore this.

Another thing I notice is that the ArmPkg.dec hunk changes the allowed
PCD types not only for
- PcdArmArchTimerSecIntrNum
- PcdArmArchTimerIntrNum
- PcdArmArchTimerHypIntrNum
- PcdArmArchTimerVirtIntrNum

but also for PcdArmArchTimerFreqInHz too -- is that intended? The commit
message doesn't mention it. And, this patch actually removes one read of
PcdArmArchTimerFreqInHz.

Thanks,
Laszlo

------------------------------------------------------------------------------
Slashdot TV.  
Video for Nerds.  Stuff that matters.
http://tv.slashdot.org/
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel