| Message ID | 20221207061537.7266-2-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
Hi Vikram, On 07/12/2022 07:15, Vikram Garhwal wrote: > > > Change function type of following function to access during runtime: > 1. map_irq_to_domain() > 2. handle_device_interrupt() > 3. map_range_to_domain() > 4. unflatten_dt_node() > 5. unflatten_device_tree() If you do not want to do this at first use, then this should be a separate patch. > > Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to > device.c. This should be a separate patch (without removing __init) to make the comparison easier. > > unflatten_device_tree(): Add handling of memory allocation failure. Apart from that you also renamed __unflatten_device_tree to unflatten_device_tree and you did not mention it. > > These changes are done to support the dynamic programming of a nodes where an > overlay node will be added to fdt and unflattened node will be added to dt_host. > Furthermore, IRQ and mmio mapping will be done for the added node. > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > --- > xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++ > xen/arch/arm/domain_build.c | 142 ------------------------------ > xen/arch/arm/include/asm/setup.h | 3 + > xen/common/device_tree.c | 27 +++--- > xen/include/xen/device_tree.h | 5 ++ > 5 files changed, 170 insertions(+), 152 deletions(-) > > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c > index 70cd6c1a19..d299c04e62 100644 > --- a/xen/arch/arm/device.c > +++ b/xen/arch/arm/device.c > @@ -21,6 +21,9 @@ > #include <xen/errno.h> > #include <xen/init.h> > #include <xen/lib.h> > +#include <xen/iocap.h> > +#include <asm/domain_build.h> > +#include <asm/setup.h> > > extern const struct device_desc _sdevice[], _edevice[]; > extern const struct acpi_device_desc _asdevice[], _aedevice[]; > @@ -84,6 +87,148 @@ enum device_class device_get_class(const struct dt_device_node *dev) > return DEVICE_UNKNOWN; > } > > +int map_irq_to_domain(struct domain *d, unsigned int irq, > + bool need_mapping, const char *devname) > +{ > + int res; > + > + res = irq_permit_access(d, irq); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n", > + d->domain_id, irq); > + return res; > + } > + > + if ( need_mapping ) > + { > + /* > + * Checking the return of vgic_reserve_virq is not > + * necessary. It should not fail except when we try to map > + * the IRQ twice. This can legitimately happen if the IRQ is shared > + */ > + vgic_reserve_virq(d, irq); > + > + res = route_irq_to_guest(d, irq, irq, devname); > + if ( res < 0 ) > + { > + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", > + irq, d->domain_id); > + return res; > + } > + } > + > + dt_dprintk(" - IRQ: %u\n", irq); > + return 0; > +} > + > +int map_range_to_domain(const struct dt_device_node *dev, > + u64 addr, u64 len, void *data) > +{ > + struct map_range_data *mr_data = data; > + struct domain *d = mr_data->d; > + int res; > + > + /* > + * reserved-memory regions are RAM carved out for a special purpose. > + * They are not MMIO and therefore a domain should not be able to > + * manage them via the IOMEM interface. > + */ > + if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", > + strlen("/reserved-memory/")) != 0 ) > + { > + res = iomem_permit_access(d, paddr_to_pfn(addr), > + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to permit to dom%d access to" > + " 0x%"PRIx64" - 0x%"PRIx64"\n", > + d->domain_id, > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > + return res; > + } > + } > + > + if ( !mr_data->skip_mapping ) > + { > + res = map_regions_p2mt(d, > + gaddr_to_gfn(addr), > + PFN_UP(len), > + maddr_to_mfn(addr), > + mr_data->p2mt); > + > + if ( res < 0 ) > + { > + printk(XENLOG_ERR "Unable to map 0x%"PRIx64 > + " - 0x%"PRIx64" in domain %d\n", > + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, > + d->domain_id); > + return res; > + } > + } > + > + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", > + addr, addr + len, mr_data->p2mt); > + > + return 0; > +} > + > +/* > + * handle_device_interrupts retrieves the interrupts configuration from > + * a device tree node and maps those interrupts to the target domain. > + * > + * Returns: > + * < 0 error > + * 0 success > + */ > +int handle_device_interrupts(struct domain *d, > + struct dt_device_node *dev, > + bool need_mapping) > +{ > + unsigned int i, nirq; > + int res; > + struct dt_raw_irq rirq; > + > + nirq = dt_number_of_irq(dev); > + > + /* Give permission and map IRQs */ > + for ( i = 0; i < nirq; i++ ) > + { > + res = dt_device_get_raw_irq(dev, i, &rirq); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", > + i, dt_node_full_name(dev)); > + return res; > + } > + > + /* > + * Don't map IRQ that have no physical meaning > + * ie: IRQ whose controller is not the GIC > + */ > + if ( rirq.controller != dt_interrupt_controller ) > + { > + dt_dprintk("irq %u not connected to primary controller. Connected to %s\n", > + i, dt_node_full_name(rirq.controller)); > + continue; > + } > + > + res = platform_get_irq(dev, i); > + if ( res < 0 ) > + { > + printk(XENLOG_ERR "Unable to get irq %u for %s\n", > + i, dt_node_full_name(dev)); > + return res; > + } > + > + res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev)); > + if ( res ) > + return res; > + } > + > + return 0; > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 4fb5c20b13..acde8e714e 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2229,41 +2229,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo) > return res; > } > > -int __init map_irq_to_domain(struct domain *d, unsigned int irq, > - bool need_mapping, const char *devname) > -{ > - int res; > - > - res = irq_permit_access(d, irq); > - if ( res ) > - { > - printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n", > - d->domain_id, irq); > - return res; > - } > - > - if ( need_mapping ) > - { > - /* > - * Checking the return of vgic_reserve_virq is not > - * necessary. It should not fail except when we try to map > - * the IRQ twice. This can legitimately happen if the IRQ is shared > - */ > - vgic_reserve_virq(d, irq); > - > - res = route_irq_to_guest(d, irq, irq, devname); > - if ( res < 0 ) > - { > - printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", > - irq, d->domain_id); > - return res; > - } > - } > - > - dt_dprintk(" - IRQ: %u\n", irq); > - return 0; > -} If you move map_irq_to_domain from domain_build.c to device.c, then the prototype needs to also be moved from domain_build.h to setup.h > - > static int __init map_dt_irq_to_domain(const struct dt_device_node *dev, > const struct dt_irq *dt_irq, > void *data) > @@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev, > return 0; > } > > -int __init map_range_to_domain(const struct dt_device_node *dev, > - u64 addr, u64 len, void *data) > -{ > - struct map_range_data *mr_data = data; > - struct domain *d = mr_data->d; > - int res; > - > - /* > - * reserved-memory regions are RAM carved out for a special purpose. > - * They are not MMIO and therefore a domain should not be able to > - * manage them via the IOMEM interface. > - */ > - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", > - strlen("/reserved-memory/")) != 0 ) > - { > - res = iomem_permit_access(d, paddr_to_pfn(addr), > - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); > - if ( res ) > - { > - printk(XENLOG_ERR "Unable to permit to dom%d access to" > - " 0x%"PRIx64" - 0x%"PRIx64"\n", > - d->domain_id, > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); > - return res; > - } > - } > - > - if ( !mr_data->skip_mapping ) > - { > - res = map_regions_p2mt(d, > - gaddr_to_gfn(addr), > - PFN_UP(len), > - maddr_to_mfn(addr), > - mr_data->p2mt); > - > - if ( res < 0 ) > - { > - printk(XENLOG_ERR "Unable to map 0x%"PRIx64 > - " - 0x%"PRIx64" in domain %d\n", > - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, > - d->domain_id); > - return res; > - } > - } > - > - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", > - addr, addr + len, mr_data->p2mt); > - > - return 0; > -} > - > /* > * For a node which describes a discoverable bus (such as a PCI bus) > * then we may need to perform additional mappings in order to make > @@ -2373,62 +2287,6 @@ static int __init map_device_children(const struct dt_device_node *dev, > return 0; > } > > -/* > - * handle_device_interrupts retrieves the interrupts configuration from > - * a device tree node and maps those interrupts to the target domain. > - * > - * Returns: > - * < 0 error > - * 0 success > - */ > -static int __init handle_device_interrupts(struct domain *d, > - struct dt_device_node *dev, > - bool need_mapping) > -{ > - unsigned int i, nirq; > - int res; > - struct dt_raw_irq rirq; > - > - nirq = dt_number_of_irq(dev); > - > - /* Give permission and map IRQs */ > - for ( i = 0; i < nirq; i++ ) > - { > - res = dt_device_get_raw_irq(dev, i, &rirq); > - if ( res ) > - { > - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", > - i, dt_node_full_name(dev)); > - return res; > - } > - > - /* > - * Don't map IRQ that have no physical meaning > - * ie: IRQ whose controller is not the GIC > - */ > - if ( rirq.controller != dt_interrupt_controller ) > - { > - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n", > - i, dt_node_full_name(rirq.controller)); > - continue; > - } > - > - res = platform_get_irq(dev, i); > - if ( res < 0 ) > - { > - printk(XENLOG_ERR "Unable to get irq %u for %s\n", > - i, dt_node_full_name(dev)); > - return res; > - } > - > - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev)); > - if ( res ) > - return res; > - } > - > - return 0; > -} > - > /* > * For a given device node: > * - Give permission to the guest to manage IRQ and MMIO range > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index fdbf68aadc..ec050848aa 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells, > u32 device_tree_get_u32(const void *fdt, int node, > const char *prop_name, u32 dflt); > > +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev, > + bool need_mapping); > + > int map_range_to_domain(const struct dt_device_node *dev, > u64 addr, u64 len, void *data); > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 6c9712ab7b..6518eff9b0 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np, > * @allnextpp: pointer to ->allnext from last allocated device_node > * @fpsize: Size of the node path up at the current depth. > */ > -static unsigned long __init unflatten_dt_node(const void *fdt, > - unsigned long mem, > - unsigned long *p, > - struct dt_device_node *dad, > - struct dt_device_node ***allnextpp, > - unsigned long fpsize) > +static unsigned long unflatten_dt_node(const void *fdt, > + unsigned long mem, > + unsigned long *p, > + struct dt_device_node *dad, > + struct dt_device_node ***allnextpp, > + unsigned long fpsize) > { > struct dt_device_node *np; > struct dt_property *pp, **prev_pp = NULL; > @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, > } > > /** > - * __unflatten_device_tree - create tree of device_nodes from flat blob > + * unflatten_device_tree - create tree of device_nodes from flat blob > * > * unflattens a device-tree, creating the > * tree of struct device_node. It also fills the "name" and "type" > @@ -2056,8 +2056,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, > * @fdt: The fdt to expand > * @mynodes: The device_node tree created by the call > */ > -static void __init __unflatten_device_tree(const void *fdt, > - struct dt_device_node **mynodes) > +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes) > { > unsigned long start, mem, size; > struct dt_device_node **allnextp = mynodes; > @@ -2079,6 +2078,12 @@ static void __init __unflatten_device_tree(const void *fdt, > /* Allocate memory for the expanded device tree */ > mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node)); > > + if ( mem == 0 ) NIT: !mem would be preffered. > + { > + printk(XENLOG_ERR "Cannot allocate memory for unflatten device tree\n"); > + return -ENOMEM; What is the point of modifying the function to return a value if ... > + } > + > ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef); > > dt_dprintk(" unflattening %lx...\n", mem); > @@ -2095,6 +2100,8 @@ static void __init __unflatten_device_tree(const void *fdt, > *allnextp = NULL; > > dt_dprintk(" <- unflatten_device_tree()\n"); > + > + return 0; > } > > static void dt_alias_add(struct dt_alias_prop *ap, > @@ -2179,7 +2186,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches) > > void __init dt_unflatten_host_device_tree(void) > { > - __unflatten_device_tree(device_tree_flattened, &dt_host); > + unflatten_device_tree(device_tree_flattened, &dt_host); ... you do not check it anyway? > dt_alias_scan(); > } > > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h > index a28937d12a..bde46d7120 100644 > --- a/xen/include/xen/device_tree.h > +++ b/xen/include/xen/device_tree.h > @@ -181,6 +181,11 @@ int device_tree_for_each_node(const void *fdt, int node, > */ > void dt_unflatten_host_device_tree(void); > > +/** > + * unflatten any device tree. > + */ > +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes); > + > /** > * IRQ translation callback > * TODO: For the moment we assume that we only have ONE > -- > 2.17.1 > > ~Michal
Hi Michal, Thanks for reviewing this. Please see my comments below. On 1/20/23 4:39 AM, Michal Orzel wrote: > Hi Vikram, > > On 07/12/2022 07:15, Vikram Garhwal wrote: >> >> Change function type of following function to access during runtime: >> 1. map_irq_to_domain() >> 2. handle_device_interrupt() >> 3. map_range_to_domain() >> 4. unflatten_dt_node() >> 5. unflatten_device_tree() > If you do not want to do this at first use, then this should be a separate patch. >> Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to >> device.c. > This should be a separate patch (without removing __init) to make the comparison easier. Okay > >> unflatten_device_tree(): Add handling of memory allocation failure. > Apart from that you also renamed __unflatten_device_tree to unflatten_device_tree > and you did not mention it. I will add this in commit. > >> These changes are done to support the dynamic programming of a nodes where an >> overlay node will be added to fdt and unflattened node will be added to dt_host. >> Furthermore, IRQ and mmio mapping will be done for the added node. >> >> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >> --- >> xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++ >> xen/arch/arm/domain_build.c | 142 ------------------------------ >> xen/arch/arm/include/asm/setup.h | 3 + >> xen/common/device_tree.c | 27 +++--- >> xen/include/xen/device_tree.h | 5 ++ >> 5 files changed, 170 insertions(+), 152 deletions(-) >> >> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c >> index 70cd6c1a19..d299c04e62 100644 >> --- a/xen/arch/arm/device.c >> +++ b/xen/arch/arm/device.c >> @@ -21,6 +21,9 @@ >> #include <xen/errno.h> >> #include <xen/init.h> >> #include <xen/lib.h> >> +#include <xen/iocap.h> >> +#include <asm/domain_build.h> >> +#include <asm/setup.h> >> >> extern const struct device_desc _sdevice[], _edevice[]; >> extern const struct acpi_device_desc _asdevice[], _aedevice[]; >> @@ -84,6 +87,148 @@ enum device_class device_get_class(const struct dt_device_node *dev) >> return DEVICE_UNKNOWN; >> } >> >> +int map_irq_to_domain(struct domain *d, unsigned int irq, >> + bool need_mapping, const char *devname) >> +{ >> + int res; >> + >> + res = irq_permit_access(d, irq); >> + if ( res ) >> + { >> + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n", >> + d->domain_id, irq); >> + return res; >> + } >> + >> + if ( need_mapping ) >> + { >> + /* >> + * Checking the return of vgic_reserve_virq is not >> + * necessary. It should not fail except when we try to map >> + * the IRQ twice. This can legitimately happen if the IRQ is shared >> + */ >> + vgic_reserve_virq(d, irq); >> + >> + res = route_irq_to_guest(d, irq, irq, devname); >> + if ( res < 0 ) >> + { >> + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", >> + irq, d->domain_id); >> + return res; >> + } >> + } >> + >> + dt_dprintk(" - IRQ: %u\n", irq); >> + return 0; >> +} >> + >> +int map_range_to_domain(const struct dt_device_node *dev, >> + u64 addr, u64 len, void *data) >> +{ >> + struct map_range_data *mr_data = data; >> + struct domain *d = mr_data->d; >> + int res; >> + >> + /* >> + * reserved-memory regions are RAM carved out for a special purpose. >> + * They are not MMIO and therefore a domain should not be able to >> + * manage them via the IOMEM interface. >> + */ >> + if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", >> + strlen("/reserved-memory/")) != 0 ) >> + { >> + res = iomem_permit_access(d, paddr_to_pfn(addr), >> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); >> + if ( res ) >> + { >> + printk(XENLOG_ERR "Unable to permit to dom%d access to" >> + " 0x%"PRIx64" - 0x%"PRIx64"\n", >> + d->domain_id, >> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); >> + return res; >> + } >> + } >> + >> + if ( !mr_data->skip_mapping ) >> + { >> + res = map_regions_p2mt(d, >> + gaddr_to_gfn(addr), >> + PFN_UP(len), >> + maddr_to_mfn(addr), >> + mr_data->p2mt); >> + >> + if ( res < 0 ) >> + { >> + printk(XENLOG_ERR "Unable to map 0x%"PRIx64 >> + " - 0x%"PRIx64" in domain %d\n", >> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, >> + d->domain_id); >> + return res; >> + } >> + } >> + >> + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", >> + addr, addr + len, mr_data->p2mt); >> + >> + return 0; >> +} >> + >> +/* >> + * handle_device_interrupts retrieves the interrupts configuration from >> + * a device tree node and maps those interrupts to the target domain. >> + * >> + * Returns: >> + * < 0 error >> + * 0 success >> + */ >> +int handle_device_interrupts(struct domain *d, >> + struct dt_device_node *dev, >> + bool need_mapping) >> +{ >> + unsigned int i, nirq; >> + int res; >> + struct dt_raw_irq rirq; >> + >> + nirq = dt_number_of_irq(dev); >> + >> + /* Give permission and map IRQs */ >> + for ( i = 0; i < nirq; i++ ) >> + { >> + res = dt_device_get_raw_irq(dev, i, &rirq); >> + if ( res ) >> + { >> + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", >> + i, dt_node_full_name(dev)); >> + return res; >> + } >> + >> + /* >> + * Don't map IRQ that have no physical meaning >> + * ie: IRQ whose controller is not the GIC >> + */ >> + if ( rirq.controller != dt_interrupt_controller ) >> + { >> + dt_dprintk("irq %u not connected to primary controller. Connected to %s\n", >> + i, dt_node_full_name(rirq.controller)); >> + continue; >> + } >> + >> + res = platform_get_irq(dev, i); >> + if ( res < 0 ) >> + { >> + printk(XENLOG_ERR "Unable to get irq %u for %s\n", >> + i, dt_node_full_name(dev)); >> + return res; >> + } >> + >> + res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev)); >> + if ( res ) >> + return res; >> + } >> + >> + return 0; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 4fb5c20b13..acde8e714e 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -2229,41 +2229,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo) >> return res; >> } >> >> -int __init map_irq_to_domain(struct domain *d, unsigned int irq, >> - bool need_mapping, const char *devname) >> -{ >> - int res; >> - >> - res = irq_permit_access(d, irq); >> - if ( res ) >> - { >> - printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n", >> - d->domain_id, irq); >> - return res; >> - } >> - >> - if ( need_mapping ) >> - { >> - /* >> - * Checking the return of vgic_reserve_virq is not >> - * necessary. It should not fail except when we try to map >> - * the IRQ twice. This can legitimately happen if the IRQ is shared >> - */ >> - vgic_reserve_virq(d, irq); >> - >> - res = route_irq_to_guest(d, irq, irq, devname); >> - if ( res < 0 ) >> - { >> - printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", >> - irq, d->domain_id); >> - return res; >> - } >> - } >> - >> - dt_dprintk(" - IRQ: %u\n", irq); >> - return 0; >> -} > If you move map_irq_to_domain from domain_build.c to device.c, then the prototype needs to also > be moved from domain_build.h to setup.h > >> - >> static int __init map_dt_irq_to_domain(const struct dt_device_node *dev, >> const struct dt_irq *dt_irq, >> void *data) >> @@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev, >> return 0; >> } >> >> -int __init map_range_to_domain(const struct dt_device_node *dev, >> - u64 addr, u64 len, void *data) >> -{ >> - struct map_range_data *mr_data = data; >> - struct domain *d = mr_data->d; >> - int res; >> - >> - /* >> - * reserved-memory regions are RAM carved out for a special purpose. >> - * They are not MMIO and therefore a domain should not be able to >> - * manage them via the IOMEM interface. >> - */ >> - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", >> - strlen("/reserved-memory/")) != 0 ) >> - { >> - res = iomem_permit_access(d, paddr_to_pfn(addr), >> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); >> - if ( res ) >> - { >> - printk(XENLOG_ERR "Unable to permit to dom%d access to" >> - " 0x%"PRIx64" - 0x%"PRIx64"\n", >> - d->domain_id, >> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); >> - return res; >> - } >> - } >> - >> - if ( !mr_data->skip_mapping ) >> - { >> - res = map_regions_p2mt(d, >> - gaddr_to_gfn(addr), >> - PFN_UP(len), >> - maddr_to_mfn(addr), >> - mr_data->p2mt); >> - >> - if ( res < 0 ) >> - { >> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64 >> - " - 0x%"PRIx64" in domain %d\n", >> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, >> - d->domain_id); >> - return res; >> - } >> - } >> - >> - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", >> - addr, addr + len, mr_data->p2mt); >> - >> - return 0; >> -} >> - >> /* >> * For a node which describes a discoverable bus (such as a PCI bus) >> * then we may need to perform additional mappings in order to make >> @@ -2373,62 +2287,6 @@ static int __init map_device_children(const struct dt_device_node *dev, >> return 0; >> } >> >> -/* >> - * handle_device_interrupts retrieves the interrupts configuration from >> - * a device tree node and maps those interrupts to the target domain. >> - * >> - * Returns: >> - * < 0 error >> - * 0 success >> - */ >> -static int __init handle_device_interrupts(struct domain *d, >> - struct dt_device_node *dev, >> - bool need_mapping) >> -{ >> - unsigned int i, nirq; >> - int res; >> - struct dt_raw_irq rirq; >> - >> - nirq = dt_number_of_irq(dev); >> - >> - /* Give permission and map IRQs */ >> - for ( i = 0; i < nirq; i++ ) >> - { >> - res = dt_device_get_raw_irq(dev, i, &rirq); >> - if ( res ) >> - { >> - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", >> - i, dt_node_full_name(dev)); >> - return res; >> - } >> - >> - /* >> - * Don't map IRQ that have no physical meaning >> - * ie: IRQ whose controller is not the GIC >> - */ >> - if ( rirq.controller != dt_interrupt_controller ) >> - { >> - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n", >> - i, dt_node_full_name(rirq.controller)); >> - continue; >> - } >> - >> - res = platform_get_irq(dev, i); >> - if ( res < 0 ) >> - { >> - printk(XENLOG_ERR "Unable to get irq %u for %s\n", >> - i, dt_node_full_name(dev)); >> - return res; >> - } >> - >> - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev)); >> - if ( res ) >> - return res; >> - } >> - >> - return 0; >> -} >> - >> /* >> * For a given device node: >> * - Give permission to the guest to manage IRQ and MMIO range >> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h >> index fdbf68aadc..ec050848aa 100644 >> --- a/xen/arch/arm/include/asm/setup.h >> +++ b/xen/arch/arm/include/asm/setup.h >> @@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells, >> u32 device_tree_get_u32(const void *fdt, int node, >> const char *prop_name, u32 dflt); >> >> +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev, >> + bool need_mapping); >> + >> int map_range_to_domain(const struct dt_device_node *dev, >> u64 addr, u64 len, void *data); >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index 6c9712ab7b..6518eff9b0 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np, >> * @allnextpp: pointer to ->allnext from last allocated device_node >> * @fpsize: Size of the node path up at the current depth. >> */ >> -static unsigned long __init unflatten_dt_node(const void *fdt, >> - unsigned long mem, >> - unsigned long *p, >> - struct dt_device_node *dad, >> - struct dt_device_node ***allnextpp, >> - unsigned long fpsize) >> +static unsigned long unflatten_dt_node(const void *fdt, >> + unsigned long mem, >> + unsigned long *p, >> + struct dt_device_node *dad, >> + struct dt_device_node ***allnextpp, >> + unsigned long fpsize) >> { >> struct dt_device_node *np; >> struct dt_property *pp, **prev_pp = NULL; >> @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, >> } >> >> /** >> - * __unflatten_device_tree - create tree of device_nodes from flat blob >> + * unflatten_device_tree - create tree of device_nodes from flat blob >> * >> * unflattens a device-tree, creating the >> * tree of struct device_node. It also fills the "name" and "type" >> @@ -2056,8 +2056,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, >> * @fdt: The fdt to expand >> * @mynodes: The device_node tree created by the call >> */ >> -static void __init __unflatten_device_tree(const void *fdt, >> - struct dt_device_node **mynodes) >> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes) >> { >> unsigned long start, mem, size; >> struct dt_device_node **allnextp = mynodes; >> @@ -2079,6 +2078,12 @@ static void __init __unflatten_device_tree(const void *fdt, >> /* Allocate memory for the expanded device tree */ >> mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node)); >> >> + if ( mem == 0 ) > NIT: !mem would be preffered. > >> + { >> + printk(XENLOG_ERR "Cannot allocate memory for unflatten device tree\n"); >> + return -ENOMEM; > What is the point of modifying the function to return a value if ... >> + } >> + >> ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef); >> >> dt_dprintk(" unflattening %lx...\n", mem); >> @@ -2095,6 +2100,8 @@ static void __init __unflatten_device_tree(const void *fdt, >> *allnextp = NULL; >> >> dt_dprintk(" <- unflatten_device_tree()\n"); >> + >> + return 0; >> } >> >> static void dt_alias_add(struct dt_alias_prop *ap, >> @@ -2179,7 +2186,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches) >> >> void __init dt_unflatten_host_device_tree(void) >> { >> - __unflatten_device_tree(device_tree_flattened, &dt_host); >> + unflatten_device_tree(device_tree_flattened, &dt_host); > ... you do not check it anyway? So, here we kept it same but unflatten_device_tree() return is checked in dt_overlay.c. Return of this function will be useful in dt_overlay as we can run out of mem during runtime. Perhaps I will add another comment about the reasoning for adding this. > >> dt_alias_scan(); >> } >> >> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h >> index a28937d12a..bde46d7120 100644 >> --- a/xen/include/xen/device_tree.h >> +++ b/xen/include/xen/device_tree.h >> @@ -181,6 +181,11 @@ int device_tree_for_each_node(const void *fdt, int node, >> */ >> void dt_unflatten_host_device_tree(void); >> >> +/** >> + * unflatten any device tree. >> + */ >> +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes); >> + >> /** >> * IRQ translation callback >> * TODO: For the moment we assume that we only have ONE >> -- >> 2.17.1 >> >> > ~Michal >
On 09/02/2023 20:11, Vikram Garhwal wrote: > Hi Michal, > > Thanks for reviewing this. Please see my comments below. > > On 1/20/23 4:39 AM, Michal Orzel wrote: >> Hi Vikram, >> >> On 07/12/2022 07:15, Vikram Garhwal wrote: >>> >>> Change function type of following function to access during runtime: >>> 1. map_irq_to_domain() >>> 2. handle_device_interrupt() >>> 3. map_range_to_domain() >>> 4. unflatten_dt_node() >>> 5. unflatten_device_tree() >> If you do not want to do this at first use, then this should be a >> separate patch. >>> Move map_irq_to_domain(), handle_device_interrupt() and >>> map_range_to_domain() to >>> device.c. >> This should be a separate patch (without removing __init) to make the >> comparison easier. > Okay >> >>> unflatten_device_tree(): Add handling of memory allocation failure. >> Apart from that you also renamed __unflatten_device_tree to >> unflatten_device_tree >> and you did not mention it. > I will add this in commit. >> >>> These changes are done to support the dynamic programming of a nodes >>> where an >>> overlay node will be added to fdt and unflattened node will be added >>> to dt_host. >>> Furthermore, IRQ and mmio mapping will be done for the added node. >>> >>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >>> --- >>> xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++ >>> xen/arch/arm/domain_build.c | 142 ------------------------------ >>> xen/arch/arm/include/asm/setup.h | 3 + >>> xen/common/device_tree.c | 27 +++--- >>> xen/include/xen/device_tree.h | 5 ++ >>> 5 files changed, 170 insertions(+), 152 deletions(-) >>> >>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c >>> index 70cd6c1a19..d299c04e62 100644 >>> --- a/xen/arch/arm/device.c >>> +++ b/xen/arch/arm/device.c >>> @@ -21,6 +21,9 @@ >>> #include <xen/errno.h> >>> #include <xen/init.h> >>> #include <xen/lib.h> >>> +#include <xen/iocap.h> >>> +#include <asm/domain_build.h> >>> +#include <asm/setup.h> >>> >>> extern const struct device_desc _sdevice[], _edevice[]; >>> extern const struct acpi_device_desc _asdevice[], _aedevice[]; >>> @@ -84,6 +87,148 @@ enum device_class device_get_class(const struct >>> dt_device_node *dev) >>> return DEVICE_UNKNOWN; >>> } >>> >>> +int map_irq_to_domain(struct domain *d, unsigned int irq, >>> + bool need_mapping, const char *devname) >>> +{ >>> + int res; >>> + >>> + res = irq_permit_access(d, irq); >>> + if ( res ) >>> + { >>> + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ >>> %u\n", >>> + d->domain_id, irq); >>> + return res; >>> + } >>> + >>> + if ( need_mapping ) >>> + { >>> + /* >>> + * Checking the return of vgic_reserve_virq is not >>> + * necessary. It should not fail except when we try to map >>> + * the IRQ twice. This can legitimately happen if the IRQ is >>> shared >>> + */ >>> + vgic_reserve_virq(d, irq); >>> + >>> + res = route_irq_to_guest(d, irq, irq, devname); >>> + if ( res < 0 ) >>> + { >>> + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", >>> + irq, d->domain_id); >>> + return res; >>> + } >>> + } >>> + >>> + dt_dprintk(" - IRQ: %u\n", irq); >>> + return 0; >>> +} >>> + >>> +int map_range_to_domain(const struct dt_device_node *dev, >>> + u64 addr, u64 len, void *data) >>> +{ >>> + struct map_range_data *mr_data = data; >>> + struct domain *d = mr_data->d; >>> + int res; >>> + >>> + /* >>> + * reserved-memory regions are RAM carved out for a special >>> purpose. >>> + * They are not MMIO and therefore a domain should not be able to >>> + * manage them via the IOMEM interface. >>> + */ >>> + if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", >>> + strlen("/reserved-memory/")) != 0 ) >>> + { >>> + res = iomem_permit_access(d, paddr_to_pfn(addr), >>> + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); >>> + if ( res ) >>> + { >>> + printk(XENLOG_ERR "Unable to permit to dom%d access to" >>> + " 0x%"PRIx64" - 0x%"PRIx64"\n", >>> + d->domain_id, >>> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); >>> + return res; >>> + } >>> + } >>> + >>> + if ( !mr_data->skip_mapping ) >>> + { >>> + res = map_regions_p2mt(d, >>> + gaddr_to_gfn(addr), >>> + PFN_UP(len), >>> + maddr_to_mfn(addr), >>> + mr_data->p2mt); >>> + >>> + if ( res < 0 ) >>> + { >>> + printk(XENLOG_ERR "Unable to map 0x%"PRIx64 >>> + " - 0x%"PRIx64" in domain %d\n", >>> + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, >>> + d->domain_id); >>> + return res; >>> + } >>> + } >>> + >>> + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", >>> + addr, addr + len, mr_data->p2mt); >>> + >>> + return 0; >>> +} >>> + >>> +/* >>> + * handle_device_interrupts retrieves the interrupts configuration from >>> + * a device tree node and maps those interrupts to the target domain. >>> + * >>> + * Returns: >>> + * < 0 error >>> + * 0 success >>> + */ >>> +int handle_device_interrupts(struct domain *d, >>> + struct dt_device_node *dev, >>> + bool need_mapping) >>> +{ >>> + unsigned int i, nirq; >>> + int res; >>> + struct dt_raw_irq rirq; >>> + >>> + nirq = dt_number_of_irq(dev); >>> + >>> + /* Give permission and map IRQs */ >>> + for ( i = 0; i < nirq; i++ ) >>> + { >>> + res = dt_device_get_raw_irq(dev, i, &rirq); >>> + if ( res ) >>> + { >>> + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", >>> + i, dt_node_full_name(dev)); >>> + return res; >>> + } >>> + >>> + /* >>> + * Don't map IRQ that have no physical meaning >>> + * ie: IRQ whose controller is not the GIC >>> + */ >>> + if ( rirq.controller != dt_interrupt_controller ) >>> + { >>> + dt_dprintk("irq %u not connected to primary controller. >>> Connected to %s\n", >>> + i, dt_node_full_name(rirq.controller)); >>> + continue; >>> + } >>> + >>> + res = platform_get_irq(dev, i); >>> + if ( res < 0 ) >>> + { >>> + printk(XENLOG_ERR "Unable to get irq %u for %s\n", >>> + i, dt_node_full_name(dev)); >>> + return res; >>> + } >>> + >>> + res = map_irq_to_domain(d, res, need_mapping, >>> dt_node_name(dev)); >>> + if ( res ) >>> + return res; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> /* >>> * Local variables: >>> * mode: C >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 4fb5c20b13..acde8e714e 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -2229,41 +2229,6 @@ int __init make_chosen_node(const struct >>> kernel_info *kinfo) >>> return res; >>> } >>> >>> -int __init map_irq_to_domain(struct domain *d, unsigned int irq, >>> - bool need_mapping, const char *devname) >>> -{ >>> - int res; >>> - >>> - res = irq_permit_access(d, irq); >>> - if ( res ) >>> - { >>> - printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ >>> %u\n", >>> - d->domain_id, irq); >>> - return res; >>> - } >>> - >>> - if ( need_mapping ) >>> - { >>> - /* >>> - * Checking the return of vgic_reserve_virq is not >>> - * necessary. It should not fail except when we try to map >>> - * the IRQ twice. This can legitimately happen if the IRQ is >>> shared >>> - */ >>> - vgic_reserve_virq(d, irq); >>> - >>> - res = route_irq_to_guest(d, irq, irq, devname); >>> - if ( res < 0 ) >>> - { >>> - printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", >>> - irq, d->domain_id); >>> - return res; >>> - } >>> - } >>> - >>> - dt_dprintk(" - IRQ: %u\n", irq); >>> - return 0; >>> -} >> If you move map_irq_to_domain from domain_build.c to device.c, then >> the prototype needs to also >> be moved from domain_build.h to setup.h >> >>> - >>> static int __init map_dt_irq_to_domain(const struct dt_device_node >>> *dev, >>> const struct dt_irq *dt_irq, >>> void *data) >>> @@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const >>> struct dt_device_node *dev, >>> return 0; >>> } >>> >>> -int __init map_range_to_domain(const struct dt_device_node *dev, >>> - u64 addr, u64 len, void *data) >>> -{ >>> - struct map_range_data *mr_data = data; >>> - struct domain *d = mr_data->d; >>> - int res; >>> - >>> - /* >>> - * reserved-memory regions are RAM carved out for a special >>> purpose. >>> - * They are not MMIO and therefore a domain should not be able to >>> - * manage them via the IOMEM interface. >>> - */ >>> - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", >>> - strlen("/reserved-memory/")) != 0 ) >>> - { >>> - res = iomem_permit_access(d, paddr_to_pfn(addr), >>> - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); >>> - if ( res ) >>> - { >>> - printk(XENLOG_ERR "Unable to permit to dom%d access to" >>> - " 0x%"PRIx64" - 0x%"PRIx64"\n", >>> - d->domain_id, >>> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); >>> - return res; >>> - } >>> - } >>> - >>> - if ( !mr_data->skip_mapping ) >>> - { >>> - res = map_regions_p2mt(d, >>> - gaddr_to_gfn(addr), >>> - PFN_UP(len), >>> - maddr_to_mfn(addr), >>> - mr_data->p2mt); >>> - >>> - if ( res < 0 ) >>> - { >>> - printk(XENLOG_ERR "Unable to map 0x%"PRIx64 >>> - " - 0x%"PRIx64" in domain %d\n", >>> - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, >>> - d->domain_id); >>> - return res; >>> - } >>> - } >>> - >>> - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", >>> - addr, addr + len, mr_data->p2mt); >>> - >>> - return 0; >>> -} >>> - >>> /* >>> * For a node which describes a discoverable bus (such as a PCI bus) >>> * then we may need to perform additional mappings in order to make >>> @@ -2373,62 +2287,6 @@ static int __init map_device_children(const >>> struct dt_device_node *dev, >>> return 0; >>> } >>> >>> -/* >>> - * handle_device_interrupts retrieves the interrupts configuration from >>> - * a device tree node and maps those interrupts to the target domain. >>> - * >>> - * Returns: >>> - * < 0 error >>> - * 0 success >>> - */ >>> -static int __init handle_device_interrupts(struct domain *d, >>> - struct dt_device_node *dev, >>> - bool need_mapping) >>> -{ >>> - unsigned int i, nirq; >>> - int res; >>> - struct dt_raw_irq rirq; >>> - >>> - nirq = dt_number_of_irq(dev); >>> - >>> - /* Give permission and map IRQs */ >>> - for ( i = 0; i < nirq; i++ ) >>> - { >>> - res = dt_device_get_raw_irq(dev, i, &rirq); >>> - if ( res ) >>> - { >>> - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", >>> - i, dt_node_full_name(dev)); >>> - return res; >>> - } >>> - >>> - /* >>> - * Don't map IRQ that have no physical meaning >>> - * ie: IRQ whose controller is not the GIC >>> - */ >>> - if ( rirq.controller != dt_interrupt_controller ) >>> - { >>> - dt_dprintk("irq %u not connected to primary controller. >>> Connected to %s\n", >>> - i, dt_node_full_name(rirq.controller)); >>> - continue; >>> - } >>> - >>> - res = platform_get_irq(dev, i); >>> - if ( res < 0 ) >>> - { >>> - printk(XENLOG_ERR "Unable to get irq %u for %s\n", >>> - i, dt_node_full_name(dev)); >>> - return res; >>> - } >>> - >>> - res = map_irq_to_domain(d, res, need_mapping, >>> dt_node_name(dev)); >>> - if ( res ) >>> - return res; >>> - } >>> - >>> - return 0; >>> -} >>> - >>> /* >>> * For a given device node: >>> * - Give permission to the guest to manage IRQ and MMIO range >>> diff --git a/xen/arch/arm/include/asm/setup.h >>> b/xen/arch/arm/include/asm/setup.h >>> index fdbf68aadc..ec050848aa 100644 >>> --- a/xen/arch/arm/include/asm/setup.h >>> +++ b/xen/arch/arm/include/asm/setup.h >>> @@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32 >>> address_cells, >>> u32 device_tree_get_u32(const void *fdt, int node, >>> const char *prop_name, u32 dflt); >>> >>> +int handle_device_interrupts(struct domain *d, struct dt_device_node >>> *dev, >>> + bool need_mapping); >>> + >>> int map_range_to_domain(const struct dt_device_node *dev, >>> u64 addr, u64 len, void *data); >>> >>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >>> index 6c9712ab7b..6518eff9b0 100644 >>> --- a/xen/common/device_tree.c >>> +++ b/xen/common/device_tree.c >>> @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct >>> dt_device_node *np, >>> * @allnextpp: pointer to ->allnext from last allocated device_node >>> * @fpsize: Size of the node path up at the current depth. >>> */ >>> -static unsigned long __init unflatten_dt_node(const void *fdt, >>> - unsigned long mem, >>> - unsigned long *p, >>> - struct dt_device_node >>> *dad, >>> - struct dt_device_node >>> ***allnextpp, >>> - unsigned long fpsize) >>> +static unsigned long unflatten_dt_node(const void *fdt, >>> + unsigned long mem, >>> + unsigned long *p, >>> + struct dt_device_node *dad, >>> + struct dt_device_node >>> ***allnextpp, >>> + unsigned long fpsize) >>> { >>> struct dt_device_node *np; >>> struct dt_property *pp, **prev_pp = NULL; >>> @@ -2047,7 +2047,7 @@ static unsigned long __init >>> unflatten_dt_node(const void *fdt, >>> } >>> >>> /** >>> - * __unflatten_device_tree - create tree of device_nodes from flat blob >>> + * unflatten_device_tree - create tree of device_nodes from flat blob >>> * >>> * unflattens a device-tree, creating the >>> * tree of struct device_node. It also fills the "name" and "type" >>> @@ -2056,8 +2056,7 @@ static unsigned long __init >>> unflatten_dt_node(const void *fdt, >>> * @fdt: The fdt to expand >>> * @mynodes: The device_node tree created by the call >>> */ >>> -static void __init __unflatten_device_tree(const void *fdt, >>> - struct dt_device_node >>> **mynodes) >>> +int unflatten_device_tree(const void *fdt, struct dt_device_node >>> **mynodes) >>> { >>> unsigned long start, mem, size; >>> struct dt_device_node **allnextp = mynodes; >>> @@ -2079,6 +2078,12 @@ static void __init >>> __unflatten_device_tree(const void *fdt, >>> /* Allocate memory for the expanded device tree */ >>> mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct >>> dt_device_node)); >>> >>> + if ( mem == 0 ) >> NIT: !mem would be preffered. >> >>> + { >>> + printk(XENLOG_ERR "Cannot allocate memory for unflatten >>> device tree\n"); >>> + return -ENOMEM; >> What is the point of modifying the function to return a value if ... >>> + } >>> + >>> ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef); >>> >>> dt_dprintk(" unflattening %lx...\n", mem); >>> @@ -2095,6 +2100,8 @@ static void __init >>> __unflatten_device_tree(const void *fdt, >>> *allnextp = NULL; >>> >>> dt_dprintk(" <- unflatten_device_tree()\n"); >>> + >>> + return 0; >>> } >>> >>> static void dt_alias_add(struct dt_alias_prop *ap, >>> @@ -2179,7 +2186,7 @@ dt_find_interrupt_controller(const struct >>> dt_device_match *matches) >>> >>> void __init dt_unflatten_host_device_tree(void) >>> { >>> - __unflatten_device_tree(device_tree_flattened, &dt_host); >>> + unflatten_device_tree(device_tree_flattened, &dt_host); >> ... you do not check it anyway? > So, here we kept it same but unflatten_device_tree() return is checked > in dt_overlay.c. Return of this function will be useful in dt_overlay as > we can run out of mem during runtime. Perhaps I will add another comment > about the reasoning for adding this. It is a good practice to check the error return. In this case, I would call panic() when there is an error. With that said, the hardening of __unflatten_device_tree() deserve its own patch. Cheers,
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c index 70cd6c1a19..d299c04e62 100644 --- a/xen/arch/arm/device.c +++ b/xen/arch/arm/device.c @@ -21,6 +21,9 @@ #include <xen/errno.h> #include <xen/init.h> #include <xen/lib.h> +#include <xen/iocap.h> +#include <asm/domain_build.h> +#include <asm/setup.h> extern const struct device_desc _sdevice[], _edevice[]; extern const struct acpi_device_desc _asdevice[], _aedevice[]; @@ -84,6 +87,148 @@ enum device_class device_get_class(const struct dt_device_node *dev) return DEVICE_UNKNOWN; } +int map_irq_to_domain(struct domain *d, unsigned int irq, + bool need_mapping, const char *devname) +{ + int res; + + res = irq_permit_access(d, irq); + if ( res ) + { + printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n", + d->domain_id, irq); + return res; + } + + if ( need_mapping ) + { + /* + * Checking the return of vgic_reserve_virq is not + * necessary. It should not fail except when we try to map + * the IRQ twice. This can legitimately happen if the IRQ is shared + */ + vgic_reserve_virq(d, irq); + + res = route_irq_to_guest(d, irq, irq, devname); + if ( res < 0 ) + { + printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", + irq, d->domain_id); + return res; + } + } + + dt_dprintk(" - IRQ: %u\n", irq); + return 0; +} + +int map_range_to_domain(const struct dt_device_node *dev, + u64 addr, u64 len, void *data) +{ + struct map_range_data *mr_data = data; + struct domain *d = mr_data->d; + int res; + + /* + * reserved-memory regions are RAM carved out for a special purpose. + * They are not MMIO and therefore a domain should not be able to + * manage them via the IOMEM interface. + */ + if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", + strlen("/reserved-memory/")) != 0 ) + { + res = iomem_permit_access(d, paddr_to_pfn(addr), + paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); + if ( res ) + { + printk(XENLOG_ERR "Unable to permit to dom%d access to" + " 0x%"PRIx64" - 0x%"PRIx64"\n", + d->domain_id, + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); + return res; + } + } + + if ( !mr_data->skip_mapping ) + { + res = map_regions_p2mt(d, + gaddr_to_gfn(addr), + PFN_UP(len), + maddr_to_mfn(addr), + mr_data->p2mt); + + if ( res < 0 ) + { + printk(XENLOG_ERR "Unable to map 0x%"PRIx64 + " - 0x%"PRIx64" in domain %d\n", + addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, + d->domain_id); + return res; + } + } + + dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", + addr, addr + len, mr_data->p2mt); + + return 0; +} + +/* + * handle_device_interrupts retrieves the interrupts configuration from + * a device tree node and maps those interrupts to the target domain. + * + * Returns: + * < 0 error + * 0 success + */ +int handle_device_interrupts(struct domain *d, + struct dt_device_node *dev, + bool need_mapping) +{ + unsigned int i, nirq; + int res; + struct dt_raw_irq rirq; + + nirq = dt_number_of_irq(dev); + + /* Give permission and map IRQs */ + for ( i = 0; i < nirq; i++ ) + { + res = dt_device_get_raw_irq(dev, i, &rirq); + if ( res ) + { + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", + i, dt_node_full_name(dev)); + return res; + } + + /* + * Don't map IRQ that have no physical meaning + * ie: IRQ whose controller is not the GIC + */ + if ( rirq.controller != dt_interrupt_controller ) + { + dt_dprintk("irq %u not connected to primary controller. Connected to %s\n", + i, dt_node_full_name(rirq.controller)); + continue; + } + + res = platform_get_irq(dev, i); + if ( res < 0 ) + { + printk(XENLOG_ERR "Unable to get irq %u for %s\n", + i, dt_node_full_name(dev)); + return res; + } + + res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev)); + if ( res ) + return res; + } + + return 0; +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 4fb5c20b13..acde8e714e 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2229,41 +2229,6 @@ int __init make_chosen_node(const struct kernel_info *kinfo) return res; } -int __init map_irq_to_domain(struct domain *d, unsigned int irq, - bool need_mapping, const char *devname) -{ - int res; - - res = irq_permit_access(d, irq); - if ( res ) - { - printk(XENLOG_ERR "Unable to permit to dom%u access to IRQ %u\n", - d->domain_id, irq); - return res; - } - - if ( need_mapping ) - { - /* - * Checking the return of vgic_reserve_virq is not - * necessary. It should not fail except when we try to map - * the IRQ twice. This can legitimately happen if the IRQ is shared - */ - vgic_reserve_virq(d, irq); - - res = route_irq_to_guest(d, irq, irq, devname); - if ( res < 0 ) - { - printk(XENLOG_ERR "Unable to map IRQ%"PRId32" to dom%d\n", - irq, d->domain_id); - return res; - } - } - - dt_dprintk(" - IRQ: %u\n", irq); - return 0; -} - static int __init map_dt_irq_to_domain(const struct dt_device_node *dev, const struct dt_irq *dt_irq, void *data) @@ -2295,57 +2260,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev, return 0; } -int __init map_range_to_domain(const struct dt_device_node *dev, - u64 addr, u64 len, void *data) -{ - struct map_range_data *mr_data = data; - struct domain *d = mr_data->d; - int res; - - /* - * reserved-memory regions are RAM carved out for a special purpose. - * They are not MMIO and therefore a domain should not be able to - * manage them via the IOMEM interface. - */ - if ( strncasecmp(dt_node_full_name(dev), "/reserved-memory/", - strlen("/reserved-memory/")) != 0 ) - { - res = iomem_permit_access(d, paddr_to_pfn(addr), - paddr_to_pfn(PAGE_ALIGN(addr + len - 1))); - if ( res ) - { - printk(XENLOG_ERR "Unable to permit to dom%d access to" - " 0x%"PRIx64" - 0x%"PRIx64"\n", - d->domain_id, - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1); - return res; - } - } - - if ( !mr_data->skip_mapping ) - { - res = map_regions_p2mt(d, - gaddr_to_gfn(addr), - PFN_UP(len), - maddr_to_mfn(addr), - mr_data->p2mt); - - if ( res < 0 ) - { - printk(XENLOG_ERR "Unable to map 0x%"PRIx64 - " - 0x%"PRIx64" in domain %d\n", - addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1, - d->domain_id); - return res; - } - } - - dt_dprintk(" - MMIO: %010"PRIx64" - %010"PRIx64" P2MType=%x\n", - addr, addr + len, mr_data->p2mt); - - return 0; -} - /* * For a node which describes a discoverable bus (such as a PCI bus) * then we may need to perform additional mappings in order to make @@ -2373,62 +2287,6 @@ static int __init map_device_children(const struct dt_device_node *dev, return 0; } -/* - * handle_device_interrupts retrieves the interrupts configuration from - * a device tree node and maps those interrupts to the target domain. - * - * Returns: - * < 0 error - * 0 success - */ -static int __init handle_device_interrupts(struct domain *d, - struct dt_device_node *dev, - bool need_mapping) -{ - unsigned int i, nirq; - int res; - struct dt_raw_irq rirq; - - nirq = dt_number_of_irq(dev); - - /* Give permission and map IRQs */ - for ( i = 0; i < nirq; i++ ) - { - res = dt_device_get_raw_irq(dev, i, &rirq); - if ( res ) - { - printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", - i, dt_node_full_name(dev)); - return res; - } - - /* - * Don't map IRQ that have no physical meaning - * ie: IRQ whose controller is not the GIC - */ - if ( rirq.controller != dt_interrupt_controller ) - { - dt_dprintk("irq %u not connected to primary controller. Connected to %s\n", - i, dt_node_full_name(rirq.controller)); - continue; - } - - res = platform_get_irq(dev, i); - if ( res < 0 ) - { - printk(XENLOG_ERR "Unable to get irq %u for %s\n", - i, dt_node_full_name(dev)); - return res; - } - - res = map_irq_to_domain(d, res, need_mapping, dt_node_name(dev)); - if ( res ) - return res; - } - - return 0; -} - /* * For a given device node: * - Give permission to the guest to manage IRQ and MMIO range diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index fdbf68aadc..ec050848aa 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -163,6 +163,9 @@ void device_tree_get_reg(const __be32 **cell, u32 address_cells, u32 device_tree_get_u32(const void *fdt, int node, const char *prop_name, u32 dflt); +int handle_device_interrupts(struct domain *d, struct dt_device_node *dev, + bool need_mapping); + int map_range_to_domain(const struct dt_device_node *dev, u64 addr, u64 len, void *data); diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 6c9712ab7b..6518eff9b0 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1811,12 +1811,12 @@ int dt_count_phandle_with_args(const struct dt_device_node *np, * @allnextpp: pointer to ->allnext from last allocated device_node * @fpsize: Size of the node path up at the current depth. */ -static unsigned long __init unflatten_dt_node(const void *fdt, - unsigned long mem, - unsigned long *p, - struct dt_device_node *dad, - struct dt_device_node ***allnextpp, - unsigned long fpsize) +static unsigned long unflatten_dt_node(const void *fdt, + unsigned long mem, + unsigned long *p, + struct dt_device_node *dad, + struct dt_device_node ***allnextpp, + unsigned long fpsize) { struct dt_device_node *np; struct dt_property *pp, **prev_pp = NULL; @@ -2047,7 +2047,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, } /** - * __unflatten_device_tree - create tree of device_nodes from flat blob + * unflatten_device_tree - create tree of device_nodes from flat blob * * unflattens a device-tree, creating the * tree of struct device_node. It also fills the "name" and "type" @@ -2056,8 +2056,7 @@ static unsigned long __init unflatten_dt_node(const void *fdt, * @fdt: The fdt to expand * @mynodes: The device_node tree created by the call */ -static void __init __unflatten_device_tree(const void *fdt, - struct dt_device_node **mynodes) +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes) { unsigned long start, mem, size; struct dt_device_node **allnextp = mynodes; @@ -2079,6 +2078,12 @@ static void __init __unflatten_device_tree(const void *fdt, /* Allocate memory for the expanded device tree */ mem = (unsigned long)_xmalloc (size + 4, __alignof__(struct dt_device_node)); + if ( mem == 0 ) + { + printk(XENLOG_ERR "Cannot allocate memory for unflatten device tree\n"); + return -ENOMEM; + } + ((__be32 *)mem)[size / 4] = cpu_to_be32(0xdeadbeef); dt_dprintk(" unflattening %lx...\n", mem); @@ -2095,6 +2100,8 @@ static void __init __unflatten_device_tree(const void *fdt, *allnextp = NULL; dt_dprintk(" <- unflatten_device_tree()\n"); + + return 0; } static void dt_alias_add(struct dt_alias_prop *ap, @@ -2179,7 +2186,7 @@ dt_find_interrupt_controller(const struct dt_device_match *matches) void __init dt_unflatten_host_device_tree(void) { - __unflatten_device_tree(device_tree_flattened, &dt_host); + unflatten_device_tree(device_tree_flattened, &dt_host); dt_alias_scan(); } diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index a28937d12a..bde46d7120 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -181,6 +181,11 @@ int device_tree_for_each_node(const void *fdt, int node, */ void dt_unflatten_host_device_tree(void); +/** + * unflatten any device tree. + */ +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes); + /** * IRQ translation callback * TODO: For the moment we assume that we only have ONE
Change function type of following function to access during runtime: 1. map_irq_to_domain() 2. handle_device_interrupt() 3. map_range_to_domain() 4. unflatten_dt_node() 5. unflatten_device_tree() Move map_irq_to_domain(), handle_device_interrupt() and map_range_to_domain() to device.c. unflatten_device_tree(): Add handling of memory allocation failure. These changes are done to support the dynamic programming of a nodes where an overlay node will be added to fdt and unflattened node will be added to dt_host. Furthermore, IRQ and mmio mapping will be done for the added node. Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> --- xen/arch/arm/device.c | 145 +++++++++++++++++++++++++++++++ xen/arch/arm/domain_build.c | 142 ------------------------------ xen/arch/arm/include/asm/setup.h | 3 + xen/common/device_tree.c | 27 +++--- xen/include/xen/device_tree.h | 5 ++ 5 files changed, 170 insertions(+), 152 deletions(-)