Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On Mon, Jun 25, 2012 at 06:50:06PM +0300, Avi Kivity wrote:
Later we can extend x86_decode_insn() and the other
functions to follow the same rule.
What rule? We cannot not initialize a context. You can reduce things
that should be initialized to minimum (getting GP registers on demand,
etc), but still some initialization is needed since ctxt holds emulation
state and it needs to be reset before each emulation.
An alternative is to use two contexts, the base context only holds ops
and is the parameter to all the callbacks on the non-state APIs, the
derived context holds the state:
struct x86_emulation_ctxt {
struct x86_ops *ops;
/* state that always needs to be initialized, preferablt none */
};
struct x86_insn_ctxt {
struct x86_emulation_ctxt em;
/* instruction state */
}
and so we have a compile-time split between users of the state and
non-users.
I do not understand how you will divide current ctxt structure between
those two.
Where will you put those for instance: interruptibility, have_exception,
perm_ok, only_vendor_specific_insn and how can they not be initialized
before each instruction emulation?
x86_emulate_ops::get_interruptibility()
x86_emulate_ops::set_interruptibility()
x86_emulate_ops::exception()
They do not remove the need for initialization before instruction
execution, they just move things that need to be initialized somewhere
else (to kvm_arch_vcpu likely).
x86_decode_insn(struct x86_insn_ctxt *ctxt, unsigned flags)
{
ctxt-flags = flags;
ctxt-perm_ok = false;
}
In short, instruction emulation state is only seen by instruction
emulation functions, the others don't get to see it.
So you want to divide emulator.c to two types of function: those without
side effect, that do some kind of calculations on vcpu state according
to weird x86 rules, and those that change vcpu state and write it back
eventually. I do not see the justification for that complication really.
emulator.c is complicated enough already and the line between two may be
blurred.
If you dislike linearize() callback so much I can make
kvm_linearize_address() to do calculation base on its parameters only.
It is almost there, only cpl and seg base/desc are missing from
parameter list. I can put it into header and x86.c/emulator.c will both
be able to use it.
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/26/2012 11:30 AM, Gleb Natapov wrote:
Where will you put those for instance: interruptibility, have_exception,
perm_ok, only_vendor_specific_insn and how can they not be initialized
before each instruction emulation?
x86_emulate_ops::get_interruptibility()
x86_emulate_ops::set_interruptibility()
x86_emulate_ops::exception()
They do not remove the need for initialization before instruction
execution, they just move things that need to be initialized somewhere
else (to kvm_arch_vcpu likely).
x86_decode_insn(struct x86_insn_ctxt *ctxt, unsigned flags)
{
ctxt-flags = flags;
ctxt-perm_ok = false;
}
In short, instruction emulation state is only seen by instruction
emulation functions, the others don't get to see it.
So you want to divide emulator.c to two types of function: those without
side effect, that do some kind of calculations on vcpu state according
to weird x86 rules, and those that change vcpu state and write it back
eventually. I do not see the justification for that complication really.
emulator.c is complicated enough already and the line between two may be
blurred.
Really, the only issue is that the read/write callbacks sometimes cannot
return a result. Otherwise the entire thing would be stateless.
If you dislike linearize() callback so much I can make
kvm_linearize_address() to do calculation base on its parameters only.
It is almost there, only cpl and seg base/desc are missing from
parameter list. I can put it into header and x86.c/emulator.c will both
be able to use it.
And all the stack mask and stuff? Yuck.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/24/2012 05:27 PM, Gleb Natapov wrote:
On Sun, Jun 24, 2012 at 04:39:22PM +0300, Avi Kivity wrote:
On 06/24/2012 04:27 PM, Gleb Natapov wrote:
On Sun, Jun 24, 2012 at 04:12:05PM +0300, Avi Kivity wrote:
On 06/12/2012 03:01 PM, Gleb Natapov wrote:
The function will be used outside of the emulator.
/*
* x86_emulate_ops:
*
@@ -194,6 +199,10 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+
+ int (*linearize)(struct x86_emulate_ctxt *ctxt,
+ struct segmented_address addr, unsigned size,
+ bool write, bool fetch, ulong *linear);
};
linearize is defined in terms of the other ops; this means that if we
get a second user they will have to replicate it.
What do you mean? This patch series adds another user, so now there are
two: one
inside the emulator another is outside.
I meant like task switching or real-mode interrupt emulation.
You mean code outside of KVM if we ever will make emulator reusable? It will
have to
have its own, much more simple version of the callback.
Why not make the current linearize available to users?
Code outside of the emulator does not call the emulator except when
emulation is actually needed. To call linearize() from the emulator.c
almost fully functional emulation ctxt will have to be set up (including
fake instruction decoding, hacky and slower).
ctxt-d use should be removed for the exported version and replaced by a
parameter. The internal version can still use it (calling the exported
version after extracting the parameter).
IMO we should stick to the pattern we have now: calling generic code from
the emulator and not vice versa. Lets not create more spaghetti.
To not duplicate the logic
I moved linearize() to generic code and made it available to emulator
via callback. It actually saves a couple of callback invocations when
emulator calls linearize() IIRC.
It's not available to other emulator users (which don't exist yet
anyway). But having linearize() in the emulator is consistent with
placing logic in emulate.c and accessors outside.
It is the question of where we draw the line. For instance MMU details
are now hidden from the emulator behind a callback. One can argue that
emulator should have access to MMU directly via callbacks and
emulate memory access by itself.
Right now the all segment related operations are behind the line; the
line is linear | physical. Having a -linearize op will change that.
Regarding initialization, we should eventually initialize nothing and
let the emulator bring in needed data via callbacks (including general
registers).
Some things will have to be initialized (or rather reset to initial value)
between emulator invocations. Access to registers can be done on demand,
but this is unrelated to this series optimization.
Right. But I think we can have x86_linearize() that doesn't take a
context parameter, only ops.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On Mon, Jun 25, 2012 at 03:57:42PM +0300, Avi Kivity wrote:
On 06/24/2012 05:27 PM, Gleb Natapov wrote:
On Sun, Jun 24, 2012 at 04:39:22PM +0300, Avi Kivity wrote:
On 06/24/2012 04:27 PM, Gleb Natapov wrote:
On Sun, Jun 24, 2012 at 04:12:05PM +0300, Avi Kivity wrote:
On 06/12/2012 03:01 PM, Gleb Natapov wrote:
The function will be used outside of the emulator.
/*
* x86_emulate_ops:
*
@@ -194,6 +199,10 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+
+ int (*linearize)(struct x86_emulate_ctxt *ctxt,
+ struct segmented_address addr, unsigned size,
+ bool write, bool fetch, ulong *linear);
};
linearize is defined in terms of the other ops; this means that if we
get a second user they will have to replicate it.
What do you mean? This patch series adds another user, so now there are
two: one
inside the emulator another is outside.
I meant like task switching or real-mode interrupt emulation.
You mean code outside of KVM if we ever will make emulator reusable? It
will have to
have its own, much more simple version of the callback.
Why not make the current linearize available to users?
Code outside of the emulator does not call the emulator except when
emulation is actually needed. To call linearize() from the emulator.c
almost fully functional emulation ctxt will have to be set up (including
fake instruction decoding, hacky and slower).
ctxt-d use should be removed for the exported version and replaced by a
parameter. The internal version can still use it (calling the exported
version after extracting the parameter).
IMO we should stick to the pattern we have now: calling generic code from
the emulator and not vice versa. Lets not create more spaghetti.
To not duplicate the logic
I moved linearize() to generic code and made it available to emulator
via callback. It actually saves a couple of callback invocations when
emulator calls linearize() IIRC.
It's not available to other emulator users (which don't exist yet
anyway). But having linearize() in the emulator is consistent with
placing logic in emulate.c and accessors outside.
It is the question of where we draw the line. For instance MMU details
are now hidden from the emulator behind a callback. One can argue that
emulator should have access to MMU directly via callbacks and
emulate memory access by itself.
Right now the all segment related operations are behind the line; the
line is linear | physical. Having a -linearize op will change that.
Regarding initialization, we should eventually initialize nothing and
let the emulator bring in needed data via callbacks (including general
registers).
Some things will have to be initialized (or rather reset to initial value)
between emulator invocations. Access to registers can be done on demand,
but this is unrelated to this series optimization.
Right. But I think we can have x86_linearize() that doesn't take a
context parameter, only ops.
All ops take context parameter though.
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/25/2012 04:12 PM, Gleb Natapov wrote: Right. But I think we can have x86_linearize() that doesn't take a context parameter, only ops. All ops take context parameter though. context is meaningful for: - saving state between executions (decode/execute/execute) - passing state that is not provided via callbacks (regs/mode/flags) - returning results Only the second is relevant, and we're trying to get rid of that too. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On Mon, Jun 25, 2012 at 04:40:35PM +0300, Avi Kivity wrote: On 06/25/2012 04:12 PM, Gleb Natapov wrote: Right. But I think we can have x86_linearize() that doesn't take a context parameter, only ops. All ops take context parameter though. context is meaningful for: - saving state between executions (decode/execute/execute) - passing state that is not provided via callbacks (regs/mode/flags) - returning results Only the second is relevant, and we're trying to get rid of that too. Callbacks were passed pointer to vcpu, but they were changed to get ctxt to better encapsulate emulator.c from rest of the KVM. Are you suggesting this was a mistake and we need to rework callbacks to receive pointer to vcpu again? I hope not :) -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/25/2012 05:17 PM, Gleb Natapov wrote: On Mon, Jun 25, 2012 at 04:40:35PM +0300, Avi Kivity wrote: On 06/25/2012 04:12 PM, Gleb Natapov wrote: Right. But I think we can have x86_linearize() that doesn't take a context parameter, only ops. All ops take context parameter though. context is meaningful for: - saving state between executions (decode/execute/execute) - passing state that is not provided via callbacks (regs/mode/flags) - returning results Only the second is relevant, and we're trying to get rid of that too. Callbacks were passed pointer to vcpu, but they were changed to get ctxt to better encapsulate emulator.c from rest of the KVM. Are you suggesting this was a mistake and we need to rework callbacks to receive pointer to vcpu again? I hope not :) Ouch. I guess we have to pass the context, but not initialize any of it except ops. Later we can extend x86_decode_insn() and the other functions to follow the same rule. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On Mon, Jun 25, 2012 at 05:32:31PM +0300, Avi Kivity wrote: On 06/25/2012 05:17 PM, Gleb Natapov wrote: On Mon, Jun 25, 2012 at 04:40:35PM +0300, Avi Kivity wrote: On 06/25/2012 04:12 PM, Gleb Natapov wrote: Right. But I think we can have x86_linearize() that doesn't take a context parameter, only ops. All ops take context parameter though. context is meaningful for: - saving state between executions (decode/execute/execute) - passing state that is not provided via callbacks (regs/mode/flags) - returning results Only the second is relevant, and we're trying to get rid of that too. Callbacks were passed pointer to vcpu, but they were changed to get ctxt to better encapsulate emulator.c from rest of the KVM. Are you suggesting this was a mistake and we need to rework callbacks to receive pointer to vcpu again? I hope not :) Ouch. I guess we have to pass the context, but not initialize any of it except ops. That's hacky and error pron. We need to audit that linearize() and all callbacks/functions it uses do not rely on un-initialized state, which is doable now, but who will remember to check that in the future, while changing seemingly unrelated code, which, by a coincidence, called during linearize()? Instant security vulnerability. For security (if not sanity) sake we should really make sure that ctxt is initialized while we are in emulator.c and make as many checks for it as possible. Later we can extend x86_decode_insn() and the other functions to follow the same rule. What rule? We cannot not initialize a context. You can reduce things that should be initialized to minimum (getting GP registers on demand, etc), but still some initialization is needed since ctxt holds emulation state and it needs to be reset before each emulation. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/25/2012 05:55 PM, Gleb Natapov wrote:
On Mon, Jun 25, 2012 at 05:32:31PM +0300, Avi Kivity wrote:
On 06/25/2012 05:17 PM, Gleb Natapov wrote:
On Mon, Jun 25, 2012 at 04:40:35PM +0300, Avi Kivity wrote:
On 06/25/2012 04:12 PM, Gleb Natapov wrote:
Right. But I think we can have x86_linearize() that doesn't take a
context parameter, only ops.
All ops take context parameter though.
context is meaningful for:
- saving state between executions (decode/execute/execute)
- passing state that is not provided via callbacks (regs/mode/flags)
- returning results
Only the second is relevant, and we're trying to get rid of that too.
Callbacks were passed pointer to vcpu, but they were changed to get ctxt
to better encapsulate emulator.c from rest of the KVM. Are you suggesting
this was a mistake and we need to rework callbacks to receive pointer
to vcpu again? I hope not :)
Ouch. I guess we have to pass the context, but not initialize any of it
except ops.
That's hacky and error pron. We need to audit that linearize() and all
callbacks/functions it uses do not rely on un-initialized state, which
is doable now, but who will remember to check that in the future, while
changing seemingly unrelated code, which, by a coincidence, called during
linearize()? Instant security vulnerability. For security (if not
sanity) sake we should really make sure that ctxt is initialized while
we are in emulator.c and make as many checks for it as possible.
Agree. Though the security issue is limited; the structure won't be
uninitialized, it would retain values from the previous call. So it's
limited to intra-guest vulnerabilities.
Later we can extend x86_decode_insn() and the other
functions to follow the same rule.
What rule? We cannot not initialize a context. You can reduce things
that should be initialized to minimum (getting GP registers on demand,
etc), but still some initialization is needed since ctxt holds emulation
state and it needs to be reset before each emulation.
An alternative is to use two contexts, the base context only holds ops
and is the parameter to all the callbacks on the non-state APIs, the
derived context holds the state:
struct x86_emulation_ctxt {
struct x86_ops *ops;
/* state that always needs to be initialized, preferablt none */
};
struct x86_insn_ctxt {
struct x86_emulation_ctxt em;
/* instruction state */
}
and so we have a compile-time split between users of the state and
non-users.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On Mon, Jun 25, 2012 at 06:03:19PM +0300, Avi Kivity wrote:
On 06/25/2012 05:55 PM, Gleb Natapov wrote:
On Mon, Jun 25, 2012 at 05:32:31PM +0300, Avi Kivity wrote:
On 06/25/2012 05:17 PM, Gleb Natapov wrote:
On Mon, Jun 25, 2012 at 04:40:35PM +0300, Avi Kivity wrote:
On 06/25/2012 04:12 PM, Gleb Natapov wrote:
Right. But I think we can have x86_linearize() that doesn't take a
context parameter, only ops.
All ops take context parameter though.
context is meaningful for:
- saving state between executions (decode/execute/execute)
- passing state that is not provided via callbacks (regs/mode/flags)
- returning results
Only the second is relevant, and we're trying to get rid of that too.
Callbacks were passed pointer to vcpu, but they were changed to get ctxt
to better encapsulate emulator.c from rest of the KVM. Are you suggesting
this was a mistake and we need to rework callbacks to receive pointer
to vcpu again? I hope not :)
Ouch. I guess we have to pass the context, but not initialize any of it
except ops.
That's hacky and error pron. We need to audit that linearize() and all
callbacks/functions it uses do not rely on un-initialized state, which
is doable now, but who will remember to check that in the future, while
changing seemingly unrelated code, which, by a coincidence, called during
linearize()? Instant security vulnerability. For security (if not
sanity) sake we should really make sure that ctxt is initialized while
we are in emulator.c and make as many checks for it as possible.
Agree. Though the security issue is limited; the structure won't be
uninitialized, it would retain values from the previous call. So it's
limited to intra-guest vulnerabilities.
Yes, that's the kind I mean, not host crash. Intra-guest vulnerabilities
should not be taken lightly. From guest POV they are like buggy CPUs
that allows privilege escalation.
Later we can extend x86_decode_insn() and the other
functions to follow the same rule.
What rule? We cannot not initialize a context. You can reduce things
that should be initialized to minimum (getting GP registers on demand,
etc), but still some initialization is needed since ctxt holds emulation
state and it needs to be reset before each emulation.
An alternative is to use two contexts, the base context only holds ops
and is the parameter to all the callbacks on the non-state APIs, the
derived context holds the state:
struct x86_emulation_ctxt {
struct x86_ops *ops;
/* state that always needs to be initialized, preferablt none */
};
struct x86_insn_ctxt {
struct x86_emulation_ctxt em;
/* instruction state */
}
and so we have a compile-time split between users of the state and
non-users.
I do not understand how you will divide current ctxt structure between
those two.
Where will you put those for instance: interruptibility, have_exception,
perm_ok, only_vendor_specific_insn and how can they not be initialized
before each instruction emulation?
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/25/2012 06:35 PM, Gleb Natapov wrote:
Agree. Though the security issue is limited; the structure won't be
uninitialized, it would retain values from the previous call. So it's
limited to intra-guest vulnerabilities.
Yes, that's the kind I mean, not host crash. Intra-guest vulnerabilities
should not be taken lightly. From guest POV they are like buggy CPUs
that allows privilege escalation.
It's a smaller disaster; I didn't mean to minimize those issues.
Later we can extend x86_decode_insn() and the other
functions to follow the same rule.
What rule? We cannot not initialize a context. You can reduce things
that should be initialized to minimum (getting GP registers on demand,
etc), but still some initialization is needed since ctxt holds emulation
state and it needs to be reset before each emulation.
An alternative is to use two contexts, the base context only holds ops
and is the parameter to all the callbacks on the non-state APIs, the
derived context holds the state:
struct x86_emulation_ctxt {
struct x86_ops *ops;
/* state that always needs to be initialized, preferablt none */
};
struct x86_insn_ctxt {
struct x86_emulation_ctxt em;
/* instruction state */
}
and so we have a compile-time split between users of the state and
non-users.
I do not understand how you will divide current ctxt structure between
those two.
Where will you put those for instance: interruptibility, have_exception,
perm_ok, only_vendor_specific_insn and how can they not be initialized
before each instruction emulation?
x86_emulate_ops::get_interruptibility()
x86_emulate_ops::set_interruptibility()
x86_emulate_ops::exception()
x86_decode_insn(struct x86_insn_ctxt *ctxt, unsigned flags)
{
ctxt-flags = flags;
ctxt-perm_ok = false;
}
In short, instruction emulation state is only seen by instruction
emulation functions, the others don't get to see it.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/12/2012 03:01 PM, Gleb Natapov wrote:
The function will be used outside of the emulator.
/*
* x86_emulate_ops:
*
@@ -194,6 +199,10 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+
+ int (*linearize)(struct x86_emulate_ctxt *ctxt,
+ struct segmented_address addr, unsigned size,
+ bool write, bool fetch, ulong *linear);
};
linearize is defined in terms of the other ops; this means that if we
get a second user they will have to replicate it.
Why not make the current linearize available to users?
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On Sun, Jun 24, 2012 at 04:12:05PM +0300, Avi Kivity wrote:
On 06/12/2012 03:01 PM, Gleb Natapov wrote:
The function will be used outside of the emulator.
/*
* x86_emulate_ops:
*
@@ -194,6 +199,10 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+
+ int (*linearize)(struct x86_emulate_ctxt *ctxt,
+ struct segmented_address addr, unsigned size,
+ bool write, bool fetch, ulong *linear);
};
linearize is defined in terms of the other ops; this means that if we
get a second user they will have to replicate it.
What do you mean? This patch series adds another user, so now there are two: one
inside the emulator another is outside.
Why not make the current linearize available to users?
Code outside of the emulator does not call the emulator except when
emulation is actually needed. To call linearize() from the emulator.c
almost fully functional emulation ctxt will have to be set up (including
fake instruction decoding, hacky and slower). To not duplicate the logic
I moved linearize() to generic code and made it available to emulator
via callback. It actually saves a couple of callback invocations when
emulator calls linearize() IIRC.
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On 06/24/2012 04:27 PM, Gleb Natapov wrote:
On Sun, Jun 24, 2012 at 04:12:05PM +0300, Avi Kivity wrote:
On 06/12/2012 03:01 PM, Gleb Natapov wrote:
The function will be used outside of the emulator.
/*
* x86_emulate_ops:
*
@@ -194,6 +199,10 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+
+ int (*linearize)(struct x86_emulate_ctxt *ctxt,
+ struct segmented_address addr, unsigned size,
+ bool write, bool fetch, ulong *linear);
};
linearize is defined in terms of the other ops; this means that if we
get a second user they will have to replicate it.
What do you mean? This patch series adds another user, so now there are two:
one
inside the emulator another is outside.
I meant like task switching or real-mode interrupt emulation.
Why not make the current linearize available to users?
Code outside of the emulator does not call the emulator except when
emulation is actually needed. To call linearize() from the emulator.c
almost fully functional emulation ctxt will have to be set up (including
fake instruction decoding, hacky and slower).
ctxt-d use should be removed for the exported version and replaced by a
parameter. The internal version can still use it (calling the exported
version after extracting the parameter).
To not duplicate the logic
I moved linearize() to generic code and made it available to emulator
via callback. It actually saves a couple of callback invocations when
emulator calls linearize() IIRC.
It's not available to other emulator users (which don't exist yet
anyway). But having linearize() in the emulator is consistent with
placing logic in emulate.c and accessors outside.
Regarding initialization, we should eventually initialize nothing and
let the emulator bring in needed data via callbacks (including general
registers).
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
On Sun, Jun 24, 2012 at 04:39:22PM +0300, Avi Kivity wrote:
On 06/24/2012 04:27 PM, Gleb Natapov wrote:
On Sun, Jun 24, 2012 at 04:12:05PM +0300, Avi Kivity wrote:
On 06/12/2012 03:01 PM, Gleb Natapov wrote:
The function will be used outside of the emulator.
/*
* x86_emulate_ops:
*
@@ -194,6 +199,10 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+
+int (*linearize)(struct x86_emulate_ctxt *ctxt,
+struct segmented_address addr, unsigned size,
+bool write, bool fetch, ulong *linear);
};
linearize is defined in terms of the other ops; this means that if we
get a second user they will have to replicate it.
What do you mean? This patch series adds another user, so now there are
two: one
inside the emulator another is outside.
I meant like task switching or real-mode interrupt emulation.
You mean code outside of KVM if we ever will make emulator reusable? It will
have to
have its own, much more simple version of the callback.
Why not make the current linearize available to users?
Code outside of the emulator does not call the emulator except when
emulation is actually needed. To call linearize() from the emulator.c
almost fully functional emulation ctxt will have to be set up (including
fake instruction decoding, hacky and slower).
ctxt-d use should be removed for the exported version and replaced by a
parameter. The internal version can still use it (calling the exported
version after extracting the parameter).
IMO we should stick to the pattern we have now: calling generic code from
the emulator and not vice versa. Lets not create more spaghetti.
To not duplicate the logic
I moved linearize() to generic code and made it available to emulator
via callback. It actually saves a couple of callback invocations when
emulator calls linearize() IIRC.
It's not available to other emulator users (which don't exist yet
anyway). But having linearize() in the emulator is consistent with
placing logic in emulate.c and accessors outside.
It is the question of where we draw the line. For instance MMU details
are now hidden from the emulator behind a callback. One can argue that
emulator should have access to MMU directly via callbacks and
emulate memory access by itself.
Regarding initialization, we should eventually initialize nothing and
let the emulator bring in needed data via callbacks (including general
registers).
Some things will have to be initialized (or rather reset to initial value)
between emulator invocations. Access to registers can be done on demand,
but this is unrelated to this series optimization.
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 4/5] KVM: emulator: move linearize() out of emulator code.
The function will be used outside of the emulator.
Signed-off-by: Gleb Natapov g...@redhat.com
---
arch/x86/include/asm/kvm_emulate.h | 14 --
arch/x86/kvm/emulate.c | 84 ++--
arch/x86/kvm/x86.c | 92 ++--
3 files changed, 103 insertions(+), 87 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h
b/arch/x86/include/asm/kvm_emulate.h
index 7c276ca..bdf8a84 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -43,6 +43,11 @@ struct x86_instruction_info {
u64 next_rip; /* rip following the instruction*/
};
+struct segmented_address {
+ ulong ea;
+ unsigned seg;
+};
+
/*
* x86_emulate_ops:
*
@@ -194,6 +199,10 @@ struct x86_emulate_ops {
bool (*get_cpuid)(struct x86_emulate_ctxt *ctxt,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+
+ int (*linearize)(struct x86_emulate_ctxt *ctxt,
+ struct segmented_address addr, unsigned size,
+ bool write, bool fetch, ulong *linear);
};
typedef u32 __attribute__((vector_size(16))) sse128_t;
@@ -208,10 +217,7 @@ struct operand {
};
union {
unsigned long *reg;
- struct segmented_address {
- ulong ea;
- unsigned seg;
- } mem;
+ struct segmented_address mem;
unsigned xmm;
unsigned mm;
} addr;
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e317588..24b1c70 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -470,14 +470,6 @@ static void set_seg_override(struct x86_emulate_ctxt
*ctxt, int seg)
ctxt-seg_override = seg;
}
-static unsigned long seg_base(struct x86_emulate_ctxt *ctxt, int seg)
-{
- if (ctxt-mode == X86EMUL_MODE_PROT64 seg VCPU_SREG_FS)
- return 0;
-
- return ctxt-ops-get_cached_segment_base(ctxt, seg);
-}
-
static unsigned seg_override(struct x86_emulate_ctxt *ctxt)
{
if (!ctxt-has_seg_override)
@@ -505,11 +497,6 @@ static int emulate_gp(struct x86_emulate_ctxt *ctxt, int
err)
return emulate_exception(ctxt, GP_VECTOR, err, true);
}
-static int emulate_ss(struct x86_emulate_ctxt *ctxt, int err)
-{
- return emulate_exception(ctxt, SS_VECTOR, err, true);
-}
-
static int emulate_ud(struct x86_emulate_ctxt *ctxt)
{
return emulate_exception(ctxt, UD_VECTOR, 0, false);
@@ -578,74 +565,15 @@ static int __linearize(struct x86_emulate_ctxt *ctxt,
unsigned size, bool write, bool fetch,
ulong *linear)
{
- struct desc_struct desc;
- bool usable;
- ulong la;
- u32 lim;
- u16 sel;
- unsigned cpl, rpl;
+ int err = ctxt-ops-linearize(ctxt, addr, size, write, fetch, linear);
- la = seg_base(ctxt, addr.seg) + addr.ea;
- switch (ctxt-mode) {
- case X86EMUL_MODE_REAL:
- break;
- case X86EMUL_MODE_PROT64:
- if (((signed long)la 16) 16 != la)
- return emulate_gp(ctxt, 0);
- break;
- default:
- usable = ctxt-ops-get_segment(ctxt, sel, desc, NULL,
- addr.seg);
- if (!usable)
- goto bad;
- /* code segment or read-only data segment */
- if (((desc.type 8) || !(desc.type 2)) write)
- goto bad;
- /* unreadable code segment */
- if (!fetch (desc.type 8) !(desc.type 2))
- goto bad;
- lim = desc_limit_scaled(desc);
- if ((desc.type 8) || !(desc.type 4)) {
- /* expand-up segment */
- if (addr.ea lim || (u32)(addr.ea + size - 1) lim)
- goto bad;
- } else {
- /* exapand-down segment */
- if (addr.ea = lim || (u32)(addr.ea + size - 1) = lim)
- goto bad;
- lim = desc.d ? 0x : 0x;
- if (addr.ea lim || (u32)(addr.ea + size - 1) lim)
- goto bad;
- }
- cpl = ctxt-ops-cpl(ctxt);
- rpl = sel 3;
- cpl = max(cpl, rpl);
- if (!(desc.type 8)) {
- /* data segment */
- if (cpl desc.dpl)
- goto bad;
- } else if ((desc.type 8) !(desc.type 4)) {
- /* nonconforming code segment */
- if (cpl != desc.dpl)
- goto bad;
- } else if ((desc.type 8) (desc.type 4)) {
-
