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

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Hi guys,

On 8/23/21 8:16 PM, Julien Grall wrote:
> Hi Jan,
> 
> 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).

Cheers,
Costin