RE: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Subject: Re: [PATCH v2] xen/arm: p2m: Populate pages for GICv2 mapping in
> arch_domain_create()
> >>> +    spin_lock(&d->arch.paging.lock);
> >>> +    rc = p2m_set_allocation(d, 16, NULL);
> >>> +    spin_unlock(&d->arch.paging.lock);
> >>> +    if ( rc != 0 )
> >>> +        goto fail;
> >>
> >> Putting this level of knowledge here feels like a layering violation to
> >> me. My first suggestion would be to move this call somewhere under
> >> p2m_init().
> >
> > That is definitely possible. If Julien or other Arm maintainers are not
> > against that I am happy to move this to p2m_init() in v3.
> I understand both of Jan and your concern. I don't really have a strong
> opinion either way.
> 
> You are the author of the patch, so I will let you chose.

Then p2m_init(), just want to make everyone happy :)))

> 
> >>> +    if ( d->arch.paging.p2m_total_pages != 0 )
> >>> +    {
> >>> +        spin_lock(&d->arch.paging.lock);
> >>> +        p2m_set_allocation(d, 0, NULL);
> >>> +        spin_unlock(&d->arch.paging.lock);
> >>> +        ASSERT(d->arch.paging.p2m_total_pages == 0);
> >>> +    }
> >>
> >> Is it intentional to largely open-code p2m_teardown_allocation() here?
> >
> > Yes, AFAICT p2m_teardown_allocation() is preemptible and we don't want
> > any preemption here.
> 
> Like Jan, I would prefer if we can avoid the duplication. The loop
> suggested by Jan should work.

I am a little bit worried about the -ENOMEM, if -ENOMEM is
returned from p2m_teardown_allocation(d), I think we are in
the infinite loop, or did I miss understood the loop that Jan referred
to?

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall