Re: Re: Xen/ia64 presentation

Re: Re: Xen/ia64 presentation

[Keir Fraser]

On 27 Apr 2005, at 20:12, Hollis Blanchard wrote:

>> Yeah, there are some artifacts and some cleanup would be good.
>> The typedef predates exec_domain (used to be a typedef of
>> struct domain) and I used it because I knew that exec_domain
>> was coming.  Personally, I prefer VCPU (virtual CPU) to exec_domain.
>
> I've had the same thought actually... an "exec_domain" is really a
> virtual CPU state, and having a separate vcpu_info_t is rather 
> confusing.
>
> However, I don't think it helps things to go renaming core structures 
> in
> arch code because it sounds better... :)

I think I agree that 'struct vcpu' is nicer than 'struct exec_domain'. 
exec_domain appears hardly at all at the hypervisor interface, and 
having two different terms used interchangeably within Xen itself is 
weird.

Another I can think of is cpuset vs. cpumask: I went with the former 
but I like the latter equally well and there is no good reason not to 
go with the Linux convention on this one.

Perhaps we should have a flag day to move to agreed consistent naming 
on some of these? The changes are trivially scriptable for the most 
part, but annoying for those with pending patches.

  -- Keir

Re: Re: Xen/ia64 presentation

[Hollis Blanchard]

Keir Fraser wrote:
> 
> I think I agree that 'struct vcpu' is nicer than 'struct exec_domain'.
> exec_domain appears hardly at all at the hypervisor interface, and
> having two different terms used interchangeably within Xen itself is weird.
> 
> Another I can think of is cpuset vs. cpumask: I went with the former but
> I like the latter equally well and there is no good reason not to go
> with the Linux convention on this one.
> 
> Perhaps we should have a flag day to move to agreed consistent naming on
> some of these? The changes are trivially scriptable for the most part,
> but annoying for those with pending patches.

Sounds good to me.

On this subject, I'd also like to ask about full_execution_context_t.
execution_context_t is used in a fair number of places in the Xen core;
however full_execution_context_t seems to only be used in the dom0
interface.

The in-Xen analog to full_execution_context_t is arch_exec_domain, with
many fields duplicated between the two. Could we consolidate these, or
at least give full_execution_context_t a name that better describes its
purpose?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: Xen/ia64 presentation

[Keir Fraser]

On 27 Apr 2005, at 20:12, Hollis Blanchard wrote:

>> Maintaining close ties to Linux makes it much easier to "go back to
>> the well".  I remember that Xen 1.0 removed a lot of the early
>> "start of day" code for other (e.g. Cyrix) processors; when the
>> user community grew and some users wanted to run on other processors,
>> the Xen team went back and grabbed the code from Linux.
>
> I don't necessarily see divergence as good or bad, but I don't rule it
> out. The Cyrix thing you described is a fine example of a lazy
> algorithm, which I can see you have lots of respect for. :) Remove
> dubious code, and if it turns out somebody complains (causing a code
> fault ;), it can be re-added. Any code that's kept has to be 
> maintained,
> and if no users even excercise it then it's quite likely to bitrot
> anyways. For example, Linux supports i386 processors as well, but I
> suspect it would be counter-productive to attempt that in Xen.

Exactly: within an architecture I think it makes sense to keep it 
simple but fault in features. I personally fear premature feature-itis 
and flexibility: I'd much rather add things in as they become 
necessary, whatever the project. In the case of Xen, we would otherwise 
forever be a gross hard-to-maintain patch on the side of Linux.

  The other concern that Dan talks about is what the arch-independent 
interface should look like -- I think that this will look really rather 
different between an OS and a hypervisor. For example, an arch with a 
software-managed TLB will not want to cope with generic interfaces 
supporting 3- or 4-level page tables, although that might well make 
sense in an OS (where you need some kind of address-space management 
structure anyway, and might as well look like a pagetable).

  -- Keir

Re: Re: Xen/ia64 presentation

[Keir Fraser]

On 27 Apr 2005, at 20:31, Hollis Blanchard wrote:

