Re: [Qemu-devel] [PATCH 2/2] s390/pci: implement stpcifc instruction

[Frank Blaschka <blaschka@linux.vnet.ibm.com>]

On Fri, Dec 05, 2014 at 02:15:07PM +0100, Thomas Huth wrote:
> 
>  Hi Frank,
> 
> On Fri,  5 Dec 2014 10:19:59 +0100
> Frank Blaschka <blaschka@linux.vnet.ibm.com> wrote:
> 
> > From: Frank Blaschka <frank.blaschka@de.ibm.com>
> > 
> > This patch implements the last remaining s390 pci instruction
> > to query the function information block.
> > 
> > Signed-off-by: Frank Blaschka <frank.blaschka@de.ibm.com>
> > ---
> >  hw/s390x/s390-pci-bus.h  |  1 +
> >  hw/s390x/s390-pci-inst.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/s390x/s390-pci-inst.h |  1 +
> >  target-s390x/kvm.c       |  9 +++++--
> >  4 files changed, 73 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
> > index 2a9f735..35f4da5 100644
> > --- a/hw/s390x/s390-pci-bus.h
> > +++ b/hw/s390x/s390-pci-bus.h
> > @@ -223,6 +223,7 @@ typedef struct S390PCIBusDevice {
> >      uint64_t g_iota;
> >      uint64_t pba;
> >      uint64_t pal;
> > +    uint64_t fmb_addr;
> >      uint8_t isc;
> >      uint16_t noi;
> >      uint8_t sum;
> > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> > index 8648594..f503665 100644
> > --- a/hw/s390x/s390-pci-inst.c
> > +++ b/hw/s390x/s390-pci-inst.c
> > @@ -766,6 +766,7 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba)
> >          pbdev->lgstg_blocked = false;
> >          break;
> >      case ZPCI_MOD_FC_SET_MEASURE:
> > +        pbdev->fmb_addr = ldq_p(&fib.fmb_addr);
> >          break;
> >      default:
> >          program_interrupt(&cpu->env, PGM_OPERAND, 6);
> > @@ -775,3 +776,66 @@ int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba)
> >      setcc(cpu, cc);
> >      return 0;
> >  }
> > +
> > +int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba)
> > +{
> > +    CPUS390XState *env = &cpu->env;
> > +    uint32_t fh;
> > +    ZpciFib fib;
> > +    S390PCIBusDevice *pbdev;
> > +    uint32_t data;
> > +    uint64_t cc = ZPCI_PCI_LS_OK;
> > +
> > +    cpu_synchronize_state(CPU(cpu));
> 
> You're calling cpu_synchronize_state twice, one time in
> kvm_stpcifc_service_call() already and one time here. So I think
> you could remove the call here.
>

Hi Thomas,

looks like this is not the only duplicate cpu_synchronize_state.
Will create an add on patch to remove all unnecessary calls to
cpu_synchronize_state. Thx for the finding.

Frank

> > +    if (env->psw.mask & PSW_MASK_PSTATE) {
> > +        program_interrupt(env, PGM_PRIVILEGED, 6);
> > +        return 0;
> > +    }
> > +
> > +    fh = env->regs[r1] >> 32;
> > +
> > +    if (fiba & 0x7) {
> > +        program_interrupt(env, PGM_SPECIFICATION, 6);
> > +        return 0;
> > +    }
> > +
> > +    pbdev = s390_pci_find_dev_by_fh(fh);
> > +    if (!pbdev) {
> > +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> > +        return 0;
> > +    }
> > +
> > +    memset(&fib, 0, sizeof(fib));
> > +    stq_p(&fib.pba, pbdev->pba);
> > +    stq_p(&fib.pal, pbdev->pal);
> > +    stq_p(&fib.iota, pbdev->g_iota);
> > +    stq_p(&fib.aibv, pbdev->routes.adapter.ind_addr);
> > +    stq_p(&fib.aisb, pbdev->routes.adapter.summary_addr);
> > +    stq_p(&fib.fmb_addr, pbdev->fmb_addr);
> > +
> > +    data = (pbdev->isc << 28) | (pbdev->noi << 16) |
> > +           (pbdev->routes.adapter.ind_offset << 8) | (pbdev->sum << 7) |
> > +           pbdev->routes.adapter.summary_offset;
> > +    stw_p(&fib.data, data);
> > +
> > +    if (pbdev->fh >> ENABLE_BIT_OFFSET) {
> > +        fib.fc |= 0x80;
> > +    }
> > +
> > +    if (pbdev->error_state) {
> > +        fib.fc |= 0x40;
> > +    }
> > +
> > +    if (pbdev->lgstg_blocked) {
> > +        fib.fc |= 0x20;
> > +    }
> > +
> > +    if (pbdev->g_iota) {
> > +        fib.fc |= 0x10;
> > +    }
> > +
> > +    cpu_physical_memory_write(fiba, (uint8_t *)&fib, sizeof(fib));
> > +    setcc(cpu, cc);
> > +    return 0;
> > +}
> > diff --git a/hw/s390x/s390-pci-inst.h b/hw/s390x/s390-pci-inst.h
> > index 609e3e0..1c2f458 100644
> > --- a/hw/s390x/s390-pci-inst.h
> > +++ b/hw/s390x/s390-pci-inst.h
> > @@ -283,5 +283,6 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2);
> >  int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2);
> >  int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr);
> >  int mpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba);
> > +int stpcifc_service_call(S390CPU *cpu, uint8_t r1, uint64_t fiba);
> > 
> >  #endif
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 32af46b..b70d482 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -876,8 +876,13 @@ static int kvm_pcistg_service_call(S390CPU *cpu, struct kvm_run *run)
> > 
> >  static int kvm_stpcifc_service_call(S390CPU *cpu, struct kvm_run *run)
> >  {
> > -    qemu_log_mask(LOG_UNIMP, "STPCIFC missing\n");
> > -    return 0;
> > +    uint8_t r1 = (run->s390_sieic.ipa & 0x00f0) >> 4;
> > +    uint64_t fiba;
> > +
> > +    cpu_synchronize_state(CPU(cpu));
> > +    fiba = get_base_disp_rxy(cpu, run);
> > +
> > +    return stpcifc_service_call(cpu, r1, fiba);
> >  }
> > 
> >  static int kvm_sic_service_call(S390CPU *cpu, struct kvm_run *run)
>