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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 09/06/2016 02:53 PM, Julien Grall wrote:
> 
> 
> On 06/09/16 12:34, George Dunlap wrote:
>> On 06/09/16 12:20, Julien Grall wrote:
>>>
>>>
>>> On 06/09/16 12:05, George Dunlap wrote:
>>>> On 06/09/16 11:51, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 06/09/16 11:45, Razvan Cojocaru wrote:
>>>>>> Hello,
>>>>>>
>>>>>>> On 06/09/16 11:00, Razvan Cojocaru wrote:
>>>>>>>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>>>>>>>> index b648a9d..e65a9b8 100644
>>>>>>>> --- 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;
>>>>>>>
>>>>>>> Why didn't you implement this function for ARM?
>>>>>>
>>>>>> Because unfortunately I don't have an ARM setup to test it on and I
>>>>>> thought it would be unfair to publish the patch with untestable ARM
>>>>>> support.
>>>>>
>>>>> So what's the plan? Who will implement the ARM solution?
>>>>>
>>>>> I don't think there is a technical challenge to implement the ARM one.
>>>>
>>>> Are we going to require that all new functionality be implemented both
>>>> on x86 and on ARM?  That seems like a bit of a lot to ask of someone
>>>> who's not going to use it (and as Razvan points out, can't even test
>>>> it).  The mem_access functionality is still fairly technical -- anyone
>>>> who decides they want to use it and runs across the same problem Razvan
>>>> did should have no trouble implementing this hypercall for ARM when
>>>> they
>>>> need it.
>>>
>>> I am not saying we should every time implement the ARM version when the
>>> x86 version is added and vice-versa.
>>>
>>> However, I don't think this is a lot to ask when an hypercall benefits
>>> both architecture. Note that nobody would have complained that the code
>>> was not tested on ARM if the hypercall was implemented in the common
>>> code. Even if it could break the platform...
>>
>> So you really don't mind if Razvan submits a patch for code that he
>> hasn't tested and isn't sure works?
> 
> To be fair, I don't mind as long as it is marked untested. I already saw
> this kind of patch on the mailing list, and I always tested the patch
> myself.
> 
> It is much better than waiting another couple of release to have a
> simple hypercall implemented.
> 
>>
>> For someone coming along later wanting to use this functionality, I
>> would think "It's not implemented yet, but here is the x86 version you
>> can easily copy" would be better than "It's been implemented but never
>> tested -- we're not sure if it actually works or not".
>>
>> Having common code not tested on ARM is slightly different.  At least in
>> that case you know that the basic algorithm is correct because the code
>> works on one platform.  The only things that might break are
>> x86-specific assumptions.  Having the code not tested *at all*, other
>> than compile-tested, seems a lot worse to me.
> 
> ARM is providing free model [1] which can boot Xen. It is not difficult
> to setup and does not take much time to test a patch series.

I'm sure it's very easy to use for someone already familiar with the
tools. I did manage to download the Foundation Model and run the test
application that comes with it, but still need some time to figure out
how to run anything more complicated.

But before that, since Tamas has expressed interest in this feature and
is already working with ARM projects, maybe he'd be interested in a
follow-up patch implementing this for ARM? IMHO this would be the best
case since he's got both the hardware and the ARM tools to do the actual
testing of the patch. Tamas, what do you think?


Thanks,
Razvan

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