Re: [Xen-devel] [PATCH v2 2/4] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sun, 2014-07-20 at 21:28 +0100, Julien Grall wrote:
> > +        mov   r3, #0x0ff             /* r2 := LPAE entries mask */
> 
> The register in the comment looks wrong to me.

Indeed, well spotted.

> 
> 
> > +        orr   r3, r3, #0x100
> 
> I took me several minutes to understand why the orr... I think the 2 
> previous assembly lines could be replaced by a single one:
>             ldr r3, =LPAE_ENTRY_MASK

Right. I'll do that.

> > +        lsl   x1, x1, #3
> > +        str   x2, [x4, x1]           /* Mapping of paddr(start) */
> >
> >   1:      /* Setup boot_first: */
> >           ldr   x4, =boot_first        /* Next level into boot_first */
> > @@ -290,7 +292,7 @@ skip_bss:
> >
> >           /* ... map of paddr(start) in boot_first */
> >           lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in 
> > boot_first */
> > -        and   x1, x2, 0x1ff          /* x1 := Slot to use */
> > +        and   x1, x2, #0x1ff         /* x1 := Slot to use */
> 
> Same here.
> 
> OOI, you only added the #. Was there any issue before?

It assembles to the same thing, it's just incorrect (or at least
non-preferred) syntax.

> > +        and   x1, x2, 0x1ff          /* x1 := Slot to use */
>
> A bit strange that few lines above you changed a similar lines to add #, 
> and here you forgot it.

Nothing strange, just an oversight.

> I would also replace the 0x1ff by LPAE_ENTRY_MASK.

Ack.

Ian.


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