xen: arm: Parse PCI DT nodes' ranges and interrupt-map
v3 cover xen: arm: Parse PCI DT nodes' ranges and interrupt-map
This series adds parsing of the DT ranges and interrupt-map properties for PCI devices, these contain the MMIOs and IRQs used by children on the bus. This replaces the specific mapping stuff on xgene.
Since last time I've added a dt_for_each_range helper, which cleans things up considerably and included the PCI specific dt_bus entries from Linux (not strictly required AFAICT, but I think a good thing to do.
The patch "xen: dt: add dt_translate_address to translate a raw address" is no longer needed, so I've dropped it.
Chen, I've not included your Tested-by from last time around since I think things here differ enough to have invalidated it.
Ian.
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
PATCH v3 1/5 xen: dt: add dt_for_each_irq_map helper
This function iterates over a nodes interrupt-map property and calls a callback for each interrupt. For now it only supplies the raw IRQ since my use case has no need of e.g. child unit address. These can be added as needed by any future users.
This follows much the same logic as dt_irq_map_raw when parsing the interrupt-map, but doesn't walk up the tree doing the actual translation and it iterates over all entries instead of just looking for the first match.
I looked into refactoring dt_irq_map_raw but I couldn't find a way which I was confident in, plus I was reluctant to diverge from the Linux roots of this function any further.
--- xen/common/device_tree.c | 140 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 12 ++++ 2 files changed, 152 insertions(+)
xen/common/device_tree.c +140 -0
@@ -811,6 +811,146 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)return (psize / onesize); } +int dt_for_each_irq_map (const struct dt_device_node *dev, +int (*cb)(const struct dt_device_node *, +const struct dt_raw_irq *, +void *), +void *data) +{ +const struct dt_device_node *ipar, *tnode, *old =NULL ; +const __be32 *tmp, *imap; +u32 intsize =1 , addrsize, pintsize =0 , paddrsize =0 ; +u32 imaplen; +int i, ret; + +struct dt_raw_irq dt_raw_irq; + +dt_dprintk ("%s: par=%s cb=%p data=%p\n" , __func__, + dev->full_name, cb, data); + + ipar = dev; + +/* First get the #interrupt-cells property of the current cursor +* that tells us how to interpret the passed-in intspec. If there +* is none, we are nice and just walk up the tree +*/ +do { + tmp =dt_get_property (ipar,"#interrupt-cells" ,NULL ); +if ( tmp !=NULL ) + { + intsize =be32_to_cpu (*tmp); +break ; + } + tnode = ipar; + ipar =dt_irq_find_parent (ipar); + }while ( ipar ); +if ( ipar ==NULL ) + { +dt_dprintk (" -> no parent found !\n" ); + goto fail; + } + +dt_dprintk ("%s: ipar=%s, size=%d\n" , __func__, ipar->full_name, intsize); + +if ( intsize >DT_MAX_IRQ_SPEC ) + { +dt_dprintk (" -> too many irq specifier cells\n" ); + goto fail; + } + +/* Look for this #address-cells. We have to implement the old linux +* trick of looking for the parent here as some device-trees rely on it +*/ + old = ipar; +do { + tmp =dt_get_property (old,"#address-cells" ,NULL ); + tnode =dt_get_parent (old); + old = tnode; + }while ( old && tmp ==NULL ); + + old =NULL ; + addrsize = (tmp ==NULL ) ?2 :be32_to_cpu (*tmp); + +dt_dprintk (" -> addrsize=%d\n" , addrsize); + +/* Now look for an interrupt-map */ + imap =dt_get_property (dev,"interrupt-map" , &imaplen); +/* No interrupt map, check for an interrupt parent */ +if ( imap ==NULL ) + { +dt_dprintk (" -> no map, ignoring\n" ); + goto fail; + } + imaplen /=sizeof (u32); + +/* Parse interrupt-map */ +while ( imaplen > (addrsize + intsize +1 ) ) + { +/* skip child unit address and child interrupt specifier */ + imap += addrsize + intsize; + imaplen -= addrsize + intsize; + +/* Get the interrupt parent */ + ipar =dt_find_node_by_phandle (be32_to_cpup (imap)); + imap++; + --imaplen; + +/* Check if not found */ +if ( ipar ==NULL ) + { +dt_dprintk (" -> imap parent not found !\n" ); + goto fail; + } + +dt_dprintk (" -> ipar %s\n" ,dt_node_name (ipar)); + +/* Get #interrupt-cells and #address-cells of new +* parent +*/ + tmp =dt_get_property (ipar,"#interrupt-cells" ,NULL ); +if ( tmp ==NULL ) + { +dt_dprintk (" -> parent lacks #interrupt-cells!\n" ); + goto fail; + } + pintsize =be32_to_cpu (*tmp); + tmp =dt_get_property (ipar,"#address-cells" ,NULL ); + paddrsize = (tmp ==NULL ) ?0 :be32_to_cpu (*tmp); + +dt_dprintk (" -> pintsize=%d, paddrsize=%d\n" , + pintsize, paddrsize); + +/* Check for malformed properties */ +if ( imaplen < (paddrsize + pintsize) ) + goto fail; + + imap += paddrsize; + imaplen -= paddrsize; + + dt_raw_irq.controller = ipar; + dt_raw_irq.size = pintsize; +for ( i =0 ; i < pintsize; i++ ) + dt_raw_irq.specifier[i] =dt_read_number (imap + i,1 ); + + ret =cb (dev, &dt_raw_irq, data); +if ( ret <0 ) + { +dt_dprintk (" -> callback failed=%d\n" , ret); +return ret; + } + + imap += pintsize; + imaplen -= pintsize; + +dt_dprintk (" -> imaplen=%d\n" , imaplen); + } + +return 0 ; + +fail: +return -EINVAL ; +} +/** * dt_irq_map_raw - Low level interrupt tree parsing * @parent: the device interrupt parent
xen/include/xen/device_tree.h +12 -0
@@ -528,6 +528,18 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,int dt_irq_translate (const struct dt_raw_irq *raw,struct dt_irq *out_irq);/** +* dt_for_each_irq_map - Iterate over a nodes interrupt-map property +* @dev: The node whose interrupt-map property should be iterated over +* @cb: Call back to call for each entry +* @data: Caller data passed to callback +*/ +int dt_for_each_irq_map (const struct dt_device_node *dev, +int (*cb)(const struct dt_device_node *, +const struct dt_raw_irq *, +void *), +void *data); + +/** * dt_n_size_cells - Helper to retrieve the number of cell for the size * @np: node to get the value *
Signature · 1 line
1.7.10.4
Re: [PATCH v3 1/5] xen: dt: add dt_for_each_irq_map helper
Hi Ian,
Quoted 25 lines · Ian Campbell
This function iterates over a nodes interrupt-map property and calls a callback for each interrupt. For now it only supplies the raw IRQ since my use case has no need of e.g. child unit address. These can be added as needed by any future users.
This follows much the same logic as
dt_irq_map_rawwhen parsing the interrupt-map, but doesn't walk up the tree doing the actual translation and it iterates over all entries instead of just looking for the first match.I looked into refactoring
dt_irq_map_rawbut I couldn't find a way which I was confident in, plus I was reluctant to diverge from the Linux roots of this function any further.Signed-off-by Ian Campbell <[email protected]>--- xen/common/device_tree.c | 140 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 12 ++++ 2 files changed, 152 insertions(+)xen/common/device_tree.c +0
-0
xen/common/device_tree.c unsigned int dt_number_of_address(const struct dt_device_node *dev) · line 816
Quoted 6 lines
xen/common/device_tree.c +3
-0@@ -811,6 +811,146 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)return (psize / onesize); } +int dt_for_each_irq_map(const struct dt_device_node *dev, + int (*cb)(const struct dt_device_node *, + const struct dt_raw_irq *,
Do you plan to have callers which require the raw IRQ? If not, I would directly pass the translated IRQ (i.e dt_irq) to the callback.
xen/common/device_tree.c unsigned int dt_number_of_address(const struct dt_device_node *dev) · line 819
Quoted 3 lines
xen/common/device_tree.c +3
-0+ void *), + void *data) +{
[..]
The implementation looks good to me. Although, I didn't read closely.
Quoted 7 lines
xen/include/xen/device_tree.h +0
-0@@ -528,6 +528,18 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,int dt_irq_translate (const struct dt_raw_irq *raw,struct dt_irq *out_irq);
xen/include/xen/device_tree.h int dt_device_get_raw_irq(const struct dt_device_node *device, · line 539
Quoted 10 lines
xen/include/xen/device_tree.h +9
-0/** +* dt_for_each_irq_map - Iterate over a nodes interrupt-map property +* @dev: The node whose interrupt-map property should be iterated over +* @cb: Call back to call for each entry +* @data: Caller data passed to callback +*/ +int dt_for_each_irq_map(const struct dt_device_node *dev, + int (*cb)(const struct dt_device_node *, + const struct dt_raw_irq *, + void *),
I would add a typedef for the callback.
Quoted 7 lines
xen/include/xen/device_tree.h +3
-0+ void *data); + +/** * dt_n_size_cells - Helper to retrieve the number of cell for the size * @np: node to get the value *
Signature · 1 line
Julien Grall
Re: [PATCH v3 1/5] xen: dt: add dt_for_each_irq_map helper
Quoted 31 lines · Julien Grall
Hi Ian,
Quoted 28 lines · Ian Campbell
This function iterates over a nodes interrupt-map property and calls a callback for each interrupt. For now it only supplies the raw IRQ since my use case has no need of e.g. child unit address. These can be added as needed by any future users.
This follows much the same logic as
dt_irq_map_rawwhen parsing the interrupt-map, but doesn't walk up the tree doing the actual translation and it iterates over all entries instead of just looking for the first match.I looked into refactoring
dt_irq_map_rawbut I couldn't find a way which I was confident in, plus I was reluctant to diverge from the Linux roots of this function any further.Signed-off-by Ian Campbell <[email protected]>--- xen/common/device_tree.c | 140 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 12 ++++ 2 files changed, 152 insertions(+)xen/common/device_tree.c +0
-0@@ -811,6 +811,146 @@ unsigned int dt_number_of_address(const struct dt_device_node *dev)return (psize / onesize); }
Quoted 6 lines
xen/common/device_tree.c unsigned int dt_number_of_address(const struct dt_device_node *dev) · line 816
Quoted 3 lines
xen/common/device_tree.c +3
-0+int dt_for_each_irq_map(const struct dt_device_node *dev, + int (*cb)(const struct dt_device_node *, + const struct dt_raw_irq *,Do you plan to have callers which require the raw IRQ? If not, I would directly pass the translated IRQ (i.e
dt_irq) to the callback.
Sure.
Quoted 16 lines
xen/common/device_tree.c unsigned int dt_number_of_address(const struct dt_device_node *dev) · line 819
Quoted 4 lines
xen/common/device_tree.c +3
-0+ void *), + void *data) +{[..]
The implementation looks good to me. Although, I didn't read closely.
Quoted 7 lines
xen/include/xen/device_tree.h +0
-0@@ -528,6 +528,18 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,int dt_irq_translate (const struct dt_raw_irq *raw,struct dt_irq *out_irq);
Quoted 12 lines
xen/include/xen/device_tree.h int dt_device_get_raw_irq(const struct dt_device_node *device, · line 539
Quoted 10 lines
xen/include/xen/device_tree.h +9
-0/** +* dt_for_each_irq_map - Iterate over a nodes interrupt-map property +* @dev: The node whose interrupt-map property should be iterated over +* @cb: Call back to call for each entry +* @data: Caller data passed to callback +*/ +int dt_for_each_irq_map(const struct dt_device_node *dev, + int (*cb)(const struct dt_device_node *, + const struct dt_raw_irq *, + void *),I would add a typedef for the callback.
It's only used in this one place, so I don't think it's really needed and the obvious name dt_for_each_irq_map_cb is super clumsy.
Also you didn't say the same thing to the cb in the next patch ;-).
Ian.
Re: [PATCH v3 1/5] xen: dt: add dt_for_each_irq_map helper
Quoted 20 lines · Ian Campbell
Quoted 7 lines
Quoted 7 lines
xen/include/xen/device_tree.h +0
-0@@ -528,6 +528,18 @@ int dt_device_get_raw_irq(const struct dt_device_node *device,int dt_irq_translate (const struct dt_raw_irq *raw,struct dt_irq *out_irq);Quoted 12 lines
xen/include/xen/device_tree.h int dt_device_get_raw_irq(const struct dt_device_node *device, · line 539
Quoted 10 lines
xen/include/xen/device_tree.h +9
-0/** +* dt_for_each_irq_map - Iterate over a nodes interrupt-map property +* @dev: The node whose interrupt-map property should be iterated over +* @cb: Call back to call for each entry +* @data: Caller data passed to callback +*/ +int dt_for_each_irq_map(const struct dt_device_node *dev, + int (*cb)(const struct dt_device_node *, + const struct dt_raw_irq *, + void *),I would add a typedef for the callback.
Quoted 4 lines
It's only used in this one place, so I don't think it's really needed and the obvious name
dt_for_each_irq_map_cbis super clumsy.Also you didn't say the same thing to the cb in the next patch ;-).
I don't mind you if you don't add a typedef. It was only for avoid duplicating the type everytime.
Regards,
Signature · 1 line
Julien Grall
PATCH v3 2/5 xen: dt: add dt_for_each_range helper
In reply to Ian CampbellThis function iterates over a node's ranges property and calls a callback for each region. For now it only supplies the MMIO range (in terms of CPU addresses, i.e. already translated).
--- xen/common/device_tree.c | 85 +++++++++++++++++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 12 ++++++ 2 files changed, 97 insertions(+)
xen/common/device_tree.c +85 -0
@@ -704,6 +704,91 @@ int dt_device_get_address(const struct dt_device_node *dev, unsigned int index,return 0 ; } + +int dt_for_each_range (const struct dt_device_node *dev, +int (*cb)(const struct dt_device_node *, +u64 addr,u64 length, +void *), +void *data) +{ +const struct dt_device_node *parent =NULL ; +const struct dt_bus *bus, *pbus; +const __be32 *ranges; +__be32 addr[DT_MAX_ADDR_CELLS ]; +unsigned int rlen; +int na, ns, pna, pns, rone; + + bus =dt_match_bus (dev); +if ( !bus ) +return 0 ;/* device is not a bus */ + + parent =dt_get_parent (dev); +if ( parent ==NULL ) +return -EINVAL ; + + ranges =dt_get_property (dev,"ranges" , &rlen); +if ( ranges ==NULL ) + { +printk (XENLOG_ERR "DT: no ranges; cannot enumerate\n" ); +return -EINVAL ; + } +if ( rlen ==0 )/* Nothing to do */ +return 0 ; + + bus->count_cells (dev, &na, &ns); +if ( !DT_CHECK_COUNTS (na, ns) ) + { +printk (XENLOG_ERR "dt_parse: Bad cell count for device %s\n" , + dev->full_name); +return -EINVAL ; + } + + pbus =dt_match_bus (parent); +if ( pbus ==NULL ) + { +printk ("DT: %s is not a valid bus\n" , parent->full_name); +return -EINVAL ; + } + + pbus->count_cells (dev, &pna, &pns); +if ( !DT_CHECK_COUNTS (pna, pns) ) + { +printk (XENLOG_ERR "dt_parse: Bad cell count for parent %s\n" , + dev->full_name); +return -EINVAL ; + } + +/* Now walk through the ranges */ + rlen /=4 ; + rone = na + pna + ns; + +dt_dprintk ("%s: dev=%s, bus=%s, parent=%s, rlen=%d, rone=%d\n" , + __func__, +dt_node_name (dev), bus->name, +dt_node_name (parent), rlen, rone); + +for ( ; rlen >= rone; rlen -= rone, ranges += rone ) + { +u64 a, s; +int ret; + +memcpy (addr, ranges + na,4 * pna); + + a =__dt_translate_address (dev, addr,"ranges" ); + s =dt_read_number (ranges + na + pna, ns); + + ret =cb (dev, a, s, data); +if ( ret <0 ) + { +dt_dprintk (" -> callback failed=%d\n" , ret); +return ret; + } + + } + +return 0 ; +} +/** * dt_find_node_by_phandle - Find a node given a phandle * @handle: phandle of the node to find
xen/include/xen/device_tree.h +12 -0
@@ -540,6 +540,18 @@ int dt_for_each_irq_map(const struct dt_device_node *dev, void *data);/** +* dt_for_each_range - Iterate over a nodes ranges property +* @dev: The node whose interrupt-map property should be iterated over +* @cb: Call back to call for each entry +* @data: Caller data passed to callback +*/ +int dt_for_each_range (const struct dt_device_node *dev, +int (*cb)(const struct dt_device_node *, +u64 addr,u64 length, +void *), +void *data); + +/** * dt_n_size_cells - Helper to retrieve the number of cell for the size * @np: node to get the value *
Signature · 1 line
1.7.10.4
Re: [PATCH v3 2/5] xen: dt: add dt_for_each_range helper
Hi Ian,
Quoted 5 lines · Ian Campbell
This function iterates over a node's ranges property and calls a callback for each region. For now it only supplies the MMIO range (in terms of CPU addresses, i.e. already translated).
Signed-off-by Ian Campbell <[email protected]>
Regards,
Signature · 1 line
Julien Grall
PATCH v3 3/5 xen: arm: slightly refactor gic DT node creation for domain 0
In reply to Ian Campbellall gics.
Although in principal #interrupt-cells can vary it must always be 3 for a GIC (and we only support GIC as a guest interrupt controller), so putting it in common code is OK.
---
v3: Retitle and reword, was "xen: arm: propagate gic's
#interrupt-cells property to dom0."
--- xen/arch/arm/domain_build.c | 8 ++++++++ xen/arch/arm/gic-hip04.c | 9 --------- xen/arch/arm/gic-v2.c | 9 --------- xen/arch/arm/gic-v3.c | 8 -------- 4 files changed, 8 insertions(+), 26 deletions(-)
xen/arch/arm/domain_build.c +8 -0
@@ -847,6 +847,14 @@ static int make_gic_node(const struct domain *d, void *fdt,return res; } + res =fdt_property_cell (fdt,"#interrupt-cells" ,3 ); +if ( res ) +return res; + + res =fdt_property (fdt,"interrupt-controller" ,NULL ,0 ); +if ( res ) +return res; + res =fdt_end_node (fdt);return res;
xen/arch/arm/gic-hip04.c +0 -9
@@ -636,15 +636,6 @@ static int hip04gic_make_dt_node(const struct domain *d,if ( res )return res; - res =fdt_property_cell (fdt,"#interrupt-cells" ,3 ); -if ( res ) -return res; - - res =fdt_property (fdt,"interrupt-controller" ,NULL ,0 ); - -if ( res ) -return res; -/* * DTB provides up to 4 regions to handle virtualization * (in order GICD, GICC, GICH and GICV interfaces)
xen/arch/arm/gic-v2.c +0 -9
@@ -624,15 +624,6 @@ static int gicv2_make_dt_node(const struct domain *d,if ( res )return res; - res =fdt_property_cell (fdt,"#interrupt-cells" ,3 ); -if ( res ) -return res; - - res =fdt_property (fdt,"interrupt-controller" ,NULL ,0 ); - -if ( res ) -return res; -/* * DTB provides up to 4 regions to handle virtualization * (in order GICD, GICC, GICH and GICV interfaces)
xen/arch/arm/gic-v3.c +0 -8
@@ -1106,14 +1106,6 @@ static int gicv3_make_dt_node(const struct domain *d,if ( res )return res; - res =fdt_property_cell (fdt,"#interrupt-cells" ,3 ); -if ( res ) -return res; - - res =fdt_property (fdt,"interrupt-controller" ,NULL ,0 ); -if ( res ) -return res; - res =dt_property_read_u32 (gic,"redistributor-stride" , &rd_stride);if ( !res ) rd_stride =0 ;
Signature · 1 line
1.7.10.4
Re: [PATCH v3 3/5] xen: arm: slightly refactor gic DT node creation for domain 0
Hi Ian,
Quoted 1 line · Ian Campbell
all gics.
Shouldn't it belong to the subject?
Quoted 5 lines
Although in principal #interrupt-cells can vary it must always be 3 for a GIC (and we only support GIC as a guest interrupt controller), so putting it in common code is OK.
Signed-off-by Ian Campbell <[email protected]>
Other than the subject title:
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH v3 3/5] xen: arm: slightly refactor gic DT node creation for domain 0
Quoted 6 lines · Julien Grall
Hi Ian,
Quoted 1 line · Ian Campbell
all gics.
Shouldn't it belong to the subject?
It doesn't seem to fit their either. I'm not really sure what it was supposed to be, perhaps a remnant of a half finished thought I intended to delete. I've just dropped it.
Quoted 10 lines
Quoted 6 lines
Although in principal #interrupt-cells can vary it must always be 3 for a GIC (and we only support GIC as a guest interrupt controller), so putting it in common code is OK.
Signed-off-by Ian Campbell <[email protected]>Other than the subject title:
Reviewed-by Julien Grall <[email protected]>
Thanks, I've kept this even though I dropped those words rather than moving them, let me know if this is a problem.
Ian.
PATCH v3 4/5 xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
In reply to Ian CampbellThis uses the dt_for_each_{irq_map,range} helpers to map the interrupt and child MMIO regions to dom0. Since PCI busses are enumerable these resources may not be otherwise described in the DT (although they can be).
Although PCI is the only bus we handle this way the code should be generic enough to apply to similar buses in the future.
This replaces the xgene specific mapping. Tested on Mustang and on a model with a PCI virtio controller.
This patch doesn't stop recursing when it finds such a node, since double mapping these resources if they do happen to be described is (or should be) harmless
---
v2: This is essentially a complete reworking, which actually parses things properly (obeying #{address,size,interrupt}-cells on the appriopriate nodes) and includes handling of interrupt-map too.
v3: Use dt_for_each_ranges and refactor handling into
handle_device_children.
Retitled from "xen: arm: handle PCI DT node ranges and interrupt-map properties" and rewrote much of the commit message.
This change also essentially obsoleted any discussion about logging unhandled buses since it no longer makes sense (or at least there is nothing convenient to hang it off)
--- xen/arch/arm/domain_build.c | 101 ++++++++++++++++++++++++ xen/arch/arm/platforms/xgene-storm.c | 143 ---------------------------------- 2 files changed, 101 insertions(+), 143 deletions(-)
xen/arch/arm/domain_build.c +101 -0
@@ -935,6 +935,103 @@ static int make_timer_node(const struct domain *d, void *fdt,return res; } +static int map_interrupt_to_domain (const struct dt_device_node *dev, +const struct dt_raw_irq *dt_raw_irq, +void *data) +{ +struct domain *d = data; +struct dt_irq dt_irq; +int res; + + res =dt_irq_translate (dt_raw_irq, &dt_irq); +if ( res <0 ) + { +printk (XENLOG_ERR "%s: Failed to translate IRQ: %d\n" , +dt_node_name (dev), res); +return res; + } + +if ( dt_irq.irq <NR_LOCAL_IRQS ) + { +printk (XENLOG_ERR "%s: IRQ%" PRId32" is not a SPI\n" , +dt_node_name (dev), dt_irq.irq); +return -EINVAL ; + } + +/* Setup the IRQ type */ + res =irq_set_spi_type (dt_irq.irq, dt_irq.type); +if ( res ) + { +printk (XENLOG_ERR +"%s: Unable to setup IRQ%" PRId32" to dom%d\n" , +dt_node_name (dev), dt_irq.irq, d->domain_id); +return res; + } + + res =route_irq_to_guest (d, dt_irq.irq, dt_irq.irq,dt_node_name (dev)); +if ( res <0 ) + { +printk (XENLOG_ERR "Unable to map IRQ%" PRId32" to dom%d\n" , + dt_irq.irq, d->domain_id); +return res; + } + +DPRINT (" - IRQ: %u\n" , dt_irq.irq); + +return 0 ; +} + +static int map_range_to_domain (const struct dt_device_node *dev, +u64 addr,u64 len, +void *data) +{ +struct domain *d = data; +int res; + + res =map_mmio_regions (d, +paddr_to_pfn (addr &PAGE_MASK ), +DIV_ROUND_UP (len,PAGE_SIZE ), +paddr_to_pfn (addr &PAGE_MASK )); +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; + } + +DPRINT (" - MMIO: %010" PRIx64" - %010" PRIx64"\n" , addr, addr+len); + +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 +* the child resources available to domain 0. +*/ +static int map_device_children (struct domain *d, +const struct dt_device_node *dev) +{ +int ret; + +if (dt_device_type_is_equal (dev,"pci" ) ) + { +DPRINT ("Mapping children of %s to guest\n" ,dt_node_full_name (dev)); + + ret =dt_for_each_irq_map (dev, &map_interrupt_to_domain, d); +if ( ret <0 ) +return ret; + + ret =dt_for_each_range (dev, &map_range_to_domain, d); +if ( ret <0 ) +return ret; + } + +return 0 ; +} +/* * For a given device node: * - Give permission to the guest to manage IRQ and MMIO range
@@ -1080,6 +1177,10 @@ static int handle_device(struct domain *d, struct dt_device_node *dev) } } + res =map_device_children (d, dev); +if ( res ) +return res; +return 0 ; }
xen/arch/arm/platforms/xgene-storm.c +0 -143
155 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Re: [PATCH v3 4/5] xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
Hi Ian,
Quoted 4 lines · Ian Campbell
xen/arch/arm/domain_build.c +4
-0+static int map_interrupt_to_domain (const struct dt_device_node *dev, +const struct dt_raw_irq *dt_raw_irq, +void *data) +{
[..]
xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 977
Quoted 7 lines
xen/arch/arm/domain_build.c +7
-0+ res =route_irq_to_guest (d, dt_irq.irq, dt_irq.irq,dt_node_name (dev)); +if ( res <0 ) + { +printk (XENLOG_ERR "Unable to map IRQ%" PRId32" to dom%d\n" , + dt_irq.irq, d->domain_id); +return res; + }
You forgot to give the IRQ permission to the domain.
Also I'm wondering if we should care about passthrough the PCI controller to a guest. If so, you would need to check need_mapping as we do in handle_device.
If not, I would add a warning if the property "xen,passthrough" exists in the node telling that we won't honor it.
Quoted 16 lines
xen/arch/arm/domain_build.c +16
-0+DPRINT (" - IRQ: %u\n" , dt_irq.irq); + +return 0 ; +} + +static int map_range_to_domain (const struct dt_device_node *dev, +u64 addr,u64 len, +void *data) +{ +struct domain *d = data; +int res; + + res =map_mmio_regions (d, +paddr_to_pfn (addr &PAGE_MASK ), +DIV_ROUND_UP (len,PAGE_SIZE ), +paddr_to_pfn (addr &PAGE_MASK ));
xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 1004
Quoted 10 lines
xen/arch/arm/domain_build.c +10
-0+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; + } + +DPRINT (" - MMIO: %010" PRIx64" - %010" PRIx64"\n" , addr, addr+len);
NIT: mising space "addr + len"
And same question as for the interrupt.
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH v3 4/5] xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
Quoted 18 lines · Julien Grall
Hi Ian,
Quoted 4 lines · Ian Campbell
xen/arch/arm/domain_build.c +4
-0+static int map_interrupt_to_domain (const struct dt_device_node *dev, +const struct dt_raw_irq *dt_raw_irq, +void *data) +{[..]
xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 977
Quoted 7 lines
xen/arch/arm/domain_build.c +7
-0+ res =route_irq_to_guest (d, dt_irq.irq, dt_irq.irq,dt_node_name (dev)); +if ( res <0 ) + { +printk (XENLOG_ERR "Unable to map IRQ%" PRId32" to dom%d\n" , + dt_irq.irq, d->domain_id); +return res; + }
Quoted 1 line
You forgot to give the IRQ permission to the domain.
Oops! I suppose that would only be noticeable when trying to pass it through which is why I didn't see it?
Quoted 6 lines
Also I'm wondering if we should care about passthrough the PCI controller to a guest. If so, you would need to check
need_mappingas we do inhandle_device.If not, I would add a warning if the property "xen,passthrough" exists in the node telling that we won't honor it.
My initial reaction was that no good can come from passing through the host controller itself, and it might have interesting interactions with the PCI infrastructure to be added in Xen (as described in a thread with Manish which I can't find right now).
So I think I'll take the coward's way out for now and check for xen,passthrough.
Ian.
Re: [PATCH v3 4/5] xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
Quoted 19 lines · Ian Campbell
Quoted 18 lines · Julien Grall
Hi Ian,
Quoted 4 lines · Ian Campbell
xen/arch/arm/domain_build.c +4
-0+static int map_interrupt_to_domain (const struct dt_device_node *dev, +const struct dt_raw_irq *dt_raw_irq, +void *data) +{[..]
Quoted 7 lines
xen/arch/arm/domain_build.c +7
-0+ res =route_irq_to_guest (d, dt_irq.irq, dt_irq.irq,dt_node_name (dev)); +if ( res <0 ) + { +printk (XENLOG_ERR "Unable to map IRQ%" PRId32" to dom%d\n" , + dt_irq.irq, d->domain_id); +return res; + }
Quoted 4 lines
Quoted 1 line
You forgot to give the IRQ permission to the domain.
Oops! I suppose that would only be noticeable when trying to pass it through which is why I didn't see it?
Right. I would only notice during PCI-passthrough which is not yet supported.
Signature · 1 line
Julien Grall
Re: [PATCH v3 4/5] xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
In reply to Julien GrallQuoted 18 lines · Julien Grall
Hi Ian,
Quoted 4 lines · Ian Campbell
xen/arch/arm/domain_build.c +4
-0+static int map_interrupt_to_domain (const struct dt_device_node *dev, +const struct dt_raw_irq *dt_raw_irq, +void *data) +{[..]
xen/arch/arm/domain_build.c static int make_timer_node(const struct domain *d, void *fdt, · line 977
Quoted 7 lines
xen/arch/arm/domain_build.c +7
-0+ res =route_irq_to_guest (d, dt_irq.irq, dt_irq.irq,dt_node_name (dev)); +if ( res <0 ) + { +printk (XENLOG_ERR "Unable to map IRQ%" PRId32" to dom%d\n" , + dt_irq.irq, d->domain_id); +return res; + }
Quoted 1 line
You forgot to give the IRQ permission to the domain.
IOW I should call irq_permit_access, between irq_set_spi_type and route_irq_to_guest I think?
Should I be calling vgic_reserve_virq here as well? Other similar looking code does.
It seems that handle_device() calls vgic_reserve_virq twice with the same arguments (once conditionally, the other not). Is that a rebase-o? Seems to have happened in cb818a58f6f785bb41a35bc1d1e52c66ffbe1e8b.
Ian.
Re: [PATCH v3 4/5] xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
Hi Ian,
Quoted 19 lines · Ian Campbell
Quoted 18 lines · Julien Grall
Hi Ian,
Quoted 4 lines · Ian Campbell
xen/arch/arm/domain_build.c +4
-0+static int map_interrupt_to_domain (const struct dt_device_node *dev, +const struct dt_raw_irq *dt_raw_irq, +void *data) +{[..]
Quoted 7 lines
xen/arch/arm/domain_build.c +7
-0+ res =route_irq_to_guest (d, dt_irq.irq, dt_irq.irq,dt_node_name (dev)); +if ( res <0 ) + { +printk (XENLOG_ERR "Unable to map IRQ%" PRId32" to dom%d\n" , + dt_irq.irq, d->domain_id); +return res; + }
Quoted 4 lines
Quoted 1 line
You forgot to give the IRQ permission to the domain.
IOW I should call
irq_permit_access, betweenirq_set_spi_typeandroute_irq_to_guestI think?
I can call here. Although it doesn't matter has irq_permit_access is not used in route_irq_to_guest.
Quoted 2 lines
Should I be calling
vgic_reserve_virqhere as well? Other similar looking code does.
Yes I forgot to tell you about it.
Quoted 3 lines
It seems that
handle_device()callsvgic_reserve_virqtwice with the same arguments (once conditionally, the other not). Is that a rebase-o? Seems to have happened incb818a58f6f785bb41a35bc1d1e52c66ffbe1e8b.
Hmmm... yes. I didn't pay enough attention when I rebased this patch, sorry.
The first call to vgic_reserve_virq should be dropped. It's only necessary to reserve the VIRQ when the domain is using the IRQ (i.e need_mapping == 1).
Do you plan to send a patch for it?
Regards,
Signature · 1 line
Julien Grall
Re: [PATCH v3 4/5] xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
Quoted 39 lines · Julien Grall
Hi Ian,
Quoted 19 lines · Ian Campbell
Quoted 18 lines · Julien Grall
Hi Ian,
Quoted 4 lines · Ian Campbell
xen/arch/arm/domain_build.c +4
-0+static int map_interrupt_to_domain (const struct dt_device_node *dev, +const struct dt_raw_irq *dt_raw_irq, +void *data) +{[..]
Quoted 7 lines
xen/arch/arm/domain_build.c +7
-0+ res =route_irq_to_guest (d, dt_irq.irq, dt_irq.irq,dt_node_name (dev)); +if ( res <0 ) + { +printk (XENLOG_ERR "Unable to map IRQ%" PRId32" to dom%d\n" , + dt_irq.irq, d->domain_id); +return res; + }Quoted 4 lines
Quoted 1 line
You forgot to give the IRQ permission to the domain.
IOW I should call
irq_permit_access, betweenirq_set_spi_typeandroute_irq_to_guestI think?I can call here. Although it doesn't matter has
irq_permit_accessis not used inroute_irq_to_guest.Quoted 2 lines
Should I be calling
vgic_reserve_virqhere as well? Other similar looking code does.Yes I forgot to tell you about it.
Quoted 3 lines
It seems that
handle_device()callsvgic_reserve_virqtwice with the same arguments (once conditionally, the other not). Is that a rebase-o? Seems to have happened incb818a58f6f785bb41a35bc1d1e52c66ffbe1e8b.
Quoted 7 lines
Hmmm... yes. I didn't pay enough attention when I rebased this patch, sorry.
The first call to
vgic_reserve_virqshould be dropped. It's only necessary to reserve the VIRQ when the domain is using the IRQ (i.eneed_mapping== 1).Do you plan to send a patch for it?
Yes, I'll tack it on the front of this series.
Ian.
PATCH v3 5/5 xen: arm: Import of_bus PCI entry from Linux (as a dt_bus entry)
In reply to Ian CampbellThis provides specific handlers for the PCI bus relating to matching and translating. It's mostly similar to the defaults but includes some additional error checks and other PCI specific bits.
Functions/types renamed and reindented (because apparently we do that for these).
Needs a selection of IORESOURCE_* defines, which I've taken from Linux and have included locally for now until we figure out where else they might be needed.
---
v3: New patch
--- xen/common/device_tree.c | 108 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 105 insertions(+), 3 deletions(-)
xen/common/device_tree.c +105 -3
@@ -399,6 +399,26 @@ int dt_n_size_cells(const struct dt_device_node *np) }/* +* These are defined in Linux where much of this code comes from, but +* are currently unused outside this file in the context of Xen. +*/ +#define IORESOURCE_BITS 0x000000ff/* Bus-specific bits */ + +#define IORESOURCE_TYPE_BITS 0x00001f00/* Resource type */ +#define IORESOURCE_IO 0x00000100/* PCI/ISA I/O ports */ +#define IORESOURCE_MEM 0x00000200 +#define IORESOURCE_REG 0x00000300/* Register offsets */ +#define IORESOURCE_IRQ 0x00000400 +#define IORESOURCE_DMA 0x00000800 +#define IORESOURCE_BUS 0x00001000 + +#define IORESOURCE_PREFETCH 0x00002000/* No side effects */ +#define IORESOURCE_READONLY 0x00004000 +#define IORESOURCE_CACHEABLE 0x00008000 +#define IORESOURCE_RANGELENGTH 0x00010000 +#define IORESOURCE_SHADOWABLE 0x00020000 + +/* * Default translator (generic bus) */ static bool_t dt_bus_default_match (const struct dt_device_node *node)
@@ -462,9 +482,81 @@ static int dt_bus_default_translate(__be32 *addr, u64 offset, int na) }static unsigned int dt_bus_default_get_flags (const __be32 *addr) { -/* TODO: Return the type of memory (device, ...) for caching -* attribute during mapping */ -return 0 ; +return IORESOURCE_MEM ; +} + +/* +* PCI bus specific translator +*/ + +static bool_t dt_bus_pci_match (const struct dt_device_node *np) +{ +/* +* "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI +* powermacs "ht" is hypertransport +*/ +return !strcmp (np->type,"pci" ) || !strcmp (np->type,"pciex" ) || + !strcmp (np->type,"vci" ) || !strcmp (np->type,"ht" ); +} + +static void dt_bus_pci_count_cells (const struct dt_device_node *np, +int *addrc,int *sizec) +{ +if (addrc) + *addrc =3 ; +if (sizec) + *sizec =2 ; +} + +static unsigned int dt_bus_pci_get_flags (const __be32 *addr) +{ +unsigned int flags =0 ; +u32 w =be32_to_cpup (addr); + +switch ((w >>24 ) &0x03 ) { +case 0x01 : + flags |=IORESOURCE_IO ; +break ; +case 0x02 :/* 32 bits */ +case 0x03 :/* 64 bits */ + flags |=IORESOURCE_MEM ; +break ; + } +if (w &0x40000000 ) + flags |=IORESOURCE_PREFETCH ; +return flags; +} + +static u64 dt_bus_pci_map (__be32 *addr,const __be32 *range,int na,int ns, +int pna) +{ +u64 cp, s, da; +unsigned int af, rf; + + af =dt_bus_pci_get_flags (addr); + rf =dt_bus_pci_get_flags (range); + +/* Check address type match */ +if ((af ^ rf) & (IORESOURCE_MEM |IORESOURCE_IO )) +return DT_BAD_ADDR ; + +/* Read address values, skipping high cell */ + cp =dt_read_number (range +1 , na -1 ); + s =dt_read_number (range + na + pna, ns); + da =dt_read_number (addr +1 , na -1 ); + +dt_dprintk ("DT: PCI map, cp=%llx, s=%llx, da=%llx\n" , + (unsigned long long )cp, (unsigned long long )s, + (unsigned long long )da); + +if (da < cp || da >= (cp + s)) +return DT_BAD_ADDR ; +return da - cp; +} + +static int dt_bus_pci_translate (__be32 *addr,u64 offset,int na) +{ +return dt_bus_default_translate (addr +1 , offset, na -1 ); }/*
@@ -472,6 +564,16 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr)*/ static const struct dt_bus dt_busses[] = { +/* PCI */ + { + .name ="pci" , + .addresses ="assigned-addresses" , + .match = dt_bus_pci_match, + .count_cells = dt_bus_pci_count_cells, + .map = dt_bus_pci_map, + .translate = dt_bus_pci_translate, + .get_flags = dt_bus_pci_get_flags, + },/* Default */ { .name ="default" ,
Signature · 1 line
1.7.10.4
Quoted 8 lines · Ian Campbell
This series adds parsing of the DT ranges and interrupt-map properties for PCI devices, these contain the MMIOs and IRQs used by children on the bus. This replaces the specific mapping stuff on xgene.
Since last time I've added a
dt_for_each_rangehelper, which cleans things up considerably and included the PCI specificdt_busentries from Linux (not strictly required AFAICT, but I think a good thing to do.
Quoted 5 lines
The patch "xen: dt: add
dt_translate_addressto translate a raw address" is no longer needed, so I've dropped it.Chen, I've not included your Tested-by from last time around since I think things here differ enough to have invalidated it.
Ok. I'll test it again. (But a few days later, for I'll be away from my board in the next two days.)
Cheers,
Baozi.
Quoted 8 lines · Ian Campbell
This series adds parsing of the DT ranges and interrupt-map properties for PCI devices, these contain the MMIOs and IRQs used by children on the bus. This replaces the specific mapping stuff on xgene.
Since last time I've added a
dt_for_each_rangehelper, which cleans things up considerably and included the PCI specificdt_busentries from Linux (not strictly required AFAICT, but I think a good thing to do.
Quoted 6 lines
The patch "xen: dt: add
dt_translate_addressto translate a raw address" is no longer needed, so I've dropped it.Chen, I've not included your Tested-by from last time around since I think things here differ enough to have invalidated it.
This patch set should be OK. However, in order to test it, I have to rebase to the latest git tree, which seems to have something wrong with tool stack... (have no clues at present). Anyway, the PCI part is good.
Cheers,
Baozi.
Quoted 12 lines · Chen Baozi
Quoted 11 lines · Ian Campbell
This series adds parsing of the DT ranges and interrupt-map properties for PCI devices, these contain the MMIOs and IRQs used by children on the bus. This replaces the specific mapping stuff on xgene.
Since last time I've added a
dt_for_each_rangehelper, which cleans things up considerably and included the PCI specificdt_busentries from Linux (not strictly required AFAICT, but I think a good thing to do.The patch "xen: dt: add
dt_translate_addressto translate a raw address" is no longer needed, so I've dropped it.
Quoted 8 lines
Quoted 3 lines
Chen, I've not included your Tested-by from last time around since I think things here differ enough to have invalidated it.
Tested-by Chen Baozi <[email protected]>This patch set should be OK. However, in order to test it, I have to rebase to the latest git tree, which seems to have something wrong with tool stack...
report:
FATAL: sd_listen_fds() failed
: File exists
when trying to run xenstored.
Quoted 5 lines
(have no clues at present). Anyway, the PCI part is good.
Cheers,
Baozi.
Quoted 22 lines · Chen Baozi
Quoted 12 lines · Chen Baozi
Quoted 11 lines · Ian Campbell
This series adds parsing of the DT ranges and interrupt-map properties for PCI devices, these contain the MMIOs and IRQs used by children on the bus. This replaces the specific mapping stuff on xgene.
Since last time I've added a
dt_for_each_rangehelper, which cleans things up considerably and included the PCI specificdt_busentries from Linux (not strictly required AFAICT, but I think a good thing to do.The patch "xen: dt: add
dt_translate_addressto translate a raw address" is no longer needed, so I've dropped it.Quoted 8 lines
Quoted 3 lines
Chen, I've not included your Tested-by from last time around since I think things here differ enough to have invalidated it.
Tested-by Chen Baozi <[email protected]>This patch set should be OK. However, in order to test it, I have to rebase to the latest git tree, which seems to have something wrong with tool stack...
Quoted 6 lines
report:
FATAL:
sd_listen_fds()failed
: File existswhen trying to run xenstored.
Well. It is related to systemd on jessie. Disabling systemd when building tools helped.
Sorry for the noise.
Baozi.
Quoted 11 lines · Chen Baozi
Quoted 6 lines
report:
FATAL:
sd_listen_fds()failed
: File existswhen trying to run xenstored.
Well. It is related to systemd on jessie. Disabling systemd when building tools helped.
Sorry for the noise.
It's not really noise, it sounds like a bug in our systemd support? If that's the case (rather than, say, a local misconfiguration due to an unexpected switch to systemd) please could you report under a new thread with an appropriate title?
Ian.
Quoted 7 lines · Ian Campbell
Quoted 5 lines · Chen Baozi
Quoted 5 lines
report:
FATAL:
sd_listen_fds()failed
: File exists
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
when trying to run xenstored.
Well. It is related to systemd on jessie. Disabling systemd when building tools helped.
Sorry for the noise.
It's not really noise, it sounds like a bug in our systemd support?
Maybe not, I'm not sure. (Probably because of the buggy systemd of Debian 8)
Quoted 7 lines
If
that's the case (rather than, say, a local misconfiguration due to an unexpected switch to systemd) please could you report under a new thread with an appropriate title?Ian.