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

Re: [PATCH v2 10/18] AMD/IOMMU: walk trees upon page fault


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 10 Dec 2021 11:23:43 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.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=oS/VCWen5cr9Gyvsjr2HYOhaDpbK9eM/3gDHjqR2rlM=; b=VFOBoZQ/lynjE4WMOlDTTs8q2xVVyprBeY8uLa7Xe2LA/m24CKFZ2kLywbORRd8AOJ6Ybwkfa3LT59A/ERpecBGJi8NrrTGDCc9+7jgs6elbYJ7JR3JczwvTpcvuvtr03irEOuGB0T4AagdzDd7/WDjy+7NZNOstE1V0fE/JToAlPz+Uw2+2zyHsQ96W7KaeO9JC80knkps7llpqy+YAzztQVGHgsFXjIYy/7LEt5+BFFGsb6lDjCinj87vagoh0KwqK/qCHI49UK5mmdRHET1++kOY6di6qCD8PZgTsfhbBsQICSsn14geLMhp50TTmtvkoRLW90A4CmGrVG3aPew==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=U3KdZCewbeq6LBW9kOZK5IQiAyhhNiLZUp/kEMZ7CQYqLUKzMMKj/cfsyuYwQKJO41g3QKtbRoprfckNEe4j/Pw/GLZTKyLgl+CaFfuCaLcGgxvTyimXbrTbLeAg9xi/Yk4fxct8FCNy2OyMtmMb8cdfqFD6BiJJlDsD7wwnKIi19a+u3tlo7XrpGfO0fYJb+1T7B2m0XwO3dxSldaPkJxDapdCLwrzzDymjZUOd9FDnUk/pUQ91PMdxdVrBCxWD+90iOWMQLJMsdC9ElZ2A7sX8Dky8IW2lFFG1WoYmbB6KW0H3U9EU7y2r5Ia1MYvNJmbOktnRh7vLzDhkVJeZ8Q==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Paul Durrant <paul@xxxxxxx>
  • Delivery-date: Fri, 10 Dec 2021 10:24:16 +0000
  • Ironport-data: A9a23:LZd/O6NJflQ24BHvrR17kMFynXyQoLVcMsEvi/4bfWQNrUp30mBWm 2sXXT/SaPqDMGT0ctlwbN+z/ExSvsLVm9YyQAto+SlhQUwRpJueD7x1DKtR0wB+jCHnZBg6h ynLQoCYdKjYdpJYz/uUGuCJQUNUjMlkfZKhTr6UUsxNbVU8En5400s7w7dRbrNA2rBVPSvc4 bsenOWHULOV82Yc3rU8sv/rRLtH5ZweiRtA1rAMTakjUGz2zhH5OKk3N6CpR0YUd6EPdgKMq 0Qv+5nilo/R109F5tpICd8XeGVSKlLZFVDmZna7x8FOK/WNz8A/+v9TCRYSVatYoxOsncFui 4lpiaC1eAAlOPfWm7UZDjANRkmSPYUekFPGCX22sMjVxEzaaXr8hf5pCSnaP6VBpLwxWzsXs 6VFdnZdNXhvhMrvqF6/YvNrick5atHiIasUu216zCGfBvEjKXzGa/ibuoYHhGlu7ixINc3eQ OwTeWFlVzvrMyFxBQZKBJcuxt790xETdBUH8QnI9MLb+VP71AVs1JD9PdyTfcaFLe1Fk0Ddq m/Y8mDRBhABKMfZ2TeD6mirhOLEgWX8Qo16PL+y++NugVaT7ncOExBQXly+ycRVkWbnBYgZc RZNvHNz8+5iryRHU+URQTWehXyVoV1Af+ZsFtw95AfX0KmF51uwUz1soiF6VPQqs8o/RDoP3 1CPns/0CTEHjIB5WU5x5Z/P82rsZHF9wXsqIHZdEFBbu4WLTJQb00qXJuuPBpJZmTEc9dvY5 zmR5BYziLwI5SLg//XqpAuX695AS3Wgc+LU2uk1dj71hu+aTNT8D2BN1bQ9xawYRGp+ZgPe1 EXoY+DEsIgz4WilzURhutklErCz/OqiOzbBm1NpFJRJ323zoC/7It0MvmkveB8B3iM4ldnBO he7VeR5vsA7AZdXRfUvP9LZ5zoCk8AM6ugJptiLN4ETM/CdhSeM/T10ZF744oweuBNErE3LA r/CKZzEJS9DUcxPlWPqL89Age5D7n1vngv7GMGkpylLJJLDPRZ5v59eawDQBg34hYvZyDjoH yF3a5HXlk4BCbKmOUE6M+c7dDg3EJTyPrivw+R/fe+fOAt2XmYnDv7a27Q6fIJ52a9Sk4/1E ruVBCe0EXLz2i/KLxukcHdmZO+9VJpztytjbyctIUypyz4oZoP2tPUTcJ4+fL8G8u1/zKErE 6lZKpvYWvkfGC7a/zk9bIXmqNAwfhqcmg/TbTGuZyIyfsA8SlWRqMPkZAbm6AIHEjGz6Zklu 7Sl2w6CGcgDSg1uAdz4cvWqy1/t73ERlPgrBxnDI8VJeVWq+49vcnSjgvgyKsAKCBPC2jrFi FrGXUZG/bHA+tZn/sPIiKaIq5aSP9F/RkcKTXPG6buWNDXB+jbxy4F3T+vVLyvWU3n5+fv+a LwNne38KvAOgH1Dr5F4T+Rw1as76tbi++1awwBjECmZZlinEOo9cHyP3M0JvaxR3L5J/wCxX xvXqNVdPLyIPuLjEUIQe1V5PrjSi6lMl2mA9+kxLWX7+DRzreiOXkhlNhWRjDBQceluO4Q/z OZ94MMb5mRTUPbx3gpqWsyMy1mxEw==
  • Ironport-hdrordr: A9a23:zQp+OqDG/OiAP1flHeg2sceALOsnbusQ8zAXPh9KJiC9I/b1qy nxppkmPH/P6Qr4WBkb6Le90Y27MAnhHPlOkPQs1NaZLXLbUQ6TQr2KgrGSoQEIdxeOk9K1kJ 0QD5SWa+eAfGSS7/yKmTVQeuxIqLLskNHK9JfjJjVWPHlXgslbnnlE422gYytLrWd9dP4E/M 323Ls5m9PsQwVcUu2LQl0+G8TTrdzCk5zrJTYAGh4c8QGLyRel8qTzHRS01goXF2on+8ZvzU H11yjCoomzufCyzRHRk0fV8pRtgdPkjv9OHtaFhMQ5IijlziyoeINicbufuy1dmpDj1H8a1P 335zswNcV67H3cOkmzvBvWwgHllA0j7nfzoGXoyEfLkIjcfnYXGsBBjYVWfl/y8Ew7puxx16 pNwiawq4dXJQmoplW92/H4EzVR0makq3srluAey1ZFV5EFVbNXpYsDuGtIDZY7Gj7g4oxPKp ghMCjl3ocUTbqmVQGagoE2q+bcG0jbXy32DXTqg/blkwS/xxtCvg8lLM92pAZ3yHtycegC2w 3+CNUbqFh5dL5gUUtMPpZzfSKJMB25ffvtChPbHb21LtBNB5ryw6SHlIndotvaPqA18A==
  • Ironport-sdr: HGusIPi4fDszCd31UddWivVt3nVGatmqjR26qwpTPsT5qFsMG6JSPUzAKsvHpOFuYh9jv0L4oi 6tLib+QsYP00dQ4vnHPQ4tKzsEVDtaH/1wIsnlsKyxxFsY4JQkRAdNM+BWlCoMu10ay2L8PXcE mNZzVTgbTLiTwniFNCANDevIy4Afu3s8d8ErEjhFa9AqPjOUdTWxW9wtR5AfB01xBhv+l3hbsK 9+bNjrwibPpjtdZbV53rr1K6K8dM7yUvCkW4XcxQV0pSHTVC+tW27DQ4nMBtl3sPsjOS/dxEk9 MfVLkjTp0woVBHqI+0ZeArwo
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Fri, Dec 03, 2021 at 10:55:54AM +0100, Jan Beulich wrote:
> On 03.12.2021 10:49, Jan Beulich wrote:
> > On 03.12.2021 10:03, Roger Pau Monné wrote:
> >> On Fri, Sep 24, 2021 at 11:51:15AM +0200, Jan Beulich wrote:
> >>> This is to aid diagnosing issues and largely matches VT-d's behavior.
> >>> Since I'm adding permissions output here as well, take the opportunity
> >>> and also add their displaying to amd_dump_page_table_level().
> >>>
> >>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >>>
> >>> --- a/xen/drivers/passthrough/amd/iommu.h
> >>> +++ b/xen/drivers/passthrough/amd/iommu.h
> >>> @@ -243,6 +243,8 @@ int __must_check amd_iommu_flush_iotlb_p
> >>>                                               unsigned long page_count,
> >>>                                               unsigned int flush_flags);
> >>>  int __must_check amd_iommu_flush_iotlb_all(struct domain *d);
> >>> +void amd_iommu_print_entries(const struct amd_iommu *iommu, unsigned int 
> >>> dev_id,
> >>> +                             dfn_t dfn);
> >>>  
> >>>  /* device table functions */
> >>>  int get_dma_requestor_id(uint16_t seg, uint16_t bdf);
> >>> --- a/xen/drivers/passthrough/amd/iommu_init.c
> >>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> >>> @@ -573,6 +573,9 @@ static void parse_event_log_entry(struct
> >>>                 (flags & 0x002) ? " NX" : "",
> >>>                 (flags & 0x001) ? " GN" : "");
> >>>  
> >>> +        if ( iommu_verbose )
> >>> +            amd_iommu_print_entries(iommu, device_id, 
> >>> daddr_to_dfn(addr));
> >>> +
> >>>          for ( bdf = 0; bdf < ivrs_bdf_entries; bdf++ )
> >>>              if ( get_dma_requestor_id(iommu->seg, bdf) == device_id )
> >>>                  pci_check_disable_device(iommu->seg, PCI_BUS(bdf),
> >>> --- a/xen/drivers/passthrough/amd/iommu_map.c
> >>> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> >>> @@ -363,6 +363,50 @@ int amd_iommu_unmap_page(struct domain *
> >>>      return 0;
> >>>  }
> >>>  
> >>> +void amd_iommu_print_entries(const struct amd_iommu *iommu, unsigned int 
> >>> dev_id,
> >>> +                             dfn_t dfn)
> >>> +{
> >>> +    mfn_t pt_mfn;
> >>> +    unsigned int level;
> >>> +    const struct amd_iommu_dte *dt = iommu->dev_table.buffer;
> >>> +
> >>> +    if ( !dt[dev_id].tv )
> >>> +    {
> >>> +        printk("%pp: no root\n", &PCI_SBDF2(iommu->seg, dev_id));
> >>> +        return;
> >>> +    }
> >>> +
> >>> +    pt_mfn = _mfn(dt[dev_id].pt_root);
> >>> +    level = dt[dev_id].paging_mode;
> >>> +    printk("%pp root @ %"PRI_mfn" (%u levels) dfn=%"PRI_dfn"\n",
> >>> +           &PCI_SBDF2(iommu->seg, dev_id), mfn_x(pt_mfn), level, 
> >>> dfn_x(dfn));
> >>> +
> >>> +    while ( level )
> >>> +    {
> >>> +        const union amd_iommu_pte *pt = map_domain_page(pt_mfn);
> >>> +        unsigned int idx = pfn_to_pde_idx(dfn_x(dfn), level);
> >>> +        union amd_iommu_pte pte = pt[idx];
> >>
> >> Don't you need to take a lock here (mapping_lock maybe?) in order to
> >> prevent changes to the IOMMU page tables while walking them?
> > 
> > Generally speaking - yes. But see the description saying "largely
> > matches VT-d's behavior". On VT-d both the IOMMU lock and the mapping
> > lock would need acquiring to be safe (the former could perhaps be
> > dropped early). Likewise here. If I wanted to do so here, I ought to
> > add a prereq patch adjusting the VT-d function. The main "excuse" not
> > to do so is/was probably the size of the series.
> 
> Which in turn would call for {amd,vtd}_dump_page_tables() to gain proper
> locking. Not sure where this would end; these further items are more and
> more unrelated to the actual purpose of this series (whereas I needed the
> patch here anyway for debugging purposes) ...

I think adding a comment regarding the lack of locking due to the
function only being used as a debug aide would help clarify this. I
don't think we support running with iommu debug or verbose modes.

Thanks, Roger.



 


Rackspace

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