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

Re: [RFC v1 5/5] xen/arm: add SCI mediator support for DomUs


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>
  • From: Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>
  • Date: Wed, 22 Dec 2021 13:41:38 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=epam.com; dmarc=pass action=none header.from=epam.com; dkim=pass header.d=epam.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=R4Q3znZPg3b8lhI6948LAFtsV5cw9WrZs6RZfilHXas=; b=I7Jz4zfXhnf4CM2MayX8Qo5QF2l5qedGyveoW2a/zggADabvVBFhssn0nJjh/0SQl8gv2lXIzOItc7EEDAi1DG6Xe6G0T8e86tBjMUqn894m9uFTBrFnBN2u3ayIzCnLiXkLKLMc2VnZJ1OD0/IGi+5DABKoQPXmwoGwcZA3wIwTditA4Cap+5Lm+JqZDs2ogwd4HO0maZz19uZRrGPsdlC15pff0TupBmWa8zfW3jF48p+y5Y6T0OX6+PuuHhEEhQXdF6gdQNuliC+XCotwGUZ6eQXLMnMbODPGcL1/JlDHdgyNzT1HvPakcQ/K9Ti7q8jGnTHlOkLO51ECD1bOZw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kf4Z5+G5S17EiSKPgSXH4B9eKLEfCpjPYFZL0hg8L4A3NoYM1pfl/P6qC1+j51z580Kbb8TRKxCtky/ttQtyMB5fUxONVvQSLkdyn5JDvur+M3BY7KdgJsMNxGD0EywY/hKn7Etg5tPgs3SxWbsnfOUYrd86Lvc8/w4e5VYbMnmq+j/eQDtfkbwV4+gqZjhVv1e2UPoWNSsBAIkKV4sZWx/5SgAJHbJNtu9q73o6/+UEOeKze0wjZ+eZ+p41iJwcXi7xIhXvXY5gh43smFWbAe3VzKwsXUHtqOUcYkKxRH9eG4e+IznkEXcVXnjBXUu/pTGcCf080/uaDJgaCEJ3lA==
  • Cc: "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Juergen Gross <jgross@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>, Julien Grall <julien@xxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>
  • Delivery-date: Wed, 22 Dec 2021 13:42:14 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Thread-index: AQHX8M3LpzWz4l8PQUiIA2nAUV0H+qw8NR4AgAJcjoA=
  • Thread-topic: [RFC v1 5/5] xen/arm: add SCI mediator support for DomUs

Hi Stefano, 

On Mon, Dec 20, 2021 at 05:37:50PM -0800, Stefano Stabellini wrote:
> On Tue, 14 Dec 2021, Oleksii Moisieiev wrote:
> > Integration of the SCMI mediator with xen libs:
> > - add hypercalls to aquire SCI channel and set device permissions
> > for DomUs;
> > - add SCMI_SMC nodes to DomUs device-tree based on partial device-tree;
> > - SCI requests redirection from DomUs to Firmware.
> > 
> > Signed-off-by: Oleksii Moisieiev <oleksii_moisieiev@xxxxxxxx>
> > ---
> >  tools/include/xenctrl.h           |   3 +
> >  tools/include/xenguest.h          |   2 +
> >  tools/libs/ctrl/xc_domain.c       |  23 ++++++
> >  tools/libs/guest/xg_dom_arm.c     |   5 +-
> >  tools/libs/light/libxl_arm.c      | 122 +++++++++++++++++++++++++++---
> >  tools/libs/light/libxl_create.c   |  54 ++++++++++++-
> >  tools/libs/light/libxl_dom.c      |   1 +
> >  tools/libs/light/libxl_internal.h |   4 +
> >  xen/arch/arm/domctl.c             |  15 ++++
> >  xen/include/public/domctl.h       |   9 +++
> >  10 files changed, 223 insertions(+), 15 deletions(-)
> > 
> > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > index 07b96e6671..cdd14f465f 100644
> > --- a/tools/include/xenctrl.h
> > +++ b/tools/include/xenctrl.h
> > @@ -1238,6 +1238,9 @@ int xc_domain_getvnuma(xc_interface *xch,
> >  int xc_domain_soft_reset(xc_interface *xch,
> >                           uint32_t domid);
> >  
> > +int xc_domain_add_sci_device(xc_interface *xch,
> > +                              uint32_t domid, char *path);
> > +
> >  #if defined(__i386__) || defined(__x86_64__)
> >  /*
> >   * PC BIOS standard E820 types and structure.
> > diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> > index 61d0a82f48..35c611ac73 100644
> > --- a/tools/include/xenguest.h
> > +++ b/tools/include/xenguest.h
> > @@ -242,6 +242,8 @@ struct xc_dom_image {
> >  
> >      /* Number of vCPUs */
> >      unsigned int max_vcpus;
> > +
> > +    xen_pfn_t sci_shmem_gfn;
> 
> We should be able to avoid introducing sci_shmem_gfn (more below)
> 
> 
> >  };
> >  
> >  /* --- arch specific hooks ----------------------------------------- */
> > diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> > index b155d6afd2..07bb390e17 100644
> > --- a/tools/libs/ctrl/xc_domain.c
> > +++ b/tools/libs/ctrl/xc_domain.c
> > @@ -2206,6 +2206,29 @@ int xc_domain_soft_reset(xc_interface *xch,
> >      domctl.domain = domid;
> >      return do_domctl(xch, &domctl);
> >  }
> > +
> > +int xc_domain_add_sci_device(xc_interface *xch,
> > +                              uint32_t domid, char *path)
> > +{
> > +    size_t size = strlen(path);
> > +    int rc;
> > +    DECLARE_DOMCTL;
> > +    DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> > +
> > +    if ( xc_hypercall_bounce_pre(xch, path) )
> > +        return -1;
> > +
> > +    domctl.cmd = XEN_DOMCTL_add_sci_device;
> > +    domctl.domain = domid;
> > +    domctl.u.sci_device_op.size = size;
> > +    set_xen_guest_handle(domctl.u.sci_device_op.path, path);
> > +    rc = do_domctl(xch, &domctl);
> > +
> > +    xc_hypercall_bounce_post(xch, path);
> > +
> > +    return rc;
> > +}
> 
> I'd skip this xc_ function and hypercall (more below)
> 
> 
> >  /*
> >   * Local variables:
> >   * mode: C
> > diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
> > index 5e3b76355e..368a670c46 100644
> > --- a/tools/libs/guest/xg_dom_arm.c
> > +++ b/tools/libs/guest/xg_dom_arm.c
> > @@ -25,11 +25,12 @@
> >  
> >  #include "xg_private.h"
> >  
> > -#define NR_MAGIC_PAGES 4
> > +#define NR_MAGIC_PAGES 5
> >  #define CONSOLE_PFN_OFFSET 0
> >  #define XENSTORE_PFN_OFFSET 1
> >  #define MEMACCESS_PFN_OFFSET 2
> >  #define VUART_PFN_OFFSET 3
> > +#define SCI_SHMEM_OFFSET 4
> >  
> >  #define LPAE_SHIFT 9
> >  
> > @@ -69,11 +70,13 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
> >      dom->console_pfn = base + CONSOLE_PFN_OFFSET;
> >      dom->xenstore_pfn = base + XENSTORE_PFN_OFFSET;
> >      dom->vuart_gfn = base + VUART_PFN_OFFSET;
> > +    dom->sci_shmem_gfn = base + SCI_SHMEM_OFFSET;
> >  
> >      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
> >      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
> >      xc_clear_domain_page(dom->xch, dom->guest_domid, base + 
> > MEMACCESS_PFN_OFFSET);
> >      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
> > +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->sci_shmem_gfn);
> 
> Given that sci_shmem_gfn doesn't actually require any allocations, just
> a remapping of an existing and already specified physical page, then I
> don't think we need to add a new page to alloc_magic_pages for it.
> 
> We can just #define a static address for the SCMI page in the domU
> addres space and use it for the mapping. No need to allocate a new
> page.
> 

I think this could be implemented. I will refactor this part in v2.

> 
> >      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
> >              dom->console_pfn);
> > diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> > index eef1de0939..73ef591822 100644
> > --- a/tools/libs/light/libxl_arm.c
> > +++ b/tools/libs/light/libxl_arm.c
> > @@ -8,6 +8,8 @@
> >  #include <assert.h>
> >  #include <xen/device_tree_defs.h>
> >  
> > +#define SCMI_NODE_PATH      "/firmware/scmi"
> > +
> >  static const char *gicv_to_string(libxl_gic_version gic_version)
> >  {
> >      switch (gic_version) {
> > @@ -101,6 +103,19 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
> >          return ERROR_FAIL;
> >      }
> >  
> > +    switch (d_config->b_info.sci) {
> > +    case LIBXL_SCI_TYPE_NONE:
> > +        config->arch.sci_type = XEN_DOMCTL_CONFIG_SCI_NONE;
> > +        break;
> > +    case LIBXL_SCI_TYPE_SCMI_SMC:
> > +        config->arch.sci_type = XEN_DOMCTL_CONFIG_SCI_SCMI_SMC;
> > +        break;
> > +    default:
> > +        LOG(ERROR, "Unknown SCI type %d",
> > +            d_config->b_info.sci);
> > +        return ERROR_FAIL;
> > +    }
> > +
> >      return 0;
> >  }
> >  
> > @@ -122,6 +137,7 @@ int libxl__arch_domain_save_config(libxl__gc *gc,
> >      }
> >  
> >      state->clock_frequency = config->arch.clock_frequency;
> > +    state->sci_agent_paddr = config->arch.sci_agent_paddr;
> >  
> >      return 0;
> >  }
> > @@ -502,9 +518,6 @@ static int make_optee_node(libxl__gc *gc, void *fdt)
> >      int res;
> >      LOG(DEBUG, "Creating OP-TEE node in dtb");
> >  
> > -    res = fdt_begin_node(fdt, "firmware");
> > -    if (res) return res;
> > -
> >      res = fdt_begin_node(fdt, "optee");
> >      if (res) return res;
> >  
> > @@ -517,9 +530,6 @@ static int make_optee_node(libxl__gc *gc, void *fdt)
> >      res = fdt_end_node(fdt);
> >      if (res) return res;
> >  
> > -    res = fdt_end_node(fdt);
> > -    if (res) return res;
> > -
> >      return 0;
> >  }
> >  
> > @@ -902,10 +912,9 @@ static int copy_node(libxl__gc *gc, void *fdt, void 
> > *pfdt,
> >      return 0;
> >  }
> >  
> > -static int copy_node_by_path(libxl__gc *gc, const char *path,
> > -                             void *fdt, void *pfdt)
> > +static int get_path_nodeoff(const char *path, void *pfdt)
> >  {
> > -    int nodeoff, r;
> > +    int nodeoff;
> >      const char *name = strrchr(path, '/');
> >  
> >      if (!name)
> > @@ -925,12 +934,101 @@ static int copy_node_by_path(libxl__gc *gc, const 
> > char *path,
> >      if (strcmp(fdt_get_name(pfdt, nodeoff, NULL), name))
> >          return -FDT_ERR_NOTFOUND;
> >  
> > +    return nodeoff;
> > +}
> > +
> > +static int copy_node_by_path(libxl__gc *gc, const char *path,
> > +                             void *fdt, void *pfdt)
> > +{
> > +    int nodeoff, r;
> > +
> > +    nodeoff = get_path_nodeoff(path, pfdt);
> > +    if (nodeoff < 0)
> > +        return nodeoff;
> > +
> >      r = copy_node(gc, fdt, pfdt, nodeoff, 0);
> >      if (r) return r;
> >  
> >      return 0;
> >  }
> >  
> > +static int get_node_phandle(const char *path, void *pfdt, uint32_t 
> > *phandle)
> > +{
> > +    int nodeoff;
> > +    const char *name = strrchr(path, '/');
> > +
> > +    if (!name)
> > +        return -FDT_ERR_INTERNAL;
> > +
> > +    name++;
> > +    nodeoff = fdt_path_offset(pfdt, path);
> > +    if (nodeoff < 0)
> > +        return nodeoff;
> > +
> > +    *phandle = fdt_get_phandle(pfdt, nodeoff);
> > +    return 0;
> > +}
> > +
> > +static int make_scmi_shmem_node(libxl__gc *gc, void *fdt, void *pfdt,
> > +                           struct xc_dom_image *dom)
> > +{
> > +    int res;
> > +    char buf[64];
> > +    uint32_t phandle = 0;
> > +
> > +    res = get_node_phandle("/scp-shmem", pfdt, &phandle);
> > +    if (res) return res;
> 
> I hope we'll be able to avoid requiring the user to write a partial
> device tree with SCMI info in order to use it.

I assuming that all SCMI related data is already present in partial
device-tree. Because all scmi nodes should be added to the original
platform device tree, which is used to build domu and dom0 dtbs.
So the structure should look the following way, taken h3ulcb as an
example;

r8a77951-scmi.dtsi - describe all scmi nodes and update
clock/power/reset etc in the device nodes;
 |
\ /
included in r8a77951-ulcb.dts - device tree for rcar h3ulcb board
 |
\ /
included in r8a77951-ulcb-xen.dts - xen dts files;
included in r8a77951-ulcb-domu.dts - domu dts file.

So the idea is that scmi configuration should be applied for both
virtualized and non-virtualized systems.

That's why I copy nodes from partial device-tree to domain device-tree.
Another advantage of this approach is that user can configure scmi node
for each domain by add/remove protocols. For example only clock and
resets could work through scmi for DomX. This can easily be configured
when using partial device-tree.

> 
> But if we have to go down this route, then we need to add a description
> of scp-shmem under docs/misc/arm/
> 
> Also, in general, we cannot mandate or require specific paths in device
> tree and instead we should find nodes based on the compatible string.
> (There are exceptions like /reserved-memory and /firmware but they are
> only a couple.)
> 

I agree about the specific path. I will make it to use compatible string
in v2.

> 
> > +    snprintf(buf, sizeof(buf), "scp-shmem@%lx",
> > +             dom->sci_shmem_gfn << XC_PAGE_SHIFT);
> > +    res = fdt_begin_node(fdt, buf);
> > +    if (res) return res;
> > +
> > +    res = fdt_property_compat(gc, fdt, 1, "arm,scmi-shmem");
> > +    if (res) return res;
> > +
> > +    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> > +                    GUEST_ROOT_SIZE_CELLS, 1,
> > +                    dom->sci_shmem_gfn << XC_PAGE_SHIFT, XC_PAGE_SHIFT);
> > +    if (res) return res;
> > +
> > +    LOG(DEBUG, "scmi: setting phandle = %u\n", phandle);
> > +    res = fdt_property_cell(fdt, "phandle", phandle);
> > +    if (res) return res;
> > +
> > +    res = fdt_end_node(fdt);
> > +    if (res) return res;
> > +
> > +    return 0;
> > +}
> > +
> > +static int make_firmware_node(libxl__gc *gc, void *fdt, void *pfdt, int 
> > tee,
> > +                              int sci)
> > +{
> > +    int res;
> > +
> > +    if ((tee != LIBXL_TEE_TYPE_OPTEE) && (sci != LIBXL_SCI_TYPE_NONE))
> > +        return 0;
> 
> Shouldn't this be:
> 
>     if ((tee != LIBXL_TEE_TYPE_OPTEE) && (sci == LIBXL_SCI_TYPE_NONE))
> 
Yeah, I should fix this in v2. Thanks.

> 
> > +    res = fdt_begin_node(fdt, "firmware");
> > +    if (res) return res;
> > +
> > +    if (tee == LIBXL_TEE_TYPE_OPTEE) {
> > +       res = make_optee_node(gc, fdt);
> > +       if (res) return res;
> > +    }
> > +
> > +    if (sci == LIBXL_SCI_TYPE_SCMI_SMC) {
> > +        res = copy_node_by_path(gc, SCMI_NODE_PATH, fdt, pfdt);
> > +        if (res) return res;
> > +    }
> 
> Do we really need to copy the node from the partial device tree? That
> makes it a lot harder to use for the user. Ideally a user would only
> need to specify sci = "scmi_smc" and everything else should be done
> automatically.
> 
> Can we automatically generate the SCMI device tree node instead? It
> looks like we should have all the information to be able to do it. If
> not, what is missing?
> 
Please see the answer above.
> 
> > +    res = fdt_end_node(fdt);
> > +    if (res) return res;
> > +
> > +    return 0;
> > +}
> > +
> >  /*
> >   * The partial device tree is not copied entirely. Only the relevant bits 
> > are
> >   * copied to the guest device tree:
> > @@ -1088,8 +1186,10 @@ next_resize:
> >          if (info->arch_arm.vuart == LIBXL_VUART_TYPE_SBSA_UART)
> >              FDT( make_vpl011_uart_node(gc, fdt, ainfo, dom) );
> >  
> > -        if (info->tee == LIBXL_TEE_TYPE_OPTEE)
> > -            FDT( make_optee_node(gc, fdt) );
> > +        FDT( make_firmware_node(gc, fdt, pfdt, info->tee, info->sci) );
> > +
> > +        if (info->sci == LIBXL_SCI_TYPE_SCMI_SMC)
> > +            FDT( make_scmi_shmem_node(gc, fdt, pfdt, dom) );
> >  
> >          if (d_config->num_pcidevs)
> >              FDT( make_vpci_node(gc, fdt, ainfo, dom) );
> > diff --git a/tools/libs/light/libxl_create.c 
> > b/tools/libs/light/libxl_create.c
> > index dcd09d32ba..c7372fd344 100644
> > --- a/tools/libs/light/libxl_create.c
> > +++ b/tools/libs/light/libxl_create.c
> > @@ -596,6 +596,37 @@ out:
> >      return ret;
> >  }
> >  
> > +static int map_sci_page(libxl__gc *gc, uint32_t domid, uint64_t paddr,
> > +                         uint64_t guest_addr)
> > +{
> > +    int ret;
> > +    uint64_t _paddr_pfn = paddr >> XC_PAGE_SHIFT;
> > +    uint64_t _guest_pfn = guest_addr >> XC_PAGE_SHIFT;
> > +
> > +    LOG(DEBUG, "iomem %"PRIx64, _paddr_pfn);
> > +
> > +    ret = xc_domain_iomem_permission(CTX->xch, domid, _paddr_pfn, 1, 1);
> > +    if (ret < 0) {
> > +        LOG(ERROR,
> > +              "failed give domain access to iomem page %"PRIx64,
> > +             _paddr_pfn);
> > +        return ret;
> > +    }
> > +
> > +    ret = xc_domain_memory_mapping(CTX->xch, domid,
> > +                                   _guest_pfn, _paddr_pfn,
> > +                                   1, 1);
> > +    if (ret < 0) {
> > +        LOG(ERROR,
> > +              "failed to map to domain iomem page %"PRIx64
> > +              " to guest address %"PRIx64,
> > +              _paddr_pfn, _guest_pfn);
> > +        return ret;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> >                         libxl__domain_build_state *state,
> >                         uint32_t *domid, bool soft_reset)
> > @@ -762,6 +793,16 @@ int libxl__domain_make(libxl__gc *gc, 
> > libxl_domain_config *d_config,
> >          goto out;
> >      }
> >  
> > +    if (state->sci_agent_paddr != 0) {
> 
> Shouldn't we also check for sci_type == XEN_DOMCTL_CONFIG_SCI_NONE ?
> 
> If the user specifies sci_type == XEN_DOMCTL_CONFIG_SCI_SCMI_SMC, we
> shouldn't automatically map any SCMI channels to the guest, right?
> 
Sounds good. I will fix this in v2` 
> 
> > +        ret = map_sci_page(gc, *domid, state->sci_agent_paddr,
> > +                            state->sci_shmem_gfn << XC_PAGE_SHIFT);
> > +        if (ret < 0) {
> > +            LOGED(ERROR, *domid, "map SCI page fail");
> > +            rc = ERROR_FAIL;
> > +            goto out;
> > +        }
> > +    }
> > +
> >      dom_path = libxl__xs_get_dompath(gc, *domid);
> >      if (!dom_path) {
> >          rc = ERROR_FAIL;
> > @@ -1817,17 +1858,24 @@ static void libxl__add_dtdevs(libxl__egc *egc, 
> > libxl__ao *ao, uint32_t domid,
> >  {
> >      AO_GC;
> >      libxl__ao_device *aodev = libxl__multidev_prepare(multidev);
> > -    int i, rc = 0;
> > +    int i, rc = 0, rc_sci = 0;
> >  
> >      for (i = 0; i < d_config->num_dtdevs; i++) {
> >          const libxl_device_dtdev *dtdev = &d_config->dtdevs[i];
> >  
> >          LOGD(DEBUG, domid, "Assign device \"%s\" to domain", dtdev->path);
> >          rc = xc_assign_dt_device(CTX->xch, domid, dtdev->path);
> > -        if (rc < 0) {
> > -            LOGD(ERROR, domid, "xc_assign_dtdevice failed: %d", rc);
> > +        rc_sci = xc_domain_add_sci_device(CTX->xch, domid, dtdev->path);
> > +
> > +        if ((rc < 0) && (rc_sci < 0)) {
> > +            LOGD(ERROR, domid, "xc_assign_dt_device failed: %d; "
> > +                 "xc_domain_add_sci_device failed: %d",
> > +                 rc, rc_sci);
> >              goto out;
> >          }
> > +
> > +        if (rc)
> > +            rc = rc_sci;
> 
> I would make this simpler actually. Do we need to pass dtdev->path
> twice, once for xc_assign_dt_device and a second time for
> xc_domain_add_sci_device?
> 
> I would skip adding xc_domain_add_sci_device altogether. A general SCMI
> enable/disable for the domain is necessary, but then we can just get the
> directly assigned devices from xc_assign_dt_device. There is no need to
> specify the list twice. If a device is not manageable via SCMI we can
> detect it automatically because it is going to be missing scmi_devid on
> device tree.
> 
In one of my previous email I described the idea to make a list of
sci_devids in dom.cfg. Such as: 
sci_devs = [0, 1, 35]
and pass this list to Xen via hypercall, then send permission requests
via scmi for each devid.
The advantages of this approach are the following:
- we don't need scmi_devid in device-tree anymore;
- we do only 1 hypercall to set permissions for devices;
- we don't need to pass device-tree node to Hypervisor, just devid.

What do you think about this approach?

> 
> >      }
> >  
> >  out:
> > diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> > index fe9f760f71..b1d288a8b9 100644
> > --- a/tools/libs/light/libxl_dom.c
> > +++ b/tools/libs/light/libxl_dom.c
> > @@ -710,6 +710,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
> >          state->console_mfn = dom->console_pfn;
> >          state->store_mfn = dom->xenstore_pfn;
> >          state->vuart_gfn = dom->vuart_gfn;
> > +        state->sci_shmem_gfn = dom->sci_shmem_gfn;
> >      } else {
> >          state->console_mfn = xc_dom_p2m(dom, dom->console_pfn);
> >          state->store_mfn = xc_dom_p2m(dom, dom->xenstore_pfn);
> > diff --git a/tools/libs/light/libxl_internal.h 
> > b/tools/libs/light/libxl_internal.h
> > index 0b4671318c..f9f9cc633a 100644
> > --- a/tools/libs/light/libxl_internal.h
> > +++ b/tools/libs/light/libxl_internal.h
> > @@ -1407,6 +1407,10 @@ typedef struct {
> >      /* Whether this domain is being migrated/restored, or booting fresh.  
> > Only
> >       * applicable to the primary domain, not support domains (e.g. stub 
> > QEMU). */
> >      bool restore;
> > +
> > +    /* sci channel paddr to be set to device-tree node */
> > +    uint64_t sci_agent_paddr;
> > +    xen_pfn_t sci_shmem_gfn;
> >  } libxl__domain_build_state;
> >  
> >  _hidden void libxl__domain_build_state_init(libxl__domain_build_state *s);
> > diff --git a/xen/arch/arm/domctl.c b/xen/arch/arm/domctl.c
> > index 6245af6d0b..ba200407da 100644
> > --- a/xen/arch/arm/domctl.c
> > +++ b/xen/arch/arm/domctl.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (c) 2012, Citrix Systems
> >   */
> >  
> > +#include <asm/sci/sci.h>
> >  #include <xen/errno.h>
> >  #include <xen/guest_access.h>
> >  #include <xen/hypercall.h>
> > @@ -175,6 +176,20 @@ long arch_do_domctl(struct xen_domctl *domctl, struct 
> > domain *d,
> >  
> >          return rc;
> >      }
> > +    case XEN_DOMCTL_add_sci_device:
> > +    {
> > +        int rc;
> > +        struct dt_device_node *dev;
> > +
> > +        rc = dt_find_node_by_gpath(domctl->u.sci_device_op.path,
> > +                                   domctl->u.sci_device_op.size,
> > +                                   &dev);
> > +        if ( rc )
> > +            return rc;
> > +
> > +        return sci_add_dt_device(d, dev);
> > +    }
> 
> I would skip it and instead I would add a call to sci_add_dt_device in
> the implementation of XEN_DOMCTL_assign_device.
> 
please see my comment above.
> 
> >      default:
> >      {
> >          int rc;
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index b85e6170b0..671c72c3e8 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -1177,6 +1177,13 @@ struct xen_domctl_vmtrace_op {
> >  #define XEN_DOMCTL_vmtrace_get_option         5
> >  #define XEN_DOMCTL_vmtrace_set_option         6
> >  };
> > +
> > +/* XEN_DOMCTL_add_sci_device: set sci device permissions */
> > +struct xen_domctl_sci_device_op {
> > +    uint32_t size; /* Length of the path */
> > +    XEN_GUEST_HANDLE_64(char) path; /* path to the device tree node */
> > +};
> > +
> >  typedef struct xen_domctl_vmtrace_op xen_domctl_vmtrace_op_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_domctl_vmtrace_op_t);
> >  
> > @@ -1265,6 +1272,7 @@ struct xen_domctl {
> >  #define XEN_DOMCTL_get_cpu_policy                82
> >  #define XEN_DOMCTL_set_cpu_policy                83
> >  #define XEN_DOMCTL_vmtrace_op                    84
> > +#define XEN_DOMCTL_add_sci_device                85
> >  #define XEN_DOMCTL_gdbsx_guestmemio            1000
> >  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
> >  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> > @@ -1326,6 +1334,7 @@ struct xen_domctl {
> >          struct xen_domctl_psr_alloc         psr_alloc;
> >          struct xen_domctl_vuart_op          vuart_op;
> >          struct xen_domctl_vmtrace_op        vmtrace_op;
> > +        struct xen_domctl_sci_device_op     sci_device_op;
> >          uint8_t                             pad[128];
> >      } u;
> >  };
> > -- 
> > 2.27.0
> > 


 


Rackspace

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