| Message ID | 20200415010255.10081-9-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/12] xen: introduce xen_dom_flags | expand |
Hi, On 15/04/2020 02:02, Stefano Stabellini wrote: > 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 > any domain that is_domain_direct_mapped. The patch has to introduce one > #ifndef CONFIG_NEW_VGIC because the new vgic doesn't support GICv3. Please no #ifdef. Instead move the creation of the DT node in vgic.c and introduce a stub for non-GICv3 platform. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> > --- > xen/arch/arm/domain_build.c | 12 +++++++++--- > xen/arch/arm/vgic-v3.c | 18 ++++++++++++++---- > 2 files changed, 23 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 303bee60f6..beec0a144c 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -1697,8 +1697,12 @@ static int __init make_gicv3_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_GICV3_GICD_BASE)); > + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, > + d->arch.vgic.dbase); > + res = fdt_begin_node(fdt, buf); > if ( res ) > return res; > > @@ -1720,9 +1724,11 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo) > > cells = ®[0]; > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > - GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE); > + d->arch.vgic.dbase, GUEST_GICV3_GICD_SIZE); > +#if defined(CONFIG_GICV3) && !defined(CONFIG_NEW_VGIC) CONFIG_GICV3 does not exist. Did you mean CONFIG_HAS_GICV3? > dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, > - GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE); > + d->arch.vgic.rdist_regions[0].base, GUEST_GICV3_GICR0_SIZE); > +#endif > > res = fdt_property(fdt, "reg", reg, sizeof(reg)); > if (res) > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > index 4e60ba15cc..4cf430f865 100644 > --- a/xen/arch/arm/vgic-v3.c > +++ b/xen/arch/arm/vgic-v3.c > @@ -1677,13 +1677,25 @@ static int vgic_v3_domain_init(struct domain *d) I think you also want to modify vgic_v3_max_rdist_count(). > * Domain 0 gets the hardware address. > * Guests get the virtual platform layout. This comment needs to be updated. > */ > - if ( is_hardware_domain(d) ) > + if ( is_domain_direct_mapped(d) ) > { > unsigned int first_cpu = 0; > + unsigned int nr_rdist_regions; > > d->arch.vgic.dbase = vgic_v3_hw.dbase; > > - for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ ) > + if ( is_hardware_domain(d) ) > + { > + nr_rdist_regions = vgic_v3_hw.nr_rdist_regions; > + d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits; > + } > + else > + { > + nr_rdist_regions = 1; What does promise your the rdist region will be big enough to cater all the re-distributors for your domain? Cheers,
On Wed, 15 Apr 2020, Julien Grall wrote: > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > > index 4e60ba15cc..4cf430f865 100644 > > --- a/xen/arch/arm/vgic-v3.c > > +++ b/xen/arch/arm/vgic-v3.c > > @@ -1677,13 +1677,25 @@ static int vgic_v3_domain_init(struct domain *d) > > > I think you also want to modify vgic_v3_max_rdist_count(). I don't think so: domUs even direct-mapped still only get 1 rdist region. This patch is not changing the layout of the domU gic, it is only finding a "hole" in the physical address space to make sure there are no conflicts (or at least minimize the chance of conflicts.) > > * Domain 0 gets the hardware address. > > * Guests get the virtual platform layout. > > This comment needs to be updated. Yep, I'll do > > */ > > - if ( is_hardware_domain(d) ) > > + if ( is_domain_direct_mapped(d) ) > > { > > unsigned int first_cpu = 0; > > + unsigned int nr_rdist_regions; > > d->arch.vgic.dbase = vgic_v3_hw.dbase; > > - for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ ) > > + if ( is_hardware_domain(d) ) > > + { > > + nr_rdist_regions = vgic_v3_hw.nr_rdist_regions; > > + d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits; > > + } > > + else > > + { > > + nr_rdist_regions = 1; > > What does promise your the rdist region will be big enough to cater all the > re-distributors for your domain? Good point. I'll add an explicit check for that with at least a warning. I don't think we want to return error because the configuration it is still likely to work. It might be better to continue this conversation on the next version of the patch -- it is going to be much clearer.
Hi Stefano, On 01/05/2020 02:31, Stefano Stabellini wrote: > On Wed, 15 Apr 2020, Julien Grall wrote: >>> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c >>> index 4e60ba15cc..4cf430f865 100644 >>> --- a/xen/arch/arm/vgic-v3.c >>> +++ b/xen/arch/arm/vgic-v3.c >>> @@ -1677,13 +1677,25 @@ static int vgic_v3_domain_init(struct domain *d) >> >> >> I think you also want to modify vgic_v3_max_rdist_count(). > > I don't think so: domUs even direct-mapped still only get 1 rdist > region. This patch is not changing the layout of the domU gic, it is > only finding a "hole" in the physical address space to make sure there > are no conflicts (or at least minimize the chance of conflicts.) How do you know the "hole" is big enough? > >>> * Domain 0 gets the hardware address. >>> * Guests get the virtual platform layout. >> >> This comment needs to be updated. > > Yep, I'll do > > >>> */ >>> - if ( is_hardware_domain(d) ) >>> + if ( is_domain_direct_mapped(d) ) >>> { >>> unsigned int first_cpu = 0; >>> + unsigned int nr_rdist_regions; >>> d->arch.vgic.dbase = vgic_v3_hw.dbase; >>> - for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ ) >>> + if ( is_hardware_domain(d) ) >>> + { >>> + nr_rdist_regions = vgic_v3_hw.nr_rdist_regions; >>> + d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits; >>> + } >>> + else >>> + { >>> + nr_rdist_regions = 1; >> >> What does promise your the rdist region will be big enough to cater all the >> re-distributors for your domain? > > Good point. I'll add an explicit check for that with at least a warning. > I don't think we want to return error because the configuration it is > still likely to work. No it is not going to work. Imagine you have have a guest with 3 vCPUs but the first re-distributor region can only cater 2 re-distributor. How is this going to be fine to continue? For dom0, we are re-using the same hole but possibly not all of them. Why can't we do that for domU? Cheers,
On Fri, 1 May 2020, Julien Grall wrote: > Hi Stefano, > > On 01/05/2020 02:31, Stefano Stabellini wrote: > > On Wed, 15 Apr 2020, Julien Grall wrote: > > > > diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c > > > > index 4e60ba15cc..4cf430f865 100644 > > > > --- a/xen/arch/arm/vgic-v3.c > > > > +++ b/xen/arch/arm/vgic-v3.c > > > > @@ -1677,13 +1677,25 @@ static int vgic_v3_domain_init(struct domain *d) > > > > > > > > > I think you also want to modify vgic_v3_max_rdist_count(). > > > > I don't think so: domUs even direct-mapped still only get 1 rdist > > region. This patch is not changing the layout of the domU gic, it is > > only finding a "hole" in the physical address space to make sure there > > are no conflicts (or at least minimize the chance of conflicts.) > > How do you know the "hole" is big enough? > > > > > > > * Domain 0 gets the hardware address. > > > > * Guests get the virtual platform layout. > > > > > > This comment needs to be updated. > > > > Yep, I'll do > > > > > > > > */ > > > > - if ( is_hardware_domain(d) ) > > > > + if ( is_domain_direct_mapped(d) ) > > > > { > > > > unsigned int first_cpu = 0; > > > > + unsigned int nr_rdist_regions; > > > > d->arch.vgic.dbase = vgic_v3_hw.dbase; > > > > - for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ ) > > > > + if ( is_hardware_domain(d) ) > > > > + { > > > > + nr_rdist_regions = vgic_v3_hw.nr_rdist_regions; > > > > + d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits; > > > > + } > > > > + else > > > > + { > > > > + nr_rdist_regions = 1; > > > > > > What does promise your the rdist region will be big enough to cater all > > > the > > > re-distributors for your domain? > > > > Good point. I'll add an explicit check for that with at least a warning. > > I don't think we want to return error because the configuration it is > > still likely to work. > > No it is not going to work. Imagine you have have a guest with 3 vCPUs but the > first re-distributor region can only cater 2 re-distributor. How is this going > to be fine to continue? > > For dom0, we are re-using the same hole but possibly not all of them. Why > can't we do that for domU? I implemented what you suggested
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 303bee60f6..beec0a144c 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1697,8 +1697,12 @@ static int __init make_gicv3_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_GICV3_GICD_BASE)); + snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64, + d->arch.vgic.dbase); + res = fdt_begin_node(fdt, buf); if ( res ) return res; @@ -1720,9 +1724,11 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo) cells = ®[0]; dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, - GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE); + d->arch.vgic.dbase, GUEST_GICV3_GICD_SIZE); +#if defined(CONFIG_GICV3) && !defined(CONFIG_NEW_VGIC) dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS, - GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE); + d->arch.vgic.rdist_regions[0].base, GUEST_GICV3_GICR0_SIZE); +#endif res = fdt_property(fdt, "reg", reg, sizeof(reg)); if (res) diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 4e60ba15cc..4cf430f865 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -1677,13 +1677,25 @@ static int vgic_v3_domain_init(struct domain *d) * Domain 0 gets the hardware address. * Guests get the virtual platform layout. */ - if ( is_hardware_domain(d) ) + if ( is_domain_direct_mapped(d) ) { unsigned int first_cpu = 0; + unsigned int nr_rdist_regions; d->arch.vgic.dbase = vgic_v3_hw.dbase; - for ( i = 0; i < vgic_v3_hw.nr_rdist_regions; i++ ) + if ( is_hardware_domain(d) ) + { + nr_rdist_regions = vgic_v3_hw.nr_rdist_regions; + d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits; + } + else + { + nr_rdist_regions = 1; + d->arch.vgic.intid_bits = 10; + } + + for ( i = 0; i < nr_rdist_regions; i++ ) { paddr_t size = vgic_v3_hw.regions[i].size; @@ -1706,8 +1718,6 @@ static int vgic_v3_domain_init(struct domain *d) * exposing unused region as they will not get emulated. */ d->arch.vgic.nr_regions = i + 1; - - d->arch.vgic.intid_bits = vgic_v3_hw.intid_bits; } else {
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 any domain that is_domain_direct_mapped. The patch has to introduce one #ifndef CONFIG_NEW_VGIC because the new vgic doesn't support GICv3. Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com> --- xen/arch/arm/domain_build.c | 12 +++++++++--- xen/arch/arm/vgic-v3.c | 18 ++++++++++++++---- 2 files changed, 23 insertions(+), 7 deletions(-)