Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp69984pxu; Tue, 5 Jan 2021 05:31:48 -0800 (PST) X-Google-Smtp-Source: ABdhPJxDWkAyIGV4YLOKkfs69EMMz/WDSvAEvW7hocDv+zctfQkjSEZVTiGiKI28QEyGDRCsIaTk X-Received: by 2002:aa7:c353:: with SMTP id j19mr75947560edr.204.1609853508389; Tue, 05 Jan 2021 05:31:48 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1609853508; cv=none; d=google.com; s=arc-20160816; b=mIH66T942UnefuwGkAsmfGmduXATRrf0J+72hvpoepQOVMYewMfyGPGaeoUFBuTeQn I3Elz0qjW5sSYPMH8a4vWzFX13sc+q8OPbEVQkMT0KRFgK4NN3CxK5zMCK3WeH16e7R0 vQ3cbBLRcf2UEUyO8ra1Ui1DuWdQHHSHfG3YY9Fc9wYEe62fdXvxhoD18y75mQhUdiXY /+eT73VaAwuCMEYP0rZd2y/mzBGgyjfVlGg5Sg3FvOixYZMjSV+2Vk6p98aHf6YVg2Ac gR/bFf+x3cydq/ZZv5Yqakt+RQsPdxXfm9BftuHRhaY3ki58/tu4yvDC8Q6s2LLqBzAS ngNg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject; bh=qTkxSIngA6nsGJrq88xJzrivG2PlJcgoo8xkon2DOhU=; b=page85WkRJApxkc7YbbYpQ5HAaGXYVJBPd218bLzi1Sv/BoYBkBVTigspP0KQ5tZXn cPtDJw7vJYx2nVzl/Xgs0pW446+17+R+nnJAmMi9Z6vwLXO6vYb5FwHxVBlmD5WCqmZO LV99hMN/PVXeBFpA4myVIu01X9fVU0mydQ+/naiHWnS3t0POejT/I3hG0lvKBEo7HY3O wXFxaMCSFwjz5EDT6eK5SDHnIogjs16Ce0BPJkY7WMe+2hIKw5j3h9PTRDZYYkfyXaGj roW+6F6nawte+BzaN23m9AjfVBI3yI9dnpII0m1xj8LRcLA7EBmjxid8mSMsr8MbOj5w tNfQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id z5si32201127ejj.425.2021.01.05.05.31.24; Tue, 05 Jan 2021 05:31:48 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730500AbhAENDk (ORCPT + 99 others); Tue, 5 Jan 2021 08:03:40 -0500 Received: from szxga07-in.huawei.com ([45.249.212.35]:10389 "EHLO szxga07-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729689AbhAENDi (ORCPT ); Tue, 5 Jan 2021 08:03:38 -0500 Received: from DGGEMS403-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4D9CLp3N5yz7QY8; Tue, 5 Jan 2021 21:01:54 +0800 (CST) Received: from [10.174.184.196] (10.174.184.196) by DGGEMS403-HUB.china.huawei.com (10.3.19.203) with Microsoft SMTP Server id 14.3.498.0; Tue, 5 Jan 2021 21:02:38 +0800 Subject: Re: [RFC PATCH v2 2/4] KVM: arm64: GICv4.1: Try to save hw pending state in save_pending_tables To: Marc Zyngier CC: Eric Auger , Will Deacon , , , , , Alex Williamson , Cornelia Huck , "Lorenzo Pieralisi" , , References: <20210104081613.100-1-lushenming@huawei.com> <20210104081613.100-3-lushenming@huawei.com> From: Shenming Lu Message-ID: <0fa19ab1-60ba-9067-e1aa-ee78191c52ed@huawei.com> Date: Tue, 5 Jan 2021 21:02:26 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.184.196] X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021/1/5 17:13, Marc Zyngier wrote: > On 2021-01-04 08:16, Shenming Lu wrote: >> After pausing all vCPUs and devices capable of interrupting, in order >> to save the information of all interrupts, besides flushing the pending >> states in kvm’s vgic, we also try to flush the states of VLPIs in the >> virtual pending tables into guest RAM, but we need to have GICv4.1 and >> safely unmap the vPEs first. >> >> Signed-off-by: Shenming Lu >> --- >>  arch/arm64/kvm/vgic/vgic-v3.c | 58 +++++++++++++++++++++++++++++++---- >>  1 file changed, 52 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c >> index 9cdf39a94a63..a58c94127cb0 100644 >> --- a/arch/arm64/kvm/vgic/vgic-v3.c >> +++ b/arch/arm64/kvm/vgic/vgic-v3.c >> @@ -1,6 +1,8 @@ >>  // SPDX-License-Identifier: GPL-2.0-only >> >>  #include >> +#include >> +#include >>  #include >>  #include >>  #include >> @@ -356,6 +358,38 @@ int vgic_v3_lpi_sync_pending_status(struct kvm >> *kvm, struct vgic_irq *irq) >>      return 0; >>  } >> >> +/* >> + * The deactivation of the doorbell interrupt will trigger the >> + * unmapping of the associated vPE. >> + */ >> +static void unmap_all_vpes(struct vgic_dist *dist) >> +{ >> +    struct irq_desc *desc; >> +    int i; >> + >> +    if (!kvm_vgic_global_state.has_gicv4_1) >> +        return; >> + >> +    for (i = 0; i < dist->its_vm.nr_vpes; i++) { >> +        desc = irq_to_desc(dist->its_vm.vpes[i]->irq); >> +        irq_domain_deactivate_irq(irq_desc_get_irq_data(desc)); >> +    } >> +} >> + >> +static void map_all_vpes(struct vgic_dist *dist) >> +{ >> +    struct irq_desc *desc; >> +    int i; >> + >> +    if (!kvm_vgic_global_state.has_gicv4_1) >> +        return; >> + >> +    for (i = 0; i < dist->its_vm.nr_vpes; i++) { >> +        desc = irq_to_desc(dist->its_vm.vpes[i]->irq); >> +        irq_domain_activate_irq(irq_desc_get_irq_data(desc), false); >> +    } >> +} >> + >>  /** >>   * vgic_v3_save_pending_tables - Save the pending tables into guest RAM >>   * kvm lock and all vcpu lock must be held >> @@ -365,14 +399,18 @@ int vgic_v3_save_pending_tables(struct kvm *kvm) >>      struct vgic_dist *dist = &kvm->arch.vgic; >>      struct vgic_irq *irq; >>      gpa_t last_ptr = ~(gpa_t)0; >> -    int ret; >> +    int ret = 0; >>      u8 val; >> >> +    /* As a preparation for getting any VLPI states. */ >> +    unmap_all_vpes(dist); > > What if the VPEs are not mapped yet? Is it possible to snapshot a VM > that has not run at all? What I see in QEMU is that the saving of the pending tables would only be called when stopping the VM and it needs the current VM state to be RUNNING. > >> + >>      list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) { >>          int byte_offset, bit_nr; >>          struct kvm_vcpu *vcpu; >>          gpa_t pendbase, ptr; >>          bool stored; >> +        bool is_pending = irq->pending_latch; >> >>          vcpu = irq->target_vcpu; >>          if (!vcpu) >> @@ -387,24 +425,32 @@ int vgic_v3_save_pending_tables(struct kvm *kvm) >>          if (ptr != last_ptr) { >>              ret = kvm_read_guest_lock(kvm, ptr, &val, 1); >>              if (ret) >> -                return ret; >> +                goto out; >>              last_ptr = ptr; >>          } >> >>          stored = val & (1U << bit_nr); >> -        if (stored == irq->pending_latch) >> + >> +        if (irq->hw) >> +            vgic_v4_get_vlpi_state(irq, &is_pending); > > You don't check the return value here, so I wonder why the checks > in vgic_v4_get_vlpi_state(). Since I have already checked the condition and reported in save_its_tables (patch 4), I just check in get_vlpi_state and don't report again here. > > Another thing that worries me is that vgic_v4_get_vlpi_state() doesn't > have any cache invalidation, and can end-up hitting in the CPU cache > (there is no guarantee of coherency between the GIC and the CPU, only > that the GIC will have flushed its caches). > > I'd expect this to happen at unmap time, though, in order to avoid > repeated single byte invalidations. Ok, I will add a cache invalidation at unmap time. > >> + >> +        if (stored == is_pending) >>              continue; >> >> -        if (irq->pending_latch) >> +        if (is_pending) >>              val |= 1 << bit_nr; >>          else >>              val &= ~(1 << bit_nr); >> >>          ret = kvm_write_guest_lock(kvm, ptr, &val, 1); >>          if (ret) >> -            return ret; >> +            goto out; >>      } >> -    return 0; >> + >> +out: >> +    map_all_vpes(dist); >> + >> +    return ret; >>  } >> >>  /** > > Thanks, > >         M.