| Message ID | 20201211100908.19696-8-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 2 | expand |
Hi, I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY. This patch of mine (the last in the i386 cleanup PART 2) breaks check-tcg. The why is not obvious at all. I'll comment below it. On 12/11/20 11:09 AM, Claudio Fontana wrote: > centralize the calls to cpu->accel_cpu_interface > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > include/hw/core/cpu.h | 6 ++++++ > hw/core/cpu.c | 9 +++++++++ > target/i386/cpu.c | 9 ++------- > 3 files changed, 17 insertions(+), 7 deletions(-) > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 97e1dd8279..cc05c8fc96 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -664,6 +664,12 @@ void cpu_list_remove(CPUState *cpu); > */ > void cpu_reset(CPUState *cpu); > > +/** > + * cpu_accel_instance_init: > + * @cpu: The CPU that needs to do accel-specific object initializations. > + */ > +void cpu_accel_instance_init(CPUState *cpu); > + > /** > * cpu_class_by_name: > * @typename: The CPU base type. > diff --git a/hw/core/cpu.c b/hw/core/cpu.c > index f41c009e6c..873cf5e4ef 100644 > --- a/hw/core/cpu.c > +++ b/hw/core/cpu.c > @@ -242,6 +242,15 @@ void cpu_reset(CPUState *cpu) > trace_guest_cpu_reset(cpu); > } > > +void cpu_accel_instance_init(CPUState *cpu) > +{ > + CPUClass *cc = CPU_GET_CLASS(cpu); > + > + if (cc->accel_cpu_interface) { > + cc->accel_cpu_interface->cpu_instance_init(cpu); > + } > +} > + > static void cpu_common_reset(DeviceState *dev) > { > CPUState *cpu = CPU(dev); > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 5615d9e8bc..8ee39bea24 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -28,7 +28,6 @@ > #include "sysemu/kvm.h" > #include "sysemu/reset.h" > #include "sysemu/hvf.h" > -#include "hw/core/accel-cpu.h" > #include "sysemu/xen.h" > #include "kvm/kvm_i386.h" > #include "sev_i386.h" > @@ -6621,8 +6620,6 @@ static void x86_cpu_initfn(Object *obj) > { > X86CPU *cpu = X86_CPU(obj); > X86CPUClass *xcc = X86_CPU_GET_CLASS(obj); > - CPUClass *cc = CPU_CLASS(xcc); > - > CPUX86State *env = &cpu->env; > FeatureWord w; > > @@ -6680,10 +6677,8 @@ static void x86_cpu_initfn(Object *obj) > x86_cpu_load_model(cpu, xcc->model); > } > > - /* if required, do the accelerator-specific cpu initialization */ > - if (cc->accel_cpu_interface) { > - cc->accel_cpu_interface->cpu_instance_init(CPU(obj)); > - } > + /* if required, do accelerator-specific cpu initializations */ > + cpu_accel_instance_init(CPU(obj)); > } > > static int64_t x86_cpu_get_arch_id(CPUState *cs) > Seems a harmless change right? Just extract the use of cc->accel_cpu_interface->cpu_instance_init from x86 so it can be a useful function for all architecture targets to start using, as we continue the refactoring past x86 into arm etc. Instead, it breaks at least check-tcg (linux-user), if not more. vvv spoiler below vvvv The reason comes down in the end to the fact that we have moved code that is using CPUClass from target/i386 to hw/core/cpu.c. If we look at hw/core/meson.build , we notice that cpu.c is in common_ss. common_ss code does NOT see CONFIG_USER_ONLY, ever. So our struct TcgCpuOperations in include/hw/core/cpu.h, which contains after this series: #ifndef CONFIG_USER_ONLY /** * @do_transaction_failed: Callback for handling failed memory transactions * (ie bus faults or external aborts; not MMU faults) */ void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr, unsigned size, MMUAccessType access_type, int mmu_idx, MemTxAttrs attrs, MemTxResult response, uintptr_t retaddr); /** * @do_unaligned_access: Callback for unaligned access handling */ void (*do_unaligned_access)(CPUState *cpu, vaddr addr, MMUAccessType access_type, int mmu_idx, uintptr_t retaddr); #endif /* !CONFIG_USER_ONLY */ Now suddenly will have some of the objects (in target/...) seeing the struct as not having do_transaction_failed and do_unaligned_access, and some of the objects (common_ss stuff) not seeing CONFIG_USER_ONLY and therefore instead _seeing_ do_transaction_failed and do_unaligned_access. Result is a set of segfaults. The reason we went on and tried to protect with CONFIG_USER_ONLY was to make sure that it is a compile time error to try to use these for linux-user, but we end up making things worse. Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about, because code is being compiled in for linux-user which explicitly should not be compiled there. There are multiple workarounds / fixes possible for my short term problem, but would it not be a good idea to fix this problem at its root once and for all? Otherwise, like I fell into this trap, others also probably will, and based on the existing cpu.h code already in mainline, indeed it seems already have. Thoughts? Thanks, Claudio
On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote: > > Hi, > > I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY. > So our struct TcgCpuOperations in include/hw/core/cpu.h, > which contains after this series: > > #ifndef CONFIG_USER_ONLY > /** > * @do_transaction_failed: Callback for handling failed memory transactions > * (ie bus faults or external aborts; not MMU faults) > */ > void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr, > unsigned size, MMUAccessType access_type, > int mmu_idx, MemTxAttrs attrs, > MemTxResult response, uintptr_t retaddr); > /** > * @do_unaligned_access: Callback for unaligned access handling > */ > void (*do_unaligned_access)(CPUState *cpu, vaddr addr, > MMUAccessType access_type, > int mmu_idx, uintptr_t retaddr); > #endif /* !CONFIG_USER_ONLY */ Yeah, don't try to ifdef out struct fields in common-compiled code... > Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about, > because code is being compiled in for linux-user which explicitly should not be compiled there. The other CONFIG_USER_ONLY checks in that file are only ifdeffing out prototypes for functions that exist only in the softmmu build, or providing do-nothing stubs for functions that are softmmu only. I think they're safe. > There are multiple workarounds / fixes possible for my short term problem, > but would it not be a good idea to fix this problem at its root once and for all? What's your proposal for fixing things ? Incidentally, this should not be a problem for CONFIG_SOFTMMU, because that is listed in include/exec/poison.h so trying to use it in a common (not compiled-per-target) file will give you a compile error. (So in theory we could make CONFIG_USER_ONLY a poisoned identifier but that will require some work to adjust places where we currently use it in "safe" ways...) thanks -- PMM
On Thu, 17 Dec 2020 at 20:15, Peter Maydell <peter.maydell@linaro.org> wrote: > (So in theory we could make CONFIG_USER_ONLY > a poisoned identifier but that will require some work to > adjust places where we currently use it in "safe" ways...) Specifically, putting it in poison.h turns up these places that would need to be made to do what they're doing in a different way somehow: ../../hw/core/cpu.c:211:14: error: attempt to use poisoned "CONFIG_USER_ONLY" include/disas/disas.h:27:13: error: attempt to use poisoned "CONFIG_USER_ONLY" include/exec/address-spaces.h:24:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/exec/cpu-common.h:20:14: error: attempt to use poisoned "CONFIG_USER_ONLY" include/exec/cpu-common.h:6:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/exec/ioport.h:43:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/exec/memory.h:17:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/exec/ramblock.h:22:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/hw/core/cpu.h:1035:8: error: attempt to use poisoned "CONFIG_USER_ONLY" include/hw/core/cpu.h:518:14: error: attempt to use poisoned "CONFIG_USER_ONLY" include/hw/core/cpu.h:602:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/hw/hw.h:4:8: error: attempt to use poisoned "CONFIG_USER_ONLY" include/hw/semihosting/semihost.h:29:8: error: attempt to use poisoned "CONFIG_USER_ONLY" include/sysemu/accel.h:40:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/sysemu/cpus.h:65:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/sysemu/dma.h:34:9: error: attempt to use poisoned "CONFIG_USER_ONLY" include/sysemu/xen.h:27:9: error: attempt to use poisoned "CONFIG_USER_ONLY" That cpu.c one is definitely dubious given it's in a C file, not a header. thanks -- PMM
On Thu, Dec 17, 2020 at 08:15:38PM +0000, Peter Maydell wrote: > On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote: > > > > Hi, > > > > I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY. > > > So our struct TcgCpuOperations in include/hw/core/cpu.h, > > which contains after this series: > > > > #ifndef CONFIG_USER_ONLY > > /** > > * @do_transaction_failed: Callback for handling failed memory transactions > > * (ie bus faults or external aborts; not MMU faults) > > */ > > void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr, > > unsigned size, MMUAccessType access_type, > > int mmu_idx, MemTxAttrs attrs, > > MemTxResult response, uintptr_t retaddr); > > /** > > * @do_unaligned_access: Callback for unaligned access handling > > */ > > void (*do_unaligned_access)(CPUState *cpu, vaddr addr, > > MMUAccessType access_type, > > int mmu_idx, uintptr_t retaddr); > > #endif /* !CONFIG_USER_ONLY */ > > Yeah, don't try to ifdef out struct fields in common-compiled code... > > > Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about, > > because code is being compiled in for linux-user which explicitly should not be compiled there. > > The other CONFIG_USER_ONLY checks in that file are only > ifdeffing out prototypes for functions that exist only in > the softmmu build, or providing do-nothing stubs for functions > that are softmmu only. I think they're safe. > > > There are multiple workarounds / fixes possible for my short term problem, > > but would it not be a good idea to fix this problem at its root once and for all? > > What's your proposal for fixing things ? > > Incidentally, this should not be a problem for CONFIG_SOFTMMU, > because that is listed in include/exec/poison.h so trying to > use it in a common (not compiled-per-target) file will give you > a compile error. (So in theory we could make CONFIG_USER_ONLY > a poisoned identifier but that will require some work to > adjust places where we currently use it in "safe" ways...) The risk of introducing this kind of bug seem to outweigh the benefits of existing safe usage. Do we know how many of these cases we have?
I will take a look, CONFIG_USER_ONLY is definitely something that should be poisoned. Paolo Il gio 17 dic 2020, 21:26 Peter Maydell <peter.maydell@linaro.org> ha scritto: > On Thu, 17 Dec 2020 at 20:15, Peter Maydell <peter.maydell@linaro.org> > wrote: > > (So in theory we could make CONFIG_USER_ONLY > > a poisoned identifier but that will require some work to > > adjust places where we currently use it in "safe" ways...) > > Specifically, putting it in poison.h turns up these places > that would need to be made to do what they're doing in a > different way somehow: > > ../../hw/core/cpu.c:211:14: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/disas/disas.h:27:13: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/exec/address-spaces.h:24:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/exec/cpu-common.h:20:14: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/exec/cpu-common.h:6:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/exec/ioport.h:43:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/exec/memory.h:17:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/exec/ramblock.h:22:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/hw/core/cpu.h:1035:8: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/hw/core/cpu.h:518:14: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/hw/core/cpu.h:602:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/hw/hw.h:4:8: error: attempt to use poisoned "CONFIG_USER_ONLY" > include/hw/semihosting/semihost.h:29:8: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/sysemu/accel.h:40:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/sysemu/cpus.h:65:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/sysemu/dma.h:34:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > include/sysemu/xen.h:27:9: error: attempt to use poisoned > "CONFIG_USER_ONLY" > > That cpu.c one is definitely dubious given it's in a C file, > not a header. > > thanks > -- PMM > >
Hi Peter, thanks for your answer, On 12/17/20 9:15 PM, Peter Maydell wrote: > On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote: >> >> Hi, >> >> I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY. > >> So our struct TcgCpuOperations in include/hw/core/cpu.h, >> which contains after this series: >> >> #ifndef CONFIG_USER_ONLY >> /** >> * @do_transaction_failed: Callback for handling failed memory transactions >> * (ie bus faults or external aborts; not MMU faults) >> */ >> void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr, >> unsigned size, MMUAccessType access_type, >> int mmu_idx, MemTxAttrs attrs, >> MemTxResult response, uintptr_t retaddr); >> /** >> * @do_unaligned_access: Callback for unaligned access handling >> */ >> void (*do_unaligned_access)(CPUState *cpu, vaddr addr, >> MMUAccessType access_type, >> int mmu_idx, uintptr_t retaddr); >> #endif /* !CONFIG_USER_ONLY */ > > Yeah, don't try to ifdef out struct fields in common-compiled code... > >> Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about, >> because code is being compiled in for linux-user which explicitly should not be compiled there. > > The other CONFIG_USER_ONLY checks in that file are only > ifdeffing out prototypes for functions that exist only in > the softmmu build, or providing do-nothing stubs for functions > that are softmmu only. I think they're safe. right, in cpu.h the extra prototypes do not cause immediate harm, but they lead to believe someone editing the file that CONFIG_USER_ONLY can be used and is effective in the file; if CONFIG_USER_ONLY is ineffective, why use it? In the same file, in other places #ifdef NEED_CPU_H #ifdef CONFIG_SOFTMMU is used. Should this pattern be used instead consistently in header files? Is this guaranteed to always do the right thing, from wherever the header file is included? Also in hw/core/cpu.c we see this: #if !defined(CONFIG_USER_ONLY) GuestPanicInformation *cpu_get_crash_info(CPUState *cpu) { CPUClass *cc = CPU_GET_CLASS(cpu); GuestPanicInformation *res = NULL; if (cc->get_crash_info) { res = cc->get_crash_info(cpu); } return res; } #endif If !CONFIG_USER_ONLY is always ineffective, why have it there? This code should probably then be in $(top_srcdir)/cpu.c ? These things may be harmless by themselves, but it takes very little to make a false step, using the existing uses as a reference, as there is no other documentation (I know of). CONFIG_USER_ONLY is used in a few other places outside of target/, including in other header files, as you noted in a follow up email. Are all these uses harmless? Not sure how to determine that for sure.. > >> There are multiple workarounds / fixes possible for my short term problem, >> but would it not be a good idea to fix this problem at its root once and for all? > > What's your proposal for fixing things ? I don't think I have the full picture yet, so I think the optimal solution can only be figured out together; I will try to flail in the dark hoping to hit on something that sparks an idea. - do we need both CONFIG_SOFTMMU and CONFIG_USER_ONLY? (I always wondered about the "ONLY" part of it, why not just CONFIG_USER?) Based on previous comments from Richard we might need both in the future, but I fail to detect which places are meaningful for the one or the other. - Is the NEED_CPU_H + CONFIG_SOFTMMU check always the right thing to do? Is it always right in header files? ... - is it possible to define very clearly where in the codebase they should be used? As an ignorant example from my side: only use CONFIG_USER_ONLY inside of target/, Making the rule "don't use this for common_ss" is very difficult to stick to in practice, with header files that end up being used from multiple sources, some in common_ss and some not, and it's so hidden from the day to day activities, one need to explicitly check. Instead if it's something obvious like: only in this subtree, then it is at least realistic to try to stick to it. - is it possible to check the [in]correct use of CONFIG_USER_ONLY and CONFIG_SOFTMMU during compilation or with a script in scripts/ ? I think you answered that already, adding it to "poison". Any downside to that? Does it still make sense to restrict the uses more, in favor of clarity? - once we figure out the solution, I would try to document the result of the whole experience clearly, in doc/devel/ for example? > > Incidentally, this should not be a problem for CONFIG_SOFTMMU, > because that is listed in include/exec/poison.h so trying to > use it in a common (not compiled-per-target) file will give you > a compile error. (So in theory we could make CONFIG_USER_ONLY > a poisoned identifier but that will require some work to > adjust places where we currently use it in "safe" ways...) > > thanks > -- PMM Thanks! Claudio
On Thu, Dec 17, 2020 at 10:13:17PM +0100, Paolo Bonzini wrote: > I will take a look, CONFIG_USER_ONLY is definitely something that should be > poisoned. Thanks! I started looking at it, but I gave up when I realized how much work it would required. :) In any case, feel free to reuse the 2 small commits I've just pushed to https://gitlab.com/ehabkost/qemu/-/commits/work/poison-user-only > > Paolo > > Il gio 17 dic 2020, 21:26 Peter Maydell <peter.maydell@linaro.org> ha > scritto: > > > On Thu, 17 Dec 2020 at 20:15, Peter Maydell <peter.maydell@linaro.org> > > wrote: > > > (So in theory we could make CONFIG_USER_ONLY > > > a poisoned identifier but that will require some work to > > > adjust places where we currently use it in "safe" ways...) > > > > Specifically, putting it in poison.h turns up these places > > that would need to be made to do what they're doing in a > > different way somehow: > > > > ../../hw/core/cpu.c:211:14: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/disas/disas.h:27:13: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/exec/address-spaces.h:24:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/exec/cpu-common.h:20:14: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/exec/cpu-common.h:6:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/exec/ioport.h:43:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/exec/memory.h:17:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/exec/ramblock.h:22:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/hw/core/cpu.h:1035:8: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/hw/core/cpu.h:518:14: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/hw/core/cpu.h:602:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/hw/hw.h:4:8: error: attempt to use poisoned "CONFIG_USER_ONLY" > > include/hw/semihosting/semihost.h:29:8: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/sysemu/accel.h:40:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/sysemu/cpus.h:65:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/sysemu/dma.h:34:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > include/sysemu/xen.h:27:9: error: attempt to use poisoned > > "CONFIG_USER_ONLY" > > > > That cpu.c one is definitely dubious given it's in a C file, > > not a header. > > > > thanks > > -- PMM > > > >
On 12/17/20 9:15 PM, Peter Maydell wrote: > On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote: >> >> Hi, >> >> I would like to highlight the current dangerous state of NEED_CPU_H / CONFIG_SOFTMMU / CONFIG_USER_ONLY. > >> So our struct TcgCpuOperations in include/hw/core/cpu.h, >> which contains after this series: >> >> #ifndef CONFIG_USER_ONLY >> /** >> * @do_transaction_failed: Callback for handling failed memory transactions >> * (ie bus faults or external aborts; not MMU faults) >> */ >> void (*do_transaction_failed)(CPUState *cpu, hwaddr physaddr, vaddr addr, >> unsigned size, MMUAccessType access_type, >> int mmu_idx, MemTxAttrs attrs, >> MemTxResult response, uintptr_t retaddr); >> /** >> * @do_unaligned_access: Callback for unaligned access handling >> */ >> void (*do_unaligned_access)(CPUState *cpu, vaddr addr, >> MMUAccessType access_type, >> int mmu_idx, uintptr_t retaddr); >> #endif /* !CONFIG_USER_ONLY */ > > Yeah, don't try to ifdef out struct fields in common-compiled code... or should I? Using #ifdef NEED_CPU_H #ifdef CONFIG_SOFTMMU seems to do what I expect. Is it wrong? Thanks, Claudio > >> Note that include/hw/core/cpu.h already uses CONFIG_USER_ONLY in other parts of the header file, and we might have hidden problems as a result we (or at least I) don't know about, >> because code is being compiled in for linux-user which explicitly should not be compiled there. > > The other CONFIG_USER_ONLY checks in that file are only > ifdeffing out prototypes for functions that exist only in > the softmmu build, or providing do-nothing stubs for functions > that are softmmu only. I think they're safe. > >> There are multiple workarounds / fixes possible for my short term problem, >> but would it not be a good idea to fix this problem at its root once and for all? > > What's your proposal for fixing things ? > > Incidentally, this should not be a problem for CONFIG_SOFTMMU, > because that is listed in include/exec/poison.h so trying to > use it in a common (not compiled-per-target) file will give you > a compile error. (So in theory we could make CONFIG_USER_ONLY > a poisoned identifier but that will require some work to > adjust places where we currently use it in "safe" ways...) > > thanks > -- PMM >
On Thu, 17 Dec 2020 at 22:45, Claudio Fontana <cfontana@suse.de> wrote: > > On 12/17/20 9:15 PM, Peter Maydell wrote: > > On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote: > > Yeah, don't try to ifdef out struct fields in common-compiled code... > > or should I? Using > > #ifdef NEED_CPU_H > #ifdef CONFIG_SOFTMMU > > seems to do what I expect. Is it wrong? I think that gives you two versions of the struct: - one seen by compiled-once files and by compiled-per-target softmmu files - one seen by compiled-per-target user-only files Since the user-only target executables link both compiled-per-target and compiled-once files I think they end up with different C files thinking the same struct has a different layout/size which seems like it's going to cause problems. thanks -- PMM
On 12/17/20 11:49 PM, Peter Maydell wrote: > On Thu, 17 Dec 2020 at 22:45, Claudio Fontana <cfontana@suse.de> wrote: >> >> On 12/17/20 9:15 PM, Peter Maydell wrote: >>> On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote: >>> Yeah, don't try to ifdef out struct fields in common-compiled code... >> >> or should I? Using >> >> #ifdef NEED_CPU_H >> #ifdef CONFIG_SOFTMMU >> >> seems to do what I expect. Is it wrong? > > I think that gives you two versions of the struct: > - one seen by compiled-once files and by compiled-per-target softmmu files > - one seen by compiled-per-target user-only files > > Since the user-only target executables link both compiled-per-target > and compiled-once files I think they end up with different C files > thinking the same struct has a different layout/size which seems > like it's going to cause problems. > > thanks > -- PMM > It doesn't with #ifdef NEED_CPU_H #ifdef CONFIG_SOFTMMU just double checked the pointers from both files compiled per target and "common"; also all tests are ok. It immediately breaks if I replace those two defines with #ifndef CONFIG_USER_ONLY and recompile. I thought it was by design, but I guess this is just a "lucky" accident? Ciao, Claudio
On 12/18/20 12:47 AM, Claudio Fontana wrote: > On 12/17/20 11:49 PM, Peter Maydell wrote: >> On Thu, 17 Dec 2020 at 22:45, Claudio Fontana <cfontana@suse.de> wrote: >>> >>> On 12/17/20 9:15 PM, Peter Maydell wrote: >>>> On Thu, 17 Dec 2020 at 19:46, Claudio Fontana <cfontana@suse.de> wrote: >>>> Yeah, don't try to ifdef out struct fields in common-compiled code... >>> >>> or should I? Using >>> >>> #ifdef NEED_CPU_H >>> #ifdef CONFIG_SOFTMMU >>> >>> seems to do what I expect. Is it wrong? >> >> I think that gives you two versions of the struct: >> - one seen by compiled-once files and by compiled-per-target softmmu files >> - one seen by compiled-per-target user-only files >> >> Since the user-only target executables link both compiled-per-target >> and compiled-once files I think they end up with different C files >> thinking the same struct has a different layout/size which seems >> like it's going to cause problems. >> >> thanks >> -- PMM >> > > It doesn't with > > #ifdef NEED_CPU_H > #ifdef CONFIG_SOFTMMU > > just double checked the pointers from both files compiled per target and "common"; also all tests are ok. > > It immediately breaks if I replace those two defines with #ifndef CONFIG_USER_ONLY and recompile. > > I thought it was by design, but I guess this is just a "lucky" accident? By lucky accident I mean that CONFIG_SOFTMMU is a check in the positive instead of the #if !defined(CONFIG_USER_ONLY), so it ends up working.. > > Ciao, > > Claudio >
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 97e1dd8279..cc05c8fc96 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -664,6 +664,12 @@ void cpu_list_remove(CPUState *cpu); */ void cpu_reset(CPUState *cpu); +/** + * cpu_accel_instance_init: + * @cpu: The CPU that needs to do accel-specific object initializations. + */ +void cpu_accel_instance_init(CPUState *cpu); + /** * cpu_class_by_name: * @typename: The CPU base type. diff --git a/hw/core/cpu.c b/hw/core/cpu.c index f41c009e6c..873cf5e4ef 100644 --- a/hw/core/cpu.c +++ b/hw/core/cpu.c @@ -242,6 +242,15 @@ void cpu_reset(CPUState *cpu) trace_guest_cpu_reset(cpu); } +void cpu_accel_instance_init(CPUState *cpu) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + + if (cc->accel_cpu_interface) { + cc->accel_cpu_interface->cpu_instance_init(cpu); + } +} + static void cpu_common_reset(DeviceState *dev) { CPUState *cpu = CPU(dev); diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 5615d9e8bc..8ee39bea24 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -28,7 +28,6 @@ #include "sysemu/kvm.h" #include "sysemu/reset.h" #include "sysemu/hvf.h" -#include "hw/core/accel-cpu.h" #include "sysemu/xen.h" #include "kvm/kvm_i386.h" #include "sev_i386.h" @@ -6621,8 +6620,6 @@ static void x86_cpu_initfn(Object *obj) { X86CPU *cpu = X86_CPU(obj); X86CPUClass *xcc = X86_CPU_GET_CLASS(obj); - CPUClass *cc = CPU_CLASS(xcc); - CPUX86State *env = &cpu->env; FeatureWord w; @@ -6680,10 +6677,8 @@ static void x86_cpu_initfn(Object *obj) x86_cpu_load_model(cpu, xcc->model); } - /* if required, do the accelerator-specific cpu initialization */ - if (cc->accel_cpu_interface) { - cc->accel_cpu_interface->cpu_instance_init(CPU(obj)); - } + /* if required, do accelerator-specific cpu initializations */ + cpu_accel_instance_init(CPU(obj)); } static int64_t x86_cpu_get_arch_id(CPUState *cs)
centralize the calls to cpu->accel_cpu_interface Signed-off-by: Claudio Fontana <cfontana@suse.de> --- include/hw/core/cpu.h | 6 ++++++ hw/core/cpu.c | 9 +++++++++ target/i386/cpu.c | 9 ++------- 3 files changed, 17 insertions(+), 7 deletions(-)