Re: [PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3

From: Stefano Stabellini <stefano.stabellini@xilinx.com>
To: Julien Grall <julien@xen.org>
Cc: xen-devel@lists.xenproject.org,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr_Babchuk@epam.com,
	Stefano Stabellini <stefano.stabellini@xilinx.com>
Subject: Re: [PATCH 09/12] xen/arm: if is_domain_direct_mapped use native addresses for GICv3
Date: Fri, 8 May 2020 17:06:50 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2005081458530.26167@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <ab281b4d-c78f-15c3-57d3-85d0cef7bec8@xen.org>

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