Re: [PATCH v6 03/11] hvf: Move common code out

[Alexander Graf]

On 21.01.21 08:26, Philippe Mathieu-Daudé wrote:
> Hi Alexander,
>
> On 1/20/21 11:44 PM, Alexander Graf wrote:
>> Until now, Hypervisor.framework has only been available on x86_64 systems.
>> With Apple Silicon shipping now, it extends its reach to aarch64. To
>> prepare for support for multiple architectures, let's move common code out
>> into its own accel directory.
>>
>> Signed-off-by: Alexander Graf <agraf@csgraf.de>
>> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>
>>
>> ---
>>
>> v3 -> v4:
>>
>>   - Use hv.h instead of Hypervisor.h for 10.15 compat
>>   - Remove manual inclusion of Hypervisor.h in common .c files
>> ---
>>  MAINTAINERS                 |   8 +
>>  accel/hvf/hvf-all.c         |  54 +++++
>>  accel/hvf/hvf-cpus.c        | 462 ++++++++++++++++++++++++++++++++++++
>>  accel/hvf/meson.build       |   7 +
>>  accel/meson.build           |   1 +
>>  include/sysemu/hvf_int.h    |  54 +++++
>>  target/i386/hvf/hvf-cpus.c  | 131 ----------
>>  target/i386/hvf/hvf-cpus.h  |  25 --
>>  target/i386/hvf/hvf-i386.h  |  33 +--
>>  target/i386/hvf/hvf.c       | 360 +---------------------------
>>  target/i386/hvf/meson.build |   1 -
>>  target/i386/hvf/x86hvf.c    |  11 +-
>>  target/i386/hvf/x86hvf.h    |   2 -
>>  13 files changed, 596 insertions(+), 553 deletions(-)
>>  create mode 100644 accel/hvf/hvf-all.c
>>  create mode 100644 accel/hvf/hvf-cpus.c
>>  create mode 100644 accel/hvf/meson.build
>>  create mode 100644 include/sysemu/hvf_int.h
>>  delete mode 100644 target/i386/hvf/hvf-cpus.c
>>  delete mode 100644 target/i386/hvf/hvf-cpus.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3216387521..e589ec02e0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -448,7 +448,15 @@ M: Roman Bolshakov <r.bolshakov@yadro.com>
>>  W: https://wiki.qemu.org/Features/HVF
>>  S: Maintained
>>  F: target/i386/hvf/
>> +
>> +HVF
>> +M: Cameron Esfahani <dirty@apple.com>
>> +M: Roman Bolshakov <r.bolshakov@yadro.com>
>> +W: https://wiki.qemu.org/Features/HVF
>> +S: Maintained
>> +F: accel/hvf/
>>  F: include/sysemu/hvf.h
>> +F: include/sysemu/hvf_int.h
>>  
>>  WHPX CPUs
>>  M: Sunil Muthuswamy <sunilmut@microsoft.com>
>> diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
>> new file mode 100644
>> index 0000000000..5b415eb0ed
>> --- /dev/null
>> +++ b/accel/hvf/hvf-all.c
>> @@ -0,0 +1,54 @@
>> +/*
>> + * QEMU Hypervisor.framework support
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + * Contributions after 2012-01-13 are licensed under the terms of the
>> + * GNU GPL, version 2 or (at your option) any later version.
> Maybe start with GPLv2+ directly?


I'd prefer to leave the license clause in here, because I'm not touching
any of the code, only moving it.


>
>> diff --git a/include/sysemu/hvf_int.h b/include/sysemu/hvf_int.h
>> new file mode 100644
>> index 0000000000..69de46db7d
>> --- /dev/null
>> +++ b/include/sysemu/hvf_int.h
>> @@ -0,0 +1,54 @@
>> +/*
>> + * QEMU Hypervisor.framework (HVF) support
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +/* header to be included in HVF-specific code */
> Can we have this header local to accel/hvf/ ?


At the end of the patch set, the list of files using the header is not
limited to accel/hvf:

MacBook-Air:qemu alex$ git grep hvf_int.h
MAINTAINERS:F: include/sysemu/hvf_int.h
accel/hvf/hvf-accel-ops.c:#include "sysemu/hvf_int.h"
accel/hvf/hvf-all.c:#include "sysemu/hvf_int.h"
target/arm/hvf/hvf.c:#include "sysemu/hvf_int.h"
target/i386/hvf/hvf-i386.h:#include "sysemu/hvf_int.h"
target/i386/hvf/hvf.c:#include "sysemu/hvf_int.h"
target/i386/hvf/vmx.h:#include "sysemu/hvf_int.h"

It also aligns with KVM's use of kvm_int.h, no? Overall, I'm not sure
putting it into the accel directory will buy us too much, I'm happy to
be convinced otherwise though.


> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Thanks :)


Alex