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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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