[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.