Re: [Xen-devel] [v8][PATCH 02/17] introduce XEN_DOMCTL_set_rdm

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

> > > > --- a/xen/drivers/passthrough/pci.c
> > > > +++ b/xen/drivers/passthrough/pci.c
> > > > @@ -34,6 +34,7 @@
> > > >   #include <xen/tasklet.h>
> > > >   #include <xsm/xsm.h>
> > > >   #include <asm/msi.h>
> > > > +#include <xen/stdbool.h>
> > > >
> > > >   struct pci_seg {
> > > >       struct list_head alldevs_list;
> > > > @@ -1553,6 +1554,44 @@ int iommu_do_pci_domctl(
> > > >           }
> > > >           break;
> > > >
> > > > +    case XEN_DOMCTL_set_rdm:
> > > > +    {
> > > > +        struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm;
> > > > +        struct xen_guest_pcidev_info *pcidevs = NULL;
> > > > +        struct domain *d = rcu_lock_domain_by_any_id(domctl->domain);
> > > > +
> > > > +        if ( d == NULL )
> > > > +            return -ESRCH;
> > > > +
> > > What if this is called on an PV domain?
> > Currently we just support this in HVM, so I'd like to add this,
> >
> >           if ( d == NULL )
> >               return -ESRCH;
> >
> > +        ASSERT( is_hvm_domain(d) );
> > +
> No. Please don't crash the hypervisor.
Okay.

> Just return -ENOSYS or such when done for PV guests.
So,

+        if ( !is_hvm_domain(d) )
+            return -ENOSYS;

> > > You are also missing the XSM checks.
> > Just see this below.
> >
> > > What if this is called multiple times. Is it OK to over-ride
> > > the 'pci_force' or should it stick once?
> > It should be fine since just xc/hvmloader need such an information while
> > creating a VM.
> >
> > And especially, currently we just call this one time to set. So why we need
> > to call this again and again? I think if anyone want to extend such a case
> > you're worrying, he should know any effect before he take a action, right?
> Program defensively and also think about preemption. If this call end up
Do you think we need a fine grain way, like lock here?

> being preempted you might need to call it again. Or if the third-party
> toolstack use this operation and call this with wacky values?
Maybe can the following address this enough,

    case XEN_DOMCTL_set_rdm:
    {
        struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm;
        struct xen_guest_pcidev_info *pcidevs = NULL;

        if ( d->arch.hvm_domain.pcidevs )
            break;

        if ( !is_hvm_domain(d) )
            return -ENOSYS;
        ...

> > > > +        d->arch.hvm_domain.pci_force =
> > > > +                            xdsr->flags & PCI_DEV_RDM_CHECK ? true : false;
> > > Won't we crash here if this is called for PV guests?
> > After the line, 'ASSERT( is_hvm_domain(d) );', is added, this problem should
> > be gone.
> No it won't be. You will just crash the hypervisor.
>
> Please please put yourself in the mind that the toolstack can (and will)
> have bugs.
Thanks for your reminder.

> > > > +        d->arch.hvm_domain.num_pcidevs = xdsr->num_pcidevs;
> > > What if the 'num_pcidevs' has some bogus value. You need to check for that.
[snip]

> > > >           return 0;
> > >
> > > Also how does this work with 32-bit dom0s? Is there a need to use the
> > > compat layer?
> > Are you saying in xsm case? Others?
> >
> > Actually this new DOMCTL is similar with XEN_DOMCTL_assign_device in some
> > senses but I don't see such an issue you're pointing.
> I was thinking about the compat layer and making sure it works properly.
Do we really need this consideration? I mean I referred to that existing 
XEN_DOMCTL_assign_device to implement this new DOMCTL, but looks there's 
nothing related to this point.
Or could you make your thought clear to me with an exiting example? Then 
I can take a look at what exactly should be taken in my new DOMCTL since 
I'm a fresh man to work out this properly inside xen.
Thanks
Tiejun

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