Re: [PATCH v25 14/20] i386: separate fpu_helper into user and sysemu parts

From: Richard Henderson <richard.henderson@linaro.org>
To: "Claudio Fontana" <cfontana@suse.de>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Peter Maydell" <peter.maydell@linaro.org>,
	"Alex Bennée" <alex.bennee@linaro.org>
Cc: Laurent Vivier <lvivier@redhat.com>,
	Thomas Huth <thuth@redhat.com>,
	Roman Bolshakov <r.bolshakov@yadro.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v25 14/20] i386: separate fpu_helper into user and sysemu parts
Date: Fri, 26 Feb 2021 10:42:51 -0800	[thread overview]
Message-ID: <d08e847d-8332-5526-7b92-04ed7379f2fd@linaro.org> (raw)
In-Reply-To: <20210226175143.22388-15-cfontana@suse.de>

On 2/26/21 9:51 AM, Claudio Fontana wrote:
> +/* fpu_helper.c */
> +
> +void do_fsave(CPUX86State *env, target_ulong ptr, int data32, uintptr_t ra);
> +void do_frstor(CPUX86State *env, target_ulong ptr, int data32, uintptr_t ra);
> +void do_fxsave(CPUX86State *env, target_ulong ptr, uintptr_t ra);
> +void do_fxrstor(CPUX86State *env, target_ulong ptr, uintptr_t ra);

Is this really worth it?  It means that we unnecessarily expose these functions
in system mode, where they are still not safe.

I had thought about doing something like

#ifdef CONFIG_USER_ONLY
#define SYSEMU_STATIC
#else
#define SYSEMU_STATIC static
#endif

SYSEMU_STATIC void
cpu_x86_fsave(...)
{
  ...
}

void helper_fsave(...)
{
    cpu_x86_fsave(..., GETPC());
}

but I thought the existing example of "do_" functions within that file to be
compelling.  I think small sections of this CONFIG within a file should be
fine, especially where it interacts with other functions like this.

I guess either way,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~