Re: [Xen-devel] [PATCH v4 08/15] x86/altp2m: add control of suppress_ve.

["Sahita, Ravi" <ravi.sahita@xxxxxxxxx>]

>From: dunlapg@xxxxxxxxx [mailto:dunlapg@xxxxxxxxx] On Behalf Of George
>Dunlap
>Sent: Friday, July 10, 2015 10:03 AM
>
>On Fri, Jul 10, 2015 at 1:52 AM, Ed White <edmund.h.white@xxxxxxxxx>
>wrote:
>> From: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>>
>> The existing ept_set_entry() and ept_get_entry() routines are extended
>> to optionally set/get suppress_ve.  Passing -1 will set suppress_ve on
>> new p2m entries, or retain suppress_ve flag on existing entries.
>>
>> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
>
>So because my patch contained code written by Ed, and this patch now
>contains code written by you, I'm pretty sure that a strict observance of
>protocol would require his SoB to be retained (as I think I did when I sent 
>it),
>and your SoB to be added, for copyright purposes.
>
>In this particular case a lawyer might argue that the code snippets inquestion
>were so small or obvious as to be uncopyrightable, but it doesn't really hurt 
>to
>be a bit more strict than we need to be. :-)

I can add my Signed-off-by to cover both Ed and my code (from Intel 
perspective).

>
>Also, a description of what you had changed could have helped speed review.
>(It seems you've only added the bits requested to the p2m-pt
>implementation?)

Thatâs correct (from discussion with Ed).

>
>Finally, one thing I missed in the discussion before...
>
>> @@ -1528,16 +1528,17 @@ bool_t p2m_mem_access_check(paddr_t gpa,
>unsigned long gla,
>>      vm_event_request_t *req;
>>      int rc;
>>      unsigned long eip = guest_cpu_user_regs()->eip;
>> +    bool_t sve;
>>
>>      /* First, handle rx2rw conversion automatically.
>>       * These calls to p2m->set_entry() must succeed: we have the gfn
>>       * locked and just did a successful get_entry(). */
>>      gfn_lock(p2m, gfn, 0);
>> -    mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL);
>> +    mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL, &sve);
>>
>>      if ( npfec.write_access && p2ma == p2m_access_rx2rw )
>>      {
>> -        rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
>p2m_access_rw);
>> +        rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, p2mt,
>> + p2m_access_rw, sve);
>>          ASSERT(rc == 0);
>>          gfn_unlock(p2m, gfn, 0);
>>          return 1;
>> @@ -1546,7 +1547,7 @@ bool_t p2m_mem_access_check(paddr_t gpa,
>unsigned long gla,
>>      {
>>          ASSERT(npfec.write_access || npfec.read_access || npfec.insn_fetch);
>>          rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K,
>> -                            p2mt, p2m_access_rwx);
>> +                            p2mt, p2m_access_rwx, -1);
>>          ASSERT(rc == 0);
>>      }
>>      gfn_unlock(p2m, gfn, 0);
>
>This definitely should not be "sve" in the 'if' clause and "-1" in the 'else' 
>clause.
>Because I was looking only at the patch, I missed that when Jan raised the
>issue before. That's a mistake on my part -- would you mind doing as Jan
>suggests, and just making these "NULL" and "-1"
>throughout this file?

Ok I will make this change but I would like you to review it - could I send you 
a version on Saturday so you can review it (since you are not working on Monday)

Thanks,
Ravi

>
>Thanks!
> -George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel