Re: [PATCH v5 6/6] amd/iommu: skip bridge devices when updating IOMMU page tables

From: "Woods, Brian" <Brian.Woods@amd.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	"Woods, Brian" <Brian.Woods@amd.com>,
	"Suthikulpanit, Suravee" <Suravee.Suthikulpanit@amd.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v5 6/6] amd/iommu: skip bridge devices when updating IOMMU page tables
Date: Tue, 20 Nov 2018 23:26:34 +0000	[thread overview]
Message-ID: <20181120232627.GA31051@amd.com> (raw)
In-Reply-To: <5BF43A1902000078001FE31E@prv1-mh.provo.novell.com>

On Tue, Nov 20, 2018 at 09:45:13AM -0700, Jan Beulich wrote:
> >>> On 20.11.18 at 17:01, <roger.pau@citrix.com> wrote:
> > Bridges are not behind an IOMMU, and are already special cased and
> > skipped in amd_iommu_add_device. Apply the same special casing when
> > updating page tables.
> > 
> > This is required or else update_paging_mode will fail and return an
> > error to the caller (amd_iommu_{un}map_page) which will destroy the
> > domain.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > Cc: Brian Woods <brian.woods@amd.com>
> > ---
> > Changes since v4:
> >  - Invert condition order so they match the order in
> >    amd_iommu_add_device.
> >  - Expand commit message to spell out why this is required.
> 
> Thanks. Nevertheless ...
> 
> > --- a/xen/drivers/passthrough/amd/iommu_map.c
> > +++ b/xen/drivers/passthrough/amd/iommu_map.c
> > @@ -612,6 +612,10 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
> >          /* Update device table entries using new root table and paging mode */
> >          for_each_pdev( d, pdev )
> >          {
> > +            if ( pdev->type == DEV_TYPE_PCI_HOST_BRIDGE &&
> > +                 is_hardware_domain(d) )
> > +                continue;
> 
> ... before spreading the issue I'd still like to see clarification of /
> justification for the is_hardware_domain() part of the condition
> you clone. Suravee, Brian?
> 
> Jan
> 

The original commit 0af438757d455f8eb6b5a6ae9a990ae245f230fd

The commit that adds is_hardware_domain (and rearrange things)
7c275549f46c5c46611592f7107c1345e93ed457

The orginal commit used the function like
setup_dom0_pci_devices(d, amd_iommu_setup_dom0_device);
which was because IOMMU needed to skip the host bridge devices on dom0.

So I assume you added the is_hardware_domain because it only needed to
be done on dom0.  I'm not familiar with the IOMMU/PCI history wrt to
what it mapped/passed through so.

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel