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.