Re: [v8][PATCH 02/17] introduce XEN_DOMCTL_set_rdm

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Mon, Dec 08, 2014 at 11:16:07AM +0800, Chen, Tiejun wrote:
> 
> On 2014/12/3 3:39, Konrad Rzeszutek Wilk wrote:
> >On Mon, Dec 01, 2014 at 05:24:20PM +0800, Tiejun Chen wrote:
> >>This should be based on a new parameter globally, 'pci_rdmforce'.
> >>
> >>pci_rdmforce = 1 => Of course this should be 0 by default.
> >>
> >>'1' means we should force check to reserve all ranges. If failed
> >>VM wouldn't be created successfully. This also can give user a
> >>chance to work well with later hotplug, even if not a device
> >>assignment while creating VM.
> >>
> >>But we can override that by one specific pci device:
> >>
> >>pci = ['AA:BB.CC,rdmforce=0/1]
> >>
> >>But this 'rdmforce' should be 1 by default since obviously any
> >>passthrough device always need to do this. Actually no one really
> >>want to set as '0' so it may be unnecessary but I'd like to leave
> >>this as a potential approach.
> >>
> >>So this domctl provides an approach to control how to populate
> >>reserved device memory by tools.
> >>
> >>Note we always post a message to user about this once we owns
> >>RMRR.
> >>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>---
> >>  docs/man/xl.cfg.pod.5              |  6 +++++
> >>  docs/misc/vtd.txt                  | 15 ++++++++++++
> >>  tools/libxc/include/xenctrl.h      |  6 +++++
> >>  tools/libxc/xc_domain.c            | 28 +++++++++++++++++++++++
> >>  tools/libxl/libxl_create.c         |  3 +++
> >>  tools/libxl/libxl_dm.c             | 47 ++++++++++++++++++++++++++++++++++++++
> >>  tools/libxl/libxl_internal.h       |  4 ++++
> >>  tools/libxl/libxl_types.idl        |  2 ++
> >>  tools/libxl/libxlu_pci.c           |  2 ++
> >>  tools/libxl/xl_cmdimpl.c           | 10 ++++++++
> >
> >In the past we had split the hypervisor and the
> >toolstack patches in two. So that one could focus
> >on the hypervisor ones first, and then in another
> >patch on the toolstack.
> >
> 
> Yes.
> 
> >But perhaps this was intended to be in one patch?
> 
> This change also involve docs so its little bit harder to understand the
> whole page if we split this.
> 
> >
> >>  xen/drivers/passthrough/pci.c      | 39 +++++++++++++++++++++++++++++++
> >>  xen/drivers/passthrough/vtd/dmar.c |  8 +++++++
> >>  xen/include/asm-x86/hvm/domain.h   |  4 ++++
> >
> >I don't see ARM here? Should there be an ARM variant of this? If not
> 
> ARM doesn't need this feature.
> 
> >should the toolstack ones only run under x86?
> 
> And I think this shouldn't broken current ARM path as well. I mean this
> would return simply since ARM hasn't such a hypercall handler.
> 
> >
> >>  xen/include/public/domctl.h        | 21 +++++++++++++++++
> >>  xen/xsm/flask/hooks.c              |  1 +
> >>  15 files changed, 196 insertions(+)
> >>
> >>diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> >>index 622ea53..9adc41e 100644
> >>--- a/docs/man/xl.cfg.pod.5
> >>+++ b/docs/man/xl.cfg.pod.5
> >>@@ -645,6 +645,12 @@ dom0 without confirmation.  Please use with care.
> >>  D0-D3hot power management states for the PCI device. False (0) by
> >>  default.
> >>
> >>+=item B<rdmforce=BOOLEAN>
> >>+
> >>+(HVM/x86 only) Specifies that the VM would force to check and try to
> >
> >s/force/forced/
> 
> I guess you're saying 'be forced'.
> 
> >>+reserve all reserved device memory, like RMRR, associated to the PCI
> >>+device. False (0) by default.
> >
> >Not sure I understand. How would the VM be forced to do this? Or is
> >it that the hvmloader would force to do this? And if it fails (as you
> 
> Yes.
> 
> >say 'try') ? What then?
> 
> In most cases we can reserve these regions but if these RMRR regions overlap
> with some fixed range, like guest BIOS, we can't succeed in this case.
> 
> >
> >>+
> >>  =back
> >>
> >>  =back
> >>diff --git a/docs/misc/vtd.txt b/docs/misc/vtd.txt
> >>index 9af0e99..23544d5 100644
> >>--- a/docs/misc/vtd.txt
> >>+++ b/docs/misc/vtd.txt
> >>@@ -111,6 +111,21 @@ in the config file:
> >>  To override for a specific device:
> >>  	pci = [ '01:00.0,msitranslate=0', '03:00.0' ]
> >>
> >>+RDM, 'reserved device memory', for PCI Device Passthrough
> >>+---------------------------------------------------------
> >>+
> >>+The BIOS controls some devices in terms of some reginos of memory used for
> >
> >Could you elaborate what 'some devices' are? Network cards? GPUs? What
> >are the most commons ones.
> 
> Some legacy USB device to perform PS2 emulation, and GPU has a stolen memory
> as I remember.
> 
> >
> >s/reginos/regions/
> 
> Fixed.
> 
> >
> >And by regions you mean BAR regions?
> 
> No. I guess you want to know some background about RMRR :)
> 
> There's a good brief description in Linux:
> 
> What is RMRR?
> -------------
> 
> There are some devices the BIOS controls, for e.g USB devices to perform
> PS2 emulation. The regions of memory used for these devices are marked
> reserved in the e820 map. When we turn on DMA translation, DMA to those
> regions will fail. Hence BIOS uses RMRR to specify these regions along with
> devices that need to access these regions. OS is expected to setup
> unity mappings for these regions for these devices to access these regions.
> 
> >
> >>+these devices. This kind of region should be reserved before creating a VM
> >>+to make sure they are not occupied by RAM/MMIO to conflict, and also we can
> >
> >You said 'This' but here you are using the plural ' are'. IF you want it plural
> >it needs to be 'These regions'
> 
> Thanks for your correction.
> 
> >>+create necessary IOMMU table successfully.
> >>+
> >>+To enable this globally, add "pci_rdmforce" in the config file:
> >>+
> >>+	pci_rdmforce = 1         (default is 0)
> >
> >The guest config file? Or /etc/xen/xl.conf ?
> 
> The guest config file. Here I just follow something about 'pci_msitranslate'
> since they have that usage in common.
> 
> >
> >>+
> >>+Or just enable for a specific device:
> >>+	pci = [ '01:00.0,rdmforce=1', '03:00.0' ]
> >>+
> >>
> >>  Caveat on Conventional PCI Device Passthrough
> >>  ---------------------------------------------
> >>diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> 
> [snip]
> 
> >>--- 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.

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

