From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravindh Puthiyaparambil Subject: Re: [PATCH 0 of 2] Add libxc API that sets mem_access type for an array of gfns Date: Thu, 26 Apr 2012 18:36:21 -0700 Message-ID: References:

Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6886508737425108488==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Christian.Limpach@gmail.com Cc: tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org --===============6886508737425108488== Content-Type: multipart/alternative; boundary=bcaec54fbd3cb2499204be9f213b --bcaec54fbd3cb2499204be9f213b Content-Type: text/plain; charset=ISO-8859-1 On Apr 26, 2012 6:06 PM, "Christian Limpach" wrote: > > On Thu, Apr 26, 2012 at 5:15 PM, Aravindh Puthiyaparambil > wrote: > > Does this look correct now? > > It addresses the issues I've pointed out, but: > - you should leave the ASSERT where it is, or is there a reason to move it? Ok, I will move the ASSERT back to where it was. > - this is wrong: > > - old_entry = *ept_entry; > > + old_entry->epte = ept_entry->epte; > You should follow the code and see what uses old_entry and you'll see > that within the function old_entry->mfn is used (your diff changes the > line that uses it) and ept_free_entry also accesses mfn. I will fix that. > - are you sure you can move the ept_sync_domain call past the iommu code? > I was hoping Tim would give me feedback about that. > I made a similar change a while ago, though it is for a more specific > case, updating the ept table to "clean" the vram tracking. My change > is: > - clear needs_sync when setting the type to logdirty for a leaf entry > if ( !is_epte_present(ept_entry) || > (!target && p2mt == p2m_ram_logdirty) ) > needs_sync = 0; > - only call ept_free_entry in the non-leaf case > if ( target && is_epte_present(&old_entry) ) > ept_free_entry(p2m, &old_entry, target); > - call ept_sync_domain from hap_clean_vram_tracking > > Maybe you can do something similar, for example passing in a hint > whether ept_sync_domain needs to be done or not. In my case, the > reasoning is that all the entries changed from hap_clean_vram_tracking > are leaf entries, so ept_free_entry will never be called and thus > ept_sync_domain can be deferred. I didn't think through/consider the > iommu case since that code is commented out in my tree. > I thought about doing that initially. But then in the bulk case I would always have to call ept_sync_domain() to be on the safe side. But if the iommu case forces me down that path, then I guess I have no choice. Aravindh --bcaec54fbd3cb2499204be9f213b Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Apr 26, 2012 6:06 PM, "Christian Limpach" <christian.limpach@gmail.com> wrote: >
> On Thu, Apr 26, 2012 at 5:15 PM, Aravindh Puthiyaparambil
> <aravindh@virtuata.com= > wrote:
> > Does this look correct now?
>
> It addresses the issues I've pointed out, but:
> - you should leave the ASSERT where it is, or is there a reason to mov= e it?

Ok, I will move the ASSERT back to where it was.

> - this is wrong:
> > - =A0 =A0 =A0 =A0old_entry =3D *ept_entry;
> > + =A0 =A0 =A0 =A0old_entry->epte =3D ept_entry->epte;
> You should follow the code and see what uses old_entry and you'll = see
> that within the function old_entry->mfn is used (your diff changes = the
> line that uses it) and ept_free_entry also accesses mfn.

I will fix that.

> - are you sure you can move the ept_sync_domain call past the iommu= code?
>

I was hoping Tim would give me feedback about that.

> I made a similar change a while ago, though it is for a more specif= ic
> case, updating the ept table to "clean" the vram tracking. = =A0My change
> is:
> - clear needs_sync when setting the type to logdirty for a leaf entry<= br> > =A0 =A0 =A0 =A0 if ( !is_epte_present(ept_entry) ||
> =A0 =A0 =A0 =A0 =A0 =A0 (!target && p2mt =3D=3D p2m_ram_logdir= ty) )
> =A0 =A0 =A0 =A0 =A0 =A0needs_sync =3D 0;
> - only call ept_free_entry in the non-leaf case
> =A0 =A0if ( target && is_epte_present(&old_entry) )
> =A0 =A0 =A0 =A0ept_free_entry(p2m, &old_entry, target);
> - call ept_sync_domain from hap_clean_vram_tracking
>
> Maybe you can do something similar, for example passing in a hint
> whether ept_sync_domain needs to be done or not. =A0In my case, the > reasoning is that all the entries changed from hap_clean_vram_tracking=
> are leaf entries, so ept_free_entry will never be called and thus
> ept_sync_domain can be deferred. =A0I didn't think through/conside= r the
> iommu case since that code is commented out in my tree.
>

I thought about doing that initially. But then in the bulk case I would = always have to call ept_sync_domain() to be on the safe side. But if the io= mmu case forces me down that path, then I guess I have no choice.

Aravindh

--bcaec54fbd3cb2499204be9f213b-- --===============6886508737425108488== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6886508737425108488==--