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
