From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Liu Subject: Re: [PATCH v5 15/15] tools/xen-access: altp2m testcases Date: Tue, 14 Jul 2015 10:56:56 +0100 Message-ID: <20150714095655.GD4152@zion.uk.xensource.com> References: <1436832903-12639-1-git-send-email-edmund.h.white@intel.com> <1436832903-12639-16-git-send-email-edmund.h.white@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1436832903-12639-16-git-send-email-edmund.h.white@intel.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: Ed White Cc: Ravi Sahita , Wei Liu , George Dunlap , Ian Jackson , Tim Deegan , xen-devel@lists.xen.org, Jan Beulich , Andrew Cooper , tlengyel@novetta.com, Daniel De Graaf List-Id: xen-devel@lists.xenproject.org On Mon, Jul 13, 2015 at 05:15:03PM -0700, Ed White wrote: > From: Tamas K Lengyel > > Working altp2m test-case. Extended the test tool to support singlestepping > to better highlight the core feature of altp2m view switching. > > Signed-off-by: Tamas K Lengyel > Signed-off-by: Ed White > > Reviewed-by: Razvan Cojocaru Acked-by: Wei Liu Some nits below. I do notice there is inconsistency in coding style, so I won't ask you to resubmit just for fixing style. But a follow-up patch to fix up all style problem is welcome. > --- > tools/tests/xen-access/xen-access.c | 173 ++++++++++++++++++++++++++++++------ > 1 file changed, 148 insertions(+), 25 deletions(-) > > diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c > index 12ab921..6b69c26 100644 > --- a/tools/tests/xen-access/xen-access.c > +++ b/tools/tests/xen-access/xen-access.c > @@ -275,6 +275,19 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, domid_t domain_id) > return NULL; > } > > +static inline > +int control_singlestep( > + xc_interface *xch, > + domid_t domain_id, > + unsigned long vcpu, > + bool enable) > +{ > + uint32_t op = enable ? > + XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON : XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF; > + > + return xc_domain_debug_control(xch, domain_id, op, vcpu); > +} > + > /* > * Note that this function is not thread safe. > */ > @@ -317,13 +330,15 @@ static void put_response(vm_event_t *vm_event, vm_event_response_t *rsp) > > void usage(char* progname) > { > - fprintf(stderr, > - "Usage: %s [-m] write|exec|breakpoint\n" > + fprintf(stderr, "Usage: %s [-m] write|exec", progname); > +#if defined(__i386__) || defined(__x86_64__) > + fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec"); > +#endif > + fprintf(stderr, > "\n" > "Logs first page writes, execs, or breakpoint traps that occur on the domain.\n" > "\n" > - "-m requires this program to run, or else the domain may pause\n", > - progname); > + "-m requires this program to run, or else the domain may pause\n"); Indentation looks wrong, but this is not your fault so don't worry about this. > } > > int main(int argc, char *argv[]) > @@ -341,6 +356,8 @@ int main(int argc, char *argv[]) > int required = 0; > int breakpoint = 0; > int shutting_down = 0; > + int altp2m = 0; > + uint16_t altp2m_view_id = 0; > > char* progname = argv[0]; > argv++; > @@ -379,10 +396,22 @@ int main(int argc, char *argv[]) > default_access = XENMEM_access_rw; > after_first_access = XENMEM_access_rwx; > } > +#if defined(__i386__) || defined(__x86_64__) > else if ( !strcmp(argv[0], "breakpoint") ) > { > breakpoint = 1; > } > + else if ( !strcmp(argv[0], "altp2m_write") ) > + { > + default_access = XENMEM_access_rx; > + altp2m = 1; > + } > + else if ( !strcmp(argv[0], "altp2m_exec") ) > + { > + default_access = XENMEM_access_rw; > + altp2m = 1; > + } > +#endif > else > { > usage(argv[0]); > @@ -415,22 +444,73 @@ int main(int argc, char *argv[]) > goto exit; > } > > - /* Set the default access type and convert all pages to it */ > - rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0); > - if ( rc < 0 ) > + /* With altp2m we just create a new, restricted view of the memory */ > + if ( altp2m ) > { > - ERROR("Error %d setting default mem access type\n", rc); > - goto exit; > - } > + xen_pfn_t gfn = 0; > + unsigned long perm_set = 0; > + > + rc = xc_altp2m_set_domain_state( xch, domain_id, 1 ); Extraneous spaces in brackets. > + if ( rc < 0 ) > + { > + ERROR("Error %d enabling altp2m on domain!\n", rc); > + goto exit; > + } > + > + rc = xc_altp2m_create_view( xch, domain_id, default_access, &altp2m_view_id ); Extraneous spaces and line too long. > + if ( rc < 0 ) > + { > + ERROR("Error %d creating altp2m view!\n", rc); > + goto exit; > + } > > - rc = xc_set_mem_access(xch, domain_id, default_access, START_PFN, > - (xenaccess->max_gpfn - START_PFN) ); > + DPRINTF("altp2m view created with id %u\n", altp2m_view_id); > + DPRINTF("Setting altp2m mem_access permissions.. "); > > - if ( rc < 0 ) > + for(; gfn < xenaccess->max_gpfn; ++gfn) > + { > + rc = xc_altp2m_set_mem_access( xch, domain_id, altp2m_view_id, gfn, > + default_access); Space. > + if ( !rc ) > + perm_set++; > + } > + > + DPRINTF("done! Permissions set on %lu pages.\n", perm_set); > + > + rc = xc_altp2m_switch_to_view( xch, domain_id, altp2m_view_id ); Ditto. The rest of code has similar issues too. I won't point them out one by one. Again, don't worry about this now. Wei.