Re: [Xen-devel] [PATCH v2 06/12] xen/gnttab: Pass max_{grant, maptrack}_frames into grant_table_create()

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andrew,

On 08/15/2018 02:08 PM, Andrew Cooper wrote:
> On 15/08/18 14:04, Julien Grall wrote:
> > Hi Andrew,
> >
> > On 08/13/2018 11:01 AM, Andrew Cooper wrote:
> > > ... rather than setting the limits up after domain_create() has
> > > completed.
> > >
> > > This removes all the gnttab infrastructure for calculating the number
> > > of dom0
> > > grant frames, opting instead to require the dom0 construction code to
> > > pass a
> > > sane value in via the configuration.
> > >
> > > In practice, this now means that there is never a partially
> > > constructed grant
> > > table for a reference-able domain.
> > >
> > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > ---
> > > CC: Jan Beulich <JBeulich@xxxxxxxx>
> > > CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > > CC: Julien Grall <julien.grall@xxxxxxx>
> > > CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> > >
> > > v2:
> > >    * Split/rearrange to avoid the post-domain-create error path.
> > > ---
> > >    xen/arch/arm/domain_build.c       |  3 ++-
> > >    xen/arch/arm/setup.c              | 12 ++++++++++++
> > >    xen/arch/x86/setup.c              |  3 +++
> > >    xen/common/domain.c               |  3 ++-
> > >    xen/common/grant_table.c          | 16 +++-------------
> > >    xen/include/asm-arm/grant_table.h | 12 ------------
> > >    xen/include/asm-x86/grant_table.h |  5 -----
> > >    xen/include/xen/grant_table.h     |  6 ++----
> > >    8 files changed, 24 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 1351572..737e0f3 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -2079,7 +2079,8 @@ static void __init find_gnttab_region(struct
> > > domain *d,
> > >         * enough space for a large grant table
> > >         */
> > >        kinfo->gnttab_start = __pa(_stext);
> > > -    kinfo->gnttab_size = gnttab_dom0_frames() << PAGE_SHIFT;
> > > +    kinfo->gnttab_size = min_t(unsigned int, opt_max_grant_frames,
> > > +                               PFN_DOWN(_etext - _stext)) <<
> > > PAGE_SHIFT;
> >
> > I agree with Jan's comment on v1 that there is a risk someone will
> > update the size here but ...
> >
> >
> > >      #ifdef CONFIG_ARM_32
> > >        /*
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > index 45f3841..3d3b30c 100644
> > > --- a/xen/arch/arm/setup.c
> > > +++ b/xen/arch/arm/setup.c
> > > @@ -20,6 +20,7 @@
> > >    #include <xen/compile.h>
> > >    #include <xen/device_tree.h>
> > >    #include <xen/domain_page.h>
> > > +#include <xen/grant_table.h>
> > >    #include <xen/types.h>
> > >    #include <xen/string.h>
> > >    #include <xen/serial.h>
> > > @@ -693,6 +694,17 @@ void __init start_xen(unsigned long
> > > boot_phys_offset,
> > >        struct domain *dom0;
> > >        struct xen_domctl_createdomain dom0_cfg = {
> > >            .max_evtchn_port = -1,
> > > +
> > > +        /*
> > > +         * The region used by Xen on the memory will never be mapped
> > > in DOM0
> > > +         * memory layout. Therefore it can be used for the grant table.
> > > +         *
> > > +         * Only use the text section as it's always present and will
> > > contain
> > > +         * enough space for a large grant table
> > > +         */
> > > +        .max_grant_frames = min_t(unsigned int, opt_max_grant_frames,
> > > +                                  PFN_DOWN(_etext - _stext)),
> > ... not here. So I would prefer if we either keep an helper to find
> > the size of pass that size around to domain_build. Do we store the
> > size in the domain information?
> I have to admit that I'm somewhat perplexed by ARM's
> find_gnttab_region(), and I'm not sure why it exists.
Dom0 is using the host memory layout that may differ between platforms. 
So there is not a region address that would fit everyone.
This function is here to find at boot a suitable region in the layout 
where the OS can map the grant-table. The result will be written in the 
firmware table.
> The value is available from d->grant_table.max_grant_frames but ISTR
> finding that the order of construction meant that it wasn't available
> when needed (although this was all from code inspection, so I could very
> easily be wrong).
I think it should be fine for Dom0 as find_gnttab_region is called from 
construct_dom0 and d->grant_table.max_grant_frames would be set before 
via domain_create().
Assuming d->grant_table.max_grant_frames can only be 0 before 
initialization, I would potentially add a 
BUG_ON(!d->grant_table.max_grant_frames) to make sure this always stay 
like that.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel