[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 15/21] AMD/IOMMU: free all-empty page tables


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 18 May 2022 12:18:45 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=fI25F0S5T7JK5JzpBnf6DwdbMN1F1TCy/dlsT0E0OgE=; b=A2nxwu8EZYGE7wQcpu/SfdWRCWy80BaS7lxYBJnk17q3jiSaE2tr9vZrTNb7lO+Av9ikNLIU72bHuT0aVh6snuoaxHUJB2RN6WozJNxCpNJDzwJROGCZQy8OwrD9z310WdzBAVMa9c1Tqs+8yMXsTnR0ZHyALnZtw7lDsMupBVUaE26Se2MKqRofre3Ox2fZ5s5PjHieLPhHk58dcLxS6YJgjnnfodv/0KDNIhaH9uYtwyUTHmxkTmB6d7TtsNYZlJNLdEXzvFY+J7XZO2xog6mHE7ybipPlG+19fKQjVza0NmGyWeJ0BprjQxmy5oF05smZZGPvGVhYuOTCfpx1oQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Fl/ydblejImuA11KWBDqXw5HGS5Vrh4VGLC1RCUDnnlmq7ImhYLh7/nalQXPYnLu09xiTHF10bj+Ngc3BMf3Mo/0KdCdG8+1m6ShdoOqzpVPXayp50wqpQ6aUyKc5E1Ro6Luvu3Bbsrfp35NFeFOqKGjenVus0TXtnmoRGDblNyUsOkWO46qvN3+WpTfjQ8pi/+JhltJGSVb2lDsfSAdzqsGnHSIOHjf7SyNAiwFlpmnWABs4Zw7jkNfcdkKxTyGF32cRAljqM0wUqMpq7GBfyYnTYcUhkTbk0MO3kiWkRsk3n7uCfnfft1QQHmfconBBZZd1s86nozz2MycEOwfhA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Wed, 18 May 2022 10:18:56 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 10.05.2022 15:30, Roger Pau Monné wrote:
> On Mon, Apr 25, 2022 at 10:42:19AM +0200, Jan Beulich wrote:
>> When a page table ends up with no present entries left, it can be
>> replaced by a non-present entry at the next higher level. The page table
>> itself can then be scheduled for freeing.
>>
>> Note that while its output isn't used there yet,
>> pt_update_contig_markers() right away needs to be called in all places
>> where entries get updated, not just the one where entries get cleared.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> @@ -85,7 +92,11 @@ static union amd_iommu_pte set_iommu_pte
>>      if ( !old.pr || old.next_level ||
>>           old.mfn != next_mfn ||
>>           old.iw != iw || old.ir != ir )
>> +    {
>>          set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
>> +        pt_update_contig_markers(&table->raw, pfn_to_pde_idx(dfn, level),
>> +                                 level, PTE_kind_leaf);
> 
> It would be better to call pt_update_contig_markers inside of
> set_iommu_pde_present, but that would imply changing the parameters
> passed to the function.  It's cumbersome (and error prone) to have to
> pair calls to set_iommu_pde_present() with pt_update_contig_markers().

Right, but then already the sheer number of parameters would become
excessive (imo).

>> @@ -474,8 +491,24 @@ int cf_check amd_iommu_unmap_page(
>>  
>>      if ( pt_mfn )
>>      {
>> +        bool free;
>> +
>>          /* Mark PTE as 'page not present'. */
>> -        old = clear_iommu_pte_present(pt_mfn, dfn_x(dfn), level);
>> +        old = clear_iommu_pte_present(pt_mfn, dfn_x(dfn), level, &free);
>> +
>> +        while ( unlikely(free) && ++level < hd->arch.amd.paging_mode )
>> +        {
>> +            struct page_info *pg = mfn_to_page(_mfn(pt_mfn));
>> +
>> +            if ( iommu_pde_from_dfn(d, dfn_x(dfn), level, &pt_mfn,
>> +                                    flush_flags, false) )
>> +                BUG();
>> +            BUG_ON(!pt_mfn);
>> +
>> +            clear_iommu_pte_present(pt_mfn, dfn_x(dfn), level, &free);
> 
> Not sure it's worth initializing free to false (at definition and
> before each call to clear_iommu_pte_present), just in case we manage
> to return early from clear_iommu_pte_present without having updated
> 'free'.

There's no such path now, so I'd view it as dead code to do so. If
necessary a patch introducing such an early exit would need to deal
with this. But even then I'd rather see this being dealt with right
in clear_iommu_pte_present().

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.