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

[Jan Beulich <jbeulich@suse.com>]

On 23.08.2021 19:16, 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"...

Oh, of course I did. I'm sorry for the confusion caused.

>>>>>> +#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.

And there I would better have said "If indeed unsigned for whatever
reason, the smallest type this should evaluate to is xen_ulong_t."

>> 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.

But we shouldn't carelessly add stuff, as we can't later remove it.

Jan