Re: [RFC PATCH] iommu: make no-quarantine mean no-quarantine

[Paul Durrant <xadimgnik@xxxxxxxxx>]

On 28/04/2021 07:15, Jan Beulich wrote:
> On 28.04.2021 00:00, Scott Davis wrote:
> > On 4/27/21, 2:56 AM, Jan Beulich wrote:
> > > On 26.04.2021 19:25, Scott Davis wrote:
> > > > This patch modifies Xen's behavior when making devices assignable while the
> > > > iommu=no-quarantine command line option is in effect. Currently this option
> > > > only affects device deassignment, causing devices to get immediately assigned
> > > > back to Dom0 instead of to the quarantine dom_io domain. This patch extends
> > > > no-quarantine to device assignment as well, preventing devices from being
> > > > assigned to dom_io when they are made assignable while no-quarantine is in
> > > > effect.
> > > Well, the term "quarantine" to me means a safety action taken _after_
> > > possible exposure to something "bad". Therefore I see this being specific
> > > to device de-assignment as the logical thing. Hence if a mode like what
> > > you describe was wanted, I don't think it should be the result of
> > > "iommu=no-quarantine".
> > Sorry I'm a bit confused by this. Correct me if wrong, but my understanding is
> > that the purpose of assigning a device to dom_io on de-assignment is to protect
> > Dom0 from the effects of in-flight DMA operations initiated by a DomU. I assumed
> > that the purpose of (temporarily) assigning to dom_io on assignment was the same
> > but in reverse: protecting a DomU from Dom0-initiated ops. Is this not the case?
> Well, no, not really. Dom0 is considered fully trusted for a variety of
> reasons.
>
> > Also, documentation and code already refer to the operation in question as a
> > "quarantine" (see xl command line docs and libxl__device_pci_assignable_add)
> > and to devices that have undergone it as being "quarantined" (see assign_device
> > in xen/drivers/passthrough/pci.c). So if that is not the correct term, there may
> > be some additional changes needed for consistency. Is there another name that
> > would be more appropriate?
> I don't see what's wrong with the term for how things are currently. If
> you talk about an adjustment to terminology to accompany your proposed
> change - not sure.
>
> > I would also point out that, currently, there does not appear to be a way for an
> > xl user to opt out of quarantine functionality in either direction other than by
> > manually making devices assignable. There is no xl command line flag to disable
> > it and iommu=no-quarantine will have no effect because any device that xl itself
> > makes assignable will have the struct pci_dev.quarantine flag set, which
> > overrides iommu=no-quarantine. Is that intentional?
> Not sure here either: It may also have been that it was assumed to not
> be of interest. Paul?
TBH I'm not sure. When I implemented quarantining it was non-optional 
for good reason and no-quarantine came along later (and somewhat 
hurriedly IIRC).
> > If I misunderstood and your objection is simply that "quarantine-on-assignment"
> > and "quarantine-on-deassignment" should be controllable by separate iommu
> > options, that's an easy enough change to make.
> Yes, effectively it's that what I think things would want to be, if
> "quarantine-on-assignment" is really something that we think is needed.
> It would default to off imo.
>
> > Although I think that might also
> > negate the need for/effect of struct pci_dev.quarantine as described above. If
> > that's what is desired, any thoughts on what the new option(s) should be called?
> Following the extension to the command line option I'm putting in place
> in "IOMMU: make DMA containment of quarantined devices optional" (which
> I still need to get around to address review feedback for and resubmit),
> I'd be inclined to suggest "iommu=quarantine=always" or
> "iommu=quarantine=on-assign". Unless of course we'd prefer to have the
> caller of the assignment operation have full control over the behavior
> here anyway (in which case a command line option control simply is not
> necessary).
I'm still not entirely sure why not quarantining on is a problem, other 
than it triggering an as-yet undiagnosed issue in QEMU, but I agree that 
that the expectation of 'no-quarantine' meaning just that (i.e. the old 
dom0->domU and domU->dom0 transitions are re-instated) is reasonable. Do 
we really want yet more command line options?
  Paul