[edk2-devel] [PATCH 0/2] ArmPkg/ArmVirtPkg: handle FEAT_VHE NS-EL2 virtual timer

[edk2-devel] [PATCH 0/2] ArmPkg/ArmVirtPkg: handle FEAT_VHE NS-EL2 virtual timer

[Leif Lindholm]

An ASSERT trips when we try to add the NS-EL2 virtual timer to qemu
mach-virt.

Add a new Pcd for the new private peripheral interrupt id,
PcdArmArchTimerHypVirtIntrNum.

Update ArmVirtTimerFdtClientLib to:
- Only assert on receiving less information that required through DT.
- Set PcdArmArchTimerHypVirtIntrNum if provided through DT.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Leif Lindholm (2):
  ArmPkg: add EL2 virtual timer interrupt Pcd
  ArmVirtPkg: handle virtual EL2 timer in DT

 ArmPkg/ArmPkg.dec                                                        |  1 +
 ArmVirtPkg/ArmVirtQemu.dsc                                               |  1 +
 ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf |  1 +
 ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c   | 13 +++++++++----
 4 files changed, 12 insertions(+), 4 deletions(-)

-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108823): https://edk2.groups.io/g/devel/message/108823
Mute This Topic: https://groups.io/mt/101453745/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 1/2] ArmPkg: add EL2 virtual timer interrupt Pcd

[Leif Lindholm]

PcdArmArchTimerHypVirtIntrNum, defaults to 28 as defined in Arm's
Base System Architecture (current version DEN0094C 1.0C).

Relevant for systems implementing FEAT_VHE, added in ARMv8.1.

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
---
 ArmPkg/ArmPkg.dec | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 1a16d044c94b..7fe2b9bca43b 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -328,6 +328,7 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
   gArmTokenSpaceGuid.PcdArmArchTimerIntrNum|30|UINT32|0x00000036
   gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum|26|UINT32|0x00000040
   gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum|27|UINT32|0x00000041
+  gArmTokenSpaceGuid.PcdArmArchTimerHypVirtIntrNum|28|UINT32|0x0000004A
 
   #
   # ARM Generic Watchdog
-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108824): https://edk2.groups.io/g/devel/message/108824
Mute This Topic: https://groups.io/mt/101453749/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH 2/2] ArmVirtPkg: handle virtual EL2 timer in DT

[Leif Lindholm]

FEAT_VHE, introduced in ARMv8.1, adds a virtual EL2 timer.
However, this library verifies that exactly 3 or 4 12-byte timer
interrupts are provided in input DT, ASSERTing when the new timer
is added.

Change the assert to >= 36.

Extend the current logic, also initializing PcdArmArchTimerHypVirtIntrNum
if 5 interrupts are provided.

Signed-off-by: Leif Lindholm <quic_llindhol@quicinc.com>
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
---
 ArmVirtPkg/ArmVirtQemu.dsc                                               |  1 +
 ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf |  1 +
 ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c   | 13 +++++++++----
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index 1e0225951aef..30e3cfc8b9cc 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -253,6 +253,7 @@ [PcdsDynamicDefault.common]
   gArmTokenSpaceGuid.PcdArmArchTimerIntrNum|0x0
   gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum|0x0
   gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum|0x0
+  gArmTokenSpaceGuid.PcdArmArchTimerHypVirtIntrNum|0x0
 
   #
   # ARM General Interrupt Controller
diff --git a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf
index 9e6f6f63a5a2..f8fc013700ad 100644
--- a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf
+++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf
@@ -40,6 +40,7 @@ [Pcd]
   gArmTokenSpaceGuid.PcdArmArchTimerIntrNum
   gArmTokenSpaceGuid.PcdArmArchTimerVirtIntrNum
   gArmTokenSpaceGuid.PcdArmArchTimerHypIntrNum
+  gArmTokenSpaceGuid.PcdArmArchTimerHypVirtIntrNum
 
 [Depex]
   gFdtClientProtocolGuid
