Re: [edk2-devel] [PATCH edk2-platforms v3 5/5] SbsaQemu: introduce helper in PPTT generation

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On Tue, Jul 09, 2024 at 15:12:37 +0200, Marcin Juszkiewicz wrote:
> Dnia wtorek, 9 lipca 2024 15:00:12 CEST Leif Lindholm via groups.io pisze:
> > On Tue, Jul 09, 2024 at 12:47:10 +0200, Marcin Juszkiewicz wrote:
> > > Function AddPpttTable() adding PPTT got too long. This change moves part
> > > of it into helper function AddCoresToPpttTable() which takes care of
> > > generating entries for Core and below (Cache, Thread).
> > > 
> > > Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> > > ---
> > > 
> > >  .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c       | 237
> > >  +++++++++++--------- 1 file changed, 133 insertions(+), 104 deletions(-)
> > > 
> > > diff --git
> > > a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > > b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c index
> > > a7a9664abdcb..a4b2ee2fdcb0 100644
> > > --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > > +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> > > @@ -29,6 +29,9 @@
> > > 
> > >  static UINTN  GicItsBase;
> > > 
> > > +static UINTN  CpuId;
> > > +static UINTN  CacheId;
> > > +
> > 
> > static variables are supposed to have g (global) or m (module) prefix.
> > This is local, so m.
> > (Yes, that means I missed that when reviewing the GitIts bits.)
> > 
> > Also, why are these in a #pragma pack(1) block?
> 
> Added right after GicItsBase. Moved out of block.

I don't think it makes any sense for GicItsBase either, but that's not
part of this review.

> > >  #pragma pack ()
>  
> > STATIC
> > 
> > > +UINT32
> > > +AddCoresToPpttTable (
> > > +  UINT8        *New,
> > > +  UINT32       ClusterIndex,
> > > +  CpuTopology  CpuTopo
> > > +  )
> 
> done
> 
> > > -      ClusterIndex = CoreIndex;
> > > +      CoresPartSize = AddCoresToPpttTable (New, ClusterIndex, CpuTopo);
> > > +      ClusterIndex += CoresPartSize;
> > 
> > This sounds like ClisterIndex is no longer an Index after this patch.
> > Should it be renamed?
> 
> It is still an Index. CoresPartSize is a size taken by Core/Cache/Thread part 
> of this Cluster.

But doesn't that make it an offset instead of an index?

/
    Leif

> 
> 
> 
> 
> 
> 
> 


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