| Message ID | 1466086345-7705-1-git-send-email-czuzu@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.06.16 at 16:12, <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> > --- > 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. > --- 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. > + 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. And looking at all the uses of this variable I get the impression that you really want a shorthand for &d->arch.monitor (if any such helper variable is worthwhile to have here in the first place). > --- a/xen/include/asm-x86/monitor.h > +++ b/xen/include/asm-x86/monitor.h > @@ -24,8 +24,6 @@ > > #include <xen/sched.h> > > -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) > - > static inline > int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) > { > --- a/xen/include/xen/monitor.h > +++ b/xen/include/xen/monitor.h > @@ -25,6 +25,10 @@ > struct domain; > struct xen_domctl_monitor_op; > > +#if CONFIG_X86 > +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) > +#endif What's the point in removing this from the x86 header if then it needs to be put in such a conditional? If the conditional gets dropped in the next patch, then I think you have two options: Leave it where it is here, and move it there. Or move it here, but omit the conditional right away - I can't see this definition being present to harm the ARM build in any way. > --- a/xen/include/xen/vm_event.h > +++ b/xen/include/xen/vm_event.h > @@ -96,6 +96,16 @@ void vm_event_vcpu_unpause(struct vcpu *v); > int vm_event_monitor_traps(struct vcpu *v, uint8_t sync, > vm_event_request_t *req); > > +#if CONFIG_X86 > +/* > + * Called for the current vCPU on control-register changes by guest. > + * The event might not fire if the client has subscribed to it in onchangeonly > + * mode, hence the bool_t return type for control register write events. > + */ > +bool_t vm_event_monitor_cr(unsigned int index, unsigned long value, > + unsigned long old); > +#endif Same goes for the declaration here. Jan
On Thu, Jun 16, 2016 at 8:12 AM, Corneliu ZUZU <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> > --- > 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(-) Some of the code-movement here touches code I cleaned up in https://patchwork.kernel.org/patch/9151229 to keep vm_event and monitor code separate as much as possible. The same separation should be followed when we move code to common. Tamas
On 6/16/2016 6:16 PM, Jan Beulich wrote: >>>> On 16.06.16 at 16:12, <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> >> --- >> 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_<arch>_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? > > And looking at all the uses of this variable I get the impression that > you really want a shorthand for &d->arch.monitor (if any such > helper variable is worthwhile to have here in the first place). Well, this was a simple cut-paste operation, not very old content aware :) Personally I prefer the current shorthand (ad) (seems more intuitive and is consistent with the other XEN_DOMCTL_MONITOR_EVENT_* cases), but if you prefer I'll change that shorthand to am = &d->arch.monitor? >> --- a/xen/include/asm-x86/monitor.h >> +++ b/xen/include/asm-x86/monitor.h >> @@ -24,8 +24,6 @@ >> >> #include <xen/sched.h> >> >> -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) >> - >> static inline >> int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) >> { >> --- a/xen/include/xen/monitor.h >> +++ b/xen/include/xen/monitor.h >> @@ -25,6 +25,10 @@ >> struct domain; >> struct xen_domctl_monitor_op; >> >> +#if CONFIG_X86 >> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) >> +#endif > What's the point in removing this from the x86 header if then it > needs to be put in such a conditional? If the conditional gets > dropped in the next patch, then I think you have two options: > Leave it where it is here, and move it there. Or move it here, > but omit the conditional right away - I can't see this definition > being present to harm the ARM build in any way. Meld comment above. > >> --- a/xen/include/xen/vm_event.h >> +++ b/xen/include/xen/vm_event.h >> @@ -96,6 +96,16 @@ void vm_event_vcpu_unpause(struct vcpu *v); >> int vm_event_monitor_traps(struct vcpu *v, uint8_t sync, >> vm_event_request_t *req); >> >> +#if CONFIG_X86 >> +/* >> + * Called for the current vCPU on control-register changes by guest. >> + * The event might not fire if the client has subscribed to it in onchangeonly >> + * mode, hence the bool_t return type for control register write events. >> + */ >> +bool_t vm_event_monitor_cr(unsigned int index, unsigned long value, >> + unsigned long old); >> +#endif > Same goes for the declaration here. > > Jan > > Meld comment above. Corneliu.
>>> On 17.06.16 at 10:25, <czuzu@bitdefender.com> wrote: > On 6/16/2016 6:16 PM, Jan Beulich wrote: >> And looking at all the uses of this variable I get the impression that >> you really want a shorthand for &d->arch.monitor (if any such >> helper variable is worthwhile to have here in the first place). > > Well, this was a simple cut-paste operation, not very old content aware :) > Personally I prefer the current shorthand (ad) (seems more intuitive and > is consistent with the other XEN_DOMCTL_MONITOR_EVENT_* cases), but if > you prefer I'll change that shorthand to am = &d->arch.monitor? I'd prefer either no shorthand, or one eliminating the longest common prefix across all uses. >>> --- a/xen/include/asm-x86/monitor.h >>> +++ b/xen/include/asm-x86/monitor.h >>> @@ -24,8 +24,6 @@ >>> >>> #include <xen/sched.h> >>> >>> -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) >>> - >>> static inline >>> int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) >>> { >>> --- a/xen/include/xen/monitor.h >>> +++ b/xen/include/xen/monitor.h >>> @@ -25,6 +25,10 @@ >>> struct domain; >>> struct xen_domctl_monitor_op; >>> >>> +#if CONFIG_X86 >>> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) >>> +#endif >> What's the point in removing this from the x86 header if then it >> needs to be put in such a conditional? If the conditional gets >> dropped in the next patch, then I think you have two options: >> Leave it where it is here, and move it there. Or move it here, >> but omit the conditional right away - I can't see this definition >> being present to harm the ARM build in any way. > > Meld comment above. I don't follow - having split the patches for reviewability was a good idea. I merely commented on some oddities resulting from that split, which - afaict - could be easily corrected without folding the patches together. Jan
On 6/16/2016 7:55 PM, Tamas K Lengyel wrote: > On Thu, Jun 16, 2016 at 8:12 AM, Corneliu ZUZU <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> >> --- >> 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(-) > Some of the code-movement here touches code I cleaned up in > https://patchwork.kernel.org/patch/9151229 to keep vm_event and > monitor code separate as much as possible. The same separation should > be followed when we move code to common. > > Tamas > Again, that didn't make it to staging. We'll see which gets in first and merge afterwards? Corneliu.
On 6/17/2016 11:38 AM, Jan Beulich wrote: >>>> On 17.06.16 at 10:25, <czuzu@bitdefender.com> wrote: >> On 6/16/2016 6:16 PM, Jan Beulich wrote: >>> And looking at all the uses of this variable I get the impression that >>> you really want a shorthand for &d->arch.monitor (if any such >>> helper variable is worthwhile to have here in the first place). >> Well, this was a simple cut-paste operation, not very old content aware :) >> Personally I prefer the current shorthand (ad) (seems more intuitive and >> is consistent with the other XEN_DOMCTL_MONITOR_EVENT_* cases), but if >> you prefer I'll change that shorthand to am = &d->arch.monitor? > I'd prefer either no shorthand, or one eliminating the longest common > prefix across all uses. am = &d->arch.monitor it is then. >>>> --- a/xen/include/asm-x86/monitor.h >>>> +++ b/xen/include/asm-x86/monitor.h >>>> @@ -24,8 +24,6 @@ >>>> >>>> #include <xen/sched.h> >>>> >>>> -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) >>>> - >>>> static inline >>>> int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) >>>> { >>>> --- a/xen/include/xen/monitor.h >>>> +++ b/xen/include/xen/monitor.h >>>> @@ -25,6 +25,10 @@ >>>> struct domain; >>>> struct xen_domctl_monitor_op; >>>> >>>> +#if CONFIG_X86 >>>> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) >>>> +#endif >>> What's the point in removing this from the x86 header if then it >>> needs to be put in such a conditional? If the conditional gets >>> dropped in the next patch, then I think you have two options: >>> Leave it where it is here, and move it there. Or move it here, >>> but omit the conditional right away - I can't see this definition >>> being present to harm the ARM build in any way. >> Meld comment above. > I don't follow - having split the patches for reviewability was > a good idea. I merely commented on some oddities resulting > from that split, which - afaict - could be easily corrected without > folding the patches together. > > Jan Ooh, haha, sorry I've re-read your comment now. You're right, there's no point in that change, I'll leave it on the X86 side until the ARM part is actually implemented (last patch). Corneliu.
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, <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> >>> --- >>> 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_<arch>_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? Tamas, Jan, thoughts on this? Corneliu.
>>> On 21.06.16 at 09:08, <czuzu@bitdefender.com> 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, <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> >>>> --- >>>> 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_<arch>_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
On Jun 21, 2016 01:20, "Jan Beulich" <JBeulich@suse.com> wrote: > > >>> On 21.06.16 at 09:08, <czuzu@bitdefender.com> 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, <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> > >>>> --- > >>>> 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_<arch>_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
>>> On 21.06.16 at 17:22, <tamas@tklengyel.com> wrote: > On Jun 21, 2016 01:20, "Jan Beulich" <JBeulich@suse.com> wrote: >> >> >>> On 21.06.16 at 09:08, <czuzu@bitdefender.com> 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, <czuzu@bitdefender.com> 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 <czuzu@bitdefender.com> >> >>>> --- >> >>>> 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_<arch>_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). > > 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. Code duplication isn't the main issue here. Inviting further conceptually wrong code additions (accessing per-arch fields from common code), by setting a(nother) bad precedent, is what I want to avoid from the beginning. Jan
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c index 26165b4..e8175e4 100644 --- a/xen/arch/x86/hvm/event.c +++ b/xen/arch/x86/hvm/event.c @@ -21,38 +21,8 @@ * this program; If not, see <http://www.gnu.org/licenses/>. */ -#include <xen/vm_event.h> #include <asm/hvm/event.h> #include <asm/paging.h> -#include <asm/monitor.h> -#include <public/vm_event.h> - -bool_t hvm_event_cr(unsigned int index, unsigned long value, unsigned long old) -{ - struct vcpu *curr = current; - struct arch_domain *ad = &curr->domain->arch; - unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index); - - if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) && - (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) || - value != old) ) - { - bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask); - - vm_event_request_t req = { - .reason = VM_EVENT_REASON_WRITE_CTRLREG, - .vcpu_id = curr->vcpu_id, - .u.write_ctrlreg.index = index, - .u.write_ctrlreg.new_value = value, - .u.write_ctrlreg.old_value = old - }; - - vm_event_monitor_traps(curr, sync, &req); - return 1; - } - - return 0; -} void hvm_event_msr(unsigned int msr, uint64_t value) { diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 4596662..26f8625 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -37,6 +37,7 @@ #include <xen/mem_access.h> #include <xen/rangeset.h> #include <xen/vm_event.h> +#include <xen/monitor.h> #include <asm/shadow.h> #include <asm/hap.h> #include <asm/current.h> @@ -52,7 +53,6 @@ #include <asm/traps.h> #include <asm/mc146818rtc.h> #include <asm/mce.h> -#include <asm/monitor.h> #include <asm/hvm/hvm.h> #include <asm/hvm/vpt.h> #include <asm/hvm/support.h> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c index 1e5445f..264f0fc 100644 --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -29,43 +29,6 @@ int arch_monitor_domctl_event(struct domain *d, switch ( mop->event ) { - case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG: - { - unsigned int ctrlreg_bitmask; - bool_t old_status; - - /* sanity check: avoid left-shift undefined behavior */ - if ( unlikely(mop->u.mov_to_cr.index > 31) ) - return -EINVAL; - - ctrlreg_bitmask = monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); - old_status = !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask); - - if ( unlikely(old_status == requested_status) ) - return -EEXIST; - - domain_pause(d); - - if ( mop->u.mov_to_cr.sync ) - ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask; - else - ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask; - - if ( mop->u.mov_to_cr.onchangeonly ) - ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask; - else - ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; - - if ( requested_status ) - ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; - else - ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; - - domain_unpause(d); - - break; - } - case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR: { bool_t old_status = ad->monitor.mov_to_msr_enabled; diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c index 94342d5..aa65840 100644 --- a/xen/arch/x86/vm_event.c +++ b/xen/arch/x86/vm_event.c @@ -19,7 +19,7 @@ */ #include <xen/vm_event.h> -#include <asm/monitor.h> +#include <xen/monitor.h> #include <asm/paging.h> #include <asm/hvm/vmx/vmx.h> diff --git a/xen/common/monitor.c b/xen/common/monitor.c index c46df5a..2366bae 100644 --- 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 + case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG: + { + struct arch_domain *ad = &d->arch; + unsigned int ctrlreg_bitmask; + bool_t old_status; + + /* sanity check: avoid left-shift undefined behavior */ + if ( unlikely(mop->u.mov_to_cr.index > 31) ) + return -EINVAL; + + ctrlreg_bitmask = monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index); + old_status = !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask); + + if ( unlikely(old_status == requested_status) ) + return -EEXIST; + + domain_pause(d); + + if ( mop->u.mov_to_cr.sync ) + ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask; + else + ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask; + + if ( mop->u.mov_to_cr.onchangeonly ) + ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask; + else + ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask; + + if ( requested_status ) + ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask; + else + ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask; + + domain_unpause(d); + + break; + } +#endif + case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST: { bool_t old_status = d->monitor.guest_request_enabled; diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c index 15152ba..53dc048 100644 --- a/xen/common/vm_event.c +++ b/xen/common/vm_event.c @@ -25,6 +25,7 @@ #include <xen/wait.h> #include <xen/vm_event.h> #include <xen/mem_access.h> +#include <xen/monitor.h> #include <asm/p2m.h> #include <asm/altp2m.h> #include <xsm/xsm.h> @@ -823,6 +824,36 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync, return 1; } +#if CONFIG_X86 +bool_t vm_event_monitor_cr(unsigned int index, unsigned long value, + unsigned long old) +{ + struct vcpu *curr = current; + struct arch_domain *ad = &curr->domain->arch; + unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index); + + if ( (ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) && + (!(ad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) || + value != old) ) + { + bool_t sync = !!(ad->monitor.write_ctrlreg_sync & ctrlreg_bitmask); + + vm_event_request_t req = { + .reason = VM_EVENT_REASON_WRITE_CTRLREG, + .vcpu_id = curr->vcpu_id, + .u.write_ctrlreg.index = index, + .u.write_ctrlreg.new_value = value, + .u.write_ctrlreg.old_value = old + }; + + vm_event_monitor_traps(curr, sync, &req); + return 1; + } + + return 0; +} +#endif + void vm_event_monitor_guest_request(void) { struct vcpu *curr = current; diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h index 504bd66..7fb9d96 100644 --- a/xen/include/asm-x86/hvm/event.h +++ b/xen/include/asm-x86/hvm/event.h @@ -19,6 +19,7 @@ #ifndef __ASM_X86_HVM_EVENT_H__ #define __ASM_X86_HVM_EVENT_H__ +#include <xen/vm_event.h> #include <public/vm_event.h> enum hvm_event_breakpoint_type @@ -27,19 +28,13 @@ enum hvm_event_breakpoint_type HVM_EVENT_SINGLESTEP_BREAKPOINT, }; -/* - * Called for current VCPU on crX/MSR changes by guest. - * The event might not fire if the client has subscribed to it in onchangeonly - * mode, hence the bool_t return type for control register write events. - */ -bool_t hvm_event_cr(unsigned int index, unsigned long value, - unsigned long old); -#define hvm_event_crX(cr, new, old) \ - hvm_event_cr(VM_EVENT_X86_##cr, new, old) void hvm_event_msr(unsigned int msr, uint64_t value); int hvm_event_breakpoint(unsigned long rip, enum hvm_event_breakpoint_type type); +#define hvm_event_crX(cr, new, old) \ + vm_event_monitor_cr(VM_EVENT_X86_##cr, new, old) + #endif /* __ASM_X86_HVM_EVENT_H__ */ /* diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h index 7a662f9..99538b9 100644 --- a/xen/include/asm-x86/monitor.h +++ b/xen/include/asm-x86/monitor.h @@ -24,8 +24,6 @@ #include <xen/sched.h> -#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) - static inline int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop) { diff --git a/xen/include/xen/monitor.h b/xen/include/xen/monitor.h index 7015e6d..422fd93 100644 --- a/xen/include/xen/monitor.h +++ b/xen/include/xen/monitor.h @@ -25,6 +25,10 @@ struct domain; struct xen_domctl_monitor_op; +#if CONFIG_X86 +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index)) +#endif + int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op); #endif /* __XEN_MONITOR_H__ */ diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h index f124143..71ae84a 100644 --- a/xen/include/xen/vm_event.h +++ b/xen/include/xen/vm_event.h @@ -96,6 +96,16 @@ void vm_event_vcpu_unpause(struct vcpu *v); int vm_event_monitor_traps(struct vcpu *v, uint8_t sync, vm_event_request_t *req); +#if CONFIG_X86 +/* + * Called for the current vCPU on control-register changes by guest. + * The event might not fire if the client has subscribed to it in onchangeonly + * mode, hence the bool_t return type for control register write events. + */ +bool_t vm_event_monitor_cr(unsigned int index, unsigned long value, + unsigned long old); +#endif + void vm_event_monitor_guest_request(void); #endif /* __VM_EVENT_H__ */
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 <czuzu@bitdefender.com> --- 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(-)