[RFC,v2,7/9] s390x/pci: fence off instructions for non-pci

If a guest running on a machine without zpci issues a pci instruction,
throw them an exception.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 13 deletions(-)

在 2017/7/18 下午10:24, Cornelia Huck 写道:
> If a guest running on a machine without zpci issues a pci instruction,
> throw them an exception.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>   target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 880eccd58a..60688888c3 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1191,7 +1191,11 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
>   {
>       uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
>
> -    return clp_service_call(cpu, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return clp_service_call(cpu, r2);
> +    } else {
> +        return -1;
> +    }
Hi Conny,

Should we use a global value to store s390_has_feat(S390_FEAT_ZPCI)?
I'm not sure it's a good idea.

Yi Min
>   }
>
>   static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1199,7 +1203,11 @@ static int kvm_pcilg_service_call(S390CPU *cpu, struct kvm_run *run)
>       uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
>       uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
>
> -    return pcilg_service_call(cpu, r1, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return pcilg_service_call(cpu, r1, r2);
> +    } else {
> +        return -1;
> +    }
>   }
>
>   static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1207,7 +1215,11 @@ static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)
>       uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
>       uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
>
> -    return pcistg_service_call(cpu, r1, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return pcistg_service_call(cpu, r1, r2);
> +    } else {
> +        return -1;
> +    }
>   }
>
>   static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1216,10 +1228,14 @@ static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>       uint64_t fiba;
>       uint8_t ar;
>
> -    cpu_synchronize_state(CPU(cpu));
> -    fiba = get_base_disp_rxy(cpu, run, &ar);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        cpu_synchronize_state(CPU(cpu));
> +        fiba = get_base_disp_rxy(cpu, run, &ar);
>
> -    return stpcifc_service_call(cpu, r1, fiba, ar);
> +        return stpcifc_service_call(cpu, r1, fiba, ar);
> +    } else {
> +        return -1;
> +    }
>   }
>
>   static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1247,7 +1263,11 @@ static int kvm_rpcit_service_call(S390CPU *cpu, struct kvm_run *run)
>       uint8_t r1 = (run->s390_sieic.ipb & 0x00f00000) >> 20;
>       uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
>
> -    return rpcit_service_call(cpu, r1, r2);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        return rpcit_service_call(cpu, r1, r2);
> +    } else {
> +        return -1;
> +    }
>   }
>
>   static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1257,10 +1277,14 @@ static int kvm_pcistb_service_call(S390CPU *cpu, struct kvm_run *run)
>       uint64_t gaddr;
>       uint8_t ar;
>
> -    cpu_synchronize_state(CPU(cpu));
> -    gaddr = get_base_disp_rsy(cpu, run, &ar);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        cpu_synchronize_state(CPU(cpu));
> +        gaddr = get_base_disp_rsy(cpu, run, &ar);
>
> -    return pcistb_service_call(cpu, r1, r3, gaddr, ar);
> +        return pcistb_service_call(cpu, r1, r3, gaddr, ar);
> +    } else {
> +        return -1;
> +    }
>   }
>
>   static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1269,10 +1293,14 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>       uint64_t fiba;
>       uint8_t ar;
>
> -    cpu_synchronize_state(CPU(cpu));
> -    fiba = get_base_disp_rxy(cpu, run, &ar);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        cpu_synchronize_state(CPU(cpu));
> +        fiba = get_base_disp_rxy(cpu, run, &ar);
>
> -    return mpcifc_service_call(cpu, r1, fiba, ar);
> +        return mpcifc_service_call(cpu, r1, fiba, ar);
> +    } else {
> +        return -1;
> +    }
>   }
>
>   static int handle_b9(S390CPU *cpu, struct kvm_run *run, uint8_t ipa1)

[restored cc: list]

On Wed, 19 Jul 2017 11:21:30 +0800
Yi Min Zhao <zyimin@linux.vnet.ibm.com> wrote:

> 在 2017/7/18 下午10:24, Cornelia Huck 写道:
> > If a guest running on a machine without zpci issues a pci instruction,
> > throw them an exception.
> >
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >   target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 41 insertions(+), 13 deletions(-)
> >
> > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> > index 880eccd58a..60688888c3 100644
> > --- a/target/s390x/kvm.c
> > +++ b/target/s390x/kvm.c
> > @@ -1191,7 +1191,11 @@ static int kvm_clp_service_call(S390CPU *cpu, struct kvm_run *run)
> >   {
> >       uint8_t r2 = (run->s390_sieic.ipb & 0x000f0000) >> 16;
> >
> > -    return clp_service_call(cpu, r2);
> > +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> > +        return clp_service_call(cpu, r2);
> > +    } else {
> > +        return -1;
> > +    }  
> Hi Conny,
> 
> Should we use a global value to store s390_has_feat(S390_FEAT_ZPCI)?
> I'm not sure it's a good idea.
> 
> Yi Min

s390_has_feat() should be fast enough after the first time it has been
called, as the cpu is cached, so I don't think it's worth it.

On 18.07.2017 16:24, Cornelia Huck wrote:
> If a guest running on a machine without zpci issues a pci instruction,
> throw them an exception.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 13 deletions(-)
[...]
>  static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> @@ -1269,10 +1293,14 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
>      uint64_t fiba;
>      uint8_t ar;
>  
> -    cpu_synchronize_state(CPU(cpu));
> -    fiba = get_base_disp_rxy(cpu, run, &ar);
> +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> +        cpu_synchronize_state(CPU(cpu));
> +        fiba = get_base_disp_rxy(cpu, run, &ar);
>  
> -    return mpcifc_service_call(cpu, r1, fiba, ar);
> +        return mpcifc_service_call(cpu, r1, fiba, ar);
> +    } else {
> +        return -1;
> +    }
>  }

I think I'd rather write these cases with early exits à la

    if (!s390_has_feat(S390_FEAT_ZPCI)) {
        return -1;
    }

so you could get along with less change and less indentation ... but
since this is rather a matter of taste, anyway:

Reviewed-by: Thomas Huth <thuth@redhat.com>

On Wed, 19 Jul 2017 10:07:53 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 18.07.2017 16:24, Cornelia Huck wrote:
> > If a guest running on a machine without zpci issues a pci instruction,
> > throw them an exception.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >  target/s390x/kvm.c | 54 +++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 41 insertions(+), 13 deletions(-)  
> [...]
> >  static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> > @@ -1269,10 +1293,14 @@ static int kvm_mpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> >      uint64_t fiba;
> >      uint8_t ar;
> >  
> > -    cpu_synchronize_state(CPU(cpu));
> > -    fiba = get_base_disp_rxy(cpu, run, &ar);
> > +    if (s390_has_feat(S390_FEAT_ZPCI)) {
> > +        cpu_synchronize_state(CPU(cpu));
> > +        fiba = get_base_disp_rxy(cpu, run, &ar);
> >  
> > -    return mpcifc_service_call(cpu, r1, fiba, ar);
> > +        return mpcifc_service_call(cpu, r1, fiba, ar);
> > +    } else {
> > +        return -1;
> > +    }
> >  }  
> 
> I think I'd rather write these cases with early exits à la
> 
>     if (!s390_has_feat(S390_FEAT_ZPCI)) {
>         return -1;
>     }
> 
> so you could get along with less change and less indentation 

Yeah, that's just the same as for the other patch, only that the
difference is smaller here. I'll just keep it, unless there are strong
feelings.

> ... but
> since this is rather a matter of taste, anyway:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks!