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

[Paul Durrant <xadimgnik@gmail.com>]

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