Re: [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns

[Aravindh Puthiyaparambil <aravindh@virtuata.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2690 bytes --]

On Thu, May 17, 2012 at 3:05 AM, Tim Deegan <tim@xen.org> wrote:
>
> Hi Aravindh,
>
> At 15:02 -0700 on 04 May (1336143748), Aravindh Puthiyaparambil wrote:
> > diff -r 9dda0efd8ce1 -r c180942992bf xen/arch/x86/mm/p2m-ept.c
> > --- a/xen/arch/x86/mm/p2m-ept.c Fri Apr 27 17:57:55 2012 +0200
> > +++ b/xen/arch/x86/mm/p2m-ept.c Fri May 04 14:56:12 2012 -0700
> > @@ -274,10 +274,13 @@ static int ept_next_level(struct p2m_dom
> >  /*
> >   * ept_set_entry() computes 'need_modify_vtd_table' for itself,
> >   * by observing whether any gfn->mfn translations are modified.
> > + * If sync_deferred is not NULL, then the caller will take care of
> > + * calling ept_sync_domain() in the cases where it can be deferred.
> >   */
> >  static int
> >  ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
> > -              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma)
> > +              unsigned int order, p2m_type_t p2mt, p2m_access_t p2ma,
> > +              bool_t *sync_deferred)
> >  {
> >      ept_entry_t *table, *ept_entry = NULL;
> >      unsigned long gfn_remainder = gfn;
> > @@ -309,6 +312,9 @@ ept_set_entry(struct p2m_domain *p2m, un
> >             (target == 1 && hvm_hap_has_2mb(d)) ||
> >             (target == 0));
> >
> > +    if (sync_deferred)
>
> Xen coding style has spaces inside those parentheses.
> > +        *sync_deferred = 1;
> > +
> >      table = map_domain_page(ept_get_asr(d));
> >
> >      for ( i = ept_get_wl(d); i > target; i-- )
> > @@ -345,8 +351,12 @@ ept_set_entry(struct p2m_domain *p2m, un
> >          ept_entry_t new_entry = { .epte = 0 };
> >
> >          /* No need to flush if the old entry wasn't valid */
> > -        if ( !is_epte_present(ept_entry) )
> > +        if ( !is_epte_present(ept_entry) || (!target && sync_deferred)
> > )
> > +        {
> >              needs_sync = 0;
> > +            if ( sync_deferred )
> > +                *sync_deferred = 0;
>
> I don't think you should do this -- you call this function in a loop and
> you may need to sync because of an earlier iteration.  Better to only
> write a 1 to *sync_deferred once you know there's a sync to be done, and
> never to write a zero.

You are right. I will fix that.

> > +        }
>
> One comment on the rest of the patch: your calling function calls
> ept_sync_domain() directly if sync_deferred == 1.  That's correct for
> now but it would be cleaner to use a sync() function pointer in the
> struct p2m_domain, the same as the other implementation-specific calls.

I can add that too.

> Other than that, I think this looks pretty good to go in after 4.2 has
> branched.

OK, I will resubmit once 4.2 has branched.

Thanks,
Aravindh

[-- Attachment #1.2: Type: text/html, Size: 3337 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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