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

Re: [PATCH v4 13/21] IOMMU/x86: prefill newly allocate page tables


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 23 May 2022 08:49:27 +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=v9YqcDAGrCEL2/2iezwsvYWtnO/JBEtf1x37ZuwsURw=; b=bYyLBm61+SpKv56VGhZm1p6S94F2TeewLAYEmMZD1pQOlDPydYtZo5bw/iCsJT6MyYKdFuNny9FcX5HY35A1qQhzlgXaAnYphu2MPya3pEbrSX0rbu3yOCRkvAMvAyfFkehAyHr2XnI0UbToyrL8CHY6JLSUP+e2FRuGK2XrxZ8TFYQ+Ddjo3v2gt9+99euO7wPp4vEhPFvIyS1SoJp+NW3LyUsYKib8IqpULuel8VviTGc6SAA/JLaLis8jOikrz2iIVXuXy19l8OzLeiJcEgMKU1p8r6N/DuhfHKmQZ5qstRiLvWt+cHqYfCpgycsafZr+zmm7boh2ZgWN8oTLRg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Cwo68RuqI+QdOg4OMIuxWfAaT8PbeczpX33r84Cpmn2VRrtMLgDIm2q21hbsq4blNG/VZPch9VhXP3K4XFhEhO+r98D6JonAu1h7SF/Ao0tztKXSqIiJs1tAImtxKkP7nnSZzvEg9WW2xHCaIH8A19w3MO+LwM7eEMgPEcJTUqUEZt/SHo/rsiZ7OqTGiOacMBNMSrdNpMDOm5LPyII3SeQL/sLL7MA2JmpZ1WYfEWlbcoo3fZuD1L2j3rXXIQK7Fl7aZnQiQfifP64fHX50QYcO3b1RUToio4T3LAp5ugEnUL9h5326xc3Xg1+0evtJVmxWUjEFXTc260ZWAhdzmQ==
  • 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>, Wei Liu <wl@xxxxxxx>
  • Delivery-date: Mon, 23 May 2022 06:49:40 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 20.05.2022 16:38, Roger Pau Monné wrote:
> On Fri, May 20, 2022 at 04:28:14PM +0200, Roger Pau Monné wrote:
>> On Fri, May 20, 2022 at 02:36:02PM +0200, Jan Beulich wrote:
>>> On 20.05.2022 14:22, Roger Pau Monné wrote:
>>>> On Fri, May 20, 2022 at 01:13:28PM +0200, Jan Beulich wrote:
>>>>> On 20.05.2022 13:11, Jan Beulich wrote:
>>>>>> On 20.05.2022 12:47, Roger Pau Monné wrote:
>>>>>>> On Thu, May 19, 2022 at 02:12:04PM +0200, Jan Beulich wrote:
>>>>>>>> On 06.05.2022 13:16, Roger Pau Monné wrote:
>>>>>>>>> On Mon, Apr 25, 2022 at 10:40:55AM +0200, Jan Beulich wrote:
>>>>>>>>>> --- a/xen/drivers/passthrough/amd/iommu_map.c
>>>>>>>>>> +++ b/xen/drivers/passthrough/amd/iommu_map.c
>>>>>>>>>> @@ -115,7 +115,19 @@ static void set_iommu_ptes_present(unsig
>>>>>>>>>>  
>>>>>>>>>>      while ( nr_ptes-- )
>>>>>>>>>>      {
>>>>>>>>>> -        set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
>>>>>>>>>> +        ASSERT(!pde->next_level);
>>>>>>>>>> +        ASSERT(!pde->u);
>>>>>>>>>> +
>>>>>>>>>> +        if ( pde > table )
>>>>>>>>>> +            ASSERT(pde->ign0 == find_first_set_bit(pde - table));
>>>>>>>>>> +        else
>>>>>>>>>> +            ASSERT(pde->ign0 == PAGE_SHIFT - 3);
>>>>>>>>>
>>>>>>>>> I think PAGETABLE_ORDER would be clearer here.
>>>>>>>>
>>>>>>>> I disagree - PAGETABLE_ORDER is a CPU-side concept. It's not used 
>>>>>>>> anywhere
>>>>>>>> in IOMMU code afaics.
>>>>>>>
>>>>>>> Isn't PAGE_SHIFT also a CPU-side concept in the same way?  I'm not
>>>>>>> sure what's the rule for declaring that PAGE_SHIFT is fine to use in
>>>>>>> IOMMU code  but not PAGETABLE_ORDER.
>>>>>>
>>>>>> Hmm, yes and no. But for consistency with other IOMMU code I may want
>>>>>> to switch to PAGE_SHIFT_4K.
>>>>>
>>>>> Except that, with the plan to re-use pt_update_contig_markers() for CPU-
>>>>> side re-coalescing, there I'd prefer to stick to PAGE_SHIFT.
>>>>
>>>> Then can PAGETABLE_ORDER be used instead of PAGE_SHIFT - 3?
>>>
>>> pt_update_contig_markers() isn't IOMMU code; since I've said I'd switch
>>> to PAGE_SHIFT_4K in IOMMU code I'm having a hard time seeing how I could
>>> at the same time start using PAGETABLE_ORDER there.
>>
>> I've got confused by the double reply and read it as if you where
>> going to stick to using PAGE_SHIFT everywhere as proposed originally.
>>
>>> What I maybe could do is use PTE_PER_TABLE_SHIFT in AMD code and
>>> LEVEL_STRIDE in VT-d one. Yet I'm not sure that would be fully correct/
>>> consistent, ...
>>>
>>>> IMO it makes the code quite easier to understand.
>>>
>>> ... or in fact helping readability.
>>
>> Looking at pt_update_contig_markers() we hardcode CONTIG_LEVEL_SHIFT
>> to 9 there, which means all users must have a page table order of 9.
>>
>> It seems to me we are just making things more complicated than
>> necessary by trying to avoid dependencies between CPU and IOMMU
>> page-table sizes and definitions, when the underlying mechanism to set
>> ->ign0 has those assumptions baked in.
>>
>> Would it help if you introduced a PAGE_TABLE_ORDER in page-defs.h?
> 
> Sorry, should be PAGE_TABLE_ORDER_4K.

Oh, good that I looked here before replying to the earlier mail: I'm
afraid I view PAGE_TABLE_ORDER_4K as not very useful. From an
abstract POV, what is the base unit meant to be that the order is
is based upon? PAGE_SHIFT? Or PAGE_SHIFT_4K? I think such an
ambiguity is going to remain even if we very clearly spelled out what
we mean things to be, as one would always need to go back to that
comment to check which of the two possible ways it is.

Furthermore I'm not convinced PAGETABLE_ORDER is really meant to be
associated with a particular page size anyway: PAGE_TABLE_ORDER_2M
imo makes no sense at all. And page-defs.h is not supposed to
express any platform properties anyway, it's merely an accumulation
of (believed) useful constants.

Hence the only thing which I might see as a (remote) option is
IOMMU_PAGE_TABLE_ORDER (for platforms where all IOMMU variants have
all page table levels using identical sizes, which isn't a given, but
which would hold for x86 and hence for the purpose here).

Jan




 


Rackspace

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