Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY (was: [PATCH v11 7/7] cpu: introduce cpu_accel_instance_init)

[Eduardo Habkost]

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?

-- 
Eduardo