| Message ID | 20190808231242.26424-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/6] xen/arm: introduce handle_interrupts | expand |
Hello Stefano, Stefano Stabellini writes: > Move the interrupt handling code out of handle_device to a new function > so that it can be reused for dom0less VMs later. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > --- > Changes in v3: > - add patch > > The diff is hard to read but I just moved the interrupts related code > from handle_devices to a new function handle_interrupts, and very little > else. > --- > xen/arch/arm/domain_build.c | 79 +++++++++++++++++++++++-------------- > 1 file changed, 49 insertions(+), 30 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 4c8404155a..00ddb3b05d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1220,41 +1220,19 @@ static int __init map_device_children(struct domain *d, > } > > /* > - * For a given device node: > - * - Give permission to the guest to manage IRQ and MMIO range > - * - Retrieve the IRQ configuration (i.e edge/level) from device tree > - * When the device is not marked for guest passthrough: > - * - Assign the device to the guest if it's protected by an IOMMU > - * - Map the IRQs and iomem regions to DOM0 > + * Return: > + * < 0 on error > + * 0 on no mapping required > + * 1 IRQ mapping done > */ Are this such returns values really needed? I don't see any code that depends on return value 0 or 1. > -static int __init handle_device(struct domain *d, struct dt_device_node *dev, > - p2m_type_t p2mt) > +static int __init handle_interrupts(struct domain *d, > + struct dt_device_node *dev, > + bool need_mapping) > { > - unsigned int nirq; > - unsigned int naddr; > - unsigned int i; > - int res; > + int i, nirq, res; > struct dt_raw_irq rirq; > - u64 addr, size; > - bool need_mapping = !dt_device_for_passthrough(dev); > > nirq = dt_number_of_irq(dev); > - naddr = dt_number_of_address(dev); > - > - dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n", > - dt_node_full_name(dev), need_mapping, nirq, naddr); > - > - if ( dt_device_is_protected(dev) && need_mapping ) > - { > - dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); > - res = iommu_assign_dt_device(d, dev); > - if ( res ) > - { > - printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n", > - dt_node_full_name(dev)); > - return res; > - } > - } > > /* Give permission and map IRQs */ > for ( i = 0; i < nirq; i++ ) > @@ -1291,6 +1269,47 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, > return res; > } > > + return !!(need_mapping && res == 0); > +} > + > +/* > + * For a given device node: > + * - Give permission to the guest to manage IRQ and MMIO range > + * - Retrieve the IRQ configuration (i.e edge/level) from device tree > + * When the device is not marked for guest passthrough: > + * - Assign the device to the guest if it's protected by an IOMMU > + * - Map the IRQs and iomem regions to DOM0 > + */ > +static int __init handle_device(struct domain *d, struct dt_device_node *dev, > + p2m_type_t p2mt) > +{ > + unsigned int naddr; > + unsigned int i; > + int res; > + u64 addr, size; > + bool need_mapping = !dt_device_for_passthrough(dev); > + > + naddr = dt_number_of_address(dev); > + > + dt_dprintk("%s passthrough = %d naddr = %u\n", > + dt_node_full_name(dev), need_mapping, naddr); > + > + if ( dt_device_is_protected(dev) && need_mapping ) > + { > + dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); > + res = iommu_assign_dt_device(d, dev); > + if ( res ) > + { > + printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n", > + dt_node_full_name(dev)); > + return res; > + } > + } > + > + res = handle_interrupts(d, dev, need_mapping); > + if ( res < 0 ) > + return res; > + > /* Give permission and map MMIOs */ > for ( i = 0; i < naddr; i++ ) > { -- Volodymyr Babchuk at EPAM
Hi Stefano, On 09/08/2019 00:12, Stefano Stabellini wrote: > Move the interrupt handling code out of handle_device to a new function > so that it can be reused for dom0less VMs later. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > --- > Changes in v3: > - add patch > > The diff is hard to read but I just moved the interrupts related code > from handle_devices to a new function handle_interrupts, and very little > else. > --- > xen/arch/arm/domain_build.c | 79 +++++++++++++++++++++++-------------- > 1 file changed, 49 insertions(+), 30 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 4c8404155a..00ddb3b05d 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1220,41 +1220,19 @@ static int __init map_device_children(struct domain *d, > } > > /* > - * For a given device node: > - * - Give permission to the guest to manage IRQ and MMIO range > - * - Retrieve the IRQ configuration (i.e edge/level) from device tree > - * When the device is not marked for guest passthrough: > - * - Assign the device to the guest if it's protected by an IOMMU > - * - Map the IRQs and iomem regions to DOM0 > + * Return: > + * < 0 on error > + * 0 on no mapping required > + * 1 IRQ mapping done This feels a bit odd to describe the return value and not what the function does. But I don't understand why you need to tell the caller whether mapping were done or not. This is already conveyed by "need_mapping" provided by the caller. Looking at the only place where you make the distinction between 0 and 1 (patch #3), you have + r = handle_interrupts(d, node, true); + if ( r < 0 ) + return r; + if ( r > 0 ) + { /* do something */ + } Not looking at the code below (which looks wrong), as you always pass true here, r can either be an error or 1. > */ > -static int __init handle_device(struct domain *d, struct dt_device_node *dev, > - p2m_type_t p2mt) > +static int __init handle_interrupts(struct domain *d, How about handle_device_interrupts? Or map_device_interrupts? > + struct dt_device_node *dev, > + bool need_mapping) > { > - unsigned int nirq; > - unsigned int naddr; > - unsigned int i; > - int res; > + int i, nirq, res; res will be used unitialized if the device has no interrupts. > struct dt_raw_irq rirq; > - u64 addr, size; > - bool need_mapping = !dt_device_for_passthrough(dev); > > nirq = dt_number_of_irq(dev); > - naddr = dt_number_of_address(dev); > - > - dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n", > - dt_node_full_name(dev), need_mapping, nirq, naddr); > - > - if ( dt_device_is_protected(dev) && need_mapping ) > - { > - dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); > - res = iommu_assign_dt_device(d, dev); > - if ( res ) > - { > - printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n", > - dt_node_full_name(dev)); > - return res; > - } > - } > > /* Give permission and map IRQs */ > for ( i = 0; i < nirq; i++ ) > @@ -1291,6 +1269,47 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, > return res; > } > > + return !!(need_mapping && res == 0); Why do you need the !! here? (a && b) is already a boolean. But this looks pretty wrong as you would return 0 when res is non-zero (i.e an error) and need_mapping is true. But looking at the code, res cannot be 0 here... So why are you checking "res" here? > +} > + > +/* > + * For a given device node: > + * - Give permission to the guest to manage IRQ and MMIO range > + * - Retrieve the IRQ configuration (i.e edge/level) from device tree > + * When the device is not marked for guest passthrough: > + * - Assign the device to the guest if it's protected by an IOMMU > + * - Map the IRQs and iomem regions to DOM0 > + */ > +static int __init handle_device(struct domain *d, struct dt_device_node *dev, > + p2m_type_t p2mt) > +{ > + unsigned int naddr; > + unsigned int i; > + int res; > + u64 addr, size; > + bool need_mapping = !dt_device_for_passthrough(dev); > + > + naddr = dt_number_of_address(dev); > + > + dt_dprintk("%s passthrough = %d naddr = %u\n", > + dt_node_full_name(dev), need_mapping, naddr); > + > + if ( dt_device_is_protected(dev) && need_mapping ) > + { > + dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); > + res = iommu_assign_dt_device(d, dev); > + if ( res ) > + { > + printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n", > + dt_node_full_name(dev)); > + return res; > + } > + } > + > + res = handle_interrupts(d, dev, need_mapping); > + if ( res < 0 ) > + return res; > + > /* Give permission and map MMIOs */ > for ( i = 0; i < naddr; i++ ) > { > Cheers,
On Fri, 9 Aug 2019, Julien Grall wrote: > Hi Stefano, > > On 09/08/2019 00:12, Stefano Stabellini wrote: > > Move the interrupt handling code out of handle_device to a new function > > so that it can be reused for dom0less VMs later. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > Changes in v3: > > - add patch > > > > The diff is hard to read but I just moved the interrupts related code > > from handle_devices to a new function handle_interrupts, and very little > > else. > > --- > > xen/arch/arm/domain_build.c | 79 +++++++++++++++++++++++-------------- > > 1 file changed, 49 insertions(+), 30 deletions(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 4c8404155a..00ddb3b05d 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -1220,41 +1220,19 @@ static int __init map_device_children(struct domain > > *d, > > } > > /* > > - * For a given device node: > > - * - Give permission to the guest to manage IRQ and MMIO range > > - * - Retrieve the IRQ configuration (i.e edge/level) from device tree > > - * When the device is not marked for guest passthrough: > > - * - Assign the device to the guest if it's protected by an IOMMU > > - * - Map the IRQs and iomem regions to DOM0 > > + * Return: > > + * < 0 on error > > + * 0 on no mapping required > > + * 1 IRQ mapping done > > This feels a bit odd to describe the return value and not what the function > does. Fair enough, I'll add a few words. > But I don't understand why you need to tell the caller whether mapping were > done or not. This is already conveyed by "need_mapping" provided by the > caller. > > Looking at the only place where you make the distinction between 0 and 1 > (patch #3), you have > > + r = handle_interrupts(d, node, true); > + if ( r < 0 ) > + return r; > + if ( r > 0 ) > + { > /* do something */ > + } > > > Not looking at the code below (which looks wrong), as you always pass true > here, r can either be an error or 1. Yes, the return statement of handle_interrupts, the way I wrote it: return !!(need_mapping && res == 0); is wrong. I'll fix it (also see below). Stepping back from this specific error, the reason to distinguish whether a mapping was done or not is to figure out whether we need to add an interrupt property to the guest device tree. The idea is the following: - call handle_interrupts to do any required interrupt mappings - if any mappings are done, copy over the interrupts property to the guest device tree > > */ > > -static int __init handle_device(struct domain *d, struct dt_device_node > > *dev, > > - p2m_type_t p2mt) > > +static int __init handle_interrupts(struct domain *d, > > How about handle_device_interrupts? Or map_device_interrupts? OK > > + struct dt_device_node *dev, > > + bool need_mapping) > > { > > - unsigned int nirq; > > - unsigned int naddr; > > - unsigned int i; > > - int res; > > + int i, nirq, res; > > res will be used unitialized if the device has no interrupts. Well spotted! > > struct dt_raw_irq rirq; > > - u64 addr, size; > > - bool need_mapping = !dt_device_for_passthrough(dev); > > nirq = dt_number_of_irq(dev); > > - naddr = dt_number_of_address(dev); > > - > > - dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n", > > - dt_node_full_name(dev), need_mapping, nirq, naddr); > > - > > - if ( dt_device_is_protected(dev) && need_mapping ) > > - { > > - dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); > > - res = iommu_assign_dt_device(d, dev); > > - if ( res ) > > - { > > - printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n", > > - dt_node_full_name(dev)); > > - return res; > > - } > > - } > > /* Give permission and map IRQs */ > > for ( i = 0; i < nirq; i++ ) > > @@ -1291,6 +1269,47 @@ static int __init handle_device(struct domain *d, > > struct dt_device_node *dev, > > return res; > > } > > + return !!(need_mapping && res == 0); > > Why do you need the !! here? (a && b) is already a boolean. Yes, I'll remove it > But this looks > pretty wrong as you would return 0 when res is non-zero (i.e an error) and > need_mapping is true. > > But looking at the code, res cannot be 0 here... So why are you checking "res" > here? That is a mistake: it should return 1 only when mappings are actually done.
Hi, On 20/08/2019 01:11, Stefano Stabellini wrote: > On Fri, 9 Aug 2019, Julien Grall wrote: >> Hi Stefano, >> >> On 09/08/2019 00:12, Stefano Stabellini wrote: >>> Move the interrupt handling code out of handle_device to a new function >>> so that it can be reused for dom0less VMs later. >>> >>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> >>> --- >>> Changes in v3: >>> - add patch >>> >>> The diff is hard to read but I just moved the interrupts related code >>> from handle_devices to a new function handle_interrupts, and very little >>> else. >>> --- >>> xen/arch/arm/domain_build.c | 79 +++++++++++++++++++++++-------------- >>> 1 file changed, 49 insertions(+), 30 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 4c8404155a..00ddb3b05d 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -1220,41 +1220,19 @@ static int __init map_device_children(struct domain >>> *d, >>> } >>> /* >>> - * For a given device node: >>> - * - Give permission to the guest to manage IRQ and MMIO range >>> - * - Retrieve the IRQ configuration (i.e edge/level) from device tree >>> - * When the device is not marked for guest passthrough: >>> - * - Assign the device to the guest if it's protected by an IOMMU >>> - * - Map the IRQs and iomem regions to DOM0 >>> + * Return: >>> + * < 0 on error >>> + * 0 on no mapping required >>> + * 1 IRQ mapping done >> >> This feels a bit odd to describe the return value and not what the function >> does. > > Fair enough, I'll add a few words. > > >> But I don't understand why you need to tell the caller whether mapping were >> done or not. This is already conveyed by "need_mapping" provided by the >> caller. >> >> Looking at the only place where you make the distinction between 0 and 1 >> (patch #3), you have >> >> + r = handle_interrupts(d, node, true); >> + if ( r < 0 ) >> + return r; >> + if ( r > 0 ) >> + { >> /* do something */ >> + } >> >> >> Not looking at the code below (which looks wrong), as you always pass true >> here, r can either be an error or 1. > > Yes, the return statement of handle_interrupts, the way I wrote it: > > return !!(need_mapping && res == 0); > > is wrong. I'll fix it (also see below). > > Stepping back from this specific error, the reason to distinguish > whether a mapping was done or not is to figure out whether we need to > add an interrupt property to the guest device tree. The idea is the > following: > > - call handle_interrupts to do any required interrupt mappings > - if any mappings are done, copy over the interrupts property to the guest > device tree I don't think we should treat interrupts property differently depending on what was routed to. As I pointed out before, you could decide to give an interrupt controller (and all the associated devices) to the guest. That controller will use a GIC interrupts but devices behind it will not. With your suggestion here, all the devices will not have the "interrupts"/"interrupts-extended" property copied over. [...] >> But this looks >> pretty wrong as you would return 0 when res is non-zero (i.e an error) and >> need_mapping is true. >> >> But looking at the code, res cannot be 0 here... So why are you checking "res" >> here? > > That is a mistake: it should return 1 only when mappings are actually > done. See above.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 4c8404155a..00ddb3b05d 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1220,41 +1220,19 @@ static int __init map_device_children(struct domain *d, } /* - * For a given device node: - * - Give permission to the guest to manage IRQ and MMIO range - * - Retrieve the IRQ configuration (i.e edge/level) from device tree - * When the device is not marked for guest passthrough: - * - Assign the device to the guest if it's protected by an IOMMU - * - Map the IRQs and iomem regions to DOM0 + * Return: + * < 0 on error + * 0 on no mapping required + * 1 IRQ mapping done */ -static int __init handle_device(struct domain *d, struct dt_device_node *dev, - p2m_type_t p2mt) +static int __init handle_interrupts(struct domain *d, + struct dt_device_node *dev, + bool need_mapping) { - unsigned int nirq; - unsigned int naddr; - unsigned int i; - int res; + int i, nirq, res; struct dt_raw_irq rirq; - u64 addr, size; - bool need_mapping = !dt_device_for_passthrough(dev); nirq = dt_number_of_irq(dev); - naddr = dt_number_of_address(dev); - - dt_dprintk("%s passthrough = %d nirq = %d naddr = %u\n", - dt_node_full_name(dev), need_mapping, nirq, naddr); - - if ( dt_device_is_protected(dev) && need_mapping ) - { - dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); - res = iommu_assign_dt_device(d, dev); - if ( res ) - { - printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n", - dt_node_full_name(dev)); - return res; - } - } /* Give permission and map IRQs */ for ( i = 0; i < nirq; i++ ) @@ -1291,6 +1269,47 @@ static int __init handle_device(struct domain *d, struct dt_device_node *dev, return res; } + return !!(need_mapping && res == 0); +} + +/* + * For a given device node: + * - Give permission to the guest to manage IRQ and MMIO range + * - Retrieve the IRQ configuration (i.e edge/level) from device tree + * When the device is not marked for guest passthrough: + * - Assign the device to the guest if it's protected by an IOMMU + * - Map the IRQs and iomem regions to DOM0 + */ +static int __init handle_device(struct domain *d, struct dt_device_node *dev, + p2m_type_t p2mt) +{ + unsigned int naddr; + unsigned int i; + int res; + u64 addr, size; + bool need_mapping = !dt_device_for_passthrough(dev); + + naddr = dt_number_of_address(dev); + + dt_dprintk("%s passthrough = %d naddr = %u\n", + dt_node_full_name(dev), need_mapping, naddr); + + if ( dt_device_is_protected(dev) && need_mapping ) + { + dt_dprintk("%s setup iommu\n", dt_node_full_name(dev)); + res = iommu_assign_dt_device(d, dev); + if ( res ) + { + printk(XENLOG_ERR "Failed to setup the IOMMU for %s\n", + dt_node_full_name(dev)); + return res; + } + } + + res = handle_interrupts(d, dev, need_mapping); + if ( res < 0 ) + return res; + /* Give permission and map MMIOs */ for ( i = 0; i < naddr; i++ ) {
Move the interrupt handling code out of handle_device to a new function so that it can be reused for dom0less VMs later. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Changes in v3: - add patch The diff is hard to read but I just moved the interrupts related code from handle_devices to a new function handle_interrupts, and very little else. --- xen/arch/arm/domain_build.c | 79 +++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 30 deletions(-)