> Sounds good to me.
>
> On this subject, I'd also like to ask about full_execution_context_t.
> execution_context_t is used in a fair number of places in the Xen core;
> however full_execution_context_t seems to only be used in the dom0
> interface.
>
> The in-Xen analog to full_execution_context_t is arch_exec_domain, with
> many fields duplicated between the two. Could we consolidate these, or
> at least give full_execution_context_t a name that better describes its
> purpose?

Yes, that's another one that's gross. Maybe rename 
full_execution_context_t to execution_context_t, and rename existing 
execution_context_t to something else (cpu_reg_t, or something like 
that)?

  -- Keir

Re: Re: Xen/ia64 presentation

[Hollis Blanchard]

Keir Fraser wrote:
> 
> On 27 Apr 2005, at 20:31, Hollis Blanchard wrote:
>>
>> On this subject, I'd also like to ask about full_execution_context_t.
>> execution_context_t is used in a fair number of places in the Xen core;
>> however full_execution_context_t seems to only be used in the dom0
>> interface.
>>
>> The in-Xen analog to full_execution_context_t is arch_exec_domain, with
>> many fields duplicated between the two. Could we consolidate these, or
>> at least give full_execution_context_t a name that better describes its
>> purpose?
> 
> Yes, that's another one that's gross. Maybe rename
> full_execution_context_t to execution_context_t, and rename existing
> execution_context_t to something else (cpu_reg_t, or something like that)?

execution_context_t is also struct xen_regs, so if we like typedefs then
xen_regs_t would be consistent. Right now, lots of code uses xen_regs
and lots uses execution_context_t... should that be made consistent?

xen_regs/execution_context_t seems to mean "state which xen code could
alter", so something to distinguish it from "all CPU state" would be
nice. Maybe something like this:

struct xen_state:  (now xen_regs) state which xen C/asm code could alter
struct vcpu_state: (now exec_domain) all virtual CPU state
struct arch_vcpu_state

("vcpu_regs" might not be good, since we could need to save other
context like software-controlled TLBs, and so "xen_state" would match
"vcpu_state".)

I guess you want to keep a separate virtual CPU struct for the dom0
interface to preserve the ABI? Calling that "execution_context_t" could
work; I don't know what else to call it.

-- 
Hollis Blanchard
IBM Linux Technology Center

RE: Re: Xen/ia64 presentation

[Magenheimer, Dan (HP Labs Fort Collins)]

As you may have noticed, Xen/ia64 completely ignores
all of the execution context structs because (as
described in the base for this thread), ia64 can't
represent its state that way.

In fact if you grep for these (as well as xen_regs)
in common files, there are very few uses of them.
And I'd argue that the uses that ARE still in common
(e.g. keyhandler.c, serial/console drivers, dom0_ops)
are places that haven't yet been properly divided between
archdep and common (because Xen/ia64 hacks around them
rather than use them).  Either that or the datatype
is entirely opaque to the routine and is just being
passed along to another (archdep) routine.

In other words, I'm suggesting that perhaps all of
these constructs are archdep and (eventually) shouldn't
appear in common code.

Dan

> -----Original Message-----
> From: Hollis Blanchard [mailto:hollisb@us.ibm.com] 
> Sent: Wednesday, April 27, 2005 2:09 PM
> To: Keir Fraser
> Cc: Xen-devel; Magenheimer, Dan (HP Labs Fort Collins)
> Subject: Re: [Xen-devel] Re: Xen/ia64 presentation
> 
> Keir Fraser wrote:
> > 
> > On 27 Apr 2005, at 20:31, Hollis Blanchard wrote:
> >>
> >> On this subject, I'd also like to ask about 
> full_execution_context_t.
> >> execution_context_t is used in a fair number of places in 
> the Xen core;
> >> however full_execution_context_t seems to only be used in the dom0
> >> interface.
> >>
> >> The in-Xen analog to full_execution_context_t is 
> arch_exec_domain, with
> >> many fields duplicated between the two. Could we 
> consolidate these, or
> >> at least give full_execution_context_t a name that better 
> describes its
> >> purpose?
> > 
> > Yes, that's another one that's gross. Maybe rename
> > full_execution_context_t to execution_context_t, and rename existing
> > execution_context_t to something else (cpu_reg_t, or 
> something like that)?
> 
> execution_context_t is also struct xen_regs, so if we like 
> typedefs then
> xen_regs_t would be consistent. Right now, lots of code uses xen_regs
> and lots uses execution_context_t... should that be made consistent?
> 
> xen_regs/execution_context_t seems to mean "state which xen code could
> alter", so something to distinguish it from "all CPU state" would be
> nice. Maybe something like this:
> 
> struct xen_state:  (now xen_regs) state which xen C/asm code 
> could alter
> struct vcpu_state: (now exec_domain) all virtual CPU state
> struct arch_vcpu_state
> 
> ("vcpu_regs" might not be good, since we could need to save other
> context like software-controlled TLBs, and so "xen_state" would match
> "vcpu_state".)
> 
> I guess you want to keep a separate virtual CPU struct for the dom0
> interface to preserve the ABI? Calling that 
> "execution_context_t" could
> work; I don't know what else to call it.
> 
> -- 
> Hollis Blanchard
> IBM Linux Technology Center
> 

