From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37945) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VR2He-0002M6-Kf for qemu-devel@nongnu.org; Tue, 01 Oct 2013 11:54:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VR2HY-0003ks-ES for qemu-devel@nongnu.org; Tue, 01 Oct 2013 11:54:46 -0400 Received: from cantor2.suse.de ([195.135.220.15]:34870 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VR2HY-0003kl-4l for qemu-devel@nongnu.org; Tue, 01 Oct 2013 11:54:40 -0400 Message-ID: <524AF03D.5090308@suse.de> Date: Tue, 01 Oct 2013 17:54:37 +0200 From: Alexander Graf MIME-Version: 1.0 References: <1379945085-29086-1-git-send-email-rth@twiddle.net> <1379945085-29086-2-git-send-email-rth@twiddle.net> <5249BD03.60608@suse.de> <5249CDDD.3080100@twiddle.net> <524AEED6.4050908@suse.de> <524AEFAB.8060701@twiddle.net> In-Reply-To: <524AEFAB.8060701@twiddle.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/10] target-s390: Move facilities bits to env List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: qemu-devel@nongnu.org, =?ISO-8859-1?Q?Andreas_F=E4rber?= On 10/01/2013 05:52 PM, Richard Henderson wrote: > On 10/01/2013 08:48 AM, Alexander Graf wrote: >> On 09/30/2013 09:15 PM, Richard Henderson wrote: >>> On 09/30/2013 11:03 AM, Alexander Graf wrote: >>>> On 09/23/2013 04:04 PM, Richard Henderson wrote: >>>>> Rather than simply hard-coding them in STFL instruction. >>>>> >>>>> Signed-off-by: Richard Henderson >>>>> --- >>>>> target-s390x/cpu.c | 3 +++ >>>>> target-s390x/cpu.h | 1 + >>>>> target-s390x/translate.c | 10 +++++----- >>>>> 3 files changed, 9 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c >>>>> index 3c89f8a..ff691df 100644 >>>>> --- a/target-s390x/cpu.c >>>>> +++ b/target-s390x/cpu.c >>>>> @@ -181,6 +181,9 @@ static void s390_cpu_initfn(Object *obj) >>>>> env->cpu_num = cpu_num++; >>>>> env->ext_index = -1; >>>>> >>>>> + env->facilities[0] = 0xc000000000000000ull; >>>>> + env->facilities[1] = 0; >>>> Could we add CPU definitions along the way here? I'm fine with making z9 the >>>> default CPU type, but we should make this explicit :). >>> Certainly that's what we should do. I just hadn't yet researched the >>> currently correct way to do that. I know there's some amount of out-of-date >>> examples in the current source base. >> I'll leave the answer to that to Andreas :). > Can we leave that for a separate patch series then? Just make sure you actually check for feature bits on every instruction (which I think you do, but the current code is way too magical to me to really understand it anymore) so that we can always implement a z900 cpu type later on. > >>>>> - TCGv_i64 f, a; >>>>> - /* We really ought to have more complete indication of facilities >>>>> - that we implement. Address this when STFLE is implemented. */ >>>>> + TCGv_i64 f = tcg_temp_new_i64(); >>>>> + TCGv_i64 a = tcg_const_i64(200); >>>>> + >>>>> check_privileged(s); >>>>> - f = tcg_const_i64(0xc0000000); >>>>> - a = tcg_const_i64(200); >>>>> + tcg_gen_ld_i64(f, cpu_env, offsetof(CPUS390XState, facilities[0])); >>>>> + tcg_gen_shri_i64(f, f, 32); >>>> IMHO the facility list should be stored in DisasContext. That way we can check >>>> whether we're generating code against the correct target. >>> See patch 4. >>> >>> As for the code we generate here, does it really matter if we load the value >>> from env, or have it encoded as a constant? It still has to get stored to >>> memory, so it's not like the TCG optimizer is going to do anything with the >>> constant. >> No, it only seemed more straight forward to me from a "single source of >> information" point of view. But it really doesn't matter. Shifting in C seems >> to be easier to read :). > Fair enough. I'll rearrange the order of the patches so that we can > update STFL to use the DisasContext data. Thanks :) Alex