[PATCH 1/1] ArmVirtPkg: fix ASSERT in ArmVirtGicArchLib with virtualization=on

[PATCH 1/1] ArmVirtPkg: fix ASSERT in ArmVirtGicArchLib with virtualization=on

[Leif Lindholm]

ArmVirtGicArchLib was originally implemented before virtualization
emulation was implemented in QEMU, and the GICv2 model implemented only
the physical copy of control registers.

Enabling virtualization emulation to QEMU adds also the virtual copy,
doubling the RegSize returned by FindCompatibleNodeReg () in
ArmVirtGicArchLibConstructor (). This triggered an ASSERT when running
QEMU with -M virt,virtualization=on. Address this by testing for both
possible valid values of RegSize.

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2588

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
index af6b3af60edf..5448865ad8e8 100644
--- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -110,7 +110,12 @@ ArmVirtGicArchLibConstructor (
     break;
 
   case 2:
-    ASSERT (RegSize == 32);
+    //
+    // When the GICv2 is emulated with virtualization=on, it adds a virtual
+    // set of control registers. This means the register property can be
+    // either 32 or 64 bytes in size.
+    //
+    ASSERT ((RegSize == 32) || (RegSize == 64));
 
     DistBase = SwapBytes64 (Reg[0]);
     CpuBase  = SwapBytes64 (Reg[2]);
-- 
2.20.1

Re: [PATCH 1/1] ArmVirtPkg: fix ASSERT in ArmVirtGicArchLib with virtualization=on

[Laszlo Ersek]

On 03/11/20 16:32, Leif Lindholm wrote:
> ArmVirtGicArchLib was originally implemented before virtualization
> emulation was implemented in QEMU, and the GICv2 model implemented only
> the physical copy of control registers.
> 
> Enabling virtualization emulation to QEMU adds also the virtual copy,
> doubling the RegSize returned by FindCompatibleNodeReg () in
> ArmVirtGicArchLibConstructor (). This triggered an ASSERT when running
> QEMU with -M virt,virtualization=on. Address this by testing for both
> possible valid values of RegSize.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2588
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>  ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> index af6b3af60edf..5448865ad8e8 100644
> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> @@ -110,7 +110,12 @@ ArmVirtGicArchLibConstructor (
>      break;
>  
>    case 2:
> -    ASSERT (RegSize == 32);
> +    //
> +    // When the GICv2 is emulated with virtualization=on, it adds a virtual
> +    // set of control registers. This means the register property can be
> +    // either 32 or 64 bytes in size.
> +    //
> +    ASSERT ((RegSize == 32) || (RegSize == 64));
>  
>      DistBase = SwapBytes64 (Reg[0]);
>      CpuBase  = SwapBytes64 (Reg[2]);
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 1/1] ArmVirtPkg: fix ASSERT in ArmVirtGicArchLib with virtualization=on

[Leif Lindholm]

On Wed, Mar 11, 2020 at 16:39:41 +0100, Laszlo Ersek wrote:
> On 03/11/20 16:32, Leif Lindholm wrote:
> > ArmVirtGicArchLib was originally implemented before virtualization
> > emulation was implemented in QEMU, and the GICv2 model implemented only
> > the physical copy of control registers.
> > 
> > Enabling virtualization emulation to QEMU adds also the virtual copy,
> > doubling the RegSize returned by FindCompatibleNodeReg () in
> > ArmVirtGicArchLibConstructor (). This triggered an ASSERT when running
> > QEMU with -M virt,virtualization=on. Address this by testing for both
> > possible valid values of RegSize.
> > 
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2588
> > 
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >  ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> > index af6b3af60edf..5448865ad8e8 100644
> > --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> > +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> > @@ -110,7 +110,12 @@ ArmVirtGicArchLibConstructor (
> >      break;
> >  
> >    case 2:
> > -    ASSERT (RegSize == 32);
> > +    //
> > +    // When the GICv2 is emulated with virtualization=on, it adds a virtual
> > +    // set of control registers. This means the register property can be
> > +    // either 32 or 64 bytes in size.
> > +    //
> > +    ASSERT ((RegSize == 32) || (RegSize == 64));
> >  
> >      DistBase = SwapBytes64 (Reg[0]);
> >      CpuBase  = SwapBytes64 (Reg[2]);
> > 
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Pushed as 01ce872739d2f.
BZ closed.

/
    Leif