Re: Re: Xen/ia64 presentation

[Hollis Blanchard]

Magenheimer, Dan (HP Labs Fort Collins) wrote:
> As you may have noticed, Xen/ia64 completely ignores
> all of the execution context structs because (as
> described in the base for this thread), ia64 can't
> represent its state that way.

To remove common users of xen_regs/execution_context_t (i.e.
drivers/char/*.c) I think we should be able to just pass 'current' to
show_registers().

To remove full_execution_context_t, we'll need to change the
arch_getdomaininfo_ctxt() interface a little bit (to do the copy
itself), then add a arch hook for do_boot_vcpu() and set_info_guest().
Right?

Sounds like that would help clean up ia64 code a bit, so let's do it. :)

Even after that's done though, I see value in having core structures
like these named identically across architectures. I understand now that
some of those won't matter to you... but at least
exec_domain/arch_exec_domain still should, and we're also talking about
those names.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: Xen/ia64 presentation

[Keir Fraser]

On 27 Apr 2005, at 21:08, Hollis Blanchard wrote:

> xen_regs/execution_context_t seems to mean "state which xen code could
> alter", so something to distinguish it from "all CPU state" would be
> nice. Maybe something like this:
>
> struct xen_state:  (now xen_regs) state which xen C/asm code could 
> alter
> struct vcpu_state: (now exec_domain) all virtual CPU state
> struct arch_vcpu_state
>
> ("vcpu_regs" might not be good, since we could need to save other
> context like software-controlled TLBs, and so "xen_state" would match
> "vcpu_state".)

x86 Xen makes the distinction between xen_regs and 
full_execution_context only because xen_regs is what gets saved on the 
stack on entry to / exit from Xen. More advanced state like TLB info 
could probably be saved directly into 'struct vcpu_state' where you 
detect that you have interrupted a guest activation.

xen_regs/xen_state should probably be entirely arch-specific anyway. 
Even now it only pokes through into common code in interrupt-handler 
definitions (the final parameter is a xen_regs pointer). It'd be great 
to nuke those last few from common code. :-)

  -- Keir

Re: Re: Xen/ia64 presentation

[Keir Fraser]

On 28 Apr 2005, at 09:32, Keir Fraser wrote:

>> struct xen_state:  (now xen_regs) state which xen C/asm code could 
>> alter
>> struct vcpu_state: (now exec_domain) all virtual CPU state
>> struct arch_vcpu_state
>>
>> ("vcpu_regs" might not be good, since we could need to save other
>> context like software-controlled TLBs, and so "xen_state" would match
>> "vcpu_state".)

How about:

xen_regs/execution_context_t --> user_cpureg_state
   (registers visible from user space)

full_execution_context_t --> guest_vcpu_state
   (entire guest execution state; includes a user_cpureg_state)

The former isn't user_vcpureg_state because it contains registers 
visible from user space. Those are the same in number and format 
regardless of whether you are running natively on a real CPU or on a 
Xen-based virtual CPU.

  -- Keir

RE: RE: Xen/ia64 presentation

[Dong, Eddie]

Magenheimer, Dan (HP Labs Fort Collins) wrote:
> Maintaining close ties to Linux makes it much easier to "go back to
> the well".  I remember that Xen 1.0 removed a lot of the early
> "start of day" code for other (e.g. Cyrix) processors; when the
> user community grew and some users wanted to run on other processors,
> the Xen team went back and grabbed the code from Linux.  When the
> ACPI tables needed to be more fully parsed, the Xen team grabbed
> the ACPI code from Linux.  Recently, when I needed to add "user
> access" capability to Xen/ia64, I grabbed the exception table code
> from Linux/ia64 (and Linux common code); it basically just worked, in
> part because Keir had grabbed the x86 equivalent code earlier.
> 
Hi, Dan:
	"user access"  capability probably is not a right example. I
think you are talking about the mechanism to support Hypercall parameter
passing that you call it as poorman's exception handler mechanism. As we
discussed before, this mechanism will trigger guest tlb fault when the
TC map carrying guest hypercall information went away. This cause
following critical problem:
	1: When this hypercall is invoked in guest vpsr.ic=0, it will
trigger nested tlb miss which the guest can never handle.
	2: If the hypercall is invoked in guest tlb miss handler,
injecting guest tlb miss may cause endless (recursive) guest tlb miss->
hypercall -> guest tlb miss...

	The possible solution to this issue is to use either guest TR
map or guest special map (always reside in guest TLB) for the area
carrying hypercall information so that the map for this area will never
go away and thus the special "user access" mechanism that linux uses is
not necessary again. (In linux sitiuation, the user application never
run in vspr.ic=0 nor tlb miss handler)
Thanks,eddie

Re: Re: Xen/ia64 presentation

[Hollis Blanchard]

Keir Fraser wrote:
> 
> x86 Xen makes the distinction between xen_regs and
> full_execution_context only because xen_regs is what gets saved on the
> stack on entry to / exit from Xen. More advanced state like TLB info
> could probably be saved directly into 'struct vcpu_state' where you
> detect that you have interrupted a guest activation.

Absolutely; I have the same thing in PPC code.

I see you just renamed some structures in the unstable tree... now how
about exec_domain? Are we happy with "vcpu_state"?

> xen_regs/xen_state should probably be entirely arch-specific anyway.
> Even now it only pokes through into common code in interrupt-handler
> definitions (the final parameter is a xen_regs pointer). It'd be great
> to nuke those last few from common code. :-)

Yup. I think this could be done by passing 'current' to
show_registers(), and let the arch code figure out what to do from
there. After I get PPC to a more useful state I will see about the
patch, if somebody hasn't beaten me to it...

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: Re: Xen/ia64 presentation

[Keir Fraser]

On 28 Apr 2005, at 19:10, Hollis Blanchard wrote:

> I see you just renamed some structures in the unstable tree... now how
> about exec_domain? Are we happy with "vcpu_state"?

The '_state' seems a bit superfluous. How about just 'struct vcpu'?

>> xen_regs/xen_state should probably be entirely arch-specific anyway.
>> Even now it only pokes through into common code in interrupt-handler
>> definitions (the final parameter is a xen_regs pointer). It'd be great
>> to nuke those last few from common code. :-)
>
> Yup. I think this could be done by passing 'current' to
> show_registers(), and let the arch code figure out what to do from
> there. After I get PPC to a more useful state I will see about the
> patch, if somebody hasn't beaten me to it...

I've decided to backtracked on this one. Every architecture will have 
the concept of an interrupted activation, and a stack frame containing 
(at least some of) that activation's state. The pointer passed to IRQ 
handlers is a pointer to that state on the stack. If we do not pass it 
explicitly to the handler then it is very hard to reliably recalculate 
it if it is needed, and it is useful for debugging purposes at the very 
least.

A 'cpu_user_regs' seems like something every arch can provide, right?

  -- Keir

Re: Re: Xen/ia64 presentation

[Hollis Blanchard]

Keir Fraser wrote:
> 
> On 28 Apr 2005, at 19:10, Hollis Blanchard wrote:
> 
>> I see you just renamed some structures in the unstable tree... now how
>> about exec_domain? Are we happy with "vcpu_state"?
> 
> The '_state' seems a bit superfluous. How about just 'struct vcpu'?

Sure.

>>> xen_regs/xen_state should probably be entirely arch-specific anyway.
>>> Even now it only pokes through into common code in interrupt-handler
>>> definitions (the final parameter is a xen_regs pointer). It'd be great
>>> to nuke those last few from common code. :-)
>>
>>
>> Yup. I think this could be done by passing 'current' to
>> show_registers(), and let the arch code figure out what to do from
>> there. After I get PPC to a more useful state I will see about the
>> patch, if somebody hasn't beaten me to it...
> 
> 
> I've decided to backtracked on this one. Every architecture will have
> the concept of an interrupted activation, and a stack frame containing
> (at least some of) that activation's state.

Doesn't necessarily have to be on the stack; on PowerPC we are currently
using a separately-allocated piece of memory.

> The pointer passed to IRQ
> handlers is a pointer to that state on the stack. If we do not pass it
> explicitly to the handler then it is very hard to reliably recalculate
> it if it is needed, and it is useful for debugging purposes at the very
> least.

What "debugging purposes"? We've already seen that the only common code
using that type is to pass to (architecture-specific) show_registers().

x86 can get at its state by masking off ESP. PPC can get at it via
'current'. Why then pass a pointer through all interrupt handlers when
none of them care?

> A 'cpu_user_regs' seems like something every arch can provide, right?

PowerPC can, yes.

-- 
Hollis Blanchard
IBM Linux Technology Center

RE: Re: Xen/ia64 presentation

[Magenheimer, Dan (HP Labs Fort Collins)]

> The '_state' seems a bit superfluous. How about just 'struct vcpu'?

Or perhaps just vcpu_t, and let the arch provide the typedef?
 
> explicitly to the handler then it is very hard to reliably 
> recalculate 
> it if it is needed, and it is useful for debugging purposes 
> at the very 
> least.
> 
> A 'cpu_user_regs' seems like something every arch can provide, right?

On ia64, the state is pretty scattered about but all of it can
be found (at least non-performantly) from a pt_regs or a vcpu
so either one works for me as long as I can typedef/define it
(e.g. cpu_user_regs -> pt_regs).

Dan

Re: Re: Xen/ia64 presentation

[Keir Fraser]

On 28 Apr 2005, at 19:58, Hollis Blanchard wrote:

>> The pointer passed to IRQ
>> handlers is a pointer to that state on the stack. If we do not pass it
>> explicitly to the handler then it is very hard to reliably recalculate
>> it if it is needed, and it is useful for debugging purposes at the 
>> very
>> least.
>
> What "debugging purposes"?

e.e., the 'd' key handler that dumps register state.

> We've already seen that the only common code
> using that type is to pass to (architecture-specific) show_registers().

Yes. Common code never dereferences cpu_user_regs, but it needs to 
receive it and pass it on.

> x86 can get at its state by masking off ESP. PPC can get at it via
> 'current'. Why then pass a pointer through all interrupt handlers when
> none of them care?

No, that gets you an exec_domain (or 'vcpu', as we plan to rename it). 
Or it can get you the guest's cpu_user_regs. But if you interrupted a 
Xen activation then there is no way of getting at that outer 
activation's register state.

Even Linux has a pt_regs parameter passed to every interrupt handler, 
so it must surely be a good thing to do. ;-)

  -- Keir

Re: Re: Xen/ia64 presentation

[Hollis Blanchard]

Keir Fraser wrote:
>> We've already seen that the only common code
>> using that type is to pass to (architecture-specific) show_registers().
> 
> Yes. Common code never dereferences cpu_user_regs, but it needs to
> receive it and pass it on.

Shrug, I don't mind things how they are, especially since Dan says it
works for him. It just seems silly to pass it everywhere for a single user.

>> x86 can get at its state by masking off ESP. PPC can get at it via
>> 'current'. Why then pass a pointer through all interrupt handlers when
>> none of them care?
> 
> No, that gets you an exec_domain (or 'vcpu', as we plan to rename it).
> Or it can get you the guest's cpu_user_regs. But if you interrupted a
> Xen activation then there is no way of getting at that outer
> activation's register state.

Ah, ok. I will have to think more about these activations.

> Even Linux has a pt_regs parameter passed to every interrupt handler, so
> it must surely be a good thing to do. ;-)

Yes yes... :)

-- 
Hollis Blanchard
IBM Linux Technology Center