Re: [PATCH 08/11] target/s390x: Restrict system-mode declarations

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 11 May 2020 14:21:27 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 5/11/20 12:48 PM, Cornelia Huck wrote:
> > On Sat,  9 May 2020 15:09:07 +0200
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> As these declarations are restricted to !CONFIG_USER_ONLY in
> >> helper.c, only declare them when system-mode emulation is used.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>   target/s390x/internal.h | 11 ++++++-----
> >>   1 file changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/target/s390x/internal.h b/target/s390x/internal.h
> >> index c1678dc6bc..ddc276cdf4 100644
> >> --- a/target/s390x/internal.h
> >> +++ b/target/s390x/internal.h
> >> @@ -236,7 +236,6 @@ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
> >>   
> >>   /* cc_helper.c */
> >>   const char *cc_name(enum cc_op cc_op);
> >> -void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);
> >>   uint32_t calc_cc(CPUS390XState *env, uint32_t cc_op, uint64_t src, uint64_t dst,
> >>                    uint64_t vr);
> >>   
> >> @@ -303,18 +302,20 @@ void s390_cpu_gdb_init(CPUState *cs);
> >>   
> >>   /* helper.c */
> >>   void s390_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
> >> -hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >> -hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
> >> +void do_restart_interrupt(CPUS390XState *env);
> >> +
> >> +#ifndef CONFIG_USER_ONLY
> >> +void load_psw(CPUS390XState *env, uint64_t mask, uint64_t addr);  
> > 
> > load_psw() is in cc_helper.c (and not in helper.c). Rather add the
> > #ifndef above, even if it is a bit awkward? Otherwise, the wrong
> > comment makes it confusing.  
> 
> I've been tempted to remove the kinda outdated /* helper.c */ comment...

I don't think they're really outdated, but not sure how useful they are
to people. I'm not personally attached to them, other opinions?

> 
> >   
> >>   uint64_t get_psw_mask(CPUS390XState *env);
> >>   void s390_cpu_recompute_watchpoints(CPUState *cs);
> >>   void s390x_tod_timer(void *opaque);
> >>   void s390x_cpu_timer(void *opaque);
> >> -void do_restart_interrupt(CPUS390XState *env);
> >>   void s390_handle_wait(S390CPU *cpu);
> >> +hwaddr s390_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> >> +hwaddr s390_cpu_get_phys_addr_debug(CPUState *cpu, vaddr addr);
> >>   #define S390_STORE_STATUS_DEF_ADDR offsetof(LowCore, floating_pt_save_area)
> >>   int s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch);
> >>   int s390_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len);
> >> -#ifndef CONFIG_USER_ONLY
> >>   LowCore *cpu_map_lowcore(CPUS390XState *env);
> >>   void cpu_unmap_lowcore(LowCore *lowcore);
> >>   #endif /* CONFIG_USER_ONLY */  
> > 
> >   
>