[Xen-ia64-devel] Re: [patch 5/5] IA64: Kexec: Use a separate RID for EFI

[Simon Horman <horms@xxxxxxxxxxxx>]

On Mon, Oct 22, 2007 at 01:33:29PM -0600, Alex Williamson wrote:
> On Mon, 2007-10-22 at 11:49 +0900, Simon Horman wrote:
> > The patch does seem to work, in the sense that the EFI mappings work.
> > I have not stress tested it to see if domains can still do nefarious
> > things. I would appreciate a review of this.
> 
> Hi Simon,
> 
>    This generally looks ok, but I'd like to get an ack from Isaku and
> Tristan.

Yes, I'm interested to see what they say too.

> > The patch is rather long, this is almost entirely due to
> > duplicating the PAL, EFI and SAL macros in order to squeeze
> > in the RID changes and wrap them in #if XEN. It would be shorter
> > if I didn't bother with #if XEN. I'm happy to do that if its
> > the prefered solution.
> 
>    #ifdef XEN is simply for us to try to track changes from upstream so
> we can re-base in the future.  I think anything that makes it obvious
> what changes are for Xen would be sufficient.  Since this is only used
> in very tight code, perhaps we could get away w/ something like:

> #ifdef XEN
> #define XEN_EFI_RR_SAVE() do {                \
>       unsigned long rr6, rr7;         \
>       rr6 = ia64_get_rr(6);           \
>       rr7 = ia64_get_rr(7);           \
>       ia64_set_rr(6, EFI_RID);        \
>       ia64_set_rr(7, EFI_RID);        \
>       ia64_srlz_d()
> 
> #define XEN_EFI_RR_RESTORE()          \
>       ia64_set_rr(6, rr6);            \
>       ia64_set_rr(7, rr7);            \
>       ia64_srlz_d();                  \
>       } while (0)
> #else
> #define       XEN_EFI_RR_SAVE()       do {} while (0)
> #define       XEN_EFI_RR_RESTORE()    do {} while (0)
> #endif

Sure, whatever is fine by you is fine by me here.
I'll update my patch as you suggest.

> > The patch also includes some header foo. This is basically
> > because pal.h needs to know about GRANULE_SIZE, and thus needs
> > pgtable.h. But without the header manipulation I added a loop
> > is formed. I can break this out into another patch if need be.
> > I'm also happy to examine alternative solutions to this problem,
> > but to be frank, the headers are a mess.
> 
>    I'd prefer a separate patch to make it clear what's happening.  Also,
> is this the reason for the duplicate EFI_RID definitions?  I don't like
> that ivt.S has it's own definition.  Perhaps including pal.h got missed
> there, although I'm not sure pal.h is the appropriate place to define
> EFI_RID, EFI_RR_RESTORE and EFI_RR_SAVE.  Thanks,

Where wasn't supposed to be a duplicate EFI_RID, I suspect I got
tired and went home at some point and then forgot where I was up to
the next day. I'll clean it up a bit more and break things up
into separate patches a bit more.

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/


_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel