From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas K Lengyel Subject: Re: [PATCH 6/7] vm-event/arm: move hvm_event_cr->common vm_event_monitor_cr Date: Tue, 21 Jun 2016 09:22:03 -0600 Message-ID: References: <1466085888-7428-1-git-send-email-czuzu@bitdefender.com> <1466086345-7705-1-git-send-email-czuzu@bitdefender.com> <5762DEE902000078000F5BF1@prv-mh.provo.novell.com> <386cf9b3-ada1-0d03-f047-e16785e8b64d@bitdefender.com> <576906E002000078000F7179@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3866391049343132938==" Return-path: In-Reply-To: <576906E002000078000F7179@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Andrew Cooper , xen-devel@lists.xen.org, Razvan Cojocaru , Corneliu ZUZU List-Id: xen-devel@lists.xenproject.org --===============3866391049343132938== Content-Type: multipart/alternative; boundary=089e012297bc0ff7280535cb6203 --089e012297bc0ff7280535cb6203 Content-Type: text/plain; charset=UTF-8 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 --089e012297bc0ff7280535cb6203 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


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 wr= ite vm-events
> >>>> by moving
> >>>> X86-specific hvm_event_cr to the common-side.
> >>>>
> >>>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> >>>> ---
> >>>>=C2=A0 =C2=A0xen/arch/x86/hvm/event.c=C2=A0 =C2=A0 =C2= =A0 =C2=A0 | 30 ------------------------------
> >>>>=C2=A0 =C2=A0xen/arch/x86/hvm/hvm.c=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 |=C2=A0 2 +-
> >>>>=C2=A0 =C2=A0xen/arch/x86/monitor.c=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 | 37
> >>>> -------------------------------------
> >>>>=C2=A0 =C2=A0xen/arch/x86/vm_event.c=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0|=C2=A0 2 +-
> >>>>=C2=A0 =C2=A0xen/common/monitor.c=C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 | 40
> >>>> ++++++++++++++++++++++++++++++++++++++++
> >>>>=C2=A0 =C2=A0xen/common/vm_event.c=C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0| 31 +++++++++++++++++++++++++++++++
> >>>>=C2=A0 =C2=A0xen/include/asm-x86/hvm/event.h | 13 ++++= ---------
> >>>>=C2=A0 =C2=A0xen/include/asm-x86/monitor.h=C2=A0 =C2= =A0|=C2=A0 2 --
> >>>>=C2=A0 =C2=A0xen/include/xen/monitor.h=C2=A0 =C2=A0 = =C2=A0 =C2=A0|=C2=A0 4 ++++
> >>>>=C2=A0 =C2=A0xen/include/xen/vm_event.h=C2=A0 =C2=A0 = =C2=A0 | 10 ++++++++++
> >>>>=C2=A0 =C2=A010 files changed, 91 insertions(+), 80 de= letions(-)
> >>> 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 mentio= n 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)
> >>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0switch ( mop->eve= nt )
> >>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0{
> >>>> +#if CONFIG_X86
> >>> #ifdef please.
> >> Ack.
> >>>> +=C2=A0 =C2=A0 case XEN_DOMCTL_MONITOR_EVENT_WRITE_CT= RLREG:
> >>>> +=C2=A0 =C2=A0 {
> >>>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct arch_domain *ad = =3D &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 duplicatio= n.
> >> The code is the same, but those bits that are currently on th= e 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_EVE= NT_ARM_*
> >> defines (wcr index), also have
> >>=C2=A0 =C2=A0 =C2=A0#define VM_EVENT_X86_CR_COUNT=C2=A0 =C2=A0= =C2=A0 =C2=A0 4
> >>=C2=A0 =C2=A0 =C2=A0#define VM_EVENT_ARM_CR_COUNT=C2=A0 =C2=A0= =C2=A0 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-depe= ndent and
> >> use the introduced VM_EVENT_<arch>_CR_COUNT
> >>
> >> Tamas, we also talked on this matter @ some point (when I sen= t the
> >> patches that moved vm-event parts to common). What do you thi= nk 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

--089e012297bc0ff7280535cb6203-- --===============3866391049343132938== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============3866391049343132938==--