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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/03/2019 15:24, Jan Beulich wrote:
>>>> 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?

This logic (which is being deleted) used to be dead, and became non-dead
with the identified commit.

The code, now being run, constitutes a functional regression on some
hardware, which worked fine with Xen 4.11.

> How's that commit related then? Segment 0 is valid irrespective of any
> MMCFG information gained from ACPI tables (see pci_segments_init()).

Exactly - that is why it is a regression.

Before pci_segments_init() (which is the first action of
acpi_mmcfg_init(), and hence is moved by the mentioned commit),
acpi_parse_one_drhd()'s query of segment zero returns "not present",
causing the check to be skipped.

>
>>  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?

Exactly the same as what happens on Xen 4.11 and earlier, whatever that
happens to be.

>
>> 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 had, and as a maintainer, I'd reject a patch like that were it
presented today.

There is a nebulous claim of security, but it is exactly that -
nebulous.  There isn't enough information to work out what the concern
was, and even if the concern was valid, disabling VT-d across the system
isn't an appropriate action 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.

The commit message states clearly the commit which caused the function
regression, and the justification for why deleting the code resolves the
regression by making the behaviour identical to 4.11 and earlier.

I'm not sure what more you are looking for, but this is very clear cut
and safe from my point of view.

~Andrew

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