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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Brian Woods <brian.woods@amd.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Subject: Re: [PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables
Date: Thu, 15 Nov 2018 16:48:08 +0100	[thread overview]
Message-ID: <20181115154808.s5natbctutnd7ztd@mac> (raw)
In-Reply-To: <5BED935B02000078001FC7A4@prv1-mh.provo.novell.com>

On Thu, Nov 15, 2018 at 08:40:11AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, <roger.pau@citrix.com> wrote:
> > Bridges are not behind an IOMMU, and are already special cased and
> > silently skipped in amd_iommu_add_device. Apply the same special
> > casing when updating page tables.
> 
> But bridges also don't issue I/O on their own if I'm not mistaken. So
> what I'm missing here is a word on the benefit of this change. I also
> question the "silently" in your wording, seeing the AMD_IOMMU_DEBUG()
> there.

I see, by silently I meant without throwing an error, but I think just
using 'skipped' would be clearer.

The benefit is that update_paging_mode doesn't return an error when it
finds a bridge attached to Dom0, which would cause the caller of
update_paging_mode (amd_iommu_{un}map_page) to crash the domain.

Ie: without this change a PVH Dom0 running on AMD hardware crashes
when the IOMMU page table is expanded.

> The code change itself looks fine to me, albeit personally I'd prefer
> if it fully matched the other conditional (i.e. if you flipped the
> operands of && ). Of course the special casing of the hardware
> domain here is somewhat odd anyway.

Do you mean because bridges would only be ever assigned to the
hardware domain?

Thanks, Roger.

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