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

[Pierre Morel <pmorel@linux.ibm.com>]

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