Re: [Xen-devel] [PATCH for-next v2 07/10] x86/domain: factor out pv_domain_destroy

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.04.17 at 14:56, <wei.liu2@xxxxxxxxxx> wrote:
> On Wed, Apr 26, 2017 at 06:32:46AM -0600, Jan Beulich wrote:
>> >>> On 25.04.17 at 15:52, <wei.liu2@xxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/domain.c
>> > +++ b/xen/arch/x86/domain.c
>> > @@ -536,6 +536,16 @@ static bool emulation_flags_ok(const struct domain 
>> > *d, 
> uint32_t emflags)
>> >      return true;
>> >  }
>> >  
>> > +static void pv_domain_destroy(struct domain *d)
>> > +{
>> > +    destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
>> > +                              GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
>> 
>> What use is this, considering both calling paths have ...
>> 
>> > @@ -712,10 +722,8 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags,
>> >          paging_final_teardown(d);
>> >      free_perdomain_mappings(d);
>> 
>> ... this and ...
>> 
>> >      if ( is_pv_domain(d) )
>> > -    {
>> > -        xfree(d->arch.pv_domain.cpuidmasks);
>> > -        free_xenheap_page(d->arch.pv_domain.gdt_ldt_l1tab);
>> > -    }
>> > +        pv_domain_destroy(d);
>> > +
>> >      return rc;
>> >  }
>> >  
>> > @@ -735,10 +743,7 @@ void arch_domain_destroy(struct domain *d)
>> >  
>> >      free_perdomain_mappings(d);
>> 
>> ... this?
>> 
> 
> Yes, I know. But IMO it would be far clearer we have matching allocation
> and deallocation.

Well, in the case here it's really pointless, but if want to keep it I
don't mind as long as ...

> We still need free_perdomain_mappings to have it free the top-level page
> table.

... you then at least switch their order, so that
free_perdomain_mappings() happens last.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel