From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v4 08/15] x86/altp2m: add control of suppress_ve. Date: Fri, 10 Jul 2015 12:56:29 +0100 Message-ID: <559FB2ED.8070803@eu.citrix.com> References: <1436489553-6300-1-git-send-email-edmund.h.white@intel.com> <1436489553-6300-9-git-send-email-edmund.h.white@intel.com> <559FAEF3020000780008F576@mail.emea.novell.com> <559FA864.4000502@eu.citrix.com> <559FCD52020000780008F775@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <559FCD52020000780008F775@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Tim Deegan , Ravi Sahita , Wei Liu , Andrew Cooper , Ian Jackson , Ed White , xen-devel@lists.xen.org, tlengyel@novetta.com, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org On 07/10/2015 12:49 PM, Jan Beulich wrote: >>>> On 10.07.15 at 13:11, wrote: >> On 07/10/2015 10:39 AM, Jan Beulich wrote: >>>>>> On 10.07.15 at 02:52, wrote: >>>> @@ -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); >>> >>> So why -1 here ... >>> >>>> @@ -1566,14 +1567,14 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla, >>>> else >>>> { >>>> 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 ( p2ma != p2m_access_n2rwx ) >>>> { >>>> /* A listener is not required, so clear the access >>>> * restrictions. This set must succeed: we have the >>>> * gfn locked and just did a successful get_entry(). */ >>>> rc = p2m->set_entry(p2m, gfn, mfn, PAGE_ORDER_4K, >>>> - p2mt, p2m_access_rwx); >>>> + p2mt, p2m_access_rwx, sve); >>> >>> ... but sve here, when -1 means "retain current setting" anyway? >>> (Same question applies elsewhere.) >> >> This is my code. I considered whether to use -1 here, but since we're >> reading and retaining gfn, mfn, and p2mt, it seemed more consistent >> stylistically to just read and re-write it along with the others. >> >> In any case I don't have strong opinions. > > I'd suggest the other mechanism so one can easily see which places > actually want to change the flag (or set it to a specific value). But in > the end it's your call which way to go. That does make sense. In fact, if there were "leave the default" options for the other values (mfn, p2mt, &c) it would be clearer that only the page order and the access rights were being changed here. Anyway that's a minor issue at this point. Ed / Ravi, feel free to change it according to Jan's suggestion, or leave it as it is for now. -George