Re: [PATCH 0/6] OvmfPkg/ResetSystemLib: clean up, refactor, fix

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 4/17/20 5:59 PM, Ard Biesheuvel wrote:
> On 4/17/20 5:37 PM, Laszlo Ersek wrote:
>> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2675
>> Repo:   https://pagure.io/lersek/edk2.git
>> Branch: rsl_cleanup
>>
>> Rebecca's
>>
>>    [PATCH 02/13] OvmfPkg: support powering off bhyve guests
>>
>> at
>>
>>    https://edk2.groups.io/g/devel/message/57450
>>    
>> http://mid.mail-archive.com/e4e9b29189b83076e1d1a0b9c989938f5226cab6.1586991816.git.rebecca@bsdio.com 
>>
>>
>> made me realize OvmfPkg/ResetSystemLib should be refreshed. This will
>> let us add a simple bhyve-specific instance (later), and also allows us
>> to fix a long time dormant bug (now).
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>>
>> Thanks,
>> Laszlo
>>
>> Laszlo Ersek (6):
>>    OvmfPkg/ResetSystemLib: wrap long lines
>>    OvmfPkg/ResetSystemLib: clean up library dependencies
>>    OvmfPkg/ResetSystemLib: improve coding style in ResetSystem()
>>    OvmfPkg/ResetSystemLib: factor out ResetShutdown()
>>    OvmfPkg/ResetSystemLib: rename to BaseResetSystemLib
>>    OvmfPkg/ResetSystemLib: introduce the DxeResetSystemLib instance
>>
> 
> For the series,
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> One nit: putting a diff block inside the commit log [6/6] doesn't help 
> legibility a lot, and the issue of not being able to access memory that 
> is not mapped for runtime is so basic that it doesn't require that level 
> of detail to describe a reproducer and the Linux kernel log output when 
> the issue is triggered.

Personally I find the kernel log relevant, it will help to understand th 
e patch when looking at it in >5years from now.
> 
>>   
>> OvmfPkg/OvmfPkgIa32.dsc                                                       
>> |  8 +-
>>   
>> OvmfPkg/OvmfPkgIa32X64.dsc                                                    
>> |  8 +-
>>   
>> OvmfPkg/OvmfPkgX64.dsc                                                        
>> |  8 +-
>>   
>> OvmfPkg/OvmfXen.dsc                                                           
>> |  6 +-
>>   OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => 
>> BaseResetSystemLib.inf} | 12 ++-
>>   
>> OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf                          
>> | 43 ++++++++++
>>   
>> OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c                            
>> | 51 ++++++++++++
>>   
>> OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c                             
>> | 62 +++++++++++++++
>>   
>> OvmfPkg/Library/ResetSystemLib/ResetSystemLib.c                               
>> | 84 +++++---------------
>>   9 files changed, 209 insertions(+), 73 deletions(-)
>>   rename OvmfPkg/Library/ResetSystemLib/{ResetSystemLib.inf => 
>> BaseResetSystemLib.inf} (65%)
>>   create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetSystemLib.inf
>>   create mode 100644 OvmfPkg/Library/ResetSystemLib/BaseResetShutdown.c
>>   create mode 100644 OvmfPkg/Library/ResetSystemLib/DxeResetShutdown.c
>>
>