| Message ID | 20201124162210.8796-10-cfontana@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | i386 cleanup | expand |
On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote: > apply this to the registration of the cpus accel interfaces, > > but this will be also in preparation for later use of this > new module init step to also register per-accel x86 cpu type > interfaces. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- [...] > diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c > index b4e731cb2b..482f89729f 100644 > --- a/accel/qtest/qtest.c > +++ b/accel/qtest/qtest.c > @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = { > > static int qtest_init_accel(MachineState *ms) > { > - cpus_register_accel(&qtest_cpus); > return 0; > } > > @@ -58,3 +57,12 @@ static void qtest_type_init(void) > } > > type_init(qtest_type_init); > + > +static void qtest_accel_cpu_init(void) > +{ > + if (qtest_enabled()) { > + cpus_register_accel(&qtest_cpus); > + } > +} > + > +accel_cpu_init(qtest_accel_cpu_init); I don't understand why this (and the similar changes on other accelerators) is an improvement. You are replacing a trivial AccelClass-specific init method with a module_init() function that has a hidden dependency on runtime state.
On 11/24/20 6:08 PM, Eduardo Habkost wrote: > On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote: >> apply this to the registration of the cpus accel interfaces, >> >> but this will be also in preparation for later use of this >> new module init step to also register per-accel x86 cpu type >> interfaces. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- > [...] >> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c >> index b4e731cb2b..482f89729f 100644 >> --- a/accel/qtest/qtest.c >> +++ b/accel/qtest/qtest.c >> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = { >> >> static int qtest_init_accel(MachineState *ms) >> { >> - cpus_register_accel(&qtest_cpus); >> return 0; >> } >> >> @@ -58,3 +57,12 @@ static void qtest_type_init(void) >> } >> >> type_init(qtest_type_init); >> + >> +static void qtest_accel_cpu_init(void) >> +{ >> + if (qtest_enabled()) { >> + cpus_register_accel(&qtest_cpus); >> + } >> +} >> + >> +accel_cpu_init(qtest_accel_cpu_init); > > I don't understand why this (and the similar changes on other > accelerators) is an improvement. > > You are replacing a trivial AccelClass-specific init method with > a module_init() function that has a hidden dependency on runtime > state. > Not a big advantage I agree, I think however there is one, in using the existing framework that exists, for the purposes that it was built for. As I understand it, the global module init framework is supposed to mark the major initialization steps, and this seems to fit the bill. The "hidden" dependency on the fact that accels need to be initialized at that time, is not hidden at all I think, it is what this module init step is all about. It is explicitly meaning, "_now that the current accelerator is chosen_, perform these initializations". But, as you mentioned elsewhere, I will in the meantime anyway squash these things so they do not start fragmented at all, and centralize immediately. Thanks, Claudio
On Tue, Nov 24, 2020 at 07:29:50PM +0100, Claudio Fontana wrote: > On 11/24/20 6:08 PM, Eduardo Habkost wrote: > > On Tue, Nov 24, 2020 at 05:22:07PM +0100, Claudio Fontana wrote: > >> apply this to the registration of the cpus accel interfaces, > >> > >> but this will be also in preparation for later use of this > >> new module init step to also register per-accel x86 cpu type > >> interfaces. > >> > >> Signed-off-by: Claudio Fontana <cfontana@suse.de> > >> --- > > [...] > >> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c > >> index b4e731cb2b..482f89729f 100644 > >> --- a/accel/qtest/qtest.c > >> +++ b/accel/qtest/qtest.c > >> @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = { > >> > >> static int qtest_init_accel(MachineState *ms) > >> { > >> - cpus_register_accel(&qtest_cpus); > >> return 0; > >> } > >> > >> @@ -58,3 +57,12 @@ static void qtest_type_init(void) > >> } > >> > >> type_init(qtest_type_init); > >> + > >> +static void qtest_accel_cpu_init(void) > >> +{ > >> + if (qtest_enabled()) { > >> + cpus_register_accel(&qtest_cpus); > >> + } > >> +} > >> + > >> +accel_cpu_init(qtest_accel_cpu_init); > > > > I don't understand why this (and the similar changes on other > > accelerators) is an improvement. > > > > You are replacing a trivial AccelClass-specific init method with > > a module_init() function that has a hidden dependency on runtime > > state. > > > > Not a big advantage I agree, > I think however there is one, in using the existing framework that exists, for the purposes that it was built for. > > As I understand it, the global module init framework is supposed to mark the major initialization steps, > and this seems to fit the bill. That seems to be the main source of disagreement. I don't agree that's the purpose of module_init(). The module init framework is used to unconditionally register module-provided entities like option names, QOM types, block drivers, trace events, etc. The entities registered by module init functions represent a passive dynamically loadable piece of code. module_init() was never used for initialization of machines, devices, CPUs, or other runtime state. We don't have MODULE_INIT_MONITOR, MODULE_INIT_OBJECTS, MODULE_INIT_MACHINE, MODULE_INIT_CPUS, MODULE_INIT_DEVICES, etc. And I'm not convinced we should, because it would hide dependencies between initialization steps. It would force us to make initialization functions affect and depend on global state. I believe this: int main() { result_of_A = init_A(input_for_A); result_of_B = init_B(input_for_B); result_of_C = init_C(input_for_C); } is clearer and more maintainable than: int main() { module_init_call(MODULE_INIT_A); /* result_of_A hidden in global state */ module_init_call(MODULE_INIT_B); /* result_of_B hidden in global state */ module_init_call(MODULE_INIT_C); /* result_of_C hidden in global state */ } > > The "hidden" dependency on the fact that accels need to be initialized at that time, is not hidden at all I think, > it is what this module init step is all about. > > It is explicitly meaning, "_now that the current accelerator is chosen_, perform these initializations". > > But, as you mentioned elsewhere, I will in the meantime anyway squash these things so they do not start fragmented at all, and centralize immediately. Agreed. We still need to sort out the disagreement above, or we'll spend a lot of energy arguing about code.
On 24/11/20 20:08, Eduardo Habkost wrote: >> Not a big advantage I agree, >> I think however there is one, in using the existing framework that exists, for the purposes that it was built for. >> >> As I understand it, the global module init framework is supposed to mark the major initialization steps, >> and this seems to fit the bill. > That seems to be the main source of disagreement. I don't agree > that's the purpose of module_init(). > > The module init framework is used to unconditionally register > module-provided entities like option names, QOM types, block > drivers, trace events, etc. The entities registered by module > init functions represent a passive dynamically loadable piece of > code. Indeed. Think of module_init() as C++ global constructors. Anything that has an "if" does not belong in module_init. If you look at my review of the previous versions, I was not necessarily against MODULE_INIT_ACCEL_CPU, but I was (and am) strongly against calling it in the middle of the machine creation sequence. Paolo
On 11/24/20 9:01 PM, Paolo Bonzini wrote: > On 24/11/20 20:08, Eduardo Habkost wrote: >>> Not a big advantage I agree, >>> I think however there is one, in using the existing framework that exists, for the purposes that it was built for. >>> >>> As I understand it, the global module init framework is supposed to mark the major initialization steps, >>> and this seems to fit the bill. >> That seems to be the main source of disagreement. I don't agree >> that's the purpose of module_init(). >> >> The module init framework is used to unconditionally register >> module-provided entities like option names, QOM types, block >> drivers, trace events, etc. The entities registered by module >> init functions represent a passive dynamically loadable piece of >> code. > > Indeed. Think of module_init() as C++ global constructors. > > Anything that has an "if" does not belong in module_init. > > If you look at my review of the previous versions, I was not necessarily > against MODULE_INIT_ACCEL_CPU, but I was (and am) strongly against > calling it in the middle of the machine creation sequence. > > Paolo > > Hi Paolo, in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right? But the fact that its behavior depends on current_accel() still disqualifies it? It is called right after the accelerator is chosen and initialized in RFC v5, this still is "in the middle of the machine creation sequence"? I am trying to find the actual things to fix, since when doing RFC v5 I tried to specifically address two points: 1) no if () inside module init functions 2) no proliferation of module init functions which I accomplished via AccelClass extension to user mode, current_accel(), and class lookup. If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the call so that it is not "in the middle"? It is interesting for me to try to discern which meaning you folks give to MODULE_INIT. Keep in mind, I will experiment with Eduardo's option of "one accel_init() to rule them all", without MODULE_INIT_ACCEL_CPU, so I am not focused on using this no matter what. Ciao, Claudio
On 25/11/20 10:21, Claudio Fontana wrote: > Hi Paolo, > > in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right? > But the fact that its behavior depends on current_accel() still disqualifies it? > It is called right after the accelerator is chosen and initialized > in RFC v5, this still is "in the middle of the machine creation sequence"? Yes, machine creation basically starts after command line parsing, or perhaps even _with_ command line parsing. Basically once the user can control the flow it is already too late. > I am trying to find the actual things to fix, since when doing RFC > v5 I tried to specifically address two points: > > 1) no if () inside module init functions > > 2) no proliferation of module init functions > > which I accomplished via AccelClass extension to user mode, current_accel(), and class lookup. Yes, the rest is great, I'm just not sure that MODULE_INIT_ACCEL_CPU is useful and if virtual functions on accel and CPU_RESOLVING_TYPE can achieve the same. > If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the call so that it is not "in the middle"? No later than the runstate_init() call, roughly. Paolo
On 11/25/20 10:30 AM, Paolo Bonzini wrote: > On 25/11/20 10:21, Claudio Fontana wrote: >> Hi Paolo, >> >> in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right? >> But the fact that its behavior depends on current_accel() still disqualifies it? >> It is called right after the accelerator is chosen and initialized >> in RFC v5, this still is "in the middle of the machine creation sequence"? > Yes, machine creation basically starts after command line parsing, or > perhaps even _with_ command line parsing. Basically once the user can > control the flow it is already too late. > >> I am trying to find the actual things to fix, since when doing RFC >> v5 I tried to specifically address two points: >> >> 1) no if () inside module init functions >> >> 2) no proliferation of module init functions >> >> which I accomplished via AccelClass extension to user mode, current_accel(), and class lookup. > > Yes, the rest is great, I'm just not sure that MODULE_INIT_ACCEL_CPU is > useful and if virtual functions on accel and CPU_RESOLVING_TYPE can > achieve the same. > >> If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the call so that it is not "in the middle"? > > No later than the runstate_init() call, roughly. > > Paolo > > Aha! Then that solves it, I don't think it is feasible to put it that early with the meaning I intended for it. Thanks for clarifying this, Claudio
On 11/24/20 5:22 PM, Claudio Fontana wrote: > apply this to the registration of the cpus accel interfaces, > > but this will be also in preparation for later use of this > new module init step to also register per-accel x86 cpu type > interfaces. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > accel/kvm/kvm-all.c | 11 +++++++++-- > accel/qtest/qtest.c | 10 +++++++++- > accel/tcg/tcg-all.c | 8 -------- > accel/tcg/tcg-cpus.c | 15 +++++++++++++++ > accel/xen/xen-all.c | 12 +++++++++--- > include/qemu/module.h | 2 ++ > roms/qboot | 2 +- > softmmu/vl.c | 6 ++++++ > target/i386/hax/hax-all.c | 12 +++++++++--- > target/i386/hvf/hvf.c | 10 +++++++++- > target/i386/whpx/whpx-all.c | 11 +++++++++-- > 11 files changed, 78 insertions(+), 21 deletions(-) ... > diff --git a/roms/qboot b/roms/qboot > index a5300c4949..cb1c49e0cf 160000 > --- a/roms/qboot > +++ b/roms/qboot > @@ -1 +1 @@ > -Subproject commit a5300c4949b8d4de2d34bedfaed66793f48ec948 > +Subproject commit cb1c49e0cfac99b9961d136ac0194da62c28cf64 Hmmm unrelated change I presume.
On 11/26/20 12:25 PM, Philippe Mathieu-Daudé wrote: > On 11/24/20 5:22 PM, Claudio Fontana wrote: >> apply this to the registration of the cpus accel interfaces, >> >> but this will be also in preparation for later use of this >> new module init step to also register per-accel x86 cpu type >> interfaces. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> accel/kvm/kvm-all.c | 11 +++++++++-- >> accel/qtest/qtest.c | 10 +++++++++- >> accel/tcg/tcg-all.c | 8 -------- >> accel/tcg/tcg-cpus.c | 15 +++++++++++++++ >> accel/xen/xen-all.c | 12 +++++++++--- >> include/qemu/module.h | 2 ++ >> roms/qboot | 2 +- >> softmmu/vl.c | 6 ++++++ >> target/i386/hax/hax-all.c | 12 +++++++++--- >> target/i386/hvf/hvf.c | 10 +++++++++- >> target/i386/whpx/whpx-all.c | 11 +++++++++-- >> 11 files changed, 78 insertions(+), 21 deletions(-) > ... >> diff --git a/roms/qboot b/roms/qboot >> index a5300c4949..cb1c49e0cf 160000 >> --- a/roms/qboot >> +++ b/roms/qboot >> @@ -1 +1 @@ >> -Subproject commit a5300c4949b8d4de2d34bedfaed66793f48ec948 >> +Subproject commit cb1c49e0cfac99b9961d136ac0194da62c28cf64 > > Hmmm unrelated change I presume. > Hi Philippe, yes, clearly, Thanks! Ciao, CLaudio
On 11/25/20 10:30 AM, Paolo Bonzini wrote: > On 25/11/20 10:21, Claudio Fontana wrote: >> Hi Paolo, >> >> in RFC v5 , module init for ACCEL_CPU is not conditional anymore, right? >> But the fact that its behavior depends on current_accel() still disqualifies it? >> It is called right after the accelerator is chosen and initialized >> in RFC v5, this still is "in the middle of the machine creation sequence"? > Yes, machine creation basically starts after command line parsing, or > perhaps even _with_ command line parsing. Basically once the user can > control the flow it is already too late. > >> I am trying to find the actual things to fix, since when doing RFC >> v5 I tried to specifically address two points: >> >> 1) no if () inside module init functions >> >> 2) no proliferation of module init functions >> >> which I accomplished via AccelClass extension to user mode, current_accel(), and class lookup. > > Yes, the rest is great, I'm just not sure that MODULE_INIT_ACCEL_CPU is > useful and if virtual functions on accel and CPU_RESOLVING_TYPE can > achieve the same. > >> If MODULE_INIT_ACCEL_CPU remains an option, where would you like to see the call so that it is not "in the middle"? > > No later than the runstate_init() call, roughly. > > Paolo > Hi Paolo, not super-related to the context above, during the implementation I am trying to offer accel/accel-softmmu.c accel/accel-common.c accel/accel-user.c But I don't seem able to use CONFIG_USER_ONLY in accel-common.c . in accel/meson.build I am saying common_ss.add(files('accel-common.c')) softmmu_ss.add(files('accel-softmmu.c')) user_ss.add(files('accel-user.c')) But this doesn't work, if I use common_ss. If I use specific_ss, it works. So the term "common" is a bit overloaded, it means stuff for libcommon and such, not common between softmmu and user, right? Ciao, Claudio
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 9ef5daf4c5..509b249f52 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -2251,8 +2251,6 @@ static int kvm_init(MachineState *ms) ret = ram_block_discard_disable(true); assert(!ret); } - - cpus_register_accel(&kvm_cpus); return 0; err: @@ -3236,3 +3234,12 @@ static void kvm_type_init(void) } type_init(kvm_type_init); + +static void kvm_accel_cpu_init(void) +{ + if (kvm_enabled()) { + cpus_register_accel(&kvm_cpus); + } +} + +accel_cpu_init(kvm_accel_cpu_init); diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c index b4e731cb2b..482f89729f 100644 --- a/accel/qtest/qtest.c +++ b/accel/qtest/qtest.c @@ -32,7 +32,6 @@ const CpusAccel qtest_cpus = { static int qtest_init_accel(MachineState *ms) { - cpus_register_accel(&qtest_cpus); return 0; } @@ -58,3 +57,12 @@ static void qtest_type_init(void) } type_init(qtest_type_init); + +static void qtest_accel_cpu_init(void) +{ + if (qtest_enabled()) { + cpus_register_accel(&qtest_cpus); + } +} + +accel_cpu_init(qtest_accel_cpu_init); diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c index ef91d13669..2b86df9ba0 100644 --- a/accel/tcg/tcg-all.c +++ b/accel/tcg/tcg-all.c @@ -108,14 +108,6 @@ static int tcg_init(MachineState *ms) * Initialize TCG regions */ tcg_region_init(); - - if (mttcg_enabled) { - cpus_register_accel(&tcg_cpus_mttcg); - } else if (icount_enabled()) { - cpus_register_accel(&tcg_cpus_icount); - } else { - cpus_register_accel(&tcg_cpus_rr); - } return 0; } diff --git a/accel/tcg/tcg-cpus.c b/accel/tcg/tcg-cpus.c index e335f9f155..c9e662f06e 100644 --- a/accel/tcg/tcg-cpus.c +++ b/accel/tcg/tcg-cpus.c @@ -80,3 +80,18 @@ void tcg_cpus_handle_interrupt(CPUState *cpu, int mask) qatomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1); } } + +static void tcg_accel_cpu_init(void) +{ + if (tcg_enabled()) { + if (qemu_tcg_mttcg_enabled()) { + cpus_register_accel(&tcg_cpus_mttcg); + } else if (icount_enabled()) { + cpus_register_accel(&tcg_cpus_icount); + } else { + cpus_register_accel(&tcg_cpus_rr); + } + } +} + +accel_cpu_init(tcg_accel_cpu_init); diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c index 594aaf6b49..be09b6ec22 100644 --- a/accel/xen/xen-all.c +++ b/accel/xen/xen-all.c @@ -185,9 +185,6 @@ static int xen_init(MachineState *ms) * opt out of system RAM being allocated by generic code */ mc->default_ram_id = NULL; - - cpus_register_accel(&xen_cpus); - return 0; } @@ -228,3 +225,12 @@ static void xen_type_init(void) } type_init(xen_type_init); + +static void xen_accel_cpu_init(void) +{ + if (xen_enabled()) { + cpus_register_accel(&xen_cpus); + } +} + +accel_cpu_init(xen_accel_cpu_init); diff --git a/include/qemu/module.h b/include/qemu/module.h index 944d403cbd..485eda986a 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -44,6 +44,7 @@ typedef enum { MODULE_INIT_BLOCK, MODULE_INIT_OPTS, MODULE_INIT_QOM, + MODULE_INIT_ACCEL_CPU, MODULE_INIT_TRACE, MODULE_INIT_XEN_BACKEND, MODULE_INIT_LIBQOS, @@ -54,6 +55,7 @@ typedef enum { #define block_init(function) module_init(function, MODULE_INIT_BLOCK) #define opts_init(function) module_init(function, MODULE_INIT_OPTS) #define type_init(function) module_init(function, MODULE_INIT_QOM) +#define accel_cpu_init(function) module_init(function, MODULE_INIT_ACCEL_CPU) #define trace_init(function) module_init(function, MODULE_INIT_TRACE) #define xen_backend_init(function) module_init(function, \ MODULE_INIT_XEN_BACKEND) diff --git a/roms/qboot b/roms/qboot index a5300c4949..cb1c49e0cf 160000 --- a/roms/qboot +++ b/roms/qboot @@ -1 +1 @@ -Subproject commit a5300c4949b8d4de2d34bedfaed66793f48ec948 +Subproject commit cb1c49e0cfac99b9961d136ac0194da62c28cf64 diff --git a/softmmu/vl.c b/softmmu/vl.c index bc20c526d2..fb92132222 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -4173,6 +4173,12 @@ void qemu_init(int argc, char **argv, char **envp) */ configure_accelerators(argv[0]); + /* + * accelerator has been chosen and initialized, now it is time to + * register the cpu accel interface. + */ + module_call_init(MODULE_INIT_ACCEL_CPU); + /* * Beware, QOM objects created before this point miss global and * compat properties. diff --git a/target/i386/hax/hax-all.c b/target/i386/hax/hax-all.c index d7f4bb44a7..77c365311c 100644 --- a/target/i386/hax/hax-all.c +++ b/target/i386/hax/hax-all.c @@ -364,9 +364,6 @@ static int hax_accel_init(MachineState *ms) !ret ? "working" : "not working", !ret ? "fast virt" : "emulation"); } - if (ret == 0) { - cpus_register_accel(&hax_cpus); - } return ret; } @@ -1141,3 +1138,12 @@ static void hax_type_init(void) } type_init(hax_type_init); + +static void hax_accel_cpu_init(void) +{ + if (hax_enabled()) { + cpus_register_accel(&hax_cpus); + } +} + +accel_cpu_init(hax_accel_cpu_init); diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c index ffc9efa40f..58794c35ae 100644 --- a/target/i386/hvf/hvf.c +++ b/target/i386/hvf/hvf.c @@ -887,7 +887,6 @@ static int hvf_accel_init(MachineState *ms) hvf_state = s; memory_listener_register(&hvf_memory_listener, &address_space_memory); - cpus_register_accel(&hvf_cpus); return 0; } @@ -911,3 +910,12 @@ static void hvf_type_init(void) } type_init(hvf_type_init); + +static void hvf_accel_cpu_init(void) +{ + if (hvf_enabled()) { + cpus_register_accel(&hvf_cpus); + } +} + +accel_cpu_init(hvf_accel_cpu_init); diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c index ee6b606194..097d6f5e60 100644 --- a/target/i386/whpx/whpx-all.c +++ b/target/i386/whpx/whpx-all.c @@ -1642,8 +1642,6 @@ static int whpx_accel_init(MachineState *ms) whpx_memory_init(); - cpus_register_accel(&whpx_cpus); - printf("Windows Hypervisor Platform accelerator is operational\n"); return 0; @@ -1713,3 +1711,12 @@ error: } type_init(whpx_type_init); + +static void whpx_accel_cpu_init(void) +{ + if (whpx_enabled()) { + cpus_register_accel(&whpx_cpus); + } +} + +accel_cpu_init(whpx_accel_cpu_init);
apply this to the registration of the cpus accel interfaces, but this will be also in preparation for later use of this new module init step to also register per-accel x86 cpu type interfaces. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- accel/kvm/kvm-all.c | 11 +++++++++-- accel/qtest/qtest.c | 10 +++++++++- accel/tcg/tcg-all.c | 8 -------- accel/tcg/tcg-cpus.c | 15 +++++++++++++++ accel/xen/xen-all.c | 12 +++++++++--- include/qemu/module.h | 2 ++ roms/qboot | 2 +- softmmu/vl.c | 6 ++++++ target/i386/hax/hax-all.c | 12 +++++++++--- target/i386/hvf/hvf.c | 10 +++++++++- target/i386/whpx/whpx-all.c | 11 +++++++++-- 11 files changed, 78 insertions(+), 21 deletions(-)