From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43376) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ehHND-0004z1-GZ for qemu-devel@nongnu.org; Thu, 01 Feb 2018 11:06:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ehHNA-0004oj-Aq for qemu-devel@nongnu.org; Thu, 01 Feb 2018 11:06:03 -0500 Date: Thu, 1 Feb 2018 17:05:45 +0100 From: Cornelia Huck Message-ID: <20180201170545.52210e9b.cohuck@redhat.com> In-Reply-To: <240bb71f-7d10-bc54-1bb7-23df6bc51081@redhat.com> References: <20180131181742.2037-1-cohuck@redhat.com> <20180131181742.2037-2-cohuck@redhat.com> <20180201134803.78ef2b65.cohuck@redhat.com> <240bb71f-7d10-bc54-1bb7-23df6bc51081@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 1/2] s390x/tcg: wire up pci instructions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Hildenbrand Cc: qemu-s390x@nongnu.org, qemu-devel@nongnu.org, rth@twiddle.net, agraf@suse.de On Thu, 1 Feb 2018 13:59:13 +0100 David Hildenbrand wrote: > On 01.02.2018 13:48, Cornelia Huck wrote: > > On Thu, 1 Feb 2018 13:42:52 +0100 > > David Hildenbrand wrote: > > > >> On 31.01.2018 19:17, Cornelia Huck wrote: > > > >>> +#ifndef CONFIG_USER_ONLY > >>> +void HELPER(clp)(CPUS390XState *env, uint32_t r2) > >>> +{ > >>> + S390CPU *cpu = s390_env_get_cpu(env); > >>> + int r; > >>> + > >>> + qemu_mutex_lock_iothread(); > >>> + r = clp_service_call(cpu, r2, GETPC()); > >>> + qemu_mutex_unlock_iothread(); > >>> + if (r) { > >>> + s390_program_interrupt(env, PGM_OPERATION, 4, GETPC()); > >>> + } > >> > >> We don't need the if (r) ... so I suggest dropping all these. (as I > >> said, will be handled later via the generic flag checking in translation > >> code). We can ignore any error from these functions. > > > > I did not check the instruction implementations in detail... was the > > error really only for the !CONFIG_PCI case? > > !FEATURE_ZPCI (which includes !CONFIG_PCI) > > Yes, that's how I remember. > > > > > (I really should know that...) > > > >> A sane guest will newer trigger this. (if we have no CONFIG_PCI, the > >> also the ZPCI feature will not be available) > > > > Hopefully we can also handle non-sane guests correctly... > > Usually, if you call instructions that are not indicated via STFL, there > is no guarantee what will happen. Some time in the future, we will > handle this globally (but haven't done so to allow new applications to > run with old CPU models - which was necessary before we bumped the CPU > model to a z12). > > If you don't want to wait until that support is added, you should be > save with something like this: > > > diff --git a/target/s390x/translate.c b/target/s390x/translate.c > index 02cd4b2627..a5db014730 100644 > --- a/target/s390x/translate.c > +++ b/target/s390x/translate.c > @@ -5907,6 +5907,10 @@ static ExitStatus translate_one(CPUS390XState > *env, DisasContext *s) > gen_illegal_opcode(s); > return EXIT_NORETURN; > } > + if (insn->fac == FAC_PCI && !s390_has_feat(FAC_PCI)) { > + gen_illegal_opcode(s); > + return EXIT_NORETURN; > + } > > #ifndef CONFIG_USER_ONLY > if (s->tb->flags & FLAG_MASK_PER) { > I think I'll just keep it (the instructions won't do anything, as no pci devices can be added without FEATURE_ZPCI) and just add a comment.