linux-kernel - Re: [PATCH 15/32] KVM: s390: pci: enable host forwarding of Adapter Event Notifications

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Dec 2021 12:42:51 -0500
From:   Matthew Rosato <mjrosato@...ux.ibm.com>
To:     Christian Borntraeger <borntraeger@...ux.ibm.com>,
        linux-s390@...r.kernel.org
Cc:     alex.williamson@...hat.com, cohuck@...hat.com,
        schnelle@...ux.ibm.com, farman@...ux.ibm.com, pmorel@...ux.ibm.com,
        hca@...ux.ibm.com, gor@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, agordeev@...ux.ibm.com,
        frankja@...ux.ibm.com, david@...hat.com, imbrenda@...ux.ibm.com,
        vneethv@...ux.ibm.com, oberpar@...ux.ibm.com, freude@...ux.ibm.com,
        thuth@...hat.com, pasic@...ux.ibm.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 15/32] KVM: s390: pci: enable host forwarding of Adapter
 Event Notifications

On 12/17/21 11:56 AM, Christian Borntraeger wrote:
> Am 07.12.21 um 21:57 schrieb Matthew Rosato:
>> In cases where interrupts are not forwarded to the guest via firmware,
>> KVM is responsible for ensuring delivery.  When an interrupt presents
>> with the forwarding bit, we must process the forwarding tables until
>> all interrupts are delivered.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@...ux.ibm.com>
>> ---
> [...]
> 
>> +static void aen_host_forward(struct zpci_aift *aift, unsigned long si)
>> +{
>> +    struct kvm_s390_gisa_interrupt *gi;
>> +    struct zpci_gaite *gaite;
>> +    struct kvm *kvm;
>> +
>> +    gaite = (struct zpci_gaite *)aift->gait +
>> +        (si * sizeof(struct zpci_gaite));
>> +    if (gaite->count == 0)
>> +        return;
>> +    if (gaite->aisb != 0)
>> +        set_bit_inv(gaite->aisbo, (unsigned long *)gaite->aisb);
>> +
>> +    kvm = kvm_s390_pci_si_to_kvm(aift, si);
>> +    if (kvm == 0)
>> +        return;
>> +    gi = &kvm->arch.gisa_int;
>> +
>> +    if (!(gi->origin->g1.simm & AIS_MODE_MASK(gaite->gisc)) ||
>> +        !(gi->origin->g1.nimm & AIS_MODE_MASK(gaite->gisc))) {
>> +        gisa_set_ipm_gisc(gi->origin, gaite->gisc);
>> +        if (hrtimer_active(&gi->timer))
>> +            hrtimer_cancel(&gi->timer);
>> +        hrtimer_start(&gi->timer, 0, HRTIMER_MODE_REL);
>> +        kvm->stat.aen_forward++;
>> +    }
>> +}
>> +
>> +static void aen_process_gait(u8 isc)
>> +{
>> +    bool found = false, first = true;
>> +    union zpci_sic_iib iib = {{0}};
>> +    unsigned long si, flags;
>> +    struct zpci_aift *aift;
>> +
>> +    aift = kvm_s390_pci_get_aift();
>> +    spin_lock_irqsave(&aift->gait_lock, flags);
>> +
>> +    if (!aift->gait) {
>> +        spin_unlock_irqrestore(&aift->gait_lock, flags);
>> +        return;
>> +    }
>> +
>> +    for (si = 0;;) {
>> +        /* Scan adapter summary indicator bit vector */
>> +        si = airq_iv_scan(aift->sbv, si, airq_iv_end(aift->sbv));
>> +        if (si == -1UL) {
>> +            if (first || found) {
>> +                /* Reenable interrupts. */
>> +                if (zpci_set_irq_ctrl(SIC_IRQ_MODE_SINGLE, isc,
>> +                              &iib))
>> +                    break;
>> +                first = found = false;
>> +            } else {
>> +                /* Interrupts on and all bits processed */
>> +                break;
>> +            }
>> +            found = false;
>> +            si = 0;
>> +            continue;
>> +        }
>> +        found = true;
>> +        aen_host_forward(aift, si);
>> +    }
>> +
>> +    spin_unlock_irqrestore(&aift->gait_lock, flags);
>> +}
>> +
>>   static void gib_alert_irq_handler(struct airq_struct *airq,
>>                     struct tpi_info *tpi_info)
>>   {
>> +    struct tpi_adapter_info *info = (struct tpi_adapter_info *)tpi_info;
>> +
>>       inc_irq_stat(IRQIO_GAL);
>> -    process_gib_alert_list();
>> +
>> +    if (info->forward || info->error)
>> +        aen_process_gait(info->isc);
>> +    else
>> +        process_gib_alert_list();
>>   }
> 
> Not sure, would it make sense to actually do both after an alert 
> interrupt or do we always get a separate interrupt for event vs. irq?
> [..]

Good point - I thought this was an either/or scenario but I went back 
and doubled checked -- looks like it is indeed possible to get a single 
interrupt that indicates processing of both AEN events and the alert 
list is required.  (It is also possible to get interrupts that indicate 
processing of only one or the other is required).  So, my code above is 
wrong.

However, we also don't need to call process_gib_alert_list() 
unconditionally after handling AEN -- there is more information we can 
check in tpi_adapter_info to decide whether that is necessary (aism);  I 
will add this.