Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bae58fd3-34b0-641a-a18b-010d48c792f0@oracle.com>
Date:   Fri, 14 Jul 2023 14:05:27 +0100
Subject: Re: [PATCH] KVM: SVM: Update destination when updating pi irte
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     Maxim Levitsky <mlevitsk@redhat.com>,
        "dengqiao.joey" <dengqiao.joey@bytedance.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, pbonzini@redhat.com,
        Alejandro Jimenez <alejandro.j.jimenez@oracle.com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
References: <3d05fcf1-dad3-826e-03e9-599ced7524b4@oracle.com>
 <20230518035806.938517-1-dengqiao.joey@bytedance.com>
 <2f6210acca81090146bc1decf61996aae2a0bfcf.camel@redhat.com>
 <36295675-2139-266d-4b07-9e029ac88fef@oracle.com>
 <ZJ4HJhQytonABUMl@google.com>
From:   Joao Martins <joao.m.martins@oracle.com>
In-Reply-To: <ZJ4HJhQytonABUMl@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OUJKMzl1S2ZXUlhBWXpWT0FtWnV6ZkNOb3BIUEVocDdpWGVCR2hpbmUzWXJG?=
 =?utf-8?B?Z2FwQjFYa0xmN2hvckg2c21ieGhuRmhudlRwZEZHb0lvU092RHR1RmZwZS82?=
 =?utf-8?B?dEdLejB0akRSMERxc2V1dXpNanZUakJKNjZXNElMMGpHNFdENnZORjRkSHQw?=
 =?utf-8?B?RkgvWGduczVUWGJNZVkxU1NCeWlNSXYzN2MvS2xYbkUzK2lvdU1Mckx4WG1y?=
 =?utf-8?B?VW94aUtUZlVWQm12ZzFUUVF3b2FkWjBLYitTOGNTRDhFb2d6c1FTMWdPUXFW?=
 =?utf-8?B?YXNHSDg5ZGpZOWZxczYzSzdjZWdFTW90MURZSWw4ZkRQREIwSmdnelZWVkth?=
 =?utf-8?B?RlNnaE9nYStJMmx1dUFyL2k4cXRENFV0emxmNEJNcm9sUW5HbWhEMmNwRVVT?=
 =?utf-8?B?NFl3SnpmQXVCZVAyQzFZTWI0Z3lING4xUis3YitZUVNSQnN0N0xWRTJ0b214?=
 =?utf-8?B?a0Y5UmFNdDZ1OEtWWm1oQ0VjSzFJSlR5c2VENmpwK051cHBqWmNqUjVJVzBk?=
 =?utf-8?B?RUVvd2VEVFRDTHEwZ3pPZWsvQmxaQ0JRUm9LS1RReURvVDFQK3JHTlF2MkZM?=
 =?utf-8?B?R1RDWUlMcUxZUlM4dy9NejJjK0oxWEJRRkdPVjhDLzEvamhCeDhZWDA4R3Mz?=
 =?utf-8?B?Y0lhb2hrdjJlOFlwWGU2MmZjUTJlU3Y5eDFVL2RZa3BIR0NTWjJvcmxXVFp0?=
 =?utf-8?B?aEhDa2JYcDdJeFB5R3YzWEEwTUkveUtjOTczNFpmWmlXSzlaVkcvcGYyRVR3?=
 =?utf-8?B?WXprK0kybjRQcmV6dzkxbGtTWHNlZDRsK0lIb3R0M284SGhzYm1PcnZZd3pS?=
 =?utf-8?B?SDdyVVE5VUtyUnRiL3YxSEJ3d2hya205aDlNUTNuV3AxYmhubm5wZGZwUlR3?=
 =?utf-8?B?RmdDYjlja2g2QUsxeW8xN0RIaEJrdTZzTWREQ3M4SVhWVnBjUUYwRHhwQW02?=
 =?utf-8?B?ZzBxaGNRRklZU3hZMTBNUXJKejN5OCszdFBFeDVWendnSGJkbGZMU2ZpUG9p?=
 =?utf-8?B?clY4RjJDT2pmc2lWK2tsRG84eFJHYWluMnFQOHNiWkNrSUNob0lOVzRFZ3JO?=
 =?utf-8?B?YmtNb0xlaDNwbnJYYU1IL2Jjby80WmpPaE1qbE9wa0FpZjFVdlg0bHFMTHFV?=
 =?utf-8?B?ZzN1OTNBek9YN3NjQVdNNXRSLzByQ3RtSTB5cEVPcnA0Ui96YXJuUGNBZzBp?=
 =?utf-8?B?NmxMVTJndzBRaDRxNjRqbmEyVFZMcjlYRTZhVjR0UWREcFJmTVlYMEhHNVRB?=
 =?utf-8?B?TWpVWVJKV09IbzVRaFVmcXNzaGYybHF0d3g0cytUU09GRVNSTEJ5OC8zcGdL?=
 =?utf-8?B?WDNkSmxyemxKMVprRk04VmZVenZUam4vbEpSY3BDSHhJOG4yZHRhT1dNYXhT?=
 =?utf-8?B?M090RDN4aXJBT1VlYnRGam9NWDFicUw0SENOUWF1dXg1MDZtZDNUSTVySWI4?=
 =?utf-8?B?dG1DT1F4azhJVGlucm1MY1B6cUhWS3ZxbStINUxsRWdrQzBYV21XbVBNbzk1?=
 =?utf-8?B?TzU0c0lRbG16dFl3VnpjYXQwR3lsUlJYd1ZzUVBnak9qVDZybUV5MnJleVps?=
 =?utf-8?B?NXFucm9KZzIxYTVDODJ4U0JPTHhuenFUeHlsNXlqWHRvbUg5VWwyOWJ1QkRs?=
 =?utf-8?B?WUwzVjRITGFRanhDMGxEa2x3a2FyWGFlM29iTE52TE1yN3RpeTQvbCtkdmxo?=
 =?utf-8?B?UmhIVXMvR3dEVXB6NGVnZWV5cENZQlJGZG5zbzMyYXRPdjdsRks5WGJkR21v?=
 =?utf-8?B?a1l0SmdDRHMrMWg1R1pXWUpPNklrbURCb2FQaldjdnlCN2hNdS95SDBhcmd0?=
 =?utf-8?B?cmd4VkVOQW9VSW5YeFRTcXVTWHp0NkpNaENiUFE2cFN4cWltRDdtOSs5U2VQ?=
 =?utf-8?B?QnFpMnBRbjFhVFpRWjVpMkx5M0hEVk9aeGJOdGExMmoyMFkzT0lpQWMyeUZD?=
 =?utf-8?B?TDRBQVFSK3V6UUFKWkF1SWFBMWR5aTZjUXROSjRrYU5Sdi9wcjQ0QS90Rnd3?=
 =?utf-8?B?MEFId05zRjFwWVZlc3ZpSnlSSWhUMXhERFUwZklpbmZnVDhJeGw5cERVM24y?=
 =?utf-8?B?YSt3V2tGMWliWGdvamhDY1VOQ2ZYQmJyU1FXQVpBc0d1ZmVma3pMU0RORHJz?=
 =?utf-8?B?VzI4c0VEeWx4RmN3U1ZpU21RY3UzbnpzYU8wZitrQ2lGM1JkMUlTeENvMXNh?=
 =?utf-8?B?R3c9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: Z/oDw1vi2BthZc3cOgIp8h1GPWSHVFD331SQS08i+w6WGOdn1BfxqQNOFVYnccFVGpazjiCzkgoGEyaAl+61bkQFCb36kzwpDP3ofUYjdwv+xXHc8RdugNmgw0Hoz1o9J4KVdyBkr1bDN3EpHWv5suyf9opHpsMCnhMXN+xvQDNTKmuy3dW0oExJCJrtrlIdg0BRZLK3/FUsJO/hYbgZoD6GPY4mf12OPOtjQT+KNU1iGqDQxjzVjFJ5DeiK/02kQbcZm+bEKjuKclunhi1k9GokLD4dK+HPQbHYNuhpEH/JNY/lZg6JZNiJmvWGUA9VDWwJQuTFAbg+DFDsZ0dHqviZVlQ+ZxVd59ZK/yLK73dxR0fBByS6rAZgg5AaIrukStX56cbJG4/NEdT88n15mvJKlGqpmMpJ9VAwNvaE6v4yQElxJOY2x3mT+US4o7ejGz2TYioGpnOxpOA6X4EbDrRdqQmbiWfVH1huXWqITHqO+UNvv4a7twzFihgKl3nJinRBXCNmmMSuj29KlRv8aqmL+0vZ8QYooP+/dBDE8BPA8G/aIbBWx88ppM0DMCHTurV8I9MGMt6sbsDlRgBi4ZfQ2VHGU58B5wlVPxGL0pnB5BW9BCCCCoFZDrYxfIf16Y8mrn9SEZqRacePfQe3B/8iK7tJzQJqHwhrkgTbSOe+d92+Q+waGn/KsVXbxcWEe4AYbh4x0hbOCyM8RBgBALX5QCEltplKpGu96RB2qqGwCFLk54dYvJfmpDXRmfENPJDWbryG7GuaftrNFWEkXaGB6GgtkGX2QzDX8gRsLgxyWPKMnT9ROeot6a9vVGH03eajr/gaU2uBVeeEp3rv+iKp3cXGHKyaCzpRHgj0cy9JI79QgFD5a0vIvU4J2cJh
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c5e0770-0bc7-4f6d-0f7e-08db846b02a0
X-MS-Exchange-CrossTenant-AuthSource: BLAPR10MB4835.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jul 2023 13:05:34.5529
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qkwqJdA8UFumnmtF/zvKA/4gQR0tJNhR2aolYGAWYTgNS/cRCTNSwCHn7S07bh+8XpUgFbGBZdNgEPX+i+5z9Ke/N+vaMBeHk3n+SNKoz3E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB6154
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-14_06,2023-07-13_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 bulkscore=0 suspectscore=0
 mlxscore=0 spamscore=0 malwarescore=0 phishscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2306200000
 definitions=main-2307140118
X-Proofpoint-GUID: JvmuxwE0jRlfNpeigKNq0xmJpJ6Xhc7k
X-Proofpoint-ORIG-GUID: JvmuxwE0jRlfNpeigKNq0xmJpJ6Xhc7k
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

+Suravee, +Alejandro

On 29/06/2023 23:35, Sean Christopherson wrote:
> On Thu, May 18, 2023, Joao Martins wrote:
>> On 18/05/2023 09:19, Maxim Levitsky wrote:
>>> I think that we do need to a flag indicating if the vCPU is currently
>>> running and if yes, then use svm->vcpu.cpu (or put -1 to it when it not
>>> running or something) (currently the vcpu->cpu remains set when vCPU is
>>> put)
>>>
>>> In other words if a vCPU is running, then avic_pi_update_irte should put
>>> correct pCPU number, and if it raced with vCPU put/load, then later should
>>> win and put the correct value.  This can be done either with a lock or
>>> barriers.
>>>
>> If this could be done, it could remove cost from other places and avoid this
>> little dance of the galog (and avoid its usage as it's not the greatest design
>> aspect of the IOMMU). We anyways already need to do IRT flushes in all these
>> things with regards to updating any piece of the IRTE, but we need some care
>> there two to avoid invalidating too much (which is just as expensive and per-VCPU).
> 
> ...
> 
>> But still quite expensive (as many IPIs as vCPUs updated), but it works as
>> intended and guest will immediately see the right vcpu affinity. But I honestly
>> prefer going towards your suggestion (via vcpu.pcpu) if we can have some
>> insurance that vcpu.cpu is safe to use in pi_update_irte if protected against
>> preemption/blocking of the VCPU.
> 
> I think we have all the necessary info, and even a handy dandy spinlock to ensure
> ordering.  Disclaimers: compile tested only, I know almost nothing about the IOMMU
> side of things, and I don't know if I understood the needs for the !IsRunning cases.
> 
I was avoiding grabbing that lock, but now that I think about it it shouldn't do
much harm.

