On 05/11/2018 14:56, Michael Mueller wrote:
On 31.10.18 13:14, Pierre Morel wrote:
On 25/10/2018 14:37, Michael Mueller wrote:
This function processes a gib alert list. It is required to
run when either a gib alert interruption has been received or
...
+ */
+static void __maybe_unused process_gib_alert_list(
+ struct kvm_s390_gisa *gisa,
+ struct kvm_s390_gisa *gisa_to_nullify,
+ struct kvm_s390_gisa *gisa_to_drop)
+{
+ struct kvm_s390_gisa *next_alert;
+ struct kvm *kvm;
+
Isn't this function a little bit complex?
I think we can make it clearer.
- The first argument is always AFAIU unlink_gib_alert_list(), then
why not just call this function inside of process_gib_alert_list() ?
That could be done, but my intention was to make that explicit because I
think
that way it is less complex...
I use a clear functional decomposition:
1. get the alert list: unlink_gib_alert_list() -> alert_list
2. process the list: process_gib_alert_list(alert_list, ...)
- The action to do is related to the KVM/GISA, may be some state
inside KVM or GISA to differentiate the processing?
Can you elaborate your thoughts here? I don't understand the question.
I mean: in the same function you perform three different actions which
depends on VM state.
But I think I should stop arguing, I do not like this function, but I
have no better solution and it does the work.
so...
If the reset is handled correctly, it is OK.
All the GISA in this list need to be handled anyway if we are coming
from an IRQ, a CLEAR_IRQ or a RESET but...
Right.
+ for (; gisa; gisa = next_alert) {
+ next_alert = (struct kvm_s390_gisa *)(u64)gisa->next_alert;
+ /* unlink from alert list */
+ gisa->next_alert = (u32)(u64)gisa;
+ /* skip if to clear or drop */
+ if (gisa == gisa_to_nullify ||
+ gisa == gisa_to_drop)
+ continue;
... here:
I am not sure what happens if two guest are being reset at a time,
it seems to me that one is reset and one get kicked.
I will answer that separately... >
+ /* wake-up a vcpu of the kvm this gisa belongs to */
+ kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
+ __floating_irq_kick(kvm, KVM_S390_INT_IO(1, 0, 0, 0));
Do we really need to insert a floating interrupt?
Wouldn't a simple kvm_s390_vcpu_wakeup() after choosing the vcpu be
enough?
We need to find a suitable vcpu before calling kvm_s390_vcpu_wakeup().
That is exactly what __floating_irq_kick() does for us, right?
Yes right, it is just an optimization, we have two unnecessary tests,
one for interrupt type and another for GISA availability.
It is *not* inserting a floating interruption.
+ }
+
+ if (gisa_to_nullify)
+ nullify_gisa(gisa_to_nullify);
we already reset the next_alert pointer
if we want to clear the interrupt we just need to clear IPM
Compare with p. 17-172, all remaining fields should be set to zero,
except the next_alert pointer. A can do some calculations to clear
the gisa up to its end without touching the first word. But the
nullify routine is currently also used in the gisa init case that
needs to set the next_alert address initially...
I was trying to optimize the IRQ path but it was too picky.
Only the test is in the PATH and all is taken on the host cycles so...
OK for me.
nullifying a GISA will not be used very often in a VM life anyway.
+}
+
void kvm_s390_gisa_clear(struct kvm *kvm)
{
if (kvm->arch.gisa) {
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
