Re: [PATCH v2 25/25] arm/altp2m: Add test of xc_altp2m_change_gfn.

[Sergej Proskurin <proskurin@sec.in.tum.de>]

Hi Razvan,


On 08/02/2016 11:14 AM, Razvan Cojocaru wrote:
> On 08/01/2016 08:10 PM, Sergej Proskurin wrote:
>> This commit extends xen-access by a simple test of the functionality
>> provided by "xc_altp2m_change_gfn". The idea is to dynamically remap a
>> trapping gfn to another mfn, which holds the same content as the
>> original mfn.
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> Cc: Tamas K Lengyel <tamas@tklengyel.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/tests/xen-access/xen-access.c | 135 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 132 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
>> index eafd7d6..39b7ddf 100644
>> --- a/tools/tests/xen-access/xen-access.c
>> +++ b/tools/tests/xen-access/xen-access.c
>> @@ -38,6 +38,7 @@
>>  #include <sys/mman.h>
>>  #include <sys/poll.h>
>>  
>> +#define XC_WANT_COMPAT_MAP_FOREIGN_API
>>  #include <xenctrl.h>
>>  #include <xenevtchn.h>
>>  #include <xen/vm_event.h>
>> @@ -49,6 +50,8 @@
>>  #define START_PFN 0ULL
>>  #endif
>>  
>> +#define INVALID_GFN ~(0UL)
>> +
>>  #define DPRINTF(a, b...) fprintf(stderr, a, ## b)
>>  #define ERROR(a, b...) fprintf(stderr, a "\n", ## b)
>>  #define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno))
>> @@ -72,9 +75,14 @@ typedef struct xenaccess {
>>      xen_pfn_t max_gpfn;
>>  
>>      vm_event_t vm_event;
>> +
>> +    unsigned int ap2m_idx;
>> +    xen_pfn_t gfn_old;
>> +    xen_pfn_t gfn_new;
>>  } xenaccess_t;
>>  
>>  static int interrupted;
>> +static int gfn_changed = 0;
>>  bool evtchn_bind = 0, evtchn_open = 0, mem_access_enable = 0;
>>  
>>  static void close_handler(int sig)
>> @@ -82,6 +90,94 @@ static void close_handler(int sig)
>>      interrupted = sig;
>>  }
>>  
>> +static int copy_gfn(xc_interface *xch, domid_t domain_id,
>> +                    xen_pfn_t dst_gfn, xen_pfn_t src_gfn)
>> +{
>> +    void *src_vaddr = NULL;
>> +    void *dst_vaddr = NULL;
>> +
>> +    src_vaddr = xc_map_foreign_range(xch, domain_id, XC_PAGE_SIZE,
>> +                                     PROT_READ, src_gfn);
>> +    if ( src_vaddr == MAP_FAILED || src_vaddr == NULL)
>> +    {
>> +        return -1;
>> +    }
> Please don't use scopes for single return statements (i.e. no { ... }
> around them).
>

I will adapt the scopes to the coding style, thank you. These are
leftovers from my test outputs.

>> +
>> +    dst_vaddr = xc_map_foreign_range(xch, domain_id, XC_PAGE_SIZE,
>> +                                     PROT_WRITE, dst_gfn);
>> +    if ( dst_vaddr == MAP_FAILED || dst_vaddr == NULL)
>> +    {
>> +        return -1;
>> +    }
> But if this failed and the first one succeeds, shouldn't we munmap() the
> first one before the return?
>

You are absolutely right. I will adapt the implementation accordingly.

Thank you very much.

Best regards,
~Sergej


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