Re: [Qemu-devel] [PATCH v2 1/2] s390x/tcg: wire up pci instructions

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 1 Feb 2018 13:59:13 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 01.02.2018 13:48, Cornelia Huck wrote:
> > On Thu, 1 Feb 2018 13:42:52 +0100
> > David Hildenbrand <david@redhat.com> 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.