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

[George Dunlap <george.dunlap@citrix.com>]

On 3/25/19 3:24 PM, Jan Beulich wrote:
>>>> On 21.03.19 at 21:26, <andrew.cooper3@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