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