| Message ID | 1402935486-29136-10-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, 16 Jun 2014, Julien Grall wrote: > DOM0 doesn't provide a generic way to get information about a device tree > node. If we want to do it in userspace, we will have to duplicate the > MMIO/IRQ translation from Xen. Therefore, we can let the hypervisor > doing the job for us and get nearly all the informations. > > This new physdev operation will let the toolstack get the IRQ/MMIO regions > and the compatible string. Most the device node can be described with only > theses 3 items. If we need to add a specific properties, then we will have > to implement it in userspace (some idea was to use a configuration file > describing the additional properties). > > The hypercall is divided in 4 parts: > - GET_INFO: get the numbers of IRQ/MMIO and the size of the > compatible string; > - GET_IRQ: get the IRQ by index. If the IRQ is not routable (i.e not > an SPIs), the errno will be set to -EINVAL; > - GET_MMIO: get the MMIO range by index. If the base and the size of > is not page-aligned, the errno will be set to -EINVAL; > - GET_COMPAT: get the compatible string > > All the information will be accessible if the device is not used by Xen > and protected by an IOMMU. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > Cc: Ian Campbell <ian.campbell@citrix.com> > I know that we talked about this face to face already, but this troubles me: is it really so uncommon for a device tree node corresponding to a device to have a key-value pair that is critical for the initialization of the device? The ACPI on ARM people are discussing how to introduce these key-value pairs in ACPI too, so I wonder if we can really dismiss them so easily for device assignment. Could Xen discard everything that it knows cannot be passed to the guest (information on clocks and phandles for example), but return to the toolstack other harmless key-value pairs, such as device specific configurations? Maybe we could introduce PHYSDEVOP_DTDEV_GET_KEYVALUE. > I'm wondering if we can let the toolstack retrieve device information for > every device not used by Xen. This would allow embedded guys using passthrough > "easily" when their devices are not under an IOMMU. > --- > tools/libxc/xc_physdev.c | 129 +++++++++++++++++++++++++++++++++++++++++ > tools/libxc/xenctrl.h | 36 ++++++++++++ > xen/arch/arm/physdev.c | 16 +++++ > xen/common/device_tree.c | 112 +++++++++++++++++++++++++++++++++++ > xen/include/public/physdev.h | 40 +++++++++++++ > xen/include/xen/device_tree.h | 3 + > 6 files changed, 336 insertions(+) > > diff --git a/tools/libxc/xc_physdev.c b/tools/libxc/xc_physdev.c > index cf02d85..405fe78 100644 > --- a/tools/libxc/xc_physdev.c > +++ b/tools/libxc/xc_physdev.c > @@ -108,3 +108,132 @@ int xc_physdev_unmap_pirq(xc_interface *xch, > return rc; > } > > +int xc_physdev_dtdev_getinfo(xc_interface *xch, > + char *path, > + xc_dtdev_info_t *info) > +{ > + int rc; > + size_t size = strlen(path); > + struct physdev_dtdev_op op; > + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); > + > + if ( xc_hypercall_bounce_pre(xch, path) ) > + return -1; > + > + op.op = PHYSDEVOP_DTDEV_GET_INFO; > + op.plen = size; > + set_xen_guest_handle(op.path, path); > + > + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); > + > + xc_hypercall_bounce_post(xch, path); > + > + if ( !rc ) > + { > + info->num_irqs = op.u.info.num_irqs; > + info->num_mmios = op.u.info.num_mmios; > + info->compat_len = op.u.info.compat_len; > + } > + > + return rc; > +} > + > +int xc_physdev_dtdev_getirq(xc_interface *xch, > + char *path, > + uint32_t index, > + xc_dtdev_irq_t *irq) > +{ > + int rc; > + size_t size = strlen(path); > + struct physdev_dtdev_op op; > + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); > + > + if ( xc_hypercall_bounce_pre(xch, path) ) > + return -1; > + > + op.op = PHYSDEVOP_DTDEV_GET_IRQ; > + op.plen = size; > + op.index = index; > + set_xen_guest_handle(op.path, path); > + > + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); > + > + xc_hypercall_bounce_post(xch, path); > + > + if ( !rc ) > + { > + irq->irq = op.u.irq.irq; > + irq->type = op.u.irq.type; > + } > + > + return rc; > +} > + > +int xc_physdev_dtdev_getmmio(xc_interface *xch, > + char *path, > + uint32_t index, > + xc_dtdev_mmio_t *mmio) > +{ > + int rc; > + size_t size = strlen(path); > + struct physdev_dtdev_op op; > + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); > + > + if ( xc_hypercall_bounce_pre(xch, path) ) > + return -1; > + > + op.op = PHYSDEVOP_DTDEV_GET_MMIO; > + op.plen = size; > + op.index = index; > + set_xen_guest_handle(op.path, path); > + > + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); > + > + xc_hypercall_bounce_post(xch, path); > + > + if ( !rc ) > + { > + mmio->mfn = op.u.mmio.mfn; > + mmio->nr_mfn = op.u.mmio.nr_mfn; > + } > + > + return rc; > +} > + > +int xc_physdev_dtdev_getcompat(xc_interface *xch, > + char *path, > + char *compat, > + uint32_t *clen) > +{ > + int rc; > + size_t size = strlen(path); > + struct physdev_dtdev_op op; > + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); > + DECLARE_HYPERCALL_BOUNCE(compat, *clen, XC_HYPERCALL_BUFFER_BOUNCE_OUT); > + > + if ( xc_hypercall_bounce_pre(xch, path) ) > + return -1; > + > + rc = -1; > + if ( xc_hypercall_bounce_pre(xch, compat) ) > + goto out; > + > + op.op = PHYSDEVOP_DTDEV_GET_COMPAT; > + op.plen = size; > + set_xen_guest_handle(op.path, path); > + > + op.u.compat.clen = *clen; > + set_xen_guest_handle(op.u.compat.compat, compat); > + > + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); > + > + if ( !rc ) > + *clen = op.u.compat.clen; > + > + xc_hypercall_bounce_post(xch, compat); > + > +out: > + xc_hypercall_bounce_post(xch, path); > + > + return rc; > +} > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index b55d857..5ad2d65 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -1143,6 +1143,42 @@ int xc_physdev_pci_access_modify(xc_interface *xch, > int func, > int enable); > > +typedef struct xc_dtdev_info { > + uint32_t num_irqs; > + uint32_t num_mmios; > + uint32_t compat_len; > +} xc_dtdev_info_t; > + > +int xc_physdev_dtdev_getinfo(xc_interface *xch, > + char *path, > + xc_dtdev_info_t *info); > + > +typedef struct xc_dtdev_irq { > + uint32_t irq; > + /* TODO: Maybe an enum here? */ > + uint32_t type; > +} xc_dtdev_irq_t; > + > +int xc_physdev_dtdev_getirq(xc_interface *xch, > + char *path, > + uint32_t index, > + xc_dtdev_irq_t *irq); > + > +typedef struct xc_dtdev_mmio { > + uint64_t mfn; > + uint64_t nr_mfn; > +} xc_dtdev_mmio_t; > + > +int xc_physdev_dtdev_getmmio(xc_interface *xch, > + char *path, > + uint32_t index, > + xc_dtdev_mmio_t *mmio); > + > +int xc_physdev_dtdev_getcompat(xc_interface *xch, > + char *path, > + char *compat, > + uint32_t *clen); > + > int xc_readconsolering(xc_interface *xch, > char *buffer, > unsigned int *pnr_chars, > diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c > index d17589c..11c5b59 100644 > --- a/xen/arch/arm/physdev.c > +++ b/xen/arch/arm/physdev.c > @@ -82,6 +82,22 @@ int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > ret = -EFAULT; > } > break; > + > + case PHYSDEVOP_dtdev_op: > + { > + physdev_dtdev_op_t info; > + > + ret = -EFAULT; > + if ( copy_from_guest(&info, arg, 1) != 0 ) > + break; > + > + /* TODO: Add xsm */ > + ret = dt_do_physdev_op(&info); > + > + if ( __copy_to_guest(arg, &info, 1) ) > + ret = -EFAULT; > + } > + break; > default: > ret = -ENOSYS; > break; > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index fd95307..482ff8f 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -24,6 +24,7 @@ > #include <xen/cpumask.h> > #include <xen/ctype.h> > #include <xen/lib.h> > +#include <xen/irq.h> > > struct dt_early_info __initdata early_info; > const void *device_tree_flattened; > @@ -2021,6 +2022,117 @@ void __init dt_unflatten_host_device_tree(void) > dt_alias_scan(); > } > > + > +/* TODO: I think we need a bit of caching in each device node to get the > + * information in constant time. > + * For now we need to translate IRQs/MMIOs every time > + */ > +int dt_do_physdev_op(physdev_dtdev_op_t *info) > +{ > + struct dt_device_node *dev; > + int ret; > + > + ret = dt_find_node_by_gpath(info->path, info->plen, &dev); > + if ( ret ) > + return ret; > + > + /* Only allow access to protected device and not used by Xen */ > + if ( !dt_device_is_protected(dev) || dt_device_used_by(dev) == DOMID_XEN ) > + return -EACCES; > + > + switch ( info->op ) > + { > + case PHYSDEVOP_DTDEV_GET_INFO: > + { > + const struct dt_property *compat; > + > + compat = dt_find_property(dev, "compatible", NULL); > + /* Hopefully, this case should never happen, print error > + * if it occurs > + */ > + if ( !compat ) > + { > + dprintk(XENLOG_G_ERR, "Unable to find compatible node for %s\n", > + dt_node_full_name(dev)); > + return -EBADFD; > + } > + > + info->u.info.num_irqs = dt_number_of_irq(dev); > + info->u.info.num_mmios = dt_number_of_address(dev); > + info->u.info.compat_len = compat->length; > + } > + break; > + > + case PHYSDEVOP_DTDEV_GET_IRQ: > + { > + struct dt_irq irq; > + > + ret = dt_device_get_irq(dev, info->index, &irq); > + if ( ret ) > + return ret; > + > + /* Check if Xen is able to route the IRQ to the guest */ > + if ( !is_routable_irq(irq.irq) ) > + return -EINVAL; > + > + info->u.irq.irq = irq.irq; > + /* TODO: Translate the type into an exportable value */ > + info->u.irq.type = irq.type; > + } > + break; > + > + case PHYSDEVOP_DTDEV_GET_MMIO: > + { > + uint64_t addr, size; > + > + ret = dt_device_get_address(dev, info->index, &addr, &size); > + if ( ret ) > + return ret; > + > + /* Make sure the address and the size are page aligned. > + * If not, we may passthrough MMIO regions which may belong > + * to another device. Deny it! > + */ > + if ( (addr & (PAGE_SIZE - 1)) || (size & (PAGE_SIZE - 1)) ) > + { > + dprintk(XENLOG_ERR, "%s: contain non-page aligned range:" > + " addr = 0x%"PRIx64" size = 0x%"PRIx64"\n", > + dt_node_full_name(dev), addr, size); > + return -EINVAL; > + } > + > + info->u.mmio.mfn = paddr_to_pfn(addr); > + info->u.mmio.nr_mfn = paddr_to_pfn(size); > + } > + break; > + > + case PHYSDEVOP_DTDEV_GET_COMPAT: > + { > + const struct dt_property *compat; > + > + compat = dt_find_property(dev, "compatible", NULL); > + if ( !compat || !compat->length ) > + return -ENOENT; > + > + if ( info->u.compat.clen < compat->length ) > + return -ENOSPC; > + > + if ( copy_to_guest(info->u.compat.compat, compat->value, > + compat->length) != 0 ) > + return -EFAULT; > + > + info->u.compat.clen = compat->length; > + } > + break; > + > + default: > + return -ENOSYS; > + } > + > + > + return 0; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h > index d547928..23cf673 100644 > --- a/xen/include/public/physdev.h > +++ b/xen/include/public/physdev.h > @@ -337,6 +337,46 @@ struct physdev_dbgp_op { > typedef struct physdev_dbgp_op physdev_dbgp_op_t; > DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t); > > +/* Retrieve informations about a device node */ > +#define PHYSDEVOP_dtdev_op 32 > + > +struct physdev_dtdev_op { > + /* IN */ > + uint32_t plen; /* Length of the path */ > + XEN_GUEST_HANDLE(char) path; /* Path to the device tree node */ > +#define PHYSDEVOP_DTDEV_GET_INFO 0 > +#define PHYSDEVOP_DTDEV_GET_IRQ 1 > +#define PHYSDEVOP_DTDEV_GET_MMIO 2 > +#define PHYSDEVOP_DTDEV_GET_COMPAT 3 > + uint8_t op; > + uint32_t pad0:24; > + uint32_t index; /* Index for the IRQ/MMIO to retrieve */ > + /* OUT */ > + union { > + struct { > + uint32_t num_irqs; /* Number of IRQs */ > + uint32_t num_mmios; /* Number of MMIOs */ > + uint32_t compat_len; /* Length of the compatible string */ > + } info; > + struct { > + /* TODO: Do we need to handle MSI-X? */ > + uint32_t irq; /* IRQ number */ > + /* TODO: Describe with defines the IRQ type */ > + uint32_t type; /* IRQ type (i.e edge, level...) */ > + } irq; > + struct { > + uint64_t mfn; > + uint64_t nr_mfn; > + } mmio; > + struct { > + uint32_t clen; /* IN: Size of buffer. OUT: Size copied */ > + XEN_GUEST_HANDLE_64(char) compat; > + } compat; > + } u; > +}; > +typedef struct physdev_dtdev_op physdev_dtdev_op_t; > +DEFINE_XEN_GUEST_HANDLE(physdev_dtdev_op_t); > + > /* > * Notify that some PIRQ-bound event channels have been unmasked. > * ** This command is obsolete since interface version 0x00030202 and is ** > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h > index bb33e54..3d5101c 100644 > --- a/xen/include/xen/device_tree.h > +++ b/xen/include/xen/device_tree.h > @@ -12,6 +12,7 @@ > > #include <asm/byteorder.h> > #include <public/xen.h> > +#include <public/physdev.h> > #include <xen/init.h> > #include <xen/string.h> > #include <xen/types.h> > @@ -711,6 +712,8 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np, > const char *cells_name, int index, > struct dt_phandle_args *out_args); > > +int dt_do_physdev_op(physdev_dtdev_op_t *info); > + > #endif /* __XEN_DEVICE_TREE_H */ > > /* > -- > 1.7.10.4 >
(Adding Christoffer) On 06/18/2014 08:38 PM, Stefano Stabellini wrote: > On Mon, 16 Jun 2014, Julien Grall wrote: >> DOM0 doesn't provide a generic way to get information about a device tree >> node. If we want to do it in userspace, we will have to duplicate the >> MMIO/IRQ translation from Xen. Therefore, we can let the hypervisor >> doing the job for us and get nearly all the informations. >> >> This new physdev operation will let the toolstack get the IRQ/MMIO regions >> and the compatible string. Most the device node can be described with only >> theses 3 items. If we need to add a specific properties, then we will have >> to implement it in userspace (some idea was to use a configuration file >> describing the additional properties). >> >> The hypercall is divided in 4 parts: >> - GET_INFO: get the numbers of IRQ/MMIO and the size of the >> compatible string; >> - GET_IRQ: get the IRQ by index. If the IRQ is not routable (i.e not >> an SPIs), the errno will be set to -EINVAL; >> - GET_MMIO: get the MMIO range by index. If the base and the size of >> is not page-aligned, the errno will be set to -EINVAL; >> - GET_COMPAT: get the compatible string >> >> All the information will be accessible if the device is not used by Xen >> and protected by an IOMMU. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> Cc: Ian Campbell <ian.campbell@citrix.com> >> > > I know that we talked about this face to face already, but this troubles > me: is it really so uncommon for a device tree node corresponding to a > device to have a key-value pair that is critical for the initialization > of the device? I remembered a chat with Christoffer (I think you were in CC) about specific device properties. But I can't find it in my mailbox. I think the idea was Xen provides the generic properties (regs, interrupts) and we implement device specific properties in a configuration file that could be share with KVM (IIRC, KVM has the same needs). > The ACPI on ARM people are discussing how to introduce these key-value > pairs in ACPI too, so I wonder if we can really dismiss them so easily > for device assignment. > > Could Xen discard everything that it knows cannot be passed to the guest > (information on clocks and phandles for example), but return to the > toolstack other harmless key-value pairs, such as device specific > configurations? Maybe we could introduce PHYSDEVOP_DTDEV_GET_KEYVALUE. A blacklist won't work here because Xen may return properties that contain a list of phandle (for instance see the SMMU bindings). The name of those properties are not necessary generic. IHMO, need to let the toolstack device whether we need to add specific properties. Those properties can be write down in a configuration file which will be parsed by the toolstack.
On Thu, 19 Jun 2014, Julien Grall wrote: > (Adding Christoffer) > > On 06/18/2014 08:38 PM, Stefano Stabellini wrote: > > On Mon, 16 Jun 2014, Julien Grall wrote: > >> DOM0 doesn't provide a generic way to get information about a device tree > >> node. If we want to do it in userspace, we will have to duplicate the > >> MMIO/IRQ translation from Xen. Therefore, we can let the hypervisor > >> doing the job for us and get nearly all the informations. > >> > >> This new physdev operation will let the toolstack get the IRQ/MMIO regions > >> and the compatible string. Most the device node can be described with only > >> theses 3 items. If we need to add a specific properties, then we will have > >> to implement it in userspace (some idea was to use a configuration file > >> describing the additional properties). > >> > >> The hypercall is divided in 4 parts: > >> - GET_INFO: get the numbers of IRQ/MMIO and the size of the > >> compatible string; > >> - GET_IRQ: get the IRQ by index. If the IRQ is not routable (i.e not > >> an SPIs), the errno will be set to -EINVAL; > >> - GET_MMIO: get the MMIO range by index. If the base and the size of > >> is not page-aligned, the errno will be set to -EINVAL; > >> - GET_COMPAT: get the compatible string > >> > >> All the information will be accessible if the device is not used by Xen > >> and protected by an IOMMU. > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> Cc: Ian Campbell <ian.campbell@citrix.com> > >> > > > > I know that we talked about this face to face already, but this troubles > > me: is it really so uncommon for a device tree node corresponding to a > > device to have a key-value pair that is critical for the initialization > > of the device? > > I remembered a chat with Christoffer (I think you were in CC) about > specific device properties. But I can't find it in my mailbox. > > I think the idea was Xen provides the generic properties (regs, > interrupts) and we implement device specific properties in a > configuration file that could be share with KVM (IIRC, KVM has the same > needs). What configuration file? Where would it live? I would rather avoid forcing the user to specify these properties in the VM config file. > > The ACPI on ARM people are discussing how to introduce these key-value > > pairs in ACPI too, so I wonder if we can really dismiss them so easily > > for device assignment. > > > > Could Xen discard everything that it knows cannot be passed to the guest > > (information on clocks and phandles for example), but return to the > > toolstack other harmless key-value pairs, such as device specific > > configurations? Maybe we could introduce PHYSDEVOP_DTDEV_GET_KEYVALUE. > > A blacklist won't work here because Xen may return properties that > contain a list of phandle (for instance see the SMMU bindings). The name > of those properties are not necessary generic. > > IHMO, need to let the toolstack device whether we need to add specific > properties. Those properties can be write down in a configuration file > which will be parsed by the toolstack. Could we simply remove anything that contains phandles? Is there a way to detect if a value is a phandle?
On 06/19/2014 01:21 PM, Stefano Stabellini wrote: >>> I know that we talked about this face to face already, but this troubles >>> me: is it really so uncommon for a device tree node corresponding to a >>> device to have a key-value pair that is critical for the initialization >>> of the device? >> >> I remembered a chat with Christoffer (I think you were in CC) about >> specific device properties. But I can't find it in my mailbox. >> >> I think the idea was Xen provides the generic properties (regs, >> interrupts) and we implement device specific properties in a >> configuration file that could be share with KVM (IIRC, KVM has the same >> needs). > > What configuration file? Where would it live? > I would rather avoid forcing the user to specify these properties in the > VM config file. I meant an host config file. >>> The ACPI on ARM people are discussing how to introduce these key-value >>> pairs in ACPI too, so I wonder if we can really dismiss them so easily >>> for device assignment. >>> >>> Could Xen discard everything that it knows cannot be passed to the guest >>> (information on clocks and phandles for example), but return to the >>> toolstack other harmless key-value pairs, such as device specific >>> configurations? Maybe we could introduce PHYSDEVOP_DTDEV_GET_KEYVALUE. >> >> A blacklist won't work here because Xen may return properties that >> contain a list of phandle (for instance see the SMMU bindings). The name >> of those properties are not necessary generic. >> >> IHMO, need to let the toolstack device whether we need to add specific >> properties. Those properties can be write down in a configuration file >> which will be parsed by the toolstack. > > Could we simply remove anything that contains phandles? Is there a way > to detect if a value is a phandle? A phandle is only a way to interpret a number. AFAIK, there is no way to differentiate it. Regards,
On 19 June 2014 13:58, Julien Grall <julien.grall@linaro.org> wrote: > (Adding Christoffer) > > On 06/18/2014 08:38 PM, Stefano Stabellini wrote: >> On Mon, 16 Jun 2014, Julien Grall wrote: >>> DOM0 doesn't provide a generic way to get information about a device tree >>> node. If we want to do it in userspace, we will have to duplicate the >>> MMIO/IRQ translation from Xen. Therefore, we can let the hypervisor >>> doing the job for us and get nearly all the informations. >>> >>> This new physdev operation will let the toolstack get the IRQ/MMIO regions >>> and the compatible string. Most the device node can be described with only >>> theses 3 items. If we need to add a specific properties, then we will have >>> to implement it in userspace (some idea was to use a configuration file >>> describing the additional properties). >>> >>> The hypercall is divided in 4 parts: >>> - GET_INFO: get the numbers of IRQ/MMIO and the size of the >>> compatible string; >>> - GET_IRQ: get the IRQ by index. If the IRQ is not routable (i.e not >>> an SPIs), the errno will be set to -EINVAL; >>> - GET_MMIO: get the MMIO range by index. If the base and the size of >>> is not page-aligned, the errno will be set to -EINVAL; >>> - GET_COMPAT: get the compatible string >>> >>> All the information will be accessible if the device is not used by Xen >>> and protected by an IOMMU. >>> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>> Cc: Ian Campbell <ian.campbell@citrix.com> >>> >> >> I know that we talked about this face to face already, but this troubles >> me: is it really so uncommon for a device tree node corresponding to a >> device to have a key-value pair that is critical for the initialization >> of the device? > > I remembered a chat with Christoffer (I think you were in CC) about > specific device properties. But I can't find it in my mailbox. > > I think the idea was Xen provides the generic properties (regs, > interrupts) and we implement device specific properties in a > configuration file that could be share with KVM (IIRC, KVM has the same > needs). > Yeah, experience just shows us that when you start exposing the raw hardware information to user space or through to VMs without have semantic control over them, then you will very likely end up in a lot of trouble. It really should be able to limit the properties of devices that you want to export to a reasonable set through a well-defined API. If you have an extremely complicated device with interesting inter-dependency, chances are you're going to need a device-specific user space driver to couple devices, tie inter-dependent devices together when you describe the machine to your VM, etc. The API suggested should take care of the common generic case. The suggestion about a VM config file was more of a loose-thought if we start having a bunch of networking devices that have special properties and we wish to support passthrough of these on both Xen and KVM, then keeping device-specific data in a config file may be a way to accomplish that. This is pretty much speculation and loose ideas at this point. Personally, I would prefer doing something along the lines of what Julien suggest and add necessary properties as needed. If it turns out you need a lot more information about devices someone actually tries to pass through to VMs, then revisit the issue. This patch doesn't seem to suggest an awfully complicated ABI that will cause a lot of headache to maintain in the future or anything like that. My 2 cents. -Christoffer
On Mon, 2014-06-16 at 17:17 +0100, Julien Grall wrote: > /* > * Local variables: > * mode: C > + struct { > + uint64_t mfn; > + uint64_t nr_mfn; xen_pfn_t for both of these I think. > + } mmio; > + struct { > + uint32_t clen; /* IN: Size of buffer. OUT: Size copied */ clen?
On Thu, 2014-06-19 at 12:58 +0100, Julien Grall wrote: > I think the idea was Xen provides the generic properties (regs, > interrupts) and we implement device specific properties in a > configuration file that could be share with KVM (IIRC, KVM has the same > needs). For this is the retrieval of the compatible string really needed? Making this interface DT specific make me uncomfortable. I'd much rather that it only contained hardware stuff (MMIO regions, irqs, etc). Ian.
On Thu, 2014-06-19 at 13:25 +0100, Julien Grall wrote: > On 06/19/2014 01:21 PM, Stefano Stabellini wrote: > >>> I know that we talked about this face to face already, but this troubles > >>> me: is it really so uncommon for a device tree node corresponding to a > >>> device to have a key-value pair that is critical for the initialization > >>> of the device? > >> > >> I remembered a chat with Christoffer (I think you were in CC) about > >> specific device properties. But I can't find it in my mailbox. > >> > >> I think the idea was Xen provides the generic properties (regs, > >> interrupts) and we implement device specific properties in a > >> configuration file that could be share with KVM (IIRC, KVM has the same > >> needs). > > > > What configuration file? Where would it live? > > I would rather avoid forcing the user to specify these properties in the > > VM config file. > > I meant an host config file. I think this is basically unavoidable. There is simply no sane way to expose the necessary stuff from the actual dtb. My opinion is that we should aim to get something which has the underlying moving parts (mmio and irq mapping etc) working and in tree sooner rather than later and leave the question of the sharp edges on the UI until later. Regardless of what syntactic sugar we add in the future we should always have the lowlevel "inject a snippet of dts" interface as an option, and that is all we really need to implement on the first pass. A host config file mapping some useful string to such snippets is a nice simple enhancement to that (and if it's already implemented, great, but it's not mandatory on this pass IMHO). > >>> The ACPI on ARM people are discussing how to introduce these key-value > >>> pairs in ACPI too, so I wonder if we can really dismiss them so easily > >>> for device assignment. > >>> > >>> Could Xen discard everything that it knows cannot be passed to the guest > >>> (information on clocks and phandles for example), but return to the > >>> toolstack other harmless key-value pairs, such as device specific > >>> configurations? Maybe we could introduce PHYSDEVOP_DTDEV_GET_KEYVALUE. > >> > >> A blacklist won't work here because Xen may return properties that > >> contain a list of phandle (for instance see the SMMU bindings). The name > >> of those properties are not necessary generic. > >> > >> IHMO, need to let the toolstack device whether we need to add specific > >> properties. Those properties can be write down in a configuration file > >> which will be parsed by the toolstack. > > > > Could we simply remove anything that contains phandles? Is there a way > > to detect if a value is a phandle? > > A phandle is only a way to interpret a number. AFAIK, there is no way to > differentiate it. Correct, it's just a number which the driver knows (by virtue of the name etc) happens to be a phandle. Ian.
Hi Ian, On 07/03/2014 12:33 PM, Ian Campbell wrote: > On Mon, 2014-06-16 at 17:17 +0100, Julien Grall wrote: > >> /* >> * Local variables: >> * mode: C > >> + struct { >> + uint64_t mfn; >> + uint64_t nr_mfn; > > xen_pfn_t for both of these I think. Ok. I will change it in the next version. >> + } mmio; >> + struct { >> + uint32_t clen; /* IN: Size of buffer. OUT: Size copied */ > > clen? compatible length. Regards,
On Thu, 2014-07-03 at 12:51 +0100, Julien Grall wrote: > Hi Ian, > > On 07/03/2014 12:33 PM, Ian Campbell wrote: > > On Mon, 2014-06-16 at 17:17 +0100, Julien Grall wrote: > > > >> /* > >> * Local variables: > >> * mode: C > > > >> + struct { > >> + uint64_t mfn; > >> + uint64_t nr_mfn; > > > > xen_pfn_t for both of these I think. > > Ok. I will change it in the next version. > > >> + } mmio; > >> + struct { > >> + uint32_t clen; /* IN: Size of buffer. OUT: Size copied */ > > > > clen? > > compatible length. I thought you had typo'd clean and didn't understand. This is in a member called compat isn't it? In that case len would be fine I think. Ian.
diff --git a/tools/libxc/xc_physdev.c b/tools/libxc/xc_physdev.c index cf02d85..405fe78 100644 --- a/tools/libxc/xc_physdev.c +++ b/tools/libxc/xc_physdev.c @@ -108,3 +108,132 @@ int xc_physdev_unmap_pirq(xc_interface *xch, return rc; } +int xc_physdev_dtdev_getinfo(xc_interface *xch, + char *path, + xc_dtdev_info_t *info) +{ + int rc; + size_t size = strlen(path); + struct physdev_dtdev_op op; + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); + + if ( xc_hypercall_bounce_pre(xch, path) ) + return -1; + + op.op = PHYSDEVOP_DTDEV_GET_INFO; + op.plen = size; + set_xen_guest_handle(op.path, path); + + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); + + xc_hypercall_bounce_post(xch, path); + + if ( !rc ) + { + info->num_irqs = op.u.info.num_irqs; + info->num_mmios = op.u.info.num_mmios; + info->compat_len = op.u.info.compat_len; + } + + return rc; +} + +int xc_physdev_dtdev_getirq(xc_interface *xch, + char *path, + uint32_t index, + xc_dtdev_irq_t *irq) +{ + int rc; + size_t size = strlen(path); + struct physdev_dtdev_op op; + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); + + if ( xc_hypercall_bounce_pre(xch, path) ) + return -1; + + op.op = PHYSDEVOP_DTDEV_GET_IRQ; + op.plen = size; + op.index = index; + set_xen_guest_handle(op.path, path); + + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); + + xc_hypercall_bounce_post(xch, path); + + if ( !rc ) + { + irq->irq = op.u.irq.irq; + irq->type = op.u.irq.type; + } + + return rc; +} + +int xc_physdev_dtdev_getmmio(xc_interface *xch, + char *path, + uint32_t index, + xc_dtdev_mmio_t *mmio) +{ + int rc; + size_t size = strlen(path); + struct physdev_dtdev_op op; + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); + + if ( xc_hypercall_bounce_pre(xch, path) ) + return -1; + + op.op = PHYSDEVOP_DTDEV_GET_MMIO; + op.plen = size; + op.index = index; + set_xen_guest_handle(op.path, path); + + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); + + xc_hypercall_bounce_post(xch, path); + + if ( !rc ) + { + mmio->mfn = op.u.mmio.mfn; + mmio->nr_mfn = op.u.mmio.nr_mfn; + } + + return rc; +} + +int xc_physdev_dtdev_getcompat(xc_interface *xch, + char *path, + char *compat, + uint32_t *clen) +{ + int rc; + size_t size = strlen(path); + struct physdev_dtdev_op op; + DECLARE_HYPERCALL_BOUNCE(path, size, XC_HYPERCALL_BUFFER_BOUNCE_IN); + DECLARE_HYPERCALL_BOUNCE(compat, *clen, XC_HYPERCALL_BUFFER_BOUNCE_OUT); + + if ( xc_hypercall_bounce_pre(xch, path) ) + return -1; + + rc = -1; + if ( xc_hypercall_bounce_pre(xch, compat) ) + goto out; + + op.op = PHYSDEVOP_DTDEV_GET_COMPAT; + op.plen = size; + set_xen_guest_handle(op.path, path); + + op.u.compat.clen = *clen; + set_xen_guest_handle(op.u.compat.compat, compat); + + rc = do_physdev_op(xch, PHYSDEVOP_dtdev_op, &op, sizeof(op)); + + if ( !rc ) + *clen = op.u.compat.clen; + + xc_hypercall_bounce_post(xch, compat); + +out: + xc_hypercall_bounce_post(xch, path); + + return rc; +} diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h index b55d857..5ad2d65 100644 --- a/tools/libxc/xenctrl.h +++ b/tools/libxc/xenctrl.h @@ -1143,6 +1143,42 @@ int xc_physdev_pci_access_modify(xc_interface *xch, int func, int enable); +typedef struct xc_dtdev_info { + uint32_t num_irqs; + uint32_t num_mmios; + uint32_t compat_len; +} xc_dtdev_info_t; + +int xc_physdev_dtdev_getinfo(xc_interface *xch, + char *path, + xc_dtdev_info_t *info); + +typedef struct xc_dtdev_irq { + uint32_t irq; + /* TODO: Maybe an enum here? */ + uint32_t type; +} xc_dtdev_irq_t; + +int xc_physdev_dtdev_getirq(xc_interface *xch, + char *path, + uint32_t index, + xc_dtdev_irq_t *irq); + +typedef struct xc_dtdev_mmio { + uint64_t mfn; + uint64_t nr_mfn; +} xc_dtdev_mmio_t; + +int xc_physdev_dtdev_getmmio(xc_interface *xch, + char *path, + uint32_t index, + xc_dtdev_mmio_t *mmio); + +int xc_physdev_dtdev_getcompat(xc_interface *xch, + char *path, + char *compat, + uint32_t *clen); + int xc_readconsolering(xc_interface *xch, char *buffer, unsigned int *pnr_chars, diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c index d17589c..11c5b59 100644 --- a/xen/arch/arm/physdev.c +++ b/xen/arch/arm/physdev.c @@ -82,6 +82,22 @@ int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) ret = -EFAULT; } break; + + case PHYSDEVOP_dtdev_op: + { + physdev_dtdev_op_t info; + + ret = -EFAULT; + if ( copy_from_guest(&info, arg, 1) != 0 ) + break; + + /* TODO: Add xsm */ + ret = dt_do_physdev_op(&info); + + if ( __copy_to_guest(arg, &info, 1) ) + ret = -EFAULT; + } + break; default: ret = -ENOSYS; break; diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index fd95307..482ff8f 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -24,6 +24,7 @@ #include <xen/cpumask.h> #include <xen/ctype.h> #include <xen/lib.h> +#include <xen/irq.h> struct dt_early_info __initdata early_info; const void *device_tree_flattened; @@ -2021,6 +2022,117 @@ void __init dt_unflatten_host_device_tree(void) dt_alias_scan(); } + +/* TODO: I think we need a bit of caching in each device node to get the + * information in constant time. + * For now we need to translate IRQs/MMIOs every time + */ +int dt_do_physdev_op(physdev_dtdev_op_t *info) +{ + struct dt_device_node *dev; + int ret; + + ret = dt_find_node_by_gpath(info->path, info->plen, &dev); + if ( ret ) + return ret; + + /* Only allow access to protected device and not used by Xen */ + if ( !dt_device_is_protected(dev) || dt_device_used_by(dev) == DOMID_XEN ) + return -EACCES; + + switch ( info->op ) + { + case PHYSDEVOP_DTDEV_GET_INFO: + { + const struct dt_property *compat; + + compat = dt_find_property(dev, "compatible", NULL); + /* Hopefully, this case should never happen, print error + * if it occurs + */ + if ( !compat ) + { + dprintk(XENLOG_G_ERR, "Unable to find compatible node for %s\n", + dt_node_full_name(dev)); + return -EBADFD; + } + + info->u.info.num_irqs = dt_number_of_irq(dev); + info->u.info.num_mmios = dt_number_of_address(dev); + info->u.info.compat_len = compat->length; + } + break; + + case PHYSDEVOP_DTDEV_GET_IRQ: + { + struct dt_irq irq; + + ret = dt_device_get_irq(dev, info->index, &irq); + if ( ret ) + return ret; + + /* Check if Xen is able to route the IRQ to the guest */ + if ( !is_routable_irq(irq.irq) ) + return -EINVAL; + + info->u.irq.irq = irq.irq; + /* TODO: Translate the type into an exportable value */ + info->u.irq.type = irq.type; + } + break; + + case PHYSDEVOP_DTDEV_GET_MMIO: + { + uint64_t addr, size; + + ret = dt_device_get_address(dev, info->index, &addr, &size); + if ( ret ) + return ret; + + /* Make sure the address and the size are page aligned. + * If not, we may passthrough MMIO regions which may belong + * to another device. Deny it! + */ + if ( (addr & (PAGE_SIZE - 1)) || (size & (PAGE_SIZE - 1)) ) + { + dprintk(XENLOG_ERR, "%s: contain non-page aligned range:" + " addr = 0x%"PRIx64" size = 0x%"PRIx64"\n", + dt_node_full_name(dev), addr, size); + return -EINVAL; + } + + info->u.mmio.mfn = paddr_to_pfn(addr); + info->u.mmio.nr_mfn = paddr_to_pfn(size); + } + break; + + case PHYSDEVOP_DTDEV_GET_COMPAT: + { + const struct dt_property *compat; + + compat = dt_find_property(dev, "compatible", NULL); + if ( !compat || !compat->length ) + return -ENOENT; + + if ( info->u.compat.clen < compat->length ) + return -ENOSPC; + + if ( copy_to_guest(info->u.compat.compat, compat->value, + compat->length) != 0 ) + return -EFAULT; + + info->u.compat.clen = compat->length; + } + break; + + default: + return -ENOSYS; + } + + + return 0; +} + /* * Local variables: * mode: C diff --git a/xen/include/public/physdev.h b/xen/include/public/physdev.h index d547928..23cf673 100644 --- a/xen/include/public/physdev.h +++ b/xen/include/public/physdev.h @@ -337,6 +337,46 @@ struct physdev_dbgp_op { typedef struct physdev_dbgp_op physdev_dbgp_op_t; DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t); +/* Retrieve informations about a device node */ +#define PHYSDEVOP_dtdev_op 32 + +struct physdev_dtdev_op { + /* IN */ + uint32_t plen; /* Length of the path */ + XEN_GUEST_HANDLE(char) path; /* Path to the device tree node */ +#define PHYSDEVOP_DTDEV_GET_INFO 0 +#define PHYSDEVOP_DTDEV_GET_IRQ 1 +#define PHYSDEVOP_DTDEV_GET_MMIO 2 +#define PHYSDEVOP_DTDEV_GET_COMPAT 3 + uint8_t op; + uint32_t pad0:24; + uint32_t index; /* Index for the IRQ/MMIO to retrieve */ + /* OUT */ + union { + struct { + uint32_t num_irqs; /* Number of IRQs */ + uint32_t num_mmios; /* Number of MMIOs */ + uint32_t compat_len; /* Length of the compatible string */ + } info; + struct { + /* TODO: Do we need to handle MSI-X? */ + uint32_t irq; /* IRQ number */ + /* TODO: Describe with defines the IRQ type */ + uint32_t type; /* IRQ type (i.e edge, level...) */ + } irq; + struct { + uint64_t mfn; + uint64_t nr_mfn; + } mmio; + struct { + uint32_t clen; /* IN: Size of buffer. OUT: Size copied */ + XEN_GUEST_HANDLE_64(char) compat; + } compat; + } u; +}; +typedef struct physdev_dtdev_op physdev_dtdev_op_t; +DEFINE_XEN_GUEST_HANDLE(physdev_dtdev_op_t); + /* * Notify that some PIRQ-bound event channels have been unmasked. * ** This command is obsolete since interface version 0x00030202 and is ** diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index bb33e54..3d5101c 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -12,6 +12,7 @@ #include <asm/byteorder.h> #include <public/xen.h> +#include <public/physdev.h> #include <xen/init.h> #include <xen/string.h> #include <xen/types.h> @@ -711,6 +712,8 @@ int dt_parse_phandle_with_args(const struct dt_device_node *np, const char *cells_name, int index, struct dt_phandle_args *out_args); +int dt_do_physdev_op(physdev_dtdev_op_t *info); + #endif /* __XEN_DEVICE_TREE_H */ /*
DOM0 doesn't provide a generic way to get information about a device tree node. If we want to do it in userspace, we will have to duplicate the MMIO/IRQ translation from Xen. Therefore, we can let the hypervisor doing the job for us and get nearly all the informations. This new physdev operation will let the toolstack get the IRQ/MMIO regions and the compatible string. Most the device node can be described with only theses 3 items. If we need to add a specific properties, then we will have to implement it in userspace (some idea was to use a configuration file describing the additional properties). The hypercall is divided in 4 parts: - GET_INFO: get the numbers of IRQ/MMIO and the size of the compatible string; - GET_IRQ: get the IRQ by index. If the IRQ is not routable (i.e not an SPIs), the errno will be set to -EINVAL; - GET_MMIO: get the MMIO range by index. If the base and the size of is not page-aligned, the errno will be set to -EINVAL; - GET_COMPAT: get the compatible string All the information will be accessible if the device is not used by Xen and protected by an IOMMU. Signed-off-by: Julien Grall <julien.grall@linaro.org> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Cc: Ian Campbell <ian.campbell@citrix.com> --- I'm wondering if we can let the toolstack retrieve device information for every device not used by Xen. This would allow embedded guys using passthrough "easily" when their devices are not under an IOMMU. --- tools/libxc/xc_physdev.c | 129 +++++++++++++++++++++++++++++++++++++++++ tools/libxc/xenctrl.h | 36 ++++++++++++ xen/arch/arm/physdev.c | 16 +++++ xen/common/device_tree.c | 112 +++++++++++++++++++++++++++++++++++ xen/include/public/physdev.h | 40 +++++++++++++ xen/include/xen/device_tree.h | 3 + 6 files changed, 336 insertions(+)