> 
> >
> >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
being preempted you might need to call it again. Or if the third-party
toolstack use this operation and call this with wacky values?
> 
> >
> >
> >>+        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.
> 
> >
> >>+        d->arch.hvm_domain.num_pcidevs = xdsr->num_pcidevs;
> >
> >What if the 'num_pcidevs' has some bogus value. You need to check for that.
> 
> This value is grabbed from that existing interface, assign_device, so I mean
> this is already checked.
> 
> >
> >
> >>+        d->arch.hvm_domain.pcidevs = NULL;
> >
> >Please first free it. It might be that the toolstack
> >is doing this a couple of times. You don't want to leak memory.
> >
> 
> Okay,
> 
> +        if ( d->arch.hvm_domain.pcidevs )
> +            xfree(d->arch.hvm_domain.pcidevs);
> 
> >
> >>+
> >>+        if ( xdsr->num_pcidevs )
> >>+        {
> >>+            pcidevs = xmalloc_array(xen_guest_pcidev_info_t,
> >>+                                    xdsr->num_pcidevs);
> >>+            if ( pcidevs == NULL )
> >>+            {
> >>+                rcu_unlock_domain(d);
> >>+                return -ENOMEM;
> >
> >But you already have set 'num_pcidevs' to some value. This copying/check
> >should be done before you modify 'd->arch.hvm_domain'...
> 
> This makes sense so I'll move down this fragment.
> 
> >>+            }
> >>+
> >>+            if ( copy_from_guest(pcidevs, xdsr->pcidevs,
> >>+                                 xdsr->num_pcidevs*sizeof(*pcidevs)) )
> >>+            {
> >>+                xfree(pcidevs);
> >>+                rcu_unlock_domain(d);
> >
> >Ditto. You need to do these checks before you modify 'd->arch.hvm_domain'.
> >
> >>+                return -EFAULT;
> >>+            }
> >>+        }
> >>+
> >>+        d->arch.hvm_domain.pcidevs = pcidevs;
> >>+        rcu_unlock_domain(d);
> >>+    }
> >>+        break;
> >>+
> >>      case XEN_DOMCTL_assign_device:
> >>          if ( unlikely(d->is_dying) )
> >>          {
> >>diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
> >>index 1152c3a..5e41e7a 100644
> >>--- a/xen/drivers/passthrough/vtd/dmar.c
> >>+++ b/xen/drivers/passthrough/vtd/dmar.c
> >>@@ -674,6 +674,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
> >>                          "  RMRR region: base_addr %"PRIx64
> >>                          " end_address %"PRIx64"\n",
> >>                          rmrru->base_address, rmrru->end_address);
> >>+            /*
> >>+             * TODO: we may provide a precise paramter just to reserve
> >
> >s/paramter/parameter/
> 
> Fixed.
> 
> >>+             * RMRR range specific to one device.
> >>+             */
> >>+            dprintk(XENLOG_WARNING VTDPREFIX,
> >>+                    "So please set pci_rdmforce to reserve these ranges"
> >>+                    " if you need such a device in hotplug case.\n");
> 
> s/hotplug/passthrough
> 
> >
> >'Please set rdmforce to reserve ranges %lx->%lx if you plan to hotplug this device.'
> >
> >But then this is going to be a bit verbose, so perhaps:
> >
> >'Ranges %lx-%lx need rdmforce to properly work.' ?
> 
> Its unnecessary to output range again since we already have such a print
> message here.
> 
> >
> >>+
> >>              acpi_register_rmrr_unit(rmrru);
> >>          }
> >>      }
> >>diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> >>index 2757c7f..38530e5 100644
> >>--- a/xen/include/asm-x86/hvm/domain.h
> >>+++ b/xen/include/asm-x86/hvm/domain.h
> >>@@ -90,6 +90,10 @@ struct hvm_domain {
> >>      /* Cached CF8 for guest PCI config cycles */
> >>      uint32_t                pci_cf8;
> >>
> >
> >Maybe a comment explaining its purpose?
> 
> Okay.
> 
> /* Force to check/reserve Reserved Device Memory. */
>     bool_t                  pci_force;
> 
> >
> >>+    bool_t                  pci_force;
> >>+    uint32_t                num_pcidevs;
> >>+    struct xen_guest_pcidev_info      *pcidevs;
> >>+
> >
> >You are also missing freeing of this in the hypervisor when the guest
> >is destroyed. Please fix that.
> 
> You're right. I will go there next revision.
> 
> >
> >>      struct pl_time         pl_time;
> >>
> >>      struct hvm_io_handler *io_handler;
> >>diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> >>index 57e2ed7..ba8970d 100644
> >>--- a/xen/include/public/domctl.h
> >>+++ b/xen/include/public/domctl.h
> >>@@ -508,6 +508,25 @@ struct xen_domctl_get_device_group {
> >>  typedef struct xen_domctl_get_device_group xen_domctl_get_device_group_t;
> >>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_get_device_group_t);
> >>
> >>+/* Currently just one bit to indicate force to check Reserved Device Memory. */
> >
> >Not sure I understand. Did you mean:
> >
> >'Check Reserved Device Memory'.
> 
> I can change this as '...force checking Reserved Device Memory.'
> 
> >
> >What happens if you do not have this flag? What are the semantics
> >of this hypercall - as in what will it mean.
> 
> If we have no this flag, these devices owned RMRR can't work in passthrough
> case.
> 
> >
> >>+#define PCI_DEV_RDM_CHECK   0x1
> >>+struct xen_guest_pcidev_info {
> >>+    uint16_t    seg;
> >>+    uint8_t     bus;
> >>+    uint8_t     devfn;
> >>+    uint32_t    flags;
> >>+};
> >>+typedef struct xen_guest_pcidev_info xen_guest_pcidev_info_t;
> >>+DEFINE_XEN_GUEST_HANDLE(xen_guest_pcidev_info_t);
> >>+/* Control whether/how we check and reserve device memory. */
> >>+struct xen_domctl_set_rdm {
> >>+    uint32_t    flags;
> >
> >What is this 'flags' purpose compared to the 'pcidevs.flags'? Please
> >explain.
> 
> I replied something to Kevin, and we just need a global flag so we can
> remove pcidevs.flags.
> 
> >
> >>+    uint32_t    num_pcidevs;
> >>+    XEN_GUEST_HANDLE_64(xen_guest_pcidev_info_t) pcidevs;
> >>+};
> >>+typedef struct xen_domctl_set_rdm xen_domctl_set_rdm_t;
> >>+DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_rdm_t);
> >>+
> >>  /* Pass-through interrupts: bind real irq -> hvm devfn. */
> >>  /* XEN_DOMCTL_bind_pt_irq */
> >>  /* XEN_DOMCTL_unbind_pt_irq */
> >>@@ -1070,6 +1089,7 @@ struct xen_domctl {
> >>  #define XEN_DOMCTL_setvnumainfo                  74
> >>  #define XEN_DOMCTL_psr_cmt_op                    75
> >>  #define XEN_DOMCTL_arm_configure_domain          76
> >>+#define XEN_DOMCTL_set_rdm                       77
> >>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
> >>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
> >>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> >>@@ -1135,6 +1155,7 @@ struct xen_domctl {
> >>          struct xen_domctl_gdbsx_domstatus   gdbsx_domstatus;
> >>          struct xen_domctl_vnuma             vnuma;
> >>          struct xen_domctl_psr_cmt_op        psr_cmt_op;
> >>+        struct xen_domctl_set_rdm           set_rdm;
> >>          uint8_t                             pad[128];
> >>      } u;
> >>  };
> >>diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> >>index d48463f..5a760e2 100644
> >>--- a/xen/xsm/flask/hooks.c
> >>+++ b/xen/xsm/flask/hooks.c
> >>@@ -592,6 +592,7 @@ static int flask_domctl(struct domain *d, int cmd)
> >>      case XEN_DOMCTL_test_assign_device:
> >>      case XEN_DOMCTL_assign_device:
> >>      case XEN_DOMCTL_deassign_device:
> >>+    case XEN_DOMCTL_set_rdm:
> >
> >There is more to XSM than just this file..
> 
> But I don't see more other stuff, like XEN_DOMCTL_assign_device.
> 
> >
> >Please compile with XSM enabled.
> 
> Anyway, I add XSM_ENABLE = y and FLASK_ENABLE = y in Config.mk then
> recompile, but looks good.
> 
> Anything I'm missing?

Ah, then it is fine!

> 
> >>  #endif
> >>          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.
> 
> Thanks
> Tiejun