X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

[Corneliu ZUZU]

I'm trying to refactor some arch-specific code into common code and was 
surprised to find out that the x86 domain structure already occupies 
PAGE_SIZE bytes, couldn't even add an unsigned short field in it w/o 
causing a compile-time error.
I'm using the master branch of git://xenbits.xenproject.org/xen.git, 
compiled Xen by simply running "./configure && make dist-xen" (on my 
x86_64 Ubuntu machine).

Concretely, the change that caused problems was the refactoring of the 
x86 arch_domain.monitor field [@ xen/include/asm-x86/domain.h], which 
originally was:
      struct {
         unsigned int write_ctrlreg_enabled       : 4;
         unsigned int write_ctrlreg_sync          : 4;
         unsigned int write_ctrlreg_onchangeonly  : 4;
         unsigned int mov_to_msr_enabled          : 1;
         unsigned int mov_to_msr_extended         : 1;
         unsigned int singlestep_enabled          : 1;
         unsigned int software_breakpoint_enabled : 1;
         unsigned int guest_request_enabled       : 1;
         unsigned int guest_request_sync          : 1;
      } monitor;

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;

After the above change, X86 compilation of [xen/arch/x86/domain.c] fails w/:
     error: static assertion failed: "!(sizeof(*d) > PAGE_SIZE)"
the line that caused the fail being:
     BUILD_BUG_ON(sizeof(*d) > PAGE_SIZE);

To investigate I compiled the unaltered domain.c & used the pahole tool 
(part of dwarves package) to obtain the complete layout of the domain 
structure (& its members).
What I obtained:
     * sizeof(struct domain) is already = 4096 bytes (= PAGE_SIZE)
     * sizeof(struct arch_domain) [x86] = sizeof(domain.arch) = 3328 
bytes (arch_domain is marked __cacheline_aligned, i.e. aligned to 128 bytes)
     * sizeof(domain.arch.hvm_domain) = 2224 bytes
     * sizeof(domain.arch.hvm_domain.pl_time) = 1088 bytes

=> overall, X86 timers-related information occupies the most.

One could shrink the domain structure by transforming some of its fields 
to pointers, e.g. I could transform the pl_time field into a pointer and 
dynamically allocate its data when domain_create is called.
Since the domain structure was designed to fit in a single page & 
arch_domain is marked __cacheline_aligned I presume such changes are 
sensible and should be done wisely.

How I should proceed?

Thank you,
Corneliu.

Re: X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 1196 bytes --]

> 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

[-- Attachment #1.2: Type: text/html, Size: 1603 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

[Jan Beulich]

>>> On 29.01.16 at 17:24, <czuzu@bitdefender.com> wrote:
> To investigate I compiled the unaltered domain.c & used the pahole tool 
> (part of dwarves package) to obtain the complete layout of the domain 
> structure (& its members).
> What I obtained:
>      * sizeof(struct domain) is already = 4096 bytes (= PAGE_SIZE)
>      * sizeof(struct arch_domain) [x86] = sizeof(domain.arch) = 3328 
> bytes (arch_domain is marked __cacheline_aligned, i.e. aligned to 128 bytes)
>      * sizeof(domain.arch.hvm_domain) = 2224 bytes
>      * sizeof(domain.arch.hvm_domain.pl_time) = 1088 bytes
> 
> => overall, X86 timers-related information occupies the most.
> 
> One could shrink the domain structure by transforming some of its fields 
> to pointers, e.g. I could transform the pl_time field into a pointer and 
> dynamically allocate its data when domain_create is called.

Sounds like a reasonable measure. I wasn't aware we're exactly on
the boundary right now.

Jan

Re: X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

[Andrew Cooper]

On 29/01/16 17:09, Jan Beulich wrote:
>>>> On 29.01.16 at 17:24, <czuzu@bitdefender.com> wrote:
>> To investigate I compiled the unaltered domain.c & used the pahole tool 
>> (part of dwarves package) to obtain the complete layout of the domain 
>> structure (& its members).
>> What I obtained:
>>      * sizeof(struct domain) is already = 4096 bytes (= PAGE_SIZE)
>>      * sizeof(struct arch_domain) [x86] = sizeof(domain.arch) = 3328 
>> bytes (arch_domain is marked __cacheline_aligned, i.e. aligned to 128 bytes)
>>      * sizeof(domain.arch.hvm_domain) = 2224 bytes
>>      * sizeof(domain.arch.hvm_domain.pl_time) = 1088 bytes
>>
>> => overall, X86 timers-related information occupies the most.
>>
>> One could shrink the domain structure by transforming some of its fields 
>> to pointers, e.g. I could transform the pl_time field into a pointer and 
>> dynamically allocate its data when domain_create is called.
> Sounds like a reasonable measure. I wasn't aware we're exactly on
> the boundary right now.

It has been teatering on the edge for a while now.

I keep on meaning to see about sliming it down somewhat (probably by
pulling an optional structure out into an explicit allocation), but
haven't had the time.

~Andrew

Re: X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

[Corneliu ZUZU]

[-- Attachment #1.1: Type: text/plain, Size: 2998 bytes --]

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. 
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 :).

Corneliu.

[-- Attachment #1.2: Type: text/html, Size: 4448 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

[Corneliu ZUZU]

On 1/29/2016 7:09 PM, Jan Beulich wrote:
>>>> On 29.01.16 at 17:24, <czuzu@bitdefender.com> wrote:
>> One could shrink the domain structure by transforming some of its fields
>> to pointers, e.g. I could transform the pl_time field into a pointer and
>> dynamically allocate its data when domain_create is called.
> Sounds like a reasonable measure. I wasn't aware we're exactly on
> the boundary right now.
>
> Jan

So then would it be ok to go w/ this?
@Andrew you mentioned something about "pulling an optional structure out 
into an explicit allocation", did you have something specific in mind? 
Would it be ok to do that for pl_time?

Thanks,
Corneliu.

Re: X86 arch_domain ginormous, sizeof(struct domain) already == PAGE_SIZE

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 3766 bytes --]

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 #1.2: Type: text/html, Size: 5630 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel