Re: [Xen-devel] [PATCH V3] tools/libxc, xen/x86: Added xc_set_mem_access_multi()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 09/06/2016 05:07 PM, Jan Beulich wrote:
>>>> On 06.09.16 at 12:00, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1836,6 +1836,15 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
>> uint32_t nr,
>>      return 0;
>>  }
>>  
>> +long p2m_set_mem_access_multi(struct domain *d,
>> +                              const XEN_GUEST_HANDLE(const_uint64) pfn_list,
>> +                              const XEN_GUEST_HANDLE(const_uint8) 
>> access_list,
>> +                              uint32_t nr, uint32_t start, uint32_t mask,
>> +                              unsigned int altp2m_idx)
>> +{
>> +    return -ENOTSUP;
>> +}
> 
> If this indeed fixes the build problem on ARM, then I'd like to have an
> explanation (by you or the ARM maintainers) where ENOTSUP gets
> defined. I can't find any single instance of this throughout the tree,
> and headers outside of the tree aren't supposed to get included in
> the hypervisor build.

It's defined in errno.h, which some in-tree files do include, but as
stated I don't, at the moment, have access to an ARM setup, so it is
theoretically possible that the code still doesn't compile on ARM. I
thought it would, since it's trivial.

I won't send the next version until it becomes possible for me to at
least compile it on ARM.

>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -28,6 +28,7 @@
>>  #include <xen/event.h>
>>  #include <public/vm_event.h>
>>  #include <asm/domain.h>
>> +#include <xen/guest_access.h> /* copy_from_guest() */
>>  #include <asm/page.h>
> 
> I thought we've been through this before: Why does this xen/
> include get added in the middle of asm/ ones?

I'll fix this. I initially #included <asm/guest_access.h>, then fixed it
to be <xen/guest_access.h> but forgot to reorder the includes.

>> +static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
>> +                          struct p2m_domain *ap2m, p2m_access_t a,
>> +                          unsigned long gfn_l)
>>  {
>> -    struct p2m_domain *p2m = p2m_get_hostp2m(d), *ap2m = NULL;
>> -    p2m_access_t a, _a;
>> -    p2m_type_t t;
>> -    mfn_t mfn;
>> -    unsigned long gfn_l;
>> -    long rc = 0;
>> +    int rc = 0;
>>  
>> +    if ( ap2m )
>> +    {
>> +        rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, _gfn(gfn_l));
> 
> With this it becomes pretty clear that, following our general direction,
> set_mem_access()'s last parameter should itself be gfn_t.

I'll change it.

>> +static bool_t p2m_xenmem_access_to_p2m_access(struct p2m_domain *p2m,
> 
> The p2m_ prefix is kind of redundant here as long as the function is
> static.
> 
> Also plain bool please in new code, and ...
> 
>> @@ -1823,6 +1839,34 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
>> uint32_t nr,
>>  #undef ACCESS
>>      };
>>  
>> +    switch ( xaccess )
>> +    {
>> +    case 0 ... ARRAY_SIZE(memaccess) - 1:
>> +        *paccess = memaccess[xaccess];
>> +        break;
>> +    case XENMEM_access_default:
>> +        *paccess = p2m->default_access;
>> +        break;
>> +    default:
>> +        return 0;
> 
> ... false and ...
> 
>> +    }
>> +
>> +    return 1;
> 
> ... true respectively then.

I'll switch to bool.

>> @@ -520,6 +534,9 @@ int compat_memory_op(unsigned int cmd, 
>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>                  rc = -EFAULT;
>>              break;
>>  
>> +        case XENMEM_access_op:
>> +            break;
> 
> There's a group of several other XENMEM_* which also require no
> action (right above the XENMEM_get_vnumainfo handling). Please
> simply add this new case there.

I'll move it there.


Thanks,
Razvan

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