On Jun 21, 2016 01:20, "Jan Beulich" wrote: > > >>> On 21.06.16 at 09:08, wrote: > > On 6/17/2016 11:25 AM, Corneliu ZUZU wrote: > >> On 6/16/2016 6:16 PM, Jan Beulich wrote: > >>>>>> On 16.06.16 at 16:12, wrote: > >>>> Prepare for ARM implementation of control-register write vm-events > >>>> by moving > >>>> X86-specific hvm_event_cr to the common-side. > >>>> > >>>> Signed-off-by: Corneliu ZUZU > >>>> --- > >>>> xen/arch/x86/hvm/event.c | 30 ------------------------------ > >>>> xen/arch/x86/hvm/hvm.c | 2 +- > >>>> xen/arch/x86/monitor.c | 37 > >>>> ------------------------------------- > >>>> xen/arch/x86/vm_event.c | 2 +- > >>>> xen/common/monitor.c | 40 > >>>> ++++++++++++++++++++++++++++++++++++++++ > >>>> xen/common/vm_event.c | 31 +++++++++++++++++++++++++++++++ > >>>> xen/include/asm-x86/hvm/event.h | 13 ++++--------- > >>>> xen/include/asm-x86/monitor.h | 2 -- > >>>> xen/include/xen/monitor.h | 4 ++++ > >>>> xen/include/xen/vm_event.h | 10 ++++++++++ > >>>> 10 files changed, 91 insertions(+), 80 deletions(-) > >>> Considering that there's no ARM file getting altered here at all, > >>> mentioning ARM in the subject is a little odd. > >> > >> This patch and the following one should be meld together. > >> I only split them to ease reviewing, sorry I forgot to mention that in > >> the cover letter. > >> > >>> > >>>> --- a/xen/common/monitor.c > >>>> +++ b/xen/common/monitor.c > >>>> @@ -62,6 +62,46 @@ int monitor_domctl(struct domain *d, struct > >>>> xen_domctl_monitor_op *mop) > >>>> switch ( mop->event ) > >>>> { > >>>> +#if CONFIG_X86 > >>> #ifdef please. > >> Ack. > >>>> + case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG: > >>>> + { > >>>> + struct arch_domain *ad = &d->arch; > >>> Peeking into the next patch I see that this stays there. Common code, > >>> however, shouldn't access ->arch sub-structures - respective fields > >>> should be moved out. > >> > >> Then we need to find a resolution that avoids code duplication. > >> The code is the same, but those bits that are currently on the arch > >> side (arch.monitor.write_ctrlreg_*) cannot be moved to common as they > >> are, since their -number- might differ from arch-to-arch. > >> But we could: > >> - in public/vm_event.h, besides the VM_EVENT_X86_* and VM_EVENT_ARM_* > >> defines (wcr index), also have > >> #define VM_EVENT_X86_CR_COUNT 4 > >> #define VM_EVENT_ARM_CR_COUNT 4 > >> - move the 3 write_ctrlreg_{enabled,sync,onchangeonly} fields from > >> arch_domain to domain (common) and make them 8-bits wide each for now > >> (widened more in the future if the need arises) > >> - let monitor_ctrlreg_bitmask() macro to be architecture-dependent and > >> use the introduced VM_EVENT__CR_COUNT > >> > >> Tamas, we also talked on this matter @ some point (when I sent the > >> patches that moved vm-event parts to common). What do you think of the > >> above? > > I don't really care about the specifics, my only request is what I > already voiced: Common code should not access arch-specific > fields. Having the field to hold the control register bits common, > but the definitions for the individual bits arch-specific is perfectly > fine for this (assuming that these per-arch definitions then again > don't get used in common code). > > Jan As Jan says it would be fine to have the holder field on the common struct but IMHO it wouldn't be easier to follow the logic that way and the only benefit is reducing code duplication a little bit. I think for now it is acceptable to just rather have some code duplication. Tamas