| Message ID | 20190821035315.12812-3-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/8] xen/arm: introduce handle_device_interrupts | expand |
Hi Stefano, On 8/21/19 4:53 AM, Stefano Stabellini wrote: > Instead of always hard-coding the GIC phandle (GUEST_PHANDLE_GIC), store > it in a variable under kinfo. This way it can be dynamically chosen per > domain. > > Initialize guest_phandle_gic to GUEST_PHANDLE_GIC at the beginning of > prepare_dtb_domU. Later patches will change the value of > guest_phandle_gic depending on user provided information. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > Changes in v4: > - new patch > --- > xen/arch/arm/domain_build.c | 42 ++++++++++++++++++++---------------- > xen/include/asm-arm/kernel.h | 3 +++ > 2 files changed, 27 insertions(+), 18 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index f92069c85f..cd585f05ca 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1510,8 +1510,9 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, > return res; > } > > -static int __init make_gicv2_domU_node(const struct domain *d, void *fdt) > +static int __init make_gicv2_domU_node(struct kernel_info *kinfo) It is a bit unclear why the first argument is dropped but not replace by the declaration of a variable. If it was never used before, then please mention it in the commit message. Similarly, please mention why the prototype has been changed. > { > + void *fdt = kinfo->fdt; > int res = 0; > __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; > __be32 *cells; > @@ -1546,11 +1547,11 @@ static int __init make_gicv2_domU_node(const struct domain *d, void *fdt) > if (res) > return res; > > - res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC); > + res = fdt_property_cell(fdt, "linux,phandle", kinfo->guest_phandle_gic); > if (res) > return res; > > - res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC); > + res = fdt_property_cell(fdt, "phandle", kinfo->guest_phandle_gic); > if (res) > return res; > > @@ -1559,8 +1560,9 @@ static int __init make_gicv2_domU_node(const struct domain *d, void *fdt) > return res; > } > > -static int __init make_gicv3_domU_node(const struct domain *d, void *fdt) > +static int __init make_gicv3_domU_node(struct kernel_info *kinfo) Ditto. > { > + void *fdt = kinfo->fdt; > int res = 0; > __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; > __be32 *cells; > @@ -1595,11 +1597,11 @@ static int __init make_gicv3_domU_node(const struct domain *d, void *fdt) > if (res) > return res; > > - res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC); > + res = fdt_property_cell(fdt, "linux,phandle", kinfo->guest_phandle_gic); > if (res) > return res; > > - res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC); > + res = fdt_property_cell(fdt, "phandle", kinfo->guest_phandle_gic); > if (res) > return res; > > @@ -1608,21 +1610,22 @@ static int __init make_gicv3_domU_node(const struct domain *d, void *fdt) > return res; > } [...] > -static int __init make_timer_domU_node(const struct domain *d, void *fdt) > +static int __init make_timer_domU_node(struct kernel_info *kinfo) This definitely needs some rebase on the latest staging to pick up the new prototype. Actually, the prototype was modified beginning of August but this was sent end of August... Please make sure you are using the latest staging at the time when sending a series. > { > + void *fdt = kinfo->fdt; > int res; > gic_interrupt_t intrs[3]; > > @@ -1630,7 +1633,7 @@ static int __init make_timer_domU_node(const struct domain *d, void *fdt) > if ( res ) > return res; > > - if ( !is_64bit_domain(d) ) > + if ( !is_64bit_domain(kinfo->d) ) > { > res = fdt_property_string(fdt, "compatible", "arm,armv7-timer"); > if ( res ) > @@ -1652,7 +1655,7 @@ static int __init make_timer_domU_node(const struct domain *d, void *fdt) > return res; > > res = fdt_property_cell(fdt, "interrupt-parent", > - GUEST_PHANDLE_GIC); > + kinfo->guest_phandle_gic); > if (res) > return res; > > @@ -1662,8 +1665,9 @@ static int __init make_timer_domU_node(const struct domain *d, void *fdt) > } > > #ifdef CONFIG_SBSA_VUART_CONSOLE > -static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) > +static int __init make_vpl011_uart_node(struct kernel_info *kinfo) > { > + void *fdt = kinfo->fdt; > int res; > gic_interrupt_t intr; > __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; > @@ -1694,7 +1698,7 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) > return res; > > res = fdt_property_cell(fdt, "interrupt-parent", > - GUEST_PHANDLE_GIC); > + kinfo->guest_phandle_gic); > if ( res ) > return res; > > @@ -1719,6 +1723,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > int addrcells, sizecells; > int ret; > > + kinfo->guest_phandle_gic = GUEST_PHANDLE_GIC; > + > addrcells = GUEST_ROOT_ADDRESS_CELLS; > sizecells = GUEST_ROOT_SIZE_CELLS; > > @@ -1762,11 +1768,11 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > if ( ret ) > goto err; > > - ret = make_gic_domU_node(d, kinfo->fdt); > + ret = make_gic_domU_node(kinfo); > if ( ret ) > goto err; > > - ret = make_timer_domU_node(d, kinfo->fdt); > + ret = make_timer_domU_node(kinfo); > if ( ret ) > goto err; > > @@ -1774,7 +1780,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > { > ret = -EINVAL; > #ifdef CONFIG_SBSA_VUART_CONSOLE > - ret = make_vpl011_uart_node(d, kinfo->fdt); > + ret = make_vpl011_uart_node(kinfo); > #endif > if ( ret ) > goto err; > diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h > index 33f3e72b11..760434369b 100644 > --- a/xen/include/asm-arm/kernel.h > +++ b/xen/include/asm-arm/kernel.h > @@ -36,6 +36,9 @@ struct kernel_info { > /* Enable pl011 emulation */ > bool vpl011; > > + /* GIC phandle */ > + uint32_t guest_phandle_gic; Looking at the usage, I think this should be fdt32_t because you are directly passing the value to the FDT calls. This makes me realize that we consistently use wrongly GUEST_PHANDLE_GIC in both Xen and libxl. Indeed, as we pass the value directly the guest will not see 65000 but 3908894720 as it will do the conversion from big-endian to little-endian. I can see two solution to fix this: 1) define GUEST_PHANDLE_GIC as cpu_to_be32(65000) 2) Use cpu_to_be32(GUEST_PHANDLE_GIC) It would be good to agree how GUEST_PHANDLE_GIC is used so we have the same behavior when the DT is created by Xen and libxl. Cheers,
Hi, On 9/10/19 10:14 PM, Julien Grall wrote: >> diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h >> index 33f3e72b11..760434369b 100644 >> --- a/xen/include/asm-arm/kernel.h >> +++ b/xen/include/asm-arm/kernel.h >> @@ -36,6 +36,9 @@ struct kernel_info { >> /* Enable pl011 emulation */ >> bool vpl011; >> + /* GIC phandle */ >> + uint32_t guest_phandle_gic; > > Looking at the usage, I think this should be fdt32_t because you are > directly passing the value to the FDT calls. > > This makes me realize that we consistently use wrongly GUEST_PHANDLE_GIC > in both Xen and libxl. Indeed, as we pass the value directly the guest > will not see 65000 but 3908894720 as it will do the conversion from > big-endian to little-endian. > > I can see two solution to fix this: > 1) define GUEST_PHANDLE_GIC as cpu_to_be32(65000) > 2) Use cpu_to_be32(GUEST_PHANDLE_GIC) > > It would be good to agree how GUEST_PHANDLE_GIC is used so we have the > same behavior when the DT is created by Xen and libxl. Hmmm, I actually misread the API, the function actually take a CPU-endian value. So uin32_t is correct here and there are nothing has to be done. Sorry for the noise. Cheers,
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index f92069c85f..cd585f05ca 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1510,8 +1510,9 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return res; } -static int __init make_gicv2_domU_node(const struct domain *d, void *fdt) +static int __init make_gicv2_domU_node(struct kernel_info *kinfo) { + void *fdt = kinfo->fdt; int res = 0; __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; __be32 *cells; @@ -1546,11 +1547,11 @@ static int __init make_gicv2_domU_node(const struct domain *d, void *fdt) if (res) return res; - res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC); + res = fdt_property_cell(fdt, "linux,phandle", kinfo->guest_phandle_gic); if (res) return res; - res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC); + res = fdt_property_cell(fdt, "phandle", kinfo->guest_phandle_gic); if (res) return res; @@ -1559,8 +1560,9 @@ static int __init make_gicv2_domU_node(const struct domain *d, void *fdt) return res; } -static int __init make_gicv3_domU_node(const struct domain *d, void *fdt) +static int __init make_gicv3_domU_node(struct kernel_info *kinfo) { + void *fdt = kinfo->fdt; int res = 0; __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; __be32 *cells; @@ -1595,11 +1597,11 @@ static int __init make_gicv3_domU_node(const struct domain *d, void *fdt) if (res) return res; - res = fdt_property_cell(fdt, "linux,phandle", GUEST_PHANDLE_GIC); + res = fdt_property_cell(fdt, "linux,phandle", kinfo->guest_phandle_gic); if (res) return res; - res = fdt_property_cell(fdt, "phandle", GUEST_PHANDLE_GIC); + res = fdt_property_cell(fdt, "phandle", kinfo->guest_phandle_gic); if (res) return res; @@ -1608,21 +1610,22 @@ static int __init make_gicv3_domU_node(const struct domain *d, void *fdt) return res; } -static int __init make_gic_domU_node(const struct domain *d, void *fdt) +static int __init make_gic_domU_node(struct kernel_info *kinfo) { - switch ( d->arch.vgic.version ) + switch ( kinfo->d->arch.vgic.version ) { case GIC_V3: - return make_gicv3_domU_node(d, fdt); + return make_gicv3_domU_node(kinfo); case GIC_V2: - return make_gicv2_domU_node(d, fdt); + return make_gicv2_domU_node(kinfo); default: panic("Unsupported GIC version\n"); } } -static int __init make_timer_domU_node(const struct domain *d, void *fdt) +static int __init make_timer_domU_node(struct kernel_info *kinfo) { + void *fdt = kinfo->fdt; int res; gic_interrupt_t intrs[3]; @@ -1630,7 +1633,7 @@ static int __init make_timer_domU_node(const struct domain *d, void *fdt) if ( res ) return res; - if ( !is_64bit_domain(d) ) + if ( !is_64bit_domain(kinfo->d) ) { res = fdt_property_string(fdt, "compatible", "arm,armv7-timer"); if ( res ) @@ -1652,7 +1655,7 @@ static int __init make_timer_domU_node(const struct domain *d, void *fdt) return res; res = fdt_property_cell(fdt, "interrupt-parent", - GUEST_PHANDLE_GIC); + kinfo->guest_phandle_gic); if (res) return res; @@ -1662,8 +1665,9 @@ static int __init make_timer_domU_node(const struct domain *d, void *fdt) } #ifdef CONFIG_SBSA_VUART_CONSOLE -static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) +static int __init make_vpl011_uart_node(struct kernel_info *kinfo) { + void *fdt = kinfo->fdt; int res; gic_interrupt_t intr; __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS]; @@ -1694,7 +1698,7 @@ static int __init make_vpl011_uart_node(const struct domain *d, void *fdt) return res; res = fdt_property_cell(fdt, "interrupt-parent", - GUEST_PHANDLE_GIC); + kinfo->guest_phandle_gic); if ( res ) return res; @@ -1719,6 +1723,8 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) int addrcells, sizecells; int ret; + kinfo->guest_phandle_gic = GUEST_PHANDLE_GIC; + addrcells = GUEST_ROOT_ADDRESS_CELLS; sizecells = GUEST_ROOT_SIZE_CELLS; @@ -1762,11 +1768,11 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; - ret = make_gic_domU_node(d, kinfo->fdt); + ret = make_gic_domU_node(kinfo); if ( ret ) goto err; - ret = make_timer_domU_node(d, kinfo->fdt); + ret = make_timer_domU_node(kinfo); if ( ret ) goto err; @@ -1774,7 +1780,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) { ret = -EINVAL; #ifdef CONFIG_SBSA_VUART_CONSOLE - ret = make_vpl011_uart_node(d, kinfo->fdt); + ret = make_vpl011_uart_node(kinfo); #endif if ( ret ) goto err; diff --git a/xen/include/asm-arm/kernel.h b/xen/include/asm-arm/kernel.h index 33f3e72b11..760434369b 100644 --- a/xen/include/asm-arm/kernel.h +++ b/xen/include/asm-arm/kernel.h @@ -36,6 +36,9 @@ struct kernel_info { /* Enable pl011 emulation */ bool vpl011; + /* GIC phandle */ + uint32_t guest_phandle_gic; + /* loader to use for this kernel */ void (*load)(struct kernel_info *info); /* loader specific state */
Instead of always hard-coding the GIC phandle (GUEST_PHANDLE_GIC), store it in a variable under kinfo. This way it can be dynamically chosen per domain. Initialize guest_phandle_gic to GUEST_PHANDLE_GIC at the beginning of prepare_dtb_domU. Later patches will change the value of guest_phandle_gic depending on user provided information. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Changes in v4: - new patch --- xen/arch/arm/domain_build.c | 42 ++++++++++++++++++++---------------- xen/include/asm-arm/kernel.h | 3 +++ 2 files changed, 27 insertions(+), 18 deletions(-)