[prev in list] [next in list] [prev in thread] [next in thread]
List: xen-devel
Subject: Re: [Xen-devel] X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE
From: "Lengyel, Tamas" <tlengyel () novetta ! com>
Date: 2016-01-31 0:22:39
Message-ID: CAD33N+5RsoM1DYQZLQo-YnLgpR9T0fwps1TYXMayqhJfeZLu0Q () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Fri, Jan 29, 2016 at 2:32 PM, Corneliu ZUZU <czuzu@bitdefender.com>
wrote:
> On 1/29/2016 6:47 PM, Lengyel, Tamas wrote:
>
>
> by leaving there only the x86-specific part, i.e.:
>> struct {
>> uint8_t mov_to_msr_enabled : 1;
>> uint8_t mov_to_msr_extended : 1;
>> } monitor;
>>
>> and moving the rest directly into the domain structure, i.e. add @ the
>> end of struct domain [@ xen/include/xen/sched.h]:
>> struct {
>> uint16_t write_ctrlreg_enabled : 4;
>> uint16_t write_ctrlreg_sync : 4;
>> uint16_t write_ctrlreg_onchangeonly : 4;
>> uint16_t singlestep_enabled : 1;
>> uint16_t software_breakpoint_enabled : 1;
>> uint16_t guest_request_enabled : 1;
>> uint16_t guest_request_sync : 1;
>> } monitor;
>>
>
> Beside guest_request_enable/sync I'm fairly sure all the other bits are
> x86 specific. For example the ctrlreg fields are 4 bits wide to correspond
> to the 4 x86 CR regs for which we can get hardware events (VM_EVENT_X86_*).
> You should not be moving those in that form into common. For ARM you should
> create an arch specific monitor structure for events that you can actually
> capture (SMC, etc.). So IMHO for 2 bits in common it's a waste to have
> things moved up from arch.
>
> Tamas
>
>
> Conceptually speaking, they are not X86-specific. Single-stepping,
> software-breakpoints, guest-to-hypervisor notifications, control/system
> registers - these are concepts that are not strictly confined exclusively
> within a single architecture, whereas for example MSRs are.
>
Right, what I meant is that currently these are only implemented for x86.
If this is to change I have no problem moving these up a layer, but that
should only happen when the feature(s) are actually implemented.
> It's true that originally the need for defining 4 bits for 4
> control-registers came because those 4 registers were the X86 CR0, CR3, CR4
> & XCR0 registers, but I was not suggesting to keep the same significance
> after this change.
> Rather, my proposition was-to-be (when sending the move-to-common patch)
> either:
> 1). to change that significance to "for now, 4 bits are enough to cover
> all the monitored control-registers for all the architectures that
> implement vm-event control register monitoring. If, in the future an
> implementation change of control-register monitoring for an architecture
> will require more than the # of bits @ that time, then that # can be
> adjusted."
> 2). define a single upper limit for control-registers count for all
> architectures, e.g. smth like #define MONITOR_CTRLREG_MAX_COUNT 8 - maybe
> paired w/ ASSERT/BUG_ON_BUILD-like checks to enforce that and/or avoid
> undesired behavior.
> Or maybe some other way, nonetheless, as I said, IMO the place for these
> bits could be here, in the common code, since conceptually they could apply
> to any architecture.
>
> I was actually hoping to approach this matter after the move-to-common
> patch because I have another patch I would like to put forward that
> implements control-registers monitoring on ARM as well and incidentally
> there are also 4 of them in that patch, as on X86. But that's to come :).
>
That's really great you working on that! I would be glad to see the ARM
events support get up to speed with x86. Notwithstanding, IMHO it would
still be preferred to keep the ctrlreg bits in the architecture specific
layer. While right now it happens that both archs need 4 bits as you say,
if this were to change in the future, some bits will be wasted on the other
archs. Moving other options into common that have 1:1 equivalency across
archs when those are implemented is OK though.
Tamas
[Attachment #5 (text/html)]
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan \
29, 2016 at 2:32 PM, Corneliu ZUZU <span dir="ltr"><<a \
href="mailto:czuzu@bitdefender.com" \
target="_blank">czuzu@bitdefender.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div class="h5">
<div>On 1/29/2016 6:47 PM, Lengyel, Tamas
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> by leaving there \
only the x86-specific part, i.e.:<br> struct {<br>
uint8_t mov_to_msr_enabled : 1;<br>
uint8_t mov_to_msr_extended : 1;<br>
} monitor;<br>
<br>
and moving the rest directly into the domain structure,
i.e. add @ the end of struct domain [@
xen/include/xen/sched.h]:<br>
struct {<br>
uint16_t write_ctrlreg_enabled : 4;<br>
uint16_t write_ctrlreg_sync : 4;<br>
uint16_t write_ctrlreg_onchangeonly : 4;<br>
uint16_t singlestep_enabled : 1;<br>
uint16_t software_breakpoint_enabled : 1;<br>
uint16_t guest_request_enabled : 1;<br>
uint16_t guest_request_sync : 1;<br>
} monitor;<br>
</blockquote>
<div><br>
</div>
<div>Beside guest_request_enable/sync I'm fairly sure all
the other bits are x86 specific. For example the ctrlreg
fields are 4 bits wide to correspond to the 4 x86 CR regs
for which we can get hardware events (VM_EVENT_X86_*). You
should not be moving those in that form into common. For
ARM you should create an arch specific monitor structure
for events that you can actually capture (SMC, etc.). So
IMHO for 2 bits in common it's a waste to have things
moved up from arch.<br>
<br>
</div>
<div>Tamas<br>
</div>
</div>
<br>
</div>
</div>
</blockquote>
<br></div></div>
Conceptually speaking, they are not X86-specific. Single-stepping,
software-breakpoints, guest-to-hypervisor notifications,
control/system registers - these are concepts that are not strictly
confined exclusively within a single architecture, whereas for
example MSRs are.</div></blockquote><div><br></div><div>Right, what I meant is \
that currently these are only implemented for x86. If this is to change I have no \
problem moving these up a layer, but that should only happen when the feature(s) are \
actually implemented.<br></div><div> </div><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div \
bgcolor="#FFFFFF" text="#000000"> It's true that originally the need for defining \
4 bits for 4 control-registers came because those 4 registers were the
X86 CR0, CR3, CR4 & XCR0 registers, but I was not suggesting to
keep the same significance after this change. <br>
Rather, my proposition was-to-be (when sending the move-to-common
patch) either:<br>
1). to change that significance to "for now, 4 bits are enough to
cover all the monitored control-registers for all the architectures
that implement vm-event control register monitoring. If, in the
future an implementation change of control-register monitoring for
an architecture will require more than the # of bits @ that time,
then that # can be adjusted."<br>
2). define a single upper limit for control-registers count for all
architectures, e.g. smth like #define MONITOR_CTRLREG_MAX_COUNT 8 -
maybe paired w/ ASSERT/BUG_ON_BUILD-like checks to enforce that
and/or avoid undesired behavior.<br>
Or maybe some other way, nonetheless, as I said, IMO the place for
these bits could be here, in the common code, since conceptually
they could apply to any architecture.<br>
<br>
I was actually hoping to approach this matter after the
move-to-common patch because I have another patch I would like to
put forward that implements control-registers monitoring on ARM as
well and incidentally there are also 4 of them in that patch, as on
X86. But that's to come :).<span class="HOEnZb"><font \
color="#888888"><br></font></span></div></blockquote><div><br></div><div>That's \
really great you working on that! I would be glad to see the ARM events support get \
up to speed with x86. Notwithstanding, IMHO it would still be preferred to keep the \
ctrlreg bits in the architecture specific layer. While right now it happens that both \
archs need 4 bits as you say, if this were to change in the future, some bits will be \
wasted on the other archs. Moving other options into common that have 1:1 equivalency \
across archs when those are implemented is OK \
though.<br><br></div><div>Tamas<br></div></div><br></div></div>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic