Re: dangers of current NEED_CPU_H, CONFIG_SOFTMMU, CONFIG_USER_ONLY

[Claudio Fontana]

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