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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 3/27/19 2:38 PM, Andrew Cooper wrote:
> On 26/03/2019 13:39, Jan Beulich wrote:
>>>>> On 26.03.19 at 13:43, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 26/03/2019 09:08, Jan Beulich wrote:
>>>>>>> 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.
>>> I severely doubt I'd accept it at all, because it is entirely
>>> unreasonable behaviour.
>>>
>>> At best, it is the equivalent of throwing your hands up in the air and
>>> saying "I give up", and that is not good enough behaviour for Xen.
>>>
>>>>> 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.
>>> No - that's simply not true, or a reasonable position to take. 
>> As is every way of thinking differently than you do?
> 
> No, but I do expect common sense to be used in the judgement of what is
> appropriate and/or reasonable end user behaviour.

Andy, you're not being reasonable here.  Just because *you* can't think
of how disabling the DRHD could be useful behavior doesn't mean there
isn't one.  The original patch took time and effort to write; so one of
two things is true:

1. The authors were attempting to address a theoretical concern; the
behavior in question didn't fix a real problem they had, or

2. The authors were attempting to address a real problem they had, and
the patch in question fixed it (for some value of "fixed").

#1 does happen, but on the whole, #2 is more likely; so it's much better
to assume that there was a problem that the patch fixed, even if it
might not have been the best *way* to fix it.

And in fact, if you go back and look at the original discussion [1]
(which involved Intel, Fujitsu, HP, and others), #2 turns out to to be
the case.  Lots of BIOSes had issues with misreporting RMRRs and DRHDs,
and on at least one of those, enabing a DRHD which had invalid RMRRs and
things behind it caused the box not to boot [2].  Recall that at the
time, VT-d was very new, and didn't have wide support.  So, Keir, seeing
all these reports, said:

"If we want to keep iommu=1 as default, then it is unacceptable to fail
to boot on a fairly wide range of modern systems. We have to warn-and
disable, partially or completely, unless iommu=force is specified. Or we
need to revert to iommu=0 as the default."  [3]

I think that was a very sensible approach, given the circumstances.

Now, as it happens, while there were lots of reports of invalid RMRR /
DRHD information from BIOSes, the only report I could find of something
actually failing to boot was a Fujitsu private platform [4].  So it
might actually be the case that, while BIOS bugs were common, failing to
boot when enabling "invalid" DRHDs was pretty rare.  Or it may have been
common.  We don't really have any way of knowing.

I continue to think that given that none of this was captured in the
commit message or code comments, and that we had two releases where this
behavior was disabled with no bug reports, that removing the code was a
reasonable thing to do.  But asserting that there is no conceivable
reason for the code to ever have existed is not -- even without doing
the archaeology.

Regarding what to do in light of this further background:  Given that
VT-d is now a more mature and widespread technology, given that it's
required on many systems, given that we've had two release cycles with
no reported problems, given XenRT's extensive testing, and finally given
the fact that the only known situation where disabling the DRHD was
necessary was on a "private" platform, think that removing the code and
seeing what happens is the best approach.

 -George

[1]
https://lists.xenproject.org/archives/html/xen-devel/2010-01/msg00665.html
[2]
https://lists.xenproject.org/archives/html/xen-devel/2010-01/msg00691.html
[3]
https://lists.xenproject.org/archives/html/xen-devel/2010-01/msg00731.html
[4]
https://lists.xenproject.org/archives/html/xen-devel/2010-01/msg00786.html

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