> -----Original Message----- > From: Jan Beulich <jbeul...@suse.com> > Sent: 13 December 2019 13:26 > To: Durrant, Paul <pdurr...@amazon.com> > Cc: xen-devel@lists.xenproject.org; Juergen Gross <jgr...@suse.com>; Kevin > Tian <kevin.t...@intel.com>; Stefano Stabellini <sstabell...@kernel.org>; > Julien Grall <jul...@xen.org>; Wei Liu <w...@xen.org>; Konrad Wilk > <konrad.w...@oracle.com>; George Dunlap <george.dun...@eu.citrix.com>; > Andrew Cooper <andrew.coop...@citrix.com>; Paul Durrant <p...@xen.org>; > Ian Jackson <ian.jack...@citrix.com>; Roger Pau Monné > <roger....@citrix.com> > Subject: Re: [PATCH v2] IOMMU: make DMA containment of quarantined devices > optional > > On 13.12.2019 14:12, Durrant, Paul wrote: > >> -----Original Message----- > >> From: Xen-devel <xen-devel-boun...@lists.xenproject.org> On Behalf Of > Jan > >> Beulich > >> Sent: 13 December 2019 12:53 > >> To: xen-devel@lists.xenproject.org > >> Cc: Juergen Gross <jgr...@suse.com>; Kevin Tian <kevin.t...@intel.com>; > >> Stefano Stabellini <sstabell...@kernel.org>; Julien Grall > >> <jul...@xen.org>; Wei Liu <w...@xen.org>; Konrad Wilk > >> <konrad.w...@oracle.com>; George Dunlap <george.dun...@eu.citrix.com>; > >> Andrew Cooper <andrew.coop...@citrix.com>; Paul Durrant <p...@xen.org>; > >> Ian Jackson <ian.jack...@citrix.com>; Roger Pau Monné > >> <roger....@citrix.com> > >> Subject: [Xen-devel] [PATCH v2] IOMMU: make DMA containment of > quarantined > >> devices optional > >> > >> Containing still in flight DMA was introduced to work around certain > >> devices / systems hanging hard upon hitting an IOMMU fault. Passing > >> through (such) devices (on such systems) is inherently insecure (as > >> guests could easily arrange for IOMMU faults to occur). Defaulting to > >> a mode where admins may not even become aware of issues with devices > can > >> be considered undesirable. Therefore convert this mode of operation to > >> an optional one, not one enabled by default. > >> > >> This involves resurrecting code commit ea38867831da ("x86 / iommu: set > >> up a scratch page in the quarantine domain") did remove, in a slightly > >> extended and abstracted fashion. Here, instead of reintroducing a > pretty > >> pointless use of "goto" in domain_context_unmap(), and instead of > making > >> the function (at least temporarily) inconsistent, take the opportunity > >> and replace the other similarly pointless "goto" as well. > >> > >> In order to key the re-instated bypasses off of there (not) being a > root > >> page table this further requires moving the allocate_domain_resources() > >> invocation from reassign_device() to amd_iommu_setup_domain_device() > (or > >> else reassign_device() would allocate a root page table anyway); this > is > >> benign to the second caller of the latter function. > >> > >> Signed-off-by: Jan Beulich <jbeul...@suse.com> > >> --- > >> As far as 4.13 is concerned, I guess if we can't come to an agreement > >> here, the only other option is to revert ea38867831da from the branch, > >> for having been committed prematurely (I'm not so much worried about > the > >> master branch, where we have ample time until 4.14). What I surely want > >> to see us avoid is a back and forth in behavior of released versions. > >> (Note that 4.12.2 is similarly blocked on a decision either way here.) > >> > >> I'm happy to take better suggestions to replace "full". > > > > How about simply "sink", since that's what it does? > > But it's not really a "sink", as we still fault writes (which is the > only thing I can see to be "sunk" if I'm getting the meaning of the > word right). > > >> --- a/xen/drivers/passthrough/iommu.c > >> +++ b/xen/drivers/passthrough/iommu.c > >> @@ -30,13 +30,17 @@ bool_t __initdata iommu_enable = 1; > >> bool_t __read_mostly iommu_enabled; > >> bool_t __read_mostly force_iommu; > >> bool_t __read_mostly iommu_verbose; > >> -bool __read_mostly iommu_quarantine = true; > >> bool_t __read_mostly iommu_igfx = 1; > >> bool_t __read_mostly iommu_snoop = 1; > >> bool_t __read_mostly iommu_qinval = 1; > >> bool_t __read_mostly iommu_intremap = 1; > >> bool_t __read_mostly iommu_crash_disable; > >> > >> +#define IOMMU_quarantine_none 0 > >> +#define IOMMU_quarantine_basic 1 > >> +#define IOMMU_quarantine_full 2 > >> +uint8_t __read_mostly iommu_quarantine = IOMMU_quarantine_basic; > > > > If we have 'IOMMU_quarantine_sink' instead of 'IOMMU_quarantine_full', > > then how about 'IOMMU_quarantine_write_fault' instead of > > 'IOMMU_quarantine_basic'? > > Why "write_fault"? Even in "full" mode you only avoid read faults > aiui (see also above). So if anything "write_fault" would be a > replacement for "full"; "basic" could be replaced by just "fault" > then.
Sorry, yes, I had things the wrong way round. "fault" and "write_fault" sound good. Paul > > Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel