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

Re: [PATCH] iommu/vtd: fix address translation for superpages


  • To: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 11 May 2023 08:27:22 +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=1/eE/QlgO9DdRdDNB43Un3ARKvzaZpNOV2K9WEcTCgc=; b=KnahjXu5TLgcjFFGJdbdxalpNIr8Io7LFNgnz5kH9hJQbE3mklIkt6lNS8fki89ecxBj0AMKat6d2gr3tHJ+6ckSPrFiW2WFGj3RYUPnxO8dXpG+GWeA9K+CwbQrQREX2D0HEQ9XhASJAxTn5cg2ODUTt3bv13YxVFrR5LtnOsk1ciXaH0WzquZdik5nBCTVJRnQXNzjPIq0+/6hJdeqyZ3nIAi6wIJRsiTHzSfmC1AEtR6WB7vKsNOrIrRKuddOMlmN47U++m7AfZLt13gNJLltN31xFfs5Hr839HvT607XmvpKubHundvJ2AeQ5WCi1L6Sd7upxB1CaVWa18t1oA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bGHiv9JIS6vUaA4IXhbq5sa6WALxzZTJszV4ba6+2y3DdevlaxSh8Kho2a9fChRgsHMWZ0MlCzJBQF4SRBC9hrfPRXT591Jo91lVJnPwUGJ7BibbxdogwEsq974MDGG10gzoIellER3PE8vDyWJ0U6IhWgzH8LkzxmIilyoo6AxYFP1dQI1Ep8xKppQafgKoQ9dpiVFX/F7U9XF718wXnX9uqIKhix69upKDaPX8LJIw7e+IR7ffee1B0OaHL1ucwcJhUwtHrv+liwKkYzY1V4cy1GPSO3hxWQJUPPD73NAkU/BsurHFMqwGhePCSPsbCaFvxVEEPvbaKf/a4PksHQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Kevin Tian <kevin.tian@xxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 11 May 2023 06:27:32 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 10.05.2023 17:19, Roger Pau Monné wrote:
> On Wed, May 10, 2023 at 03:30:21PM +0200, Jan Beulich wrote:
>> On 10.05.2023 12:22, Roger Pau Monné wrote:
>>> On Wed, May 10, 2023 at 12:00:51PM +0200, Jan Beulich wrote:
>>>> On 10.05.2023 10:27, Roger Pau Monné wrote:
>>>>> On Tue, May 09, 2023 at 06:06:45PM +0200, Jan Beulich wrote:
>>>>>> On 09.05.2023 12:41, Roger Pau Monne wrote:
>>>>>>> When translating an address that falls inside of a superpage in the
>>>>>>> IOMMU page tables the fetching of the PTE physical address field
>>>>>>> wasn't using dma_pte_addr(), which caused the returned data to be
>>>>>>> corrupt as it would contain bits not related to the address field.
>>>>>>
>>>>>> I'm afraid I don't understand:
>>>>>>
>>>>>>> --- a/xen/drivers/passthrough/vtd/iommu.c
>>>>>>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>>>>>>> @@ -359,16 +359,18 @@ static uint64_t addr_to_dma_page_maddr(struct 
>>>>>>> domain *domain, daddr_t addr,
>>>>>>>  
>>>>>>>              if ( !alloc )
>>>>>>>              {
>>>>>>> -                pte_maddr = 0;
>>>>>>>                  if ( !dma_pte_present(*pte) )
>>>>>>> +                {
>>>>>>> +                    pte_maddr = 0;
>>>>>>>                      break;
>>>>>>> +                }
>>>>>>>  
>>>>>>>                  /*
>>>>>>>                   * When the leaf entry was requested, pass back the 
>>>>>>> full PTE,
>>>>>>>                   * with the address adjusted to account for the 
>>>>>>> residual of
>>>>>>>                   * the walk.
>>>>>>>                   */
>>>>>>> -                pte_maddr = pte->val +
>>>>>>> +                pte_maddr +=
>>>>>>>                      (addr & ((1UL << level_to_offset_bits(level)) - 1) 
>>>>>>> &
>>>>>>>                       PAGE_MASK);
>>>>>>
>>>>>> With this change you're now violating what the comment says (plus what
>>>>>> the comment ahead of the function says). And it says what it says for
>>>>>> a reason - see intel_iommu_lookup_page(), which I think your change is
>>>>>> breaking.
>>>>>
>>>>> Hm, but the code in intel_iommu_lookup_page() is now wrong as it takes
>>>>> the bits in DMA_PTE_CONTIG_MASK as part of the physical address when
>>>>> doing the conversion to mfn?  maddr_to_mfn() doesn't perform a any
>>>>> masking to remove the bits above PADDR_BITS.
>>>>
>>>> Oh, right. But that's a missing dma_pte_addr() in intel_iommu_lookup_page()
>>>> then. (It would likely be better anyway to switch "uint64_t val" to
>>>> "struct dma_pte pte" there, to make more visible that it's a PTE we're
>>>> dealing with.) I indeed overlooked this aspect when doing the earlier
>>>> change.
>>>
>>> I guess I'm still confused, as the other return value for target == 0
>>> (when the address is not part of a superpage) does return
>>> dma_pte_addr(pte).  I think that needs further fixing then.
>>
>> Hmm, indeed. But I think it's worse than this: addr_to_dma_page_maddr()
>> also does one too many iterations in that case. All "normal" callers
>> supply a positive "target". We need to terminate the walk at level 1
>> also when target == 0.
> 
> Don't we do that already due to the following check:
> 
> if ( --level == target )
>     break;
> 
> Which prevents mapping the PTE address as a page table directory?

I don't think this is enough - this code, afaict right now, is only
sufficient when target >= 1.

Jan



 


Rackspace

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