Re: [PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3
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
Re: [PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3
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,
--
Julien Grall
Re: [PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3
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.
Re: [PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3
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
---
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(, 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(, 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,
--
Julien Grall
[PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3
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
---
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(, 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(, 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
{
--
2.17.1
