Re: [Xen-devel] [PATCH v6 02/14] arm/mem_access: Move PAGE_*_* macros to xen/page-defs.h

[Sergej Proskurin <proskurin@xxxxxxxxxxxxx>]

Hi Jan,


On 07/06/2017 05:24 PM, Jan Beulich wrote:
>>>> On 06.07.17 at 16:53, <proskurin@xxxxxxxxxxxxx> wrote:
>> On 07/06/2017 02:10 PM, Jan Beulich wrote:
>>>>>> On 06.07.17 at 13:50, <proskurin@xxxxxxxxxxxxx> wrote:
>>>> --- /dev/null
>>>> +++ b/xen/include/xen/page-defs.h
>>>> @@ -0,0 +1,24 @@
>>>> +#ifndef __XEN_PAGE_DEFS_H__
>>>> +#define __XEN_PAGE_DEFS_H__
>>>> +
>>>> +/* Helpers for different page granularities. */
>>>> +#define PAGE_SIZE_GRAN(gran)        (1UL << PAGE_SHIFT_##gran)
>>>> +#define PAGE_MASK_GRAN(gran)        (~(0ULL) << PAGE_SHIFT_##gran)
>>> Stray parentheses. I'm also unhappy about the type difference
>>> between size and mask. I guess both would best be paddr_t.
>>> That'll then also allow mask to be defined as -size. Another
>>> alternative would be to use 1L for size, thus guaranteeing
>>> suitable sign extension when used in contexts requiring a width
>>> wider than long.
>>>
>> Sounds reasonable. How about using 1L for PAGE_SIZE_GRAN to ensure a
>> suitable sign extension for types wider than long and ~((paddr_t)0) for
>> PAGE_MASK_GRAN?
> Once again - I really think the two should be of identical type.

Alright, then I will use paddr_t in both cases. Thank you.

>>>> +#define PAGE_ALIGN_GRAN(gran, addr) (((addr) + ~PAGE_MASK_##gran) & 
>>>> PAGE_MASK_##gran)
>>>> +
>>>> +#define PAGE_SHIFT_4K               (12)
>>> Stray parentheses again.
>>>
>>> Also, with you adding a new header that'll fall under REST
>>> maintainership, you should have Cc-ed all the REST maintainers
>>> imo.
>> Thank you for adding the REST maintainers in Cc. In addition, I have
>> included Konrad Rzeszutek Wilk and Julien Grall.
> Except that Konrad was Cc-ed, just that his list settings result in
> you not seeing that he was.

Cheers,
~Sergej


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