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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.03.19 at 18:36, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Understood. But whether you'd accept it with a better description
is unknown, I assume.

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

This heavily depends on the position the system's admin takes:
Enabling VT-d in an incomplete fashion may as well be considered
worse than not enabling it at all. That's because without VT-d
pass-through of devices to HVM guests won't be allowed at all.
Whereas with VT-d enabled passing through a device may, in
such a situation, put the system at higher risk than the admin is
aware.

Furthermore, as much as the security related claim there is
nebulous, your description - I'm sorry to say that - isn't much
better, as you don't clarify why there's _no_ security aspect
there. Stating that "this leaves the system in better overall
state" without making clear why that is _for everyone_ is not
helpful at all.

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

But are you seriously suggesting the behavior in 4.11 and earlier
was intended? I'm of the opinion that the assumption was that
pci_known_segment() would unconditionally return true for
segment 0. When writing my original reply it didn't even occur to
me that this could not have been the case.

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

Well, your claim regarding "4.11 and earlier" is clearly wrong, albeit
in another way than I first thought: Looking back far enough, prior
to the introduction of support for segments other than 0 the code
in question did not skip the checking on segment 0. Obviously at
that time nothing other than segment 0 would have been checked
(and as it seems the DRHD's segment specification was silently
ignored).

IOW there was an earlier regression causing the checks to be
skipped. This looks to have been my fault with fd1e17183b ("VT-d:
don't reject possibly valid DRHD or RMRR"), not realizing that the
call to pt_pci_init() would have had to happen ahead of that to
acpi_boot_init() in order for pci_known_segment() to return true
for segment 0 at all times.

So along the lines of what I've been saying before - what I would
have expected in the commit message (and what I still hope for
in a reply of yours) is a discussion of why exactly the original
commit adding the checking was wrong, the more with its later
amending by the command line (sub-)option you've now removed.
As said above - it should be the admin's decision whether to
enable the IOMMU despite there being firmware flaws. You've
taken away this (policy) decision from them, uniformly forcing the
behavior you consider the only possible good one onto everyone.

As an aside - the description of your change also makes it sound
as if the observed behavior was correct. I doubt this to be the
case; I think devices disabled by user choice should not have
references left in ACPI tables.

Jan


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