LocalApicLib: Why two separate directories?

LocalApicLib: Why two separate directories?

[Duran, Leo]

All,
Just a quick observation to request comments:

Since a lot of the code in BaseXApicX2ApicLib.c and BaseXApicLib is the same, how about we merge the common code and build the libraries from the same directory?

UefiCpuPkg/Library/LocalApilLib/
- LocalApicLib.c --> common code
- BaseXApicLib.c --> legacy APIC code
- BaseXApicX2ApicLib.c --> X2APIC code
- BaseXApicLib.inf -> builds from LocalApicLib.c + BaseXApicLib.c
- BaseXApicX2ApicLib.inf -> builds from LocalApicLib.c + BaseXApicX2ApicLib.c

Of course, doing this would require modification to existing .DSC files, to point to the appropriate .INF under the merged LocalApicLib directory.
Would that be too disruptive?

Leo.

Re: LocalApicLib: Why two separate directories?

[Laszlo Ersek]

On 10/28/16 21:03, Duran, Leo wrote:
> All,
> Just a quick observation to request comments:
> 
> Since a lot of the code in BaseXApicX2ApicLib.c and BaseXApicLib is
> the same, how about we merge the common code and build the libraries
> from the same directory?
> 
> UefiCpuPkg/Library/LocalApilLib/
> - LocalApicLib.c --> common code
> - BaseXApicLib.c --> legacy APIC code
> - BaseXApicX2ApicLib.c --> X2APIC code
> - BaseXApicLib.inf -> builds from LocalApicLib.c + BaseXApicLib.c
> - BaseXApicX2ApicLib.inf -> builds from LocalApicLib.c + BaseXApicX2ApicLib.c
> 
> Of course, doing this would require modification to existing .DSC files, to point to the appropriate .INF under the merged LocalApicLib directory.
> Would that be too disruptive?

In my opinion, if:
- you post all the patches (for all affected platforms) in a series,
- keep the series bisectable (i.e., all the platforms build at any stage
throughout the series),
- CC the relevant maintainers,
- and they accept (R-b) the patches,

then it should be fine.

Conversions like this usually involve creating a copy with the existent
(or to-be-migrated) functionality in the new place, gradually pointing
the platform DSCs to that place, and once the old INF files / library
instances become unreferenced, they are removed in the last patches.

For simpler (Pkg-internal) code movement this is not always necessary
(you can usually solve it all within a single atomic patch, like the one
you just posted). However, when multiple Pkgs (with multiple sets of
maintainers) are affected, we mostly avoid patches that would
simultaneously straddle two or more Pkgs.

It's almost always possible to structure a series like described above
-- modify just one Pkg per step, gradually flipping the pointers from
"old" to "new", and when "old"'s refcount goes to zero, remove "old".

(Clearly the actual answer comes from the UefiCpuPkg maintainers; I'm
just talking about the process.)

Thanks!
Laszlo

Re: LocalApicLib: Why two separate directories?

[Kinney, Michael D]

Leo,

Your observation is correct, but the reason not to make this change now is 
the DSC file changes required that would break platform builds.  As Laszlo
points out, it is possible to do this type of change and coordinate update
to all platforms in edk2/master.  However, there are many other platforms 
that use edk2 and a change like this would break them on next pull of 
edk2/master.

I believe the original X2 APIC implementation did not have as much common
code, so that was likely why it was added as a different library.

I recommend we just leave them in their own directories for right now.

Thanks,

Mike
 

> -----Original Message-----
> From: Duran, Leo [mailto:leo.duran@amd.com]
> Sent: Friday, October 28, 2016 12:04 PM
> To: edk2-devel@ml01.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com>;
> Gao, Liming <liming.gao@intel.com>; 'Laszlo Ersek' <lersek@redhat.com>
> Subject: LocalApicLib: Why two separate directories?
> 
> All,
> Just a quick observation to request comments:
> 
> Since a lot of the code in BaseXApicX2ApicLib.c and BaseXApicLib is the same, how about
> we merge the common code and build the libraries from the same directory?
> 
> UefiCpuPkg/Library/LocalApilLib/
> - LocalApicLib.c --> common code
> - BaseXApicLib.c --> legacy APIC code
> - BaseXApicX2ApicLib.c --> X2APIC code
> - BaseXApicLib.inf -> builds from LocalApicLib.c + BaseXApicLib.c
> - BaseXApicX2ApicLib.inf -> builds from LocalApicLib.c + BaseXApicX2ApicLib.c
> 
> Of course, doing this would require modification to existing .DSC files, to point to
> the appropriate .INF under the merged LocalApicLib directory.
> Would that be too disruptive?
> 
> Leo.
> 
> 
> 
> 

