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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Nov 21, 2018 at 06:23:30AM -0700, Jan Beulich wrote:
> >>> On 21.11.18 at 12:51, <roger.pau@xxxxxxxxxx> wrote:
> > On Wed, Nov 21, 2018 at 03:58:34AM -0700, Jan Beulich wrote:
> >> >>> On 21.11.18 at 11:37, <roger.pau@xxxxxxxxxx> wrote:
> >> > On Wed, Nov 21, 2018 at 02:21:36AM -0700, Jan Beulich wrote:
> >> >> >>> On 21.11.18 at 00:26, <Brian.Woods@xxxxxxx> wrote:
> >> >> > 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.
> >> >> 
> >> >> Well, I added it presumably to retain original semantics. I still
> >> >> think that the extra check would better be dropped there, not
> >> >> the least to also cover the case of devices eventually getting
> >> >> assigned to dom_xen.
> >> >> 
> >> >> Looking at this another time I find some other questionable
> >> >> aspect though (both to pre-existing code and to the change
> >> >> made here): "host bridge" != "bridge". The title here as much
> >> >> as the comment next to the original piece of code both
> >> >> suggest the wider general category is meant, but the code
> >> >> cloned checks for host bridges only. In
> >> >> amd_iommu_add_device() the check is used solely to emit a
> >> >> less scary log message, but the change here goes beyond
> >> >> that.
> >> > 
> >> > The check in amd_iommu_add_device allows host bridge devices to be
> >> > assigned to the hardware domain without returning an error since they
> >> > are not behind an IOMMU. Note that even if amd_iommu_add_device
> >> > returned an error the device would still be added to the hardware
> >> > domain since the error is eaten by setup_one_hwdom_device and the
> >> > device is assigned to the hardware domain regardless.
> >> 
> >> Hence me saying this check is mainly/just to make the log message
> >> less scary.
> >> 
> >> > So either all devices that are not behind an IOMMU are not assigned to
> >> > the hardware domain, or update_paging_mode needs this workaround in
> >> > order to be able to handle IOMMU page table expansion for the hardware
> >> > domain.
> >> 
> >> It looks like we're talking about different aspects: I don't put under
> >> question that assignment wants to be avoided. What I question is
> >> why the avoidance gets restricted to the hardware domain.
> > 
> > Not having written this code I can only make guesses. I would say this
> > is because you don't want to hand a PCI host bridge to a guest so it
> > cannot play with the registers there and likely move the MCFG or the
> > MMIO windows.
> 
> That's all correct, but only indirectly related. The question is what's
> wrong when the conditionals (both yours and the one you derive
> from) have the is_hardware_domain() part dropped.

I think the is_hardware_domain part can be dropped from the
conditional I'm adding. update_paging_mode shouldn't be used to decide
whether a domain can or cannot have bridges attached. Whether a DomU
can or cannot have a host bridge assigned should be decided at
assignation time, and hence update_paging_mode shouldn't have hardware
domain specific checks.

Regarding the check in amd_iommu_add_device, if it's removed from
there amd_iommu_add_device would return an error when adding a host
bridge device, and that would cause setup_one_hwdom_device to return
early and not setup vPCI handlers for host bridges, so I think we want
to leave that one as-is.

Thanks, Roger.

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