Re: [PATCH v2 1/4] public: Add page related definitions for accessing guests memory

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2021 15:03, Costin Lupu wrote:
> On 8/23/21 8:16 PM, Julien Grall wrote:
>> On 20/08/2021 10:26, Jan Beulich wrote:
>>> On 20.08.2021 11:08, Julien Grall wrote:
>>>> On 20/08/2021 08:44, Costin Lupu wrote:
>>>>> On 8/20/21 9:52 AM, Jan Beulich wrote:
>>>>>>> --- /dev/null
>>>>>>> +++ b/xen/include/public/page.h
>>>>>>> @@ -0,0 +1,36 @@
>>>>>>> +/******************************************************************************
>>>>>>>
>>>>>>> + * page.h
>>>>>>> + *
>>>>>>> + * Page definitions for accessing guests memory
>>>>>>> + *
>>>>>>> + * Permission is hereby granted, free of charge, to any person
>>>>>>> obtaining a copy
>>>>>>> + * of this software and associated documentation files (the
>>>>>>> "Software"), to
>>>>>>> + * deal in the Software without restriction, including without
>>>>>>> limitation the
>>>>>>> + * rights to use, copy, modify, merge, publish, distribute,
>>>>>>> sublicense, and/or
>>>>>>> + * sell copies of the Software, and to permit persons to whom the
>>>>>>> Software is
>>>>>>> + * furnished to do so, subject to the following conditions:
>>>>>>> + *
>>>>>>> + * The above copyright notice and this permission notice shall be
>>>>>>> included in
>>>>>>> + * all copies or substantial portions of the Software.
>>>>>>> + *
>>>>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
>>>>>>> KIND, EXPRESS OR
>>>>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>>>>>> MERCHANTABILITY,
>>>>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>>>>>>> EVENT SHALL THE
>>>>>>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>>>>>> OR OTHER
>>>>>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>>>>>> OTHERWISE, ARISING
>>>>>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>>>>>> OTHER
>>>>>>> + * DEALINGS IN THE SOFTWARE.
>>>>>>> + *
>>>>>>> + * Copyright (c) 2021, Costin Lupu
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef __XEN_PUBLIC_PAGE_H__
>>>>>>> +#define __XEN_PUBLIC_PAGE_H__
>>>>>>> +
>>>>>>> +#include "xen.h"
>>>>>>> +
>>>>>>> +#define XEN_PAGE_SHIFT           12
>>>>>>> +#define XEN_PAGE_SIZE            (xen_mk_long(1) << XEN_PAGE_SHIFT)
>>>>
>>>> This will use UL whereas on Arm a page frame should always be 64-bit
>>>> regardless the bitness. Shouldn't this be converted to use xen_ulong_t
>>>> instead?
>>>
>>> As pointed out on v1, XEN_PAGE_SIZE would better not end up as a
>>> value of signed type, for ...
>>
>> Did you mean "not end up as a value of **unsigned** type"...
>>
>>>
>>>>>>> +#define XEN_PAGE_MASK            (~(XEN_PAGE_SIZE - 1))
>>>
>>> ... this to suitably sign-extend to wider types is necessary.
>>
>> ... because, if I am not mistaken, the sign-extension wouldn't happen
>> with unsigned type. But then on v1 you wrote:
>>
>> "Imo the smallest type this should evaluate to is xen_ulong_t"
>>
>> Which I interpreted as this value should be 64-bit on Arm32. If this not
>> what you meant then I am lost.
>>
>>>
>>> Also unless you expect someone to use typeof(XEN_PAGE_SIZE) I'm
>>> afraid I don't see where the constant being long vs xen_long_t
>>> (if such existed) might matter.
>>> Otoh perhaps xen_mk_ulong() would
>>> better have produced a xen_ulong_t typed values in the first
>>> place, but I'm afraid we can't alter the existing macro.
>>
>> We can create a new one.
>>
>>>> Our stable ABI has not been designed with multiple page granularity in
>>>> mind. We could introduce a hypercall to query the page size used by the
>>>> ABI. But then, I don't think we have the full picture of how this is
>>>> going to pan out (I haven't try to use another page size on Xen yet).
>>>>
>>>> I think we have three choices here:
>>>>     1) Stick with the existing definition in the tools
>>>>     2) Move the definition in the public headers and only expose them to
>>>> the tools.
>>>>     3) Query the page size via a new hypervisor
>>>>
>>>> As I wrote above, 3) is going to take some time to get it right. So the
>>>> question here is whether 2) is temporarily better than 1).
>>>
>>> Because I understand 3) is some way out, and because I think 2) is
>>> better than 1), I wrote "might be an option" for what you call 2).
>>> But I could see people (Andrew for example) to take a different
>>> position and object to such a temporary measure.
>>
>> I think we need to make a decision so Costin doesn't keep sending
>> version on something that can't be merged. What does the others thinks?
> 
> From what I understood, in his last reply to 'stubdom: foreignmemory:
> Fix build after 0dbb4be739c5' thread, Andrew was OK with solution 2).

I agree it can be read this way. 

Jan