Re: [Xen-devel] [PATCH v8 2/7] VMX: Properly handle pi when all the assigned devices are removed

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.11.16 at 06:22, <feng.wu@xxxxxxxxx> wrote:
>> From: Tian, Kevin
>> Sent: Friday, November 18, 2016 12:59 PM
>> > From: Wu, Feng
>> > Sent: Friday, November 18, 2016 12:27 PM
>> > > > diff --git a/xen/drivers/passthrough/pci.c 
>> > > > b/xen/drivers/passthrough/pci.c
>> > > > index 8bce213..e71732f 100644
>> > > > --- a/xen/drivers/passthrough/pci.c
>> > > > +++ b/xen/drivers/passthrough/pci.c
>> > > > @@ -1602,6 +1602,13 @@ int iommu_do_pci_domctl(
>> > > >          break;
>> > > >
>> > > >      case XEN_DOMCTL_assign_device:
>> > > > +        /* no domain_pause() */
>> > > > +        if ( d == current->domain )
>> > > > +        {
>> > > > +            ret = -EINVAL;
>> > > > +            break;
>> > > > +        }
>> > > > +
>> > >
>> > > don't understand why adding above check, and why "no domain_pause"
>> > > matters in this change.
>> >
>> > In fact, this change is according Jan's following comments on v6:
>> >
>> > " There's one additional caveat here which no-one of us so far thought
>> > of: Currently there's nothing preventing the domctl-s under which
>> > this sits from being issued by the control domain for itself. Various
>> > other domctl-s, however, guard against this case when intending
>> > to pause the target domain. The same needs to be done for the
>> > ones leading here."
>> >
>> > We need to prevent the domain from pausing itself.
>> >
>> 
>> XEN_DOMCTL_assign_device doesn't imply a domain_pause operation,
>> at least not obvious in this level. If we don't have PI enabled underneath,
>> is above guard still necessary? If the answer is yes, the comment should
>> be elaborated for easy understanding... :-)
> 
> I think the domain_pause() is introduced in PI related logic. So maybe I
> need Jan's comments about whether we need to add this check unconditionally
> here. Anyway, the comments need to be more detailed.

I've never said anything about the specifics of the check (e.g.
whether it needs to be unconditional), I had only pointed out that
such a check is needed if you're adding pausing. That said, I don't
think there's anything wrong with having it unconditional: In the
end we don't mean to support self-(de)assignment of devices.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel