| Message ID | 20230819002850.32349-4-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
Hi Vikram, On 19/08/2023 01:28, Vikram Garhwal wrote: > Remove __init from following function to access during runtime: > 1. map_irq_to_domain() > 2. handle_device_interrupts() > 3. map_range_to_domain() > 4. unflatten_dt_node() We are at v9 now, so this is more a remark for the future. In general we are trying to avoid modifying the same code multiple time within the series because this makes it more difficult to review. In this case, you could have removed the __init in patch #4 where you also export it. > > Move map_irq_to_domain() prototype from domain_build.h to setup.h. > > To avoid breaking the build, following changes are also done: I am guessing by "breaking the build", you mean that you will get an error because an non-init function is implemented in domain_build.c. Right? If so, I think this should be spelled out. > 1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain() > to device.c. After removing __init type, these functions are not specific > to domain building, so moving them out of domain_build.c to device.c. > 2. Remove static type from handle_device_interrupt(). Typo: s/interrupt/interrupts/ to match the function name. But I don't link the name when it is exported as it leads to think this is dealing with real interrupt. Looking at the overlay code, the caller (i.e. add_resources()) is very similar to handle_device(). AFAICT the only differences are: 1/ add_resources() doesn't deal with mapping (you said it will in the future) 2/ You need to update some rangeset For 1/ it means this is a superset. For 2/, I think this could be abstracted by adding a pointer to the rangesets in map_range_data. They could be NULL for the domain build case. So can you look at abstracting? This will make easier to maintain a single place to parse a device node and map it. A possible name for the function would be dt_map_resources_to_domain(). Cheers,
Hi Julien, On Tue, Aug 22, 2023 at 07:59:13PM +0100, Julien Grall wrote: > Hi Vikram, > > On 19/08/2023 01:28, Vikram Garhwal wrote: > > Remove __init from following function to access during runtime: > > 1. map_irq_to_domain() > > 2. handle_device_interrupts() > > 3. map_range_to_domain() > > 4. unflatten_dt_node() > > We are at v9 now, so this is more a remark for the future. In general we are > trying to avoid modifying the same code multiple time within the series > because this makes it more difficult to review. In this case, you could have > removed the __init in patch #4 where you also export it. > > > > > Move map_irq_to_domain() prototype from domain_build.h to setup.h. > > > > To avoid breaking the build, following changes are also done: > > I am guessing by "breaking the build", you mean that you will get an error > because an non-init function is implemented in domain_build.c. Right? If so, > I think this should be spelled out. Yeah, i will change the commit with right reasoning. > > > 1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain() > > to device.c. After removing __init type, these functions are not specific > > to domain building, so moving them out of domain_build.c to device.c. > > 2. Remove static type from handle_device_interrupt(). > > Typo: s/interrupt/interrupts/ to match the function name. But I don't link > the name when it is exported as it leads to think this is dealing with real > interrupt. With using handle_device() in overlay as your below suggestion will anyway need this handle_device_interrupts() function here. > > Looking at the overlay code, the caller (i.e. add_resources()) is very > similar to handle_device(). AFAICT the only differences are: > 1/ add_resources() doesn't deal with mapping (you said it will in the > future) > 2/ You need to update some rangeset > > For 1/ it means this is a superset. For 2/, I think this could be abstracted > by adding a pointer to the rangesets in map_range_data. They could be NULL > for the domain build case. > > So can you look at abstracting? This will make easier to maintain a single > place to parse a device node and map it. > > A possible name for the function would be dt_map_resources_to_domain(). For this part of dynamic overlay programming this function can be used. I updated the overlay code to use handle_device() as per your suggestions. Moved handle_device() and other relevant function out of domain_build. About renaming the function name to dt_map_resource_to_domain(): I will see if i can come with better name else will keep your suggestion. Now with this case, overlay device tree needs to have "xen,passthrough" enabled else it will try to map and fail as Xen supports irq_route and mapping only at the domain creation. In earlier patches this worked fine as we were always skip the routing and map. Anyway, I will add this in overlay commit message. I will send v10 tonight. Testing a few things. > > Cheers, > > -- > Julien Grall
Hi, On 25/08/2023 01:52, Vikram Garhwal wrote: > Hi Julien, > On Tue, Aug 22, 2023 at 07:59:13PM +0100, Julien Grall wrote: >> Hi Vikram, >> >> On 19/08/2023 01:28, Vikram Garhwal wrote: >>> Remove __init from following function to access during runtime: >>> 1. map_irq_to_domain() >>> 2. handle_device_interrupts() >>> 3. map_range_to_domain() >>> 4. unflatten_dt_node() >> >> We are at v9 now, so this is more a remark for the future. In general we are >> trying to avoid modifying the same code multiple time within the series >> because this makes it more difficult to review. In this case, you could have >> removed the __init in patch #4 where you also export it. >> >>> >>> Move map_irq_to_domain() prototype from domain_build.h to setup.h. >>> >>> To avoid breaking the build, following changes are also done: >> >> I am guessing by "breaking the build", you mean that you will get an error >> because an non-init function is implemented in domain_build.c. Right? If so, >> I think this should be spelled out. > Yeah, i will change the commit with right reasoning. >> >>> 1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain() >>> to device.c. After removing __init type, these functions are not specific >>> to domain building, so moving them out of domain_build.c to device.c. >>> 2. Remove static type from handle_device_interrupt(). >> >> Typo: s/interrupt/interrupts/ to match the function name. But I don't link >> the name when it is exported as it leads to think this is dealing with real >> interrupt. > With using handle_device() in overlay as your below suggestion will anyway need > this handle_device_interrupts() function here. Ah I didn't notice it was used in handle_passthrough_prop(). So it indeed needs to be exported. In which case, I would suggest to rename to "map_device_irqs_to_domain()". >> >> Looking at the overlay code, the caller (i.e. add_resources()) is very >> similar to handle_device(). AFAICT the only differences are: >> 1/ add_resources() doesn't deal with mapping (you said it will in the >> future) >> 2/ You need to update some rangeset >> >> For 1/ it means this is a superset. For 2/, I think this could be abstracted >> by adding a pointer to the rangesets in map_range_data. They could be NULL >> for the domain build case. >> >> So can you look at abstracting? This will make easier to maintain a single >> place to parse a device node and map it. >> >> A possible name for the function would be dt_map_resources_to_domain(). > For this part of dynamic overlay programming this function can be used. > I updated the overlay code to use handle_device() as per your suggestions. Moved > handle_device() and other relevant function out of domain_build. > About renaming the function name to dt_map_resource_to_domain(): I will see if > i can come with better name else will keep your suggestion. > Now with this case, overlay device tree needs to have "xen,passthrough" enabled > else it will try to map and fail as Xen supports irq_route and mapping only at > the domain creation. In earlier patches this worked fine as we were always skip > the routing and map. Right, but this also meant the behavior when parsing the DT overlay would not have been consistent with parsing the host DT. You would have then had to break backward compatibility to add support for mapping the resources. If this was planned to be dealt before DT Overlay becomes supported, then this would have been fine as we don't guarantee stable interface for experimental/tech preview feature. The new approach should at least prevent any backward compatibility breakage and would allow us to support DT overlay even before the resource mapping is supported. So I think this is better. Cheers,
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c index ca8539dee5..1652d765bd 100644 --- a/xen/arch/arm/device.c +++ b/xen/arch/arm/device.c @@ -9,8 +9,10 @@ */ #include <asm/device.h> +#include <asm/setup.h> #include <xen/errno.h> #include <xen/init.h> +#include <xen/iocap.h> #include <xen/lib.h> extern const struct device_desc _sdevice[], _edevice[]; @@ -75,6 +77,153 @@ 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 %pd access to IRQ %u\n", d, 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%u to %pd\n", irq, d); + return res; + } + } + + dt_dprintk(" - IRQ: %u\n", irq); + return 0; +} + +int map_range_to_domain(const struct dt_device_node *dev, + uint64_t addr, uint64_t len, void *data) +{ + struct map_range_data *mr_data = data; + struct domain *d = mr_data->d; + int res; + + if ( (addr != (paddr_t)addr) || (((paddr_t)~0 - addr) < len) ) + { + printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the maximum allowed PA width (%u bits)", + dt_node_full_name(dev), addr, (addr + len), PADDR_BITS); + return -ERANGE; + } + + /* + * 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 39b4ee03a5..57b6a43314 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2293,39 +2293,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 %pd access to IRQ %u\n", d, 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%u to %pd\n", irq, d); - 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) @@ -2355,64 +2322,6 @@ static int __init map_dt_irq_to_domain(const struct dt_device_node *dev, return res; } -int __init map_range_to_domain(const struct dt_device_node *dev, - uint64_t addr, uint64_t len, void *data) -{ - struct map_range_data *mr_data = data; - struct domain *d = mr_data->d; - int res; - - if ( (addr != (paddr_t)addr) || (((paddr_t)~0 - addr) < len) ) - { - printk(XENLOG_ERR "%s: [0x%"PRIx64", 0x%"PRIx64"] exceeds the maximum allowed PA width (%u bits)", - dt_node_full_name(dev), addr, (addr + len), PADDR_BITS); - return -ERANGE; - } - - /* - * 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 @@ -2440,62 +2349,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/domain_build.h b/xen/arch/arm/include/asm/domain_build.h index 34ceddc995..b9329c9ee0 100644 --- a/xen/arch/arm/include/asm/domain_build.h +++ b/xen/arch/arm/include/asm/domain_build.h @@ -4,8 +4,6 @@ #include <xen/sched.h> #include <asm/kernel.h> -int map_irq_to_domain(struct domain *d, unsigned int irq, - bool need_mapping, const char *devname); int make_chosen_node(const struct kernel_info *kinfo); void evtchn_allocate(struct domain *d); diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index 19dc637d55..f532332d6c 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -165,9 +165,15 @@ void device_tree_get_reg(const __be32 **cell, uint32_t 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, uint64_t addr, uint64_t len, void *data); +int map_irq_to_domain(struct domain *d, unsigned int irq, + bool need_mapping, const char *devname); + extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES]; #ifdef CONFIG_ARM_64 diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index cd9a6a5c93..d70b6a7ac9 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -1847,12 +1847,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;
Remove __init from following function to access during runtime: 1. map_irq_to_domain() 2. handle_device_interrupts() 3. map_range_to_domain() 4. unflatten_dt_node() Move map_irq_to_domain() prototype from domain_build.h to setup.h. To avoid breaking the build, following changes are also done: 1. Move map_irq_to_domain(), handle_device_interrupts() and map_range_to_domain() to device.c. After removing __init type, these functions are not specific to domain building, so moving them out of domain_build.c to device.c. 2. Remove static type from handle_device_interrupt(). Overall, 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> Reviewed-by: Michal Orzel <michal.orzel@amd.com> --- xen/arch/arm/device.c | 149 ++++++++++++++++++++++++ xen/arch/arm/domain_build.c | 147 ----------------------- xen/arch/arm/include/asm/domain_build.h | 2 - xen/arch/arm/include/asm/setup.h | 6 + xen/common/device_tree.c | 12 +- 5 files changed, 161 insertions(+), 155 deletions(-)