Re: [qemu-s390x] [PATCH v2 6/6] s390x/vfio: ap: Implementing AP Queue Interrupt Control

[Pierre Morel]

On 29/11/2018 22:53, Tony Krowiak wrote:
> On 11/22/18 11:35 AM, Pierre Morel wrote:
> > We intercept the PQAP(AQIC) instruction and transform

...

> > +static int ap_pqap_aqic(CPUS390XState *env)
> > +{
> > +    uint64_t g_nib = env->regs[2];
> > +    uint8_t apid = ap_reg_get_apid(env->regs[0]);
> > +    uint8_t apqi = ap_reg_get_apqi(env->regs[0]);
> > +    uint32_t retval;
> > +    APDevice *ap = s390_get_ap();
>
> To be consistent with the naming convention in the rest of
> this file, can you name this variable 'apdev'?

OK

> > +    VFIODevice *vdev;
> > +    VFIOAPDevice *ap_vdev;
>
> To be consistent with the naming convention in the rest of
> this file, can you name this variable 'vapdev'?
>
> > +    APQueue *apq;
> > +
> > +    ap_vdev = DO_UPCAST(VFIOAPDevice, apdev, ap);
>
> Where is 'apdev' defined/initialized?

It is part of the VFIOAPDevice structure

> > +    vdev = &ap_vdev->vdev;
> > +    apq = &ap->card[apid].queue[apqi];
> > +    apq->isc = ap_reg_get_isc(env->regs[1]);
> > +    apq->apqn = (apid << 8) | apqi;
> > +
> > +    if (ap_reg_get_ir(env->regs[1])) {
> > +        retval = ap_pqap_set_irq(vdev, apq, g_nib);
> > +    } else {
> > +        retval = ap_pqap_clear_irq(vdev, apq);
> > +    }
> > +
> > +    env->regs[1] = retval;
> > +    if (retval & AP_STATUS_RC_MASK) {
> > +        return 3;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * ap_pqap
> > + * @env: environment pointing to registers
> > + * return value: Code Condition
> > + */
> > +int ap_pqap(CPUS390XState *env)
> > +{
> > +    int cc = 0;
> > +
> > +    switch (ap_reg_get_fc(env->regs[0])) {
> > +    case AQIC:
> > +        if (!s390_has_feat(S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL)) {
> > +            return -PGM_OPERATION;
>
> Shouldn't this be PGM_SPECIFICATION?

Before the patch, when PQAP is not intercepted we sent PGM_OPERATION.
I think we should continue doing the same as before if the feature is 
not activated.


> > +        }

...snip...


> > +typedef struct APQueue {
> > +    AdapterRoutes routes;
> > +    IndAddr *nib;
> > +    uint16_t apqn;
> > +    uint8_t isc;
> > +} APQueue;
> > +
> > +typedef struct APCard {
> > +    APQueue queue[MAX_AP_DOMAIN];
>
> This seems to be an unnecessary allocation of memory, particularly
> if there is only a few queues. I understand this
> maps to the concept of a matrix and makes for quick retrieval of
> an APQueue, but I think it would be more efficient to use something
> like a GTree, or a GHashTable both of which would save you space and
> allow for fairly quick retrieval.


OK.
We can optimize this later.

...

> > +/* Register 1 as input hold the AQIC information */
> > +static inline uint32_t ap_reg_set_status(uint8_t status)
>
> This function does not set anything, but returns an APQSW.
> Maybe it should be named ap_reg_get_status_word or ap_reg_get_apqsw
> or ap_reg_get_status.

All these operations are "get" when retrieving informations from 
registers and "set" when setting information inside registers.


>
...snip...
> >   }
> > +static int kvm_ap_pqap(S390CPU *cpu, uint16_t ipbh0)
> > +{
> > +    CPUS390XState *env = &cpu->env;
> > +    int r;
> > +
> > +    if (env->psw.mask & PSW_MASK_PSTATE) {
> > +        kvm_s390_program_interrupt(cpu, PGM_PRIVILEGED);
> > +        return 0;
> > +    }
> > +
> > +    if (env->regs[0] & AP_AQIC_ZERO_BITS) {
> > +        kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> > +        return 0;
> > +    }
>
> This check does not belong here; for example, if the PQAP(TAPQ)
> instruction is intercepted and the T bit (bit 40) is set, a
> specification exception would be erroneously recognized. This check
> should be done in the ap_pqap_aqic() function.

Right, I will move the test.


Regards,
Pierre

--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany