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 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/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 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 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 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: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 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 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 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/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 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
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: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/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