My only concern has mostly been whether we mark the IRQ isRunning=1 on a vcpu
that is about to block as then the doorbell rang by the IOMMU won't do anything
to the guest. But IIUC the physical ID cache read-once should cover that

> Disclaimers aside, this should point the IOMMU at the right pCPU when the target
> vCPU changes and the new vCPU is actively running.
> 
Yeap, it should. I am gonna see if we can test this next week (even if not me,
but someone from my team)

> ---
>  arch/x86/kvm/svm/avic.c | 44 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index cfc8ab773025..703ad9af73eb 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -791,6 +791,7 @@ static int svm_ir_list_add(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi)
>  	int ret = 0;
>  	unsigned long flags;
>  	struct amd_svm_iommu_ir *ir;
> +	u64 entry;
>  
>  	/**
>  	 * In some cases, the existing irte is updated and re-set,
> @@ -824,6 +825,18 @@ static int svm_ir_list_add(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi)
>  	ir->data = pi->ir_data;
>  
>  	spin_lock_irqsave(&svm->ir_list_lock, flags);
> +
> +	/*
> +	 * Update the target pCPU for IOMMU doorbells if the vCPU is running.
> +	 * If the vCPU is NOT running, i.e. is blocking or scheduled out, KVM
> +	 * will update the pCPU info when the vCPU awkened and/or scheduled in.
> +	 * See also avic_vcpu_load().
> +	 */
> +	entry = READ_ONCE(*(svm->avic_physical_id_cache));
> +	if (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK)
> +		amd_iommu_update_ga(entry & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK,
> +				    true, pi->ir_data);
> +