diff --git a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
index 38dd6c533329..3b8491525eb3 100644
--- a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
+++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
@@ -34,7 +34,8 @@ ArmVirtTimerFdtClientLibConstructor (
   FDT_CLIENT_PROTOCOL       *FdtClient;
   CONST INTERRUPT_PROPERTY  *InterruptProp;
   UINT32                    PropSize;
-  INT32                     SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
+  INT32                     SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum,
+                            HypVirtIntrNum;
   RETURN_STATUS             PcdStatus;
 
   Status = gBS->LocateProtocol (
@@ -66,10 +67,10 @@ ArmVirtTimerFdtClientLibConstructor (
   }
 
   //
-  // - interrupts : Interrupt list for secure, non-secure, virtual and
-  //  hypervisor timers, in that order.
+  // - interrupts : Interrupt list for secure, non-secure, virtual,
+  //  hypervisor and hypervisor virtual timers, in that order.
   //
-  ASSERT (PropSize == 36 || PropSize == 48);
+  ASSERT (PropSize >= 36);
 
   SecIntrNum = SwapBytes32 (InterruptProp[0].Number)
                + (InterruptProp[0].Type ? 16 : 0);
@@ -79,6 +80,8 @@ ArmVirtTimerFdtClientLibConstructor (
                 + (InterruptProp[2].Type ? 16 : 0);
   HypIntrNum = PropSize < 48 ? 0 : SwapBytes32 (InterruptProp[3].Number)
                + (InterruptProp[3].Type ? 16 : 0);
+  HypVirtIntrNum = PropSize < 60 ? 0 : SwapBytes32 (InterruptProp[4].Number)
+               + (InterruptProp[4].Type ? 16 : 0);
 
   DEBUG ((
     DEBUG_INFO,
@@ -97,6 +100,8 @@ ArmVirtTimerFdtClientLibConstructor (
   ASSERT_RETURN_ERROR (PcdStatus);
   PcdStatus = PcdSet32S (PcdArmArchTimerHypIntrNum, HypIntrNum);
   ASSERT_RETURN_ERROR (PcdStatus);
+  PcdStatus = PcdSet32S (PcdArmArchTimerHypVirtIntrNum, HypVirtIntrNum);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   return EFI_SUCCESS;
 }
-- 
2.30.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108825): https://edk2.groups.io/g/devel/message/108825
Mute This Topic: https://groups.io/mt/101453750/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] ArmPkg/ArmVirtPkg: handle FEAT_VHE NS-EL2 virtual timer

[Peter Maydell]

On Tue, 19 Sept 2023 at 12:20, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> An ASSERT trips when we try to add the NS-EL2 virtual timer to qemu
> mach-virt.
>
> Add a new Pcd for the new private peripheral interrupt id,
> PcdArmArchTimerHypVirtIntrNum.
>
> Update ArmVirtTimerFdtClientLib to:
> - Only assert on receiving less information that required through DT.
> - Set PcdArmArchTimerHypVirtIntrNum if provided through DT.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>

Thanks for writing and sending out these patches. I've tested, and
they are sufficient to get EDK2 to not assert and successfully
boot a guest OS when using my QEMU RFC patchset that reports the
new interrupt ID via the dtb:
https://patchew.org/QEMU/20230919101240.2569334-1-peter.maydell@linaro.org/

-- PMM


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108835): https://edk2.groups.io/g/devel/message/108835
Mute This Topic: https://groups.io/mt/101453745/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] ArmPkg/ArmVirtPkg: handle FEAT_VHE NS-EL2 virtual timer

[Ard Biesheuvel]

On Tue, 19 Sept 2023 at 13:20, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> An ASSERT trips when we try to add the NS-EL2 virtual timer to qemu
> mach-virt.
>
> Add a new Pcd for the new private peripheral interrupt id,
> PcdArmArchTimerHypVirtIntrNum.
>
> Update ArmVirtTimerFdtClientLib to:
> - Only assert on receiving less information that required through DT.
> - Set PcdArmArchTimerHypVirtIntrNum if provided through DT.
>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
>
> Leif Lindholm (2):
>   ArmPkg: add EL2 virtual timer interrupt Pcd
>   ArmVirtPkg: handle virtual EL2 timer in DT
>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

>  ArmPkg/ArmPkg.dec                                                        |  1 +
>  ArmVirtPkg/ArmVirtQemu.dsc                                               |  1 +
>  ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.inf |  1 +
>  ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c   | 13 +++++++++----
>  4 files changed, 12 insertions(+), 4 deletions(-)
>
> --
> 2.30.2
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108837): https://edk2.groups.io/g/devel/message/108837
Mute This Topic: https://groups.io/mt/101453745/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] ArmPkg/ArmVirtPkg: handle FEAT_VHE NS-EL2 virtual timer

[Leif Lindholm]

On 2023-09-19 14:18, Peter Maydell wrote:
> On Tue, 19 Sept 2023 at 12:20, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>>
>> An ASSERT trips when we try to add the NS-EL2 virtual timer to qemu
>> mach-virt.
>>
>> Add a new Pcd for the new private peripheral interrupt id,
>> PcdArmArchTimerHypVirtIntrNum.
>>
>> Update ArmVirtTimerFdtClientLib to:
>> - Only assert on receiving less information that required through DT.
>> - Set PcdArmArchTimerHypVirtIntrNum if provided through DT.
>>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
> 
> Thanks for writing and sending out these patches. I've tested, and
> they are sufficient to get EDK2 to not assert and successfully
> boot a guest OS when using my QEMU RFC patchset that reports the
> new interrupt ID via the dtb:
> https://patchew.org/QEMU/20230919101240.2569334-1-peter.maydell@linaro.org/

Are you happy for me to interpret that as Tested-by for 2/2? (1/2 just 
being internal plumbing not related to preventing the ASSERT)

/
     Leif




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108849): https://edk2.groups.io/g/devel/message/108849
Mute This Topic: https://groups.io/mt/101453745/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] ArmPkg/ArmVirtPkg: handle FEAT_VHE NS-EL2 virtual timer

[Peter Maydell]

On Tue, 19 Sept 2023 at 16:19, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On 2023-09-19 14:18, Peter Maydell wrote:
> > On Tue, 19 Sept 2023 at 12:20, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >>
> >> An ASSERT trips when we try to add the NS-EL2 virtual timer to qemu
> >> mach-virt.
> >>
> >> Add a new Pcd for the new private peripheral interrupt id,
> >> PcdArmArchTimerHypVirtIntrNum.
> >>
> >> Update ArmVirtTimerFdtClientLib to:
> >> - Only assert on receiving less information that required through DT.
> >> - Set PcdArmArchTimerHypVirtIntrNum if provided through DT.
> >>
> >> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> >> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >
> > Thanks for writing and sending out these patches. I've tested, and
> > they are sufficient to get EDK2 to not assert and successfully
> > boot a guest OS when using my QEMU RFC patchset that reports the
> > new interrupt ID via the dtb:
> > https://patchew.org/QEMU/20230919101240.2569334-1-peter.maydell@linaro.org/
>
> Are you happy for me to interpret that as Tested-by for 2/2? (1/2 just
> being internal plumbing not related to preventing the ASSERT)

Yes, if you do Tested-by tags in this project feel free to add
that from me.

-- PMM


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108855): https://edk2.groups.io/g/devel/message/108855
Mute This Topic: https://groups.io/mt/101453745/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 0/2] ArmPkg/ArmVirtPkg: handle FEAT_VHE NS-EL2 virtual timer

