| Message ID | 20211015030945.2082898-5-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | direct-map memory map | expand |
Hi Penny, On 15/10/2021 04:09, Penny Zheng wrote: > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Today we use native addresses to map the GICv3 for Dom0 and fixed > addresses for DomUs. > > This patch changes the behavior so that native addresses are used for > all direct-map domains(including Dom0). > > Considering that dom0 may not always be directly mapped in the future, > this patch introduces a new helper "is_domain_use_host_layout()" that > wraps both two check "is_domain_direct_mapped(d) || is_hardware_domain(d)" > for more flexible useage. Typo: s/useage/usage/ > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/arch/arm/domain_build.c | 46 +++++++++++++++++++++++++++--------- > xen/arch/arm/vgic-v3.c | 20 +++++++++------- > xen/include/asm-arm/domain.h | 3 +++ > 3 files changed, 50 insertions(+), 19 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 6cd03e4d0f..7e0ee07e06 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -2255,16 +2255,20 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) > return res; > } > > +#ifdef CONFIG_ARM_64 The code below is specific to the GICv3 (and not 64-bit). So this should be gated with CONFIG_GICV3. Personally, I would have gated the code in a separate patch. But I am fine if this is added in this patch so long this is mentionned in the commit message. > 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 *reg; > __be32 *cells; > + struct domain *d = kinfo->d; AFAICT, 'd' is not going to be modified. So please add const. > + char buf[38]; Please explain how 38 was found. For an example, see the comment on top of 'buf' in make_memory_node(). > + unsigned int i, len = 0; > > - res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE)); > - if ( res ) > - return res; > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > + vgic_dist_base(&d->arch.vgic)); > + res = fdt_begin_node(fdt, buf); You set res but it gets overwritten just below. Were you meant to check the return value? > > res = fdt_property_cell(fdt, "#address-cells", 0); > if ( res ) > @@ -2282,35 +2286,55 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo) > if ( res ) > return res; > > + /* reg specifies all re-distributors and Distributor. */ > + len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * > + (d->arch.vgic.nr_regions + 1) * sizeof(__be32); > + reg = xmalloc_bytes(len); > + if ( reg == NULL ) > + return -ENOMEM; > + > cells = ®[0]; > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > - GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE); > - dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > - GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE); > + vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE); > > - res = fdt_property(fdt, "reg", reg, sizeof(reg)); > + for ( i = 0; > + i < d->arch.vgic.nr_regions; > + i++, cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) ) dt_child_set_range() will already update cells to the next ones. So this needs to be dropped. I was expecting this to be caugt during test. So did you test this series with GICv3? How about the new vGIC? > + { > + dt_child_set_range(&cells, > + GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > + d->arch.vgic.rdist_regions[i].base, > + d->arch.vgic.rdist_regions[i].size); > + } > + > + res = fdt_property(fdt, "reg", reg, len); I would suggest to free 'reg' right here as you don't need it afterwards. This will also remove the requirement to add a 'out' label and the addition of 'goto'. > if (res) > - return res; > + goto out; > > res = fdt_property_cell(fdt, "linux,phandle", kinfo->phandle_gic); > if (res) > - return res; > + goto out; > > res = fdt_property_cell(fdt, "phandle", kinfo->phandle_gic); > if (res) > - return res; > + goto out; > > res = fdt_end_node(fdt); > > + out: > + xfree(reg); > return res; > } > +#endif > > static int __init make_gic_domU_node(struct kernel_info *kinfo) > { > switch ( kinfo->d->arch.vgic.version ) > { > +#ifdef CONFIG_ARM_64 > case GIC_V3: > return make_gicv3_domU_node(kinfo); > +#endif > case GIC_V2: > return make_gicv2_domU_node(kinfo); > default: > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index cb5a70c42e..70168ca1ac 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -1641,14 +1641,15 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d) > * Normally there is only one GICv3 redistributor region. > * The GICv3 DT binding provisions for multiple regions, since there are > * platforms out there which need those (multi-socket systems). > - * For Dom0 we have to live with the MMIO layout the hardware provides, > - * so we have to copy the multiple regions - as the first region may not > - * provide enough space to hold all redistributors we need. > + * For direct-map domain(including dom0), we have to live with the MMIO I find the sentence somewhat misleading because it implies that dom0 is is a direct-map domain (this is true today, but this merely an implementation details). However, with the change below, I think it would be better to write "For domain using the host memory layout..." and not going into details and what sort of domain we refer to here. Instead... > + * layout the hardware provides, so we have to copy the multiple regions > + * - as the first region may not provide enough space to hold all > + * redistributors we need. > * However DomU get a constructed memory map, so we can go with > * the architected single redistributor region. > */ > - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : > - GUEST_GICV3_RDIST_REGIONS; > + return is_domain_use_host_layout(d) ? vgic_v3_hw.nr_rdist_regions : > + GUEST_GICV3_RDIST_REGIONS; > } > > static int vgic_v3_domain_init(struct domain *d) > @@ -1670,10 +1671,13 @@ static int vgic_v3_domain_init(struct domain *d) > radix_tree_init(&d->arch.vgic.pend_lpi_tree); > > /* > - * Domain 0 gets the hardware address. > - * Guests get the virtual platform layout. > + * Since we map the whole GICv3 register memory map(64KB) for > + * all guests(including DOM0), DOM0 and direct-map guests could be > + * treated the same way here. > + * direct-map domain (including Dom0) gets the hardware address. > + * Other guests get the virtual platform layout. > */ > - if ( is_hardware_domain(d) ) > + if ( is_domain_use_host_layout(d) ) > { > unsigned int first_cpu = 0; > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index fc42c6a310..e8ce3ac8d2 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -32,6 +32,9 @@ enum domain_type { > /* Check if domain is direct-map memory map. */ > #define is_domain_direct_mapped(d) (d->options & XEN_DOMCTL_CDF_directmap) > > +#define is_domain_use_host_layout(d) (is_domain_direct_mapped(d) || \ > + is_hardware_domain(d)) ... the details should be on top of this comment. The advantage is there is less chance for a comment to rot. Regarding the name, I would either drop the 'is_' or s/use/using/. My preference goes for the former as it makes the name sightly shorter. Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Wednesday, October 20, 2021 7:11 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Wei Chen <Wei.Chen@arm.com>; Bertrand Marquis > <Bertrand.Marquis@arm.com> > Subject: Re: [PATCH v2 4/6] xen/arm: if direct-map domain use native > addresses for GICv3 > > Hi Penny, > > On 15/10/2021 04:09, Penny Zheng wrote: > > From: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > Today we use native addresses to map the GICv3 for Dom0 and fixed > > addresses for DomUs. > > > > This patch changes the behavior so that native addresses are used for > > all direct-map domains(including Dom0). > > > > Considering that dom0 may not always be directly mapped in the future, > > this patch introduces a new helper "is_domain_use_host_layout()" that > > wraps both two check "is_domain_direct_mapped(d) || > is_hardware_domain(d)" > > for more flexible useage. > > Typo: s/useage/usage/ > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > xen/arch/arm/domain_build.c | 46 +++++++++++++++++++++++++++--------- > > xen/arch/arm/vgic-v3.c | 20 +++++++++------- > > xen/include/asm-arm/domain.h | 3 +++ > > 3 files changed, 50 insertions(+), 19 deletions(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 6cd03e4d0f..7e0ee07e06 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -2255,16 +2255,20 @@ static int __init make_gicv2_domU_node(struct > kernel_info *kinfo) > > return res; > > } > > > > +#ifdef CONFIG_ARM_64 > > The code below is specific to the GICv3 (and not 64-bit). So this should be > gated with CONFIG_GICV3. > > Personally, I would have gated the code in a separate patch. But I am fine if > this is added in this patch so long this is mentionned in the commit message. > > > 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 *reg; > > __be32 *cells; > > + struct domain *d = kinfo->d; > > AFAICT, 'd' is not going to be modified. So please add const. > > > + char buf[38]; > > Please explain how 38 was found. For an example, see the comment on top of > 'buf' in make_memory_node(). > > > + unsigned int i, len = 0; > > > > - res = fdt_begin_node(fdt, "interrupt- > controller@"__stringify(GUEST_GICV3_GICD_BASE)); > > - if ( res ) > > - return res; > > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > > + vgic_dist_base(&d->arch.vgic)); > > + res = fdt_begin_node(fdt, buf); > > You set res but it gets overwritten just below. Were you meant to check the > return value? > > > > > res = fdt_property_cell(fdt, "#address-cells", 0); > > if ( res ) > > @@ -2282,35 +2286,55 @@ static int __init make_gicv3_domU_node(struct > kernel_info *kinfo) > > if ( res ) > > return res; > > > > + /* reg specifies all re-distributors and Distributor. */ > > + len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * > > + (d->arch.vgic.nr_regions + 1) * sizeof(__be32); > > + reg = xmalloc_bytes(len); > > + if ( reg == NULL ) > > + return -ENOMEM; > > + > > cells = ®[0]; > > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, > GUEST_ROOT_SIZE_CELLS, > > - GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE); > > - dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, > GUEST_ROOT_SIZE_CELLS, > > - GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE); > > + vgic_dist_base(&d->arch.vgic), > > + GUEST_GICV3_GICD_SIZE); > > > > - res = fdt_property(fdt, "reg", reg, sizeof(reg)); > > + for ( i = 0; > > + i < d->arch.vgic.nr_regions; > > + i++, cells += (GUEST_ROOT_ADDRESS_CELLS + > > + GUEST_ROOT_SIZE_CELLS) ) > > dt_child_set_range() will already update cells to the next ones. So this needs > to be dropped. > You're so right!!! I checked the code and it will already points to the next ones > I was expecting this to be caugt during test. So did you test this series with > GICv3? How about the new vGIC? > Yes, I test it with both on core A and R. It's working and I think that's because that the redistributor region size is always 1 in my test, so ... > > + { > > + dt_child_set_range(&cells, > > + GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > > + d->arch.vgic.rdist_regions[i].base, > > + d->arch.vgic.rdist_regions[i].size); > > + } > > + > > + res = fdt_property(fdt, "reg", reg, len); > > I would suggest to free 'reg' right here as you don't need it afterwards. This will > also remove the requirement to add a 'out' label and the addition of 'goto'. > > > if (res) > > - return res; > > + goto out; > > > res = fdt_property_cell(fdt, "linux,phandle", kinfo->phandle_gic); > > if (res) > > - return res; > > + goto out; > > > > res = fdt_property_cell(fdt, "phandle", kinfo->phandle_gic); > > if (res) > > - return res; > > + goto out; > > > > res = fdt_end_node(fdt); > > > > + out: > > + xfree(reg); > > return res; > > } > > +#endif > > > > static int __init make_gic_domU_node(struct kernel_info *kinfo) > > { > > switch ( kinfo->d->arch.vgic.version ) > > { > > +#ifdef CONFIG_ARM_64 > > case GIC_V3: > > return make_gicv3_domU_node(kinfo); > > +#endif > > case GIC_V2: > > return make_gicv2_domU_node(kinfo); > > default: > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index > > cb5a70c42e..70168ca1ac 100644 > > --- a/xen/arch/arm/vgic-v3.c > > +++ b/xen/arch/arm/vgic-v3.c > > @@ -1641,14 +1641,15 @@ static inline unsigned int > vgic_v3_max_rdist_count(struct domain *d) > > * Normally there is only one GICv3 redistributor region. > > * The GICv3 DT binding provisions for multiple regions, since there are > > * platforms out there which need those (multi-socket systems). > > - * For Dom0 we have to live with the MMIO layout the hardware provides, > > - * so we have to copy the multiple regions - as the first region may not > > - * provide enough space to hold all redistributors we need. > > + * For direct-map domain(including dom0), we have to live with > > + the MMIO > > I find the sentence somewhat misleading because it implies that dom0 is is a > direct-map domain (this is true today, but this merely an implementation > details). > > However, with the change below, I think it would be better to write "For > domain using the host memory layout..." and not going into details and what > sort of domain we refer to here. Instead... > > > + * layout the hardware provides, so we have to copy the multiple regions > > + * - as the first region may not provide enough space to hold all > > + * redistributors we need. > > * However DomU get a constructed memory map, so we can go with > > * the architected single redistributor region. > > */ > > - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : > > - GUEST_GICV3_RDIST_REGIONS; > > + return is_domain_use_host_layout(d) ? vgic_v3_hw.nr_rdist_regions : > > + GUEST_GICV3_RDIST_REGIONS; > > } > > > > static int vgic_v3_domain_init(struct domain *d) @@ -1670,10 > > +1671,13 @@ static int vgic_v3_domain_init(struct domain *d) > > radix_tree_init(&d->arch.vgic.pend_lpi_tree); > > > > /* > > - * Domain 0 gets the hardware address. > > - * Guests get the virtual platform layout. > > + * Since we map the whole GICv3 register memory map(64KB) for > > + * all guests(including DOM0), DOM0 and direct-map guests could be > > + * treated the same way here. > > + * direct-map domain (including Dom0) gets the hardware address. > > + * Other guests get the virtual platform layout. > > */ > > - if ( is_hardware_domain(d) ) > > + if ( is_domain_use_host_layout(d) ) > > { > > unsigned int first_cpu = 0; > > > > diff --git a/xen/include/asm-arm/domain.h > > b/xen/include/asm-arm/domain.h index fc42c6a310..e8ce3ac8d2 100644 > > --- a/xen/include/asm-arm/domain.h > > +++ b/xen/include/asm-arm/domain.h > > @@ -32,6 +32,9 @@ enum domain_type { > > /* Check if domain is direct-map memory map. */ > > #define is_domain_direct_mapped(d) (d->options & > > XEN_DOMCTL_CDF_directmap) > > > > +#define is_domain_use_host_layout(d) (is_domain_direct_mapped(d) || \ > > + is_hardware_domain(d)) > > ... the details should be on top of this comment. The advantage is there is less > chance for a comment to rot. > > Regarding the name, I would either drop the 'is_' or s/use/using/. My > preference goes for the former as it makes the name sightly shorter. > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 6cd03e4d0f..7e0ee07e06 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2255,16 +2255,20 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) return res; } +#ifdef CONFIG_ARM_64 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 *reg; __be32 *cells; + struct domain *d = kinfo->d; + char buf[38]; + unsigned int i, len = 0; - res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE)); - if ( res ) - return res; + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, + vgic_dist_base(&d->arch.vgic)); + res = fdt_begin_node(fdt, buf); res = fdt_property_cell(fdt, "#address-cells", 0); if ( res ) @@ -2282,35 +2286,55 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo) if ( res ) return res; + /* reg specifies all re-distributors and Distributor. */ + len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * + (d->arch.vgic.nr_regions + 1) * sizeof(__be32); + reg = xmalloc_bytes(len); + if ( reg == NULL ) + return -ENOMEM; + cells = ®[0]; dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, - GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE); - dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, - GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE); + vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE); - res = fdt_property(fdt, "reg", reg, sizeof(reg)); + for ( i = 0; + i < d->arch.vgic.nr_regions; + i++, cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) ) + { + dt_child_set_range(&cells, + GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, + d->arch.vgic.rdist_regions[i].base, + d->arch.vgic.rdist_regions[i].size); + } + + res = fdt_property(fdt, "reg", reg, len); if (res) - return res; + goto out; res = fdt_property_cell(fdt, "linux,phandle", kinfo->phandle_gic); if (res) - return res; + goto out; res = fdt_property_cell(fdt, "phandle", kinfo->phandle_gic); if (res) - return res; + goto out; res = fdt_end_node(fdt); + out: + xfree(reg); return res; } +#endif static int __init make_gic_domU_node(struct kernel_info *kinfo) { switch ( kinfo->d->arch.vgic.version ) { +#ifdef CONFIG_ARM_64 case GIC_V3: return make_gicv3_domU_node(kinfo); +#endif case GIC_V2: return make_gicv2_domU_node(kinfo); default: diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index cb5a70c42e..70168ca1ac 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1641,14 +1641,15 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d) * Normally there is only one GICv3 redistributor region. * The GICv3 DT binding provisions for multiple regions, since there are * platforms out there which need those (multi-socket systems). - * For Dom0 we have to live with the MMIO layout the hardware provides, - * so we have to copy the multiple regions - as the first region may not - * provide enough space to hold all redistributors we need. + * For direct-map domain(including dom0), we have to live with the MMIO + * layout the hardware provides, so we have to copy the multiple regions + * - as the first region may not provide enough space to hold all + * redistributors we need. * However DomU get a constructed memory map, so we can go with * the architected single redistributor region. */ - return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions : - GUEST_GICV3_RDIST_REGIONS; + return is_domain_use_host_layout(d) ? vgic_v3_hw.nr_rdist_regions : + GUEST_GICV3_RDIST_REGIONS; } static int vgic_v3_domain_init(struct domain *d) @@ -1670,10 +1671,13 @@ static int vgic_v3_domain_init(struct domain *d) radix_tree_init(&d->arch.vgic.pend_lpi_tree); /* - * Domain 0 gets the hardware address. - * Guests get the virtual platform layout. + * Since we map the whole GICv3 register memory map(64KB) for + * all guests(including DOM0), DOM0 and direct-map guests could be + * treated the same way here. + * direct-map domain (including Dom0) gets the hardware address. + * Other guests get the virtual platform layout. */ - if ( is_hardware_domain(d) ) + if ( is_domain_use_host_layout(d) ) { unsigned int first_cpu = 0; diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index fc42c6a310..e8ce3ac8d2 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -32,6 +32,9 @@ enum domain_type { /* Check if domain is direct-map memory map. */ #define is_domain_direct_mapped(d) (d->options & XEN_DOMCTL_CDF_directmap) +#define is_domain_use_host_layout(d) (is_domain_direct_mapped(d) || \ + is_hardware_domain(d)) + struct vtimer { struct vcpu *v; int irq;