Re: [Xen-devel] [PATCH v3 38/39] ARM: new VGIC: Allocate two pages for struct vcpu

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 22 Mar 2018, Julien Grall wrote:
> Hi Andre,
> 
> On 03/21/2018 04:32 PM, Andre Przywara wrote:
> > At the moment we allocate exactly one page for struct vcpu on ARM, also
> > have a check in place to prevent it growing beyond 4KB.
> > As the struct includes the state of all 32 private (per-VCPU) interrupts,
> > we are at 3840 bytes on arm64 at the moment already. Growing the per-IRQ
> > VGIC structure even slightly makes the VCPU quickly exceed the 4K limit.
> > The new VGIC will need more space per virtual IRQ. I spent a few hours
> > trying to trim this down, but couldn't get it below 4KB, even with the
> > nasty hacks piling up to save some bytes here and there.
> > It turns out that beyond efficiency, maybe, there is no real technical
> > reason this struct has to fit in one page, so lifting the limit to two
> > pages seems like the most pragmatic solution.
> > Restrict this to compiling with the new VGIC and for ARM64 only.
> > 
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> > ---
> > Changelog v2 ... v3:
> > - rework alloc_vcpu_struct() to avoid nasty #ifdef
> > 
> > Changelog v1 ... v2:
> > - confine change to new VGIC and ARM64 only
> > 
> >   xen/arch/arm/domain.c | 25 +++++++++++++++++++++----
> >   1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 9688e62f78..23bda3f7db 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -505,19 +505,36 @@ void dump_pageframe_info(struct domain *d)
> >     }
> >   +/*
> > + * The new VGIC has a bigger per-IRQ structure, so we need more than one
> > + * page on ARM64. Cowardly increase the limit in this case.
> > + */
> > +#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
> > +#define PAGES_PER_VCPU  2
> > +#else
> > +#define PAGES_PER_VCPU  1
> > +#endif
> > +
> >   struct vcpu *alloc_vcpu_struct(void)
> >   {
> >       struct vcpu *v;
> > -    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
> > -    v = alloc_xenheap_pages(0, 0);
> > +
> > +    BUILD_BUG_ON(sizeof(*v) > PAGES_PER_VCPU * PAGE_SIZE);
> > +    v = alloc_xenheap_pages(get_order_from_pages(PAGES_PER_VCPU), 0);
> 
> I was suggesting to use get_order_from_pages(sizeof (...)) so if we end up to
> be smaller, you don't lose a page for nothing. But I am ok with that too and
> can revisit later. So:
> 
> Acked-by: Julien Grall <julien.grall@xxxxxxx>

That is actually a good suggestion, let's not lose track of it.

With that fix:

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>



> >       if ( v != NULL )
> > -        clear_page(v);
> > +    {
> > +        unsigned int i;
> > +
> > +        for ( i = 0; i < PAGES_PER_VCPU; i++ )
> > +            clear_page((void *)v + i * PAGE_SIZE);
> > +    }
> > +
> >       return v;
> >   }
> >     void free_vcpu_struct(struct vcpu *v)
> >   {
> > -    free_xenheap_page(v);
> > +    free_xenheap_pages(v, get_order_from_pages(PAGES_PER_VCPU));
> >   }
> >     int vcpu_initialise(struct vcpu *v)
> > 
 

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