[Leif Lindholm]

On Tue, Sep 19, 2023 at 14:18:01 +0100, Peter Maydell wrote:
> On Tue, 19 Sept 2023 at 12:20, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > An ASSERT trips when we try to add the NS-EL2 virtual timer to qemu
> > mach-virt.
> >
> > Add a new Pcd for the new private peripheral interrupt id,
> > PcdArmArchTimerHypVirtIntrNum.
> >
> > Update ArmVirtTimerFdtClientLib to:
> > - Only assert on receiving less information that required through DT.
> > - Set PcdArmArchTimerHypVirtIntrNum if provided through DT.
> >
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Sami Mujawar <sami.mujawar@arm.com>
> 
> Thanks for writing and sending out these patches. I've tested, and
> they are sufficient to get EDK2 to not assert and successfully
> boot a guest OS when using my QEMU RFC patchset that reports the
> new interrupt ID via the dtb:
> https://patchew.org/QEMU/20230919101240.2569334-1-peter.maydell@linaro.org/

CI told me I was a muppet and needed to add the same
[PcdsDynamicDefault.common] stanza to all the other .dscs in
ArmVirtPkg, so given they were all identical I took the liberty not to
ask for re-review.
Pushed as 7275993dc644..eb485b6438f4.

Thanks all!

/
    Leif


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108859): https://edk2.groups.io/g/devel/message/108859
Mute This Topic: https://groups.io/mt/101453745/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-