On 3/25/19 3:24 PM, Jan Beulich wrote: >>>> On 21.03.19 at 21:26, <andrew.coop...@citrix.com> wrote: >> It turns out that this code was previously dead. > > If it was entirely dead, why the rush to get the change into 4.12? > (I suppose the later parts of description are then justifying why > the code wasn't actually dead, and why it was getting in the way, > but I think this way of putting it is at least confusing.) > >> c/s dcf41790 " x86/mmcfg/drhd: Move acpi_mmcfg_init() call before calling >> acpi_parse_dmar()" resulted in PCI segment 0 now having been initialised >> enough for acpi_parse_one_drhd() to not take the >> >> /* Skip checking if segment is not accessible yet. */ >> >> path unconditionally. > > Or am I misreading the initial sentence, and you're really suggesting > that prior to said commit the code in question had been dead? How's > that commit related then? Segment 0 is valid irrespective of any > MMCFG information gained from ACPI tables (see pci_segments_init()). > >> However, some systems have DMAR tables which list >> devices which are disabled by user choice (in particular, Dell PowerEdge R740 >> with I/O AT DMA disabled), and turning off all IOMMU functionality in this >> case is entirely unhelpful behaviour. > > As in many other cases, what is or is not unhelpful is often a matter > of perception and hence possibly subjective. I can see your point, > but I also can see why the authors of the code considered it a rather > bad sign if non-existing PCI devices get named by an ACPI table. > What if e.g. later a device gets hot-plugged into that very SBDF? > >> Leave the warning which identifies the problematic devices, but drop the >> remaining logic. This leaves the system in better overall state, and working >> in the same way that it did in previous releases. > > I wonder whether you've taken the time to look at the description > of the commit first introducing this logic (a8059ffced "VT-d: improve > RMRR validity checking"). I find it worrying in particular to > effectively revert a change which claims 'to avoid any security > vulnerability with malicious s/s re-enabling "supposed disabled" > devices' without any discussion of why that may have been a > wrong perspective to take.
Having read the patch description, I think you have the polarity of that comment wrong. If I understand correctly, that patch disables part of the IOMMU if it finds something strange, *unless* iommu=force is on. The text gives the idea that it is *less* safe to disable the IOMMU; and that enabling the IOMMU functionality, even with invalid entries, is safer than leaving it off. The patch we checked in (if I understand correctly), enables the IOMMU in more situations, even when iommu=force is not set; and thus (by the logic of the original patch) is more "safe" by default than the previous patch. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
