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

[Scott Davis <scott.davis@xxxxxxxxxx>]

Thanks for the feedback, Jan!

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?

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 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?

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. 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?

>> Background: I am setting up a QEMU-based development and testing environment 
>> for
>> the Crucible team at Star Lab that includes emulated PCIe devices for
>> passthrough and hotplug. I encountered an issue with `xl pci-assignable-add`
>> that causes the host QEMU to rapidly allocate memory until getting 
>> OOM-killed.
>> I then found that the issue could be worked around either by using manual 
>> sysfs
>> commands to rebind devices to pciback or by skipping over the quarantine 
>> logic
>> in `libxl__device_pci_assignable_add`, producing a working system. I hoped 
>> that
>> setting iommu=no-quarantine on the command line would have the same effect, 
>> only
>> to be surprised that it did not.
>
> ... some of this "why do we want this" would belong in the commit message
> imo, not just here.

Thanks for the tip, I will include this info in commit message in any future 
revisions.

Good day,
Scott