Ah! Totally forgot about the ID cache from AVIC. And it's already paired with
barriers Maxim was alluding to. Much better than trying to get a safe read of
vcpu::cpu.

>  	list_add(&ir->node, &svm->ir_list);
>  	spin_unlock_irqrestore(&svm->ir_list_lock, flags);
>  out:
> @@ -986,10 +999,11 @@ static inline int
>  avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
>  {
>  	int ret = 0;
> -	unsigned long flags;
>  	struct amd_svm_iommu_ir *ir;
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> +	lockdep_assert_held(&svm->ir_list_lock);
> +
>  	if (!kvm_arch_has_assigned_device(vcpu->kvm))
>  		return 0;
>  
> @@ -997,19 +1011,15 @@ avic_update_iommu_vcpu_affinity(struct kvm_vcpu *vcpu, int cpu, bool r)
>  	 * Here, we go through the per-vcpu ir_list to update all existing
>  	 * interrupt remapping table entry targeting this vcpu.
>  	 */
> -	spin_lock_irqsave(&svm->ir_list_lock, flags);
> -
>  	if (list_empty(&svm->ir_list))
> -		goto out;
> +		return 0;
>  
>  	list_for_each_entry(ir, &svm->ir_list, node) {
>  		ret = amd_iommu_update_ga(cpu, r, ir->data);
>  		if (ret)
> -			break;
> +			return ret;
>  	}
> -out:
> -	spin_unlock_irqrestore(&svm->ir_list_lock, flags);
> -	return ret;
> +	return 0;
>  }
>  
>  void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> @@ -1017,6 +1027,7 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	u64 entry;
>  	int h_physical_id = kvm_cpu_get_apicid(cpu);
>  	struct vcpu_svm *svm = to_svm(vcpu);
> +	unsigned long flags;
>  
>  	lockdep_assert_preemption_disabled();
>  
> @@ -1033,6 +1044,15 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  	if (kvm_vcpu_is_blocking(vcpu))
>  		return;
>  
> +	/*
> +	 * Grab the per-vCPU interrupt remapping lock even if the VM doesn't
> +	 * _currently_ have assigned devices, as that can change.  Holding
> +	 * ir_list_lock ensures that either svm_ir_list_add() will consume
> +	 * up-to-date entry information, or that this task will wait until
> +	 * svm_ir_list_add() completes to set the new target pCPU.
> +	 */
> +	spin_lock_irqsave(&svm->ir_list_lock, flags);
> +
>  	entry = READ_ONCE(*(svm->avic_physical_id_cache));
>  	WARN_ON_ONCE(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK);
>  
> @@ -1042,12 +1062,15 @@ void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  
>  	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
>  	avic_update_iommu_vcpu_affinity(vcpu, h_physical_id, true);
> +
> +	spin_unlock_irqrestore(&svm->ir_list_lock, flags);
>  }
>  
>  void avic_vcpu_put(struct kvm_vcpu *vcpu)
>  {
>  	u64 entry;
>  	struct vcpu_svm *svm = to_svm(vcpu);
> +	unsigned long flags;
>  
>  	lockdep_assert_preemption_disabled();
>  
> @@ -1057,10 +1080,15 @@ void avic_vcpu_put(struct kvm_vcpu *vcpu)
>  	if (!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK))
>  		return;
>  
> +	spin_lock_irqsave(&svm->ir_list_lock, flags);
> +
>  	avic_update_iommu_vcpu_affinity(vcpu, -1, 0);
>  
>  	entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK;
>  	WRITE_ONCE(*(svm->avic_physical_id_cache), entry);
> +
> +	spin_unlock_irqrestore(&svm->ir_list_lock, flags);
> +
>  }

Other than that, looks to be good. To some extent we are doing an update_ga()
just like Dengqiao was doing, but with the right protection both from IR
perspective and safe read of the VCPU cpu.