| Message ID | 20200415010255.10081-8-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] xen: introduce xen_dom_flags | expand |
On 15/04/2020 02:02, Stefano Stabellini wrote: > Today we use native addresses to map the GICv2 for Dom0 and fixed > addresses for DomUs. > > This patch changes the behavior so that native addresses are used for > any domain that is_domain_direct_mapped. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > xen/arch/arm/domain_build.c | 10 +++++++--- > xen/arch/arm/vgic-v2.c | 12 ++++++------ > xen/arch/arm/vgic/vgic-v2.c | 2 +- > xen/include/asm-arm/vgic.h | 1 + > 4 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 627e0c5e8e..303bee60f6 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1643,8 +1643,12 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) > int res = 0; > __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; > __be32 *cells; > + struct domain *d = kinfo->d; > + char buf[38]; > > - res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICD_BASE)); > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > + d->arch.vgic.dbase); > + res = fdt_begin_node(fdt, buf); > if ( res ) > return res; > > @@ -1666,9 +1670,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) > > cells = ®[0]; > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > - GUEST_GICD_BASE, GUEST_GICD_SIZE); > + d->arch.vgic.dbase, GUEST_GICD_SIZE); > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > - GUEST_GICC_BASE, GUEST_GICC_SIZE); > + d->arch.vgic.cbase, GUEST_GICC_SIZE); You can't use the native base address and not the native size. Particularly, this is going to screw any GIC using 8KB alias. It would be preferable if only expose part of the CPU interface as we do for the guest. So d->arch.vgic.cbase would be equal to vgic_v2_hw.dbase + vgic_v2.hw.aliased_offset. Cheers,
On Wed, 15 Apr 2020, Julien Grall wrote: > On 15/04/2020 02:02, Stefano Stabellini wrote: > > Today we use native addresses to map the GICv2 for Dom0 and fixed > > addresses for DomUs. > > > > This patch changes the behavior so that native addresses are used for > > any domain that is_domain_direct_mapped. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > --- > > xen/arch/arm/domain_build.c | 10 +++++++--- > > xen/arch/arm/vgic-v2.c | 12 ++++++------ > > xen/arch/arm/vgic/vgic-v2.c | 2 +- > > xen/include/asm-arm/vgic.h | 1 + > > 4 files changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 627e0c5e8e..303bee60f6 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -1643,8 +1643,12 @@ static int __init make_gicv2_domU_node(struct > > kernel_info *kinfo) > > int res = 0; > > __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; > > __be32 *cells; > > + struct domain *d = kinfo->d; > > + char buf[38]; > > - res = fdt_begin_node(fdt, > > "interrupt-controller@"__stringify(GUEST_GICD_BASE)); > > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > > + d->arch.vgic.dbase); > > + res = fdt_begin_node(fdt, buf); > > if ( res ) > > return res; > > @@ -1666,9 +1670,9 @@ static int __init make_gicv2_domU_node(struct > > kernel_info *kinfo) > > cells = ®[0]; > > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, > > GUEST_ROOT_SIZE_CELLS, > > - GUEST_GICD_BASE, GUEST_GICD_SIZE); > > + d->arch.vgic.dbase, GUEST_GICD_SIZE); > > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, > > GUEST_ROOT_SIZE_CELLS, > > - GUEST_GICC_BASE, GUEST_GICC_SIZE); > > + d->arch.vgic.cbase, GUEST_GICC_SIZE); > > You can't use the native base address and not the native size. Particularly, > this is going to screw any GIC using 8KB alias. I don't follow why it could cause problems with a GIC using the 8KB alias. Maybe an example (even a fake example) would help. > It would be preferable if only expose part of the CPU interface as we do for > the guest. So d->arch.vgic.cbase would be equal to vgic_v2_hw.dbase + > vgic_v2.hw.aliased_offset.
On 01/05/2020 02:26, Stefano Stabellini wrote: > On Wed, 15 Apr 2020, Julien Grall wrote: >> On 15/04/2020 02:02, Stefano Stabellini wrote: >>> Today we use native addresses to map the GICv2 for Dom0 and fixed >>> addresses for DomUs. >>> >>> This patch changes the behavior so that native addresses are used for >>> any domain that is_domain_direct_mapped. >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> >>> --- >>> xen/arch/arm/domain_build.c | 10 +++++++--- >>> xen/arch/arm/vgic-v2.c | 12 ++++++------ >>> xen/arch/arm/vgic/vgic-v2.c | 2 +- >>> xen/include/asm-arm/vgic.h | 1 + >>> 4 files changed, 15 insertions(+), 10 deletions(-) >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 627e0c5e8e..303bee60f6 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -1643,8 +1643,12 @@ static int __init make_gicv2_domU_node(struct >>> kernel_info *kinfo) >>> int res = 0; >>> __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; >>> __be32 *cells; >>> + struct domain *d = kinfo->d; >>> + char buf[38]; >>> - res = fdt_begin_node(fdt, >>> "interrupt-controller@"__stringify(GUEST_GICD_BASE)); >>> + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, >>> + d->arch.vgic.dbase); >>> + res = fdt_begin_node(fdt, buf); >>> if ( res ) >>> return res; >>> @@ -1666,9 +1670,9 @@ static int __init make_gicv2_domU_node(struct >>> kernel_info *kinfo) >>> cells = ®[0]; >>> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, >>> GUEST_ROOT_SIZE_CELLS, >>> - GUEST_GICD_BASE, GUEST_GICD_SIZE); >>> + d->arch.vgic.dbase, GUEST_GICD_SIZE); >>> dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, >>> GUEST_ROOT_SIZE_CELLS, >>> - GUEST_GICC_BASE, GUEST_GICC_SIZE); >>> + d->arch.vgic.cbase, GUEST_GICC_SIZE); >> >> You can't use the native base address and not the native size. Particularly, >> this is going to screw any GIC using 8KB alias. > > I don't follow why it could cause problems with a GIC using the 8KB > alias. Maybe an example (even a fake example) would help. The GICC interface is composed of the two 4KB pages. In some of the implementation, each pages starts at a 64KB aligned address. Each page are also aliased every 4KB in the 64KB region. For guest, we don't expose the full 128KB region but only part of it (8KB). So the guest interface is the same regardless the underlying implementation of the GIC. vgic.cbase points to the beginning of the first region. So what you are mapping is the first 8KB of the first region. The second region is not mapped at all. As the pages are aliased, the trick we use is to map from vgic.cbase + 60KB (vgic_v2.hw.aliased_offset). This means the 2 pages will now be contiguous in the guest physical memory. >> It would be preferable if only expose part of the CPU interface as we do for >> the guest. So d->arch.vgic.cbase would be equal to vgic_v2_hw.dbase + I meant cbase rather than dbase here. >> vgic_v2.hw.aliased_offset. > Cheers,
On Fri, 1 May 2020, Julien Grall wrote: > On 01/05/2020 02:26, Stefano Stabellini wrote: > > On Wed, 15 Apr 2020, Julien Grall wrote: > > > On 15/04/2020 02:02, Stefano Stabellini wrote: > > > > Today we use native addresses to map the GICv2 for Dom0 and fixed > > > > addresses for DomUs. > > > > > > > > This patch changes the behavior so that native addresses are used for > > > > any domain that is_domain_direct_mapped. > > > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > > > > --- > > > > xen/arch/arm/domain_build.c | 10 +++++++--- > > > > xen/arch/arm/vgic-v2.c | 12 ++++++------ > > > > xen/arch/arm/vgic/vgic-v2.c | 2 +- > > > > xen/include/asm-arm/vgic.h | 1 + > > > > 4 files changed, 15 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > > > index 627e0c5e8e..303bee60f6 100644 > > > > --- a/xen/arch/arm/domain_build.c > > > > +++ b/xen/arch/arm/domain_build.c > > > > @@ -1643,8 +1643,12 @@ static int __init make_gicv2_domU_node(struct > > > > kernel_info *kinfo) > > > > int res = 0; > > > > __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * > > > > 2]; > > > > __be32 *cells; > > > > + struct domain *d = kinfo->d; > > > > + char buf[38]; > > > > - res = fdt_begin_node(fdt, > > > > "interrupt-controller@"__stringify(GUEST_GICD_BASE)); > > > > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > > > > + d->arch.vgic.dbase); > > > > + res = fdt_begin_node(fdt, buf); > > > > if ( res ) > > > > return res; > > > > @@ -1666,9 +1670,9 @@ static int __init make_gicv2_domU_node(struct > > > > kernel_info *kinfo) > > > > cells = ®[0]; > > > > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, > > > > GUEST_ROOT_SIZE_CELLS, > > > > - GUEST_GICD_BASE, GUEST_GICD_SIZE); > > > > + d->arch.vgic.dbase, GUEST_GICD_SIZE); > > > > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, > > > > GUEST_ROOT_SIZE_CELLS, > > > > - GUEST_GICC_BASE, GUEST_GICC_SIZE); > > > > + d->arch.vgic.cbase, GUEST_GICC_SIZE); > > > > > > You can't use the native base address and not the native size. > > > Particularly, > > > this is going to screw any GIC using 8KB alias. > > > > I don't follow why it could cause problems with a GIC using the 8KB > > alias. Maybe an example (even a fake example) would help. > > The GICC interface is composed of the two 4KB pages. In some of the > implementation, each pages starts at a 64KB aligned address. Each page are > also aliased every 4KB in the 64KB region. > > For guest, we don't expose the full 128KB region but only part of it (8KB). So > the guest interface is the same regardless the underlying implementation of > the GIC. > > vgic.cbase points to the beginning of the first region. So what you are > mapping is the first 8KB of the first region. The second region is not mapped > at all. > > As the pages are aliased, the trick we use is to map from vgic.cbase + 60KB > (vgic_v2.hw.aliased_offset). This means the 2 pages will now be contiguous in > the guest physical memory. I understood the issue and I fixed it by applying vgic_v2.hw.aliased_offset to vbase and cbase. (Although only vbase is actually necessary as far as I can tell.)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 627e0c5e8e..303bee60f6 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1643,8 +1643,12 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) int res = 0; __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2]; __be32 *cells; + struct domain *d = kinfo->d; + char buf[38]; - res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICD_BASE)); + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, + d->arch.vgic.dbase); + res = fdt_begin_node(fdt, buf); if ( res ) return res; @@ -1666,9 +1670,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo) cells = ®[0]; dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, - GUEST_GICD_BASE, GUEST_GICD_SIZE); + d->arch.vgic.dbase, GUEST_GICD_SIZE); dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, - GUEST_GICC_BASE, GUEST_GICC_SIZE); + d->arch.vgic.cbase, GUEST_GICC_SIZE); res = fdt_property(fdt, "reg", reg, sizeof(reg)); if (res) diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index 64b141fea5..9a74e2ed38 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -650,14 +650,14 @@ static int vgic_v2_vcpu_init(struct vcpu *v) static int vgic_v2_domain_init(struct domain *d) { int ret; - paddr_t cbase, csize; + paddr_t csize; paddr_t vbase; /* * The hardware domain gets the hardware address. * Guests get the virtual platform layout. */ - if ( is_hardware_domain(d) ) + if ( is_domain_direct_mapped(d) ) { d->arch.vgic.dbase = vgic_v2_hw.dbase; /* @@ -667,7 +667,7 @@ static int vgic_v2_domain_init(struct domain *d) * Note that we assume the size of the CPU interface is always * aligned to PAGE_SIZE. */ - cbase = vgic_v2_hw.cbase; + d->arch.vgic.cbase = vgic_v2_hw.cbase; csize = vgic_v2_hw.csize; vbase = vgic_v2_hw.vbase; } @@ -681,7 +681,7 @@ static int vgic_v2_domain_init(struct domain *d) * region. */ BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K); - cbase = GUEST_GICC_BASE; + d->arch.vgic.cbase = GUEST_GICC_BASE; csize = GUEST_GICC_SIZE; vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset; } @@ -690,8 +690,8 @@ static int vgic_v2_domain_init(struct domain *d) * Map the gic virtual cpu interface in the gic cpu interface * region of the guest. */ - ret = map_mmio_regions(d, gaddr_to_gfn(cbase), csize / PAGE_SIZE, - maddr_to_mfn(vbase)); + ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase), + csize / PAGE_SIZE, maddr_to_mfn(vbase)); if ( ret ) return ret; diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c index 09cb92039a..275dd8bea9 100644 --- a/xen/arch/arm/vgic/vgic-v2.c +++ b/xen/arch/arm/vgic/vgic-v2.c @@ -266,7 +266,7 @@ int vgic_v2_map_resources(struct domain *d) * The hardware domain gets the hardware address. * Guests get the virtual platform layout. */ - if ( is_hardware_domain(d) ) + if ( is_domain_direct_mapped(d) ) { d->arch.vgic.dbase = gic_v2_hw_data.dbase; /* diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index ce1e3c4bbd..f151d98773 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -152,6 +152,7 @@ struct vgic_dist { struct pending_irq *pending_irqs; /* Base address for guest GIC */ paddr_t dbase; /* Distributor base address */ + paddr_t cbase; /* CPU interface base address */ #ifdef CONFIG_GICV3 /* GIC V3 addressing */ /* List of contiguous occupied by the redistributors */
Today we use native addresses to map the GICv2 for Dom0 and fixed addresses for DomUs. This patch changes the behavior so that native addresses are used for any domain that is_domain_direct_mapped. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> --- xen/arch/arm/domain_build.c | 10 +++++++--- xen/arch/arm/vgic-v2.c | 12 ++++++------ xen/arch/arm/vgic/vgic-v2.c | 2 +- xen/include/asm-arm/vgic.h | 1 + 4 files changed, 15 insertions(+), 10 deletions(-)