Re: [Xen-devel] [PATCH for-4.12] passthrough/vtd: Drop the "workaround_bios_bug" logic entirely

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.03.19 at 21:26, <andrew.cooper3@xxxxxxxxxx> 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.

I'd also appreciate clarification on you saying "working in the same
way that it did in previous releases" - it sounds as if you might
have spotted a regression here, but it's not really becoming clear
to me what that regression then would have been.

> This is a candidate for 4.12.  Given the absense of Jan as the maintaner, and
> the proximity to the 4.12 release, I put this patch to The Rest for a
> hopefully-more-timely decision and review.

To be honest, I have two problems with this: For one the main
part of your change falls in Kevin's realm, not mine. And then,
even if it would have been mainly me to ack the change, I was
gone for three days, not three months. Yet the code had been
this way for over 9 years. One thing seems pretty clear to me:
It is at best non-obvious that there is no risk of regressions
here.

Much less risky changes have been rejected as coming too late
for 4.12. I don't think this should have been rushed into the
tree, and even less so for a release about to be cut. Especially
not without a proper maintainer ack.

The main reason why, at least for the moment, I'm not meaning
to officially request a revert is that I'm not sure the original
commit's description is fully correct and / or it was really
addressing some ia64-specific quirk.

Jan



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