Re: [Xen-devel] [PATCH v2 44/45] ARM: new VGIC: Allocate two pages for struct vcpu

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 03/19/2018 10:00 AM, Julien Grall wrote:
> Hi Andre,
>
> Title: Please update it.
>
> On 03/15/2018 08:30 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 v1 ... v2:
> > - confine change to new VGIC and ARM64 only
> >
> >   xen/arch/arm/domain.c | 15 +++++++++++++++
> >   1 file changed, 15 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 18b915d2e9..3fba05bda5 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -508,10 +508,25 @@ void dump_pageframe_info(struct domain *d)
> >   struct vcpu *alloc_vcpu_struct(void)
> >   {
> >       struct vcpu *v;
> > +
> > +    /*
> > +     * 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)
> > +    BUILD_BUG_ON(sizeof(*v) > 2 * PAGE_SIZE);
> > +    v = alloc_xenheap_pages(1, 0);
> > +    if ( v != NULL ) {
> > +        clear_page(v);
> > +        clear_page((void *)v + PAGE_SIZE);
> > +    }
> > +#else
> >       BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
> >       v = alloc_xenheap_pages(0, 0);
> >       if ( v != NULL )
> >           clear_page(v);
> > +#endif.
> All this logic can just be simplified using get_order_* helpers. This 
> will avoid #ifdef like that in alloc_vcpu_struct which is really really 
> ugly.
To clarify, what I suggest is:

unsigned int order = get_order(...);
BUILD_BUG_ON(sizeof(*v) > 2 * PAGE_SIZE);

v = alloc_xenheap_pages(order, 0);
if ( v != NULL )
{
   for ( i = 0; i < (1U << order) )
      clear_page(v + (i * PAGE_SIZE));
}

Cheers,

> Cheers,
--
Julien Grall

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