Re: LocalApicLib: Why two separate directories?

[Duran, Leo]

Thanks Lazlo.

The process as you described it makes a lot of sense.
However, I worry about the effect on external consumers of EDK2 once "old" is removed.

Leo.


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, October 28, 2016 2:22 PM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org
> Cc: michael.d.kinney@intel.com; jeff.fan@intel.com; liming.gao@intel.com
> Subject: Re: LocalApicLib: Why two separate directories?
> 
> On 10/28/16 21:03, Duran, Leo wrote:
> > All,
> > Just a quick observation to request comments:
> >
> > Since a lot of the code in BaseXApicX2ApicLib.c and BaseXApicLib is
> > the same, how about we merge the common code and build the libraries
> > from the same directory?
> >
> > UefiCpuPkg/Library/LocalApilLib/
> > - LocalApicLib.c --> common code
> > - BaseXApicLib.c --> legacy APIC code
> > - BaseXApicX2ApicLib.c --> X2APIC code
> > - BaseXApicLib.inf -> builds from LocalApicLib.c + BaseXApicLib.c
> > - BaseXApicX2ApicLib.inf -> builds from LocalApicLib.c +
> > BaseXApicX2ApicLib.c
> >
> > Of course, doing this would require modification to existing .DSC files, to
> point to the appropriate .INF under the merged LocalApicLib directory.
> > Would that be too disruptive?
> 
> In my opinion, if:
> - you post all the patches (for all affected platforms) in a series,
> - keep the series bisectable (i.e., all the platforms build at any stage
> throughout the series),
> - CC the relevant maintainers,
> - and they accept (R-b) the patches,
> 
> then it should be fine.
> 
> Conversions like this usually involve creating a copy with the existent (or to-
> be-migrated) functionality in the new place, gradually pointing the platform
> DSCs to that place, and once the old INF files / library instances become
> unreferenced, they are removed in the last patches.
> 
> For simpler (Pkg-internal) code movement this is not always necessary (you
> can usually solve it all within a single atomic patch, like the one you just
> posted). However, when multiple Pkgs (with multiple sets of
> maintainers) are affected, we mostly avoid patches that would
> simultaneously straddle two or more Pkgs.
> 
> It's almost always possible to structure a series like described above
> -- modify just one Pkg per step, gradually flipping the pointers from "old" to
> "new", and when "old"'s refcount goes to zero, remove "old".
> 
> (Clearly the actual answer comes from the UefiCpuPkg maintainers; I'm just
> talking about the process.)
> 
> Thanks!
> Laszlo

Re: LocalApicLib: Why two separate directories?

[Duran, Leo]

> -----Original Message-----
> From: Kinney, Michael D [mailto:michael.d.kinney@intel.com]
> Sent: Friday, October 28, 2016 2:28 PM
> To: Duran, Leo <leo.duran@amd.com>; edk2-devel@ml01.01.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Fan, Jeff <jeff.fan@intel.com>; Gao, Liming <liming.gao@intel.com>;
> 'Laszlo Ersek' <lersek@redhat.com>
> Subject: RE: LocalApicLib: Why two separate directories?
> 
> Leo,
> 
> Your observation is correct, but the reason not to make this change now is
> the DSC file changes required that would break platform builds.  As Laszlo
> points out, it is possible to do this type of change and coordinate update to all
> platforms in edk2/master.  However, there are many other platforms that
> use edk2 and a change like this would break them on next pull of
> edk2/master.
> 
> I believe the original X2 APIC implementation did not have as much common
> code, so that was likely why it was added as a different library.
> 
> I recommend we just leave them in their own directories for right now.
> 
> Thanks,
> 
> Mike
> 
[Duran, Leo] 
Yes, that was my main concern.
I suppose that in hindsight the two libraries could have being  built from a common directory, allowing for (future) refactoring of code with breaking the .DSC's
But, it is what it is... Thanks for the comments.
Leo


> 
> > -----Original Message-----
> > From: Duran, Leo [mailto:leo.duran@amd.com]
> > Sent: Friday, October 28, 2016 12:04 PM
> > To: edk2-devel@ml01.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff
> > <jeff.fan@intel.com>; Gao, Liming <liming.gao@intel.com>; 'Laszlo
> > Ersek' <lersek@redhat.com>
> > Subject: LocalApicLib: Why two separate directories?
> >
> > All,
> > Just a quick observation to request comments:
> >
> > Since a lot of the code in BaseXApicX2ApicLib.c and BaseXApicLib is
> > the same, how about we merge the common code and build the libraries
> from the same directory?
> >
> > UefiCpuPkg/Library/LocalApilLib/
> > - LocalApicLib.c --> common code
> > - BaseXApicLib.c --> legacy APIC code
> > - BaseXApicX2ApicLib.c --> X2APIC code
> > - BaseXApicLib.inf -> builds from LocalApicLib.c + BaseXApicLib.c
> > - BaseXApicX2ApicLib.inf -> builds from LocalApicLib.c +
> > BaseXApicX2ApicLib.c
> >
> > Of course, doing this would require modification to existing .DSC
> > files, to point to the appropriate .INF under the merged LocalApicLib
> directory.
> > Would that be too disruptive?
> >
> > Leo.
> >
> >
> >
> >

Re: LocalApicLib: Why two separate directories?

[Laszlo Ersek]

On 10/28/16 21:28, Kinney, Michael D wrote:
> Leo,
> 
> Your observation is correct, but the reason not to make this change now is 
> the DSC file changes required that would break platform builds.  As Laszlo
> points out, it is possible to do this type of change and coordinate update
> to all platforms in edk2/master.  However, there are many other platforms 
> that use edk2 and a change like this would break them on next pull of 
> edk2/master.
> 
> I believe the original X2 APIC implementation did not have as much common
> code, so that was likely why it was added as a different library.
> 
> I recommend we just leave them in their own directories for right now.

It would also be possible to extract the common code into a library
*class* (internal to UefiCpuPkg), along with a single instance for that
new lib class. This would leave the two current INFs undisturbed.

However, as far as I understand, such arbitrary library classes could be
frowned upon -- there's no other "guiding principle" to them than "well
these functions happen to be shared by two other modules".

Sometimes that's a good enough reason for introducing a new library
class (with a single imlementation), sometimes it isn't.

Thanks
Laszlo

>> -----Original Message-----
>> From: Duran, Leo [mailto:leo.duran@amd.com]
>> Sent: Friday, October 28, 2016 12:04 PM
>> To: edk2-devel@ml01.01.org
>> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Fan, Jeff <jeff.fan@intel.com>;
>> Gao, Liming <liming.gao@intel.com>; 'Laszlo Ersek' <lersek@redhat.com>
>> Subject: LocalApicLib: Why two separate directories?
>>
>> All,
>> Just a quick observation to request comments:
>>
>> Since a lot of the code in BaseXApicX2ApicLib.c and BaseXApicLib is the same, how about
>> we merge the common code and build the libraries from the same directory?
>>
>> UefiCpuPkg/Library/LocalApilLib/
>> - LocalApicLib.c --> common code
>> - BaseXApicLib.c --> legacy APIC code
>> - BaseXApicX2ApicLib.c --> X2APIC code
>> - BaseXApicLib.inf -> builds from LocalApicLib.c + BaseXApicLib.c
>> - BaseXApicX2ApicLib.inf -> builds from LocalApicLib.c + BaseXApicX2ApicLib.c
>>
>> Of course, doing this would require modification to existing .DSC files, to point to
>> the appropriate .INF under the merged LocalApicLib directory.
>> Would that be too disruptive?
>>
>> Leo.
>>
>>
>>
>>
>