Re: [Xen-devel] [PATCH v6 2/4] xen/arm: use SYMBOL when required

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.01.19 at 18:44, <sstabellini@xxxxxxxxxx> wrote:
> On Thu, 10 Jan 2019, Jan Beulich wrote:
>> >>> On 10.01.19 at 00:42, <sstabellini@xxxxxxxxxx> wrote:
>> > @@ -1138,9 +1138,10 @@ void free_init_memory(void)
>> >      for ( i = 0; i < nr; i++ )
>> >          *(p + i) = insn;
>> >  
>> > -    set_pte_flags_on_range(__init_begin, len, mg_clear);
>> > +    set_pte_flags_on_range(SYMBOL(__init_begin), len, mg_clear);
>> >      init_domheap_pages(pa, pa + len);
>> > -    printk("Freed %ldkB init memory.\n", 
>> > (long)(__init_end-__init_begin)>>10);
>> > +    printk("Freed %ldkB init memory.\n",
>> > +           (long)(SYMBOL(__init_end)-SYMBOL(__init_begin))>>10);
>> 
>> I've noticed this only here, but I can't exclude I've overlooked other
>> instances: I think it would be really nice if you corrected formatting
>> at the same time (here: add the missing blanks).
> 
> OK
> 
> I tend not to do cleanups together with meaningful changes, because
> typically I find the resulting patch harder to review, but I am OK with
> doing cleanups if you the maintainer asks for them

Well, I wouldn't normally ask for more involved cleanups, but formatting
ones are surely okay in general. That's the best way after all to get rid
of formatting violations without dedicated (and often voluminous)
patches.


>> > --- a/xen/arch/arm/setup.c
>> > +++ b/xen/arch/arm/setup.c
>> > @@ -772,8 +772,10 @@ void __init start_xen(unsigned long boot_phys_offset,
>> >  
>> >      /* Register Xen's load address as a boot module. */
>> >      xen_bootmodule = add_boot_module(BOOTMOD_XEN,
>> > -                             (paddr_t)(uintptr_t)(_start + 
>> > boot_phys_offset),
>> > -                             (paddr_t)(uintptr_t)(_end - _start + 1), 
>> > false);
>> > +                             (paddr_t)(uintptr_t)(SYMBOL(_start) +
>> > +                                                  boot_phys_offset),
>> > +                             (paddr_t)(uintptr_t)(SYMBOL(_end) -
>> > +                                                  SYMBOL(_start) + 1), 
>> > false);
>> 
>> Why you need the double casts, i.e. why does (uintptr_t) alone not
>> suffice?
> 
> The original reason was just not to change the existing code outside of
> adding SYMBOL :-)
> 
> But to answer your question, uintptr_t is the same size of char*, while
> paddr_t is always 64bit. uintptr_t casts to integer type, paddr_t casts
> to the right size. I don't think it is allowed to change from pointer to
> integer and change integer size in a single cast.

Correct, but that's not what I've been asking for. Instead I'd like
to see the (paddr_t) casts dropped, at least if this was in code I'm
the maintainer for.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel