| Message ID | 1475563424-6604-16-git-send-email-emilcondrea@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 04/10/2016 08:43, Emil Condrea wrote: > xen_be_frontend_changed -> xen_fe_frontend_changed This is not correct. The front-end is implemented in the guest domain, while the back-end is implemented in the dom0 or stubdom. This function processes *in the backed* a notification that the frontend state changed, hence the name should be xen_be_frontend_changed. Paolo > Signed-off-by: Emil Condrea <emilcondrea@gmail.com> > --- > hw/xen/xen_backend.c | 2 +- > hw/xen/xen_frontend.c | 4 ++-- > include/hw/xen/xen_frontend.h | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c > index 30d3aaa..b79e83e 100644 > --- a/hw/xen/xen_backend.c > +++ b/hw/xen/xen_backend.c > @@ -213,7 +213,7 @@ static int xen_be_try_setup(struct XenDevice *xendev) > xen_be_set_state(xendev, XenbusStateInitialising); > > xen_be_backend_changed(xendev, NULL); > - xen_be_frontend_changed(xendev, NULL); > + xen_fe_frontend_changed(xendev, NULL); > return 0; > } > > diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c > index 1407f5f..761688b 100644 > --- a/hw/xen/xen_frontend.c > +++ b/hw/xen/xen_frontend.c > @@ -39,7 +39,7 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, > return xenstore_read_uint64(xendev->fe, node, uval); > } > > -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node) > +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node) > { > int fe_state; > > @@ -85,6 +85,6 @@ void xenstore_update_fe(char *watch, struct XenDevice *xendev) > } > node = watch + len + 1; > > - xen_be_frontend_changed(xendev, node); > + xen_fe_frontend_changed(xendev, node); > xen_be_check_state(xendev); > } > diff --git a/include/hw/xen/xen_frontend.h b/include/hw/xen/xen_frontend.h > index bb0bc23..2a5f03f 100644 > --- a/include/hw/xen/xen_frontend.h > +++ b/include/hw/xen/xen_frontend.h > @@ -9,6 +9,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, > uint64_t *uval); > void xenstore_update_fe(char *watch, struct XenDevice *xendev); > > -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node); > +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node); > > #endif /* QEMU_HW_XEN_FRONTEND_H */ >
On 04/10/2016 10:06, Paolo Bonzini wrote: > > > On 04/10/2016 08:43, Emil Condrea wrote: >> xen_be_frontend_changed -> xen_fe_frontend_changed > > This is not correct. The front-end is implemented in the guest domain, > while the back-end is implemented in the dom0 or stubdom. > > This function processes *in the backed* a notification that the frontend > state changed, hence the name should be xen_be_frontend_changed. Sorry, this comment was in the wrong place. It should have referred only to the hunk in hw/xen/xen_backend.c. Paolo > Paolo > >> Signed-off-by: Emil Condrea <emilcondrea@gmail.com> >> --- >> hw/xen/xen_backend.c | 2 +- >> hw/xen/xen_frontend.c | 4 ++-- >> include/hw/xen/xen_frontend.h | 2 +- >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c >> index 30d3aaa..b79e83e 100644 >> --- a/hw/xen/xen_backend.c >> +++ b/hw/xen/xen_backend.c >> @@ -213,7 +213,7 @@ static int xen_be_try_setup(struct XenDevice *xendev) >> xen_be_set_state(xendev, XenbusStateInitialising); >> >> xen_be_backend_changed(xendev, NULL); >> - xen_be_frontend_changed(xendev, NULL); >> + xen_fe_frontend_changed(xendev, NULL); >> return 0; >> } >> >> diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c >> index 1407f5f..761688b 100644 >> --- a/hw/xen/xen_frontend.c >> +++ b/hw/xen/xen_frontend.c >> @@ -39,7 +39,7 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >> return xenstore_read_uint64(xendev->fe, node, uval); >> } >> >> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node) >> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node) >> { >> int fe_state; >> >> @@ -85,6 +85,6 @@ void xenstore_update_fe(char *watch, struct XenDevice *xendev) >> } >> node = watch + len + 1; >> >> - xen_be_frontend_changed(xendev, node); >> + xen_fe_frontend_changed(xendev, node); >> xen_be_check_state(xendev); >> } >> diff --git a/include/hw/xen/xen_frontend.h b/include/hw/xen/xen_frontend.h >> index bb0bc23..2a5f03f 100644 >> --- a/include/hw/xen/xen_frontend.h >> +++ b/include/hw/xen/xen_frontend.h >> @@ -9,6 +9,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >> uint64_t *uval); >> void xenstore_update_fe(char *watch, struct XenDevice *xendev); >> >> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node); >> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node); >> >> #endif /* QEMU_HW_XEN_FRONTEND_H */ >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel >
On Tue, Oct 4, 2016 at 11:06 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 04/10/2016 08:43, Emil Condrea wrote: >> xen_be_frontend_changed -> xen_fe_frontend_changed > > This is not correct. The front-end is implemented in the guest domain, > while the back-end is implemented in the dom0 or stubdom. > You are right, thanks for the feedback! I will drop this patch together with the hunk from 04/15 patch which moves this function to xen_frontend.c > This function processes *in the backed* a notification that the frontend > state changed, hence the name should be xen_be_frontend_changed. > > Paolo > >> Signed-off-by: Emil Condrea <emilcondrea@gmail.com> >> --- >> hw/xen/xen_backend.c | 2 +- >> hw/xen/xen_frontend.c | 4 ++-- >> include/hw/xen/xen_frontend.h | 2 +- >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c >> index 30d3aaa..b79e83e 100644 >> --- a/hw/xen/xen_backend.c >> +++ b/hw/xen/xen_backend.c >> @@ -213,7 +213,7 @@ static int xen_be_try_setup(struct XenDevice *xendev) >> xen_be_set_state(xendev, XenbusStateInitialising); >> >> xen_be_backend_changed(xendev, NULL); >> - xen_be_frontend_changed(xendev, NULL); >> + xen_fe_frontend_changed(xendev, NULL); >> return 0; >> } >> >> diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c >> index 1407f5f..761688b 100644 >> --- a/hw/xen/xen_frontend.c >> +++ b/hw/xen/xen_frontend.c >> @@ -39,7 +39,7 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >> return xenstore_read_uint64(xendev->fe, node, uval); >> } >> >> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node) >> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node) >> { >> int fe_state; >> >> @@ -85,6 +85,6 @@ void xenstore_update_fe(char *watch, struct XenDevice *xendev) >> } >> node = watch + len + 1; >> >> - xen_be_frontend_changed(xendev, node); >> + xen_fe_frontend_changed(xendev, node); >> xen_be_check_state(xendev); >> } >> diff --git a/include/hw/xen/xen_frontend.h b/include/hw/xen/xen_frontend.h >> index bb0bc23..2a5f03f 100644 >> --- a/include/hw/xen/xen_frontend.h >> +++ b/include/hw/xen/xen_frontend.h >> @@ -9,6 +9,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >> uint64_t *uval); >> void xenstore_update_fe(char *watch, struct XenDevice *xendev); >> >> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node); >> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node); >> >> #endif /* QEMU_HW_XEN_FRONTEND_H */ >>
On 09/10/2016 21:50, Emil Condrea wrote: > On Tue, Oct 4, 2016 at 11:06 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 04/10/2016 08:43, Emil Condrea wrote: >>> xen_be_frontend_changed -> xen_fe_frontend_changed >> >> This is not correct. The front-end is implemented in the guest domain, >> while the back-end is implemented in the dom0 or stubdom. >> > > You are right, thanks for the feedback! I will drop this patch > together with the hunk > from 04/15 patch which moves this function to xen_frontend.c Actually all of your new xen_frontend.c seems to be reading frontend information from XenStore, which is typically something that the backend does. So I suggest dropping the patch altogether. Thanks, Paolo >> This function processes *in the backed* a notification that the frontend >> state changed, hence the name should be xen_be_frontend_changed. >> >> Paolo >> >>> Signed-off-by: Emil Condrea <emilcondrea@gmail.com> >>> --- >>> hw/xen/xen_backend.c | 2 +- >>> hw/xen/xen_frontend.c | 4 ++-- >>> include/hw/xen/xen_frontend.h | 2 +- >>> 3 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c >>> index 30d3aaa..b79e83e 100644 >>> --- a/hw/xen/xen_backend.c >>> +++ b/hw/xen/xen_backend.c >>> @@ -213,7 +213,7 @@ static int xen_be_try_setup(struct XenDevice *xendev) >>> xen_be_set_state(xendev, XenbusStateInitialising); >>> >>> xen_be_backend_changed(xendev, NULL); >>> - xen_be_frontend_changed(xendev, NULL); >>> + xen_fe_frontend_changed(xendev, NULL); >>> return 0; >>> } >>> >>> diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c >>> index 1407f5f..761688b 100644 >>> --- a/hw/xen/xen_frontend.c >>> +++ b/hw/xen/xen_frontend.c >>> @@ -39,7 +39,7 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >>> return xenstore_read_uint64(xendev->fe, node, uval); >>> } >>> >>> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node) >>> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node) >>> { >>> int fe_state; >>> >>> @@ -85,6 +85,6 @@ void xenstore_update_fe(char *watch, struct XenDevice *xendev) >>> } >>> node = watch + len + 1; >>> >>> - xen_be_frontend_changed(xendev, node); >>> + xen_fe_frontend_changed(xendev, node); >>> xen_be_check_state(xendev); >>> } >>> diff --git a/include/hw/xen/xen_frontend.h b/include/hw/xen/xen_frontend.h >>> index bb0bc23..2a5f03f 100644 >>> --- a/include/hw/xen/xen_frontend.h >>> +++ b/include/hw/xen/xen_frontend.h >>> @@ -9,6 +9,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >>> uint64_t *uval); >>> void xenstore_update_fe(char *watch, struct XenDevice *xendev); >>> >>> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node); >>> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node); >>> >>> #endif /* QEMU_HW_XEN_FRONTEND_H */ >>> > >
As you suggested, I've dropped the all patches for xen_frontend. Emil On Wed, Oct 12, 2016 at 2:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 09/10/2016 21:50, Emil Condrea wrote: >> On Tue, Oct 4, 2016 at 11:06 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> >>> >>> On 04/10/2016 08:43, Emil Condrea wrote: >>>> xen_be_frontend_changed -> xen_fe_frontend_changed >>> >>> This is not correct. The front-end is implemented in the guest domain, >>> while the back-end is implemented in the dom0 or stubdom. >>> >> >> You are right, thanks for the feedback! I will drop this patch >> together with the hunk >> from 04/15 patch which moves this function to xen_frontend.c > > Actually all of your new xen_frontend.c seems to be reading frontend > information from XenStore, which is typically something that the backend > does. So I suggest dropping the patch altogether. > > Thanks, > > Paolo > >>> This function processes *in the backed* a notification that the frontend >>> state changed, hence the name should be xen_be_frontend_changed. >>> >>> Paolo >>> >>>> Signed-off-by: Emil Condrea <emilcondrea@gmail.com> >>>> --- >>>> hw/xen/xen_backend.c | 2 +- >>>> hw/xen/xen_frontend.c | 4 ++-- >>>> include/hw/xen/xen_frontend.h | 2 +- >>>> 3 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c >>>> index 30d3aaa..b79e83e 100644 >>>> --- a/hw/xen/xen_backend.c >>>> +++ b/hw/xen/xen_backend.c >>>> @@ -213,7 +213,7 @@ static int xen_be_try_setup(struct XenDevice *xendev) >>>> xen_be_set_state(xendev, XenbusStateInitialising); >>>> >>>> xen_be_backend_changed(xendev, NULL); >>>> - xen_be_frontend_changed(xendev, NULL); >>>> + xen_fe_frontend_changed(xendev, NULL); >>>> return 0; >>>> } >>>> >>>> diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c >>>> index 1407f5f..761688b 100644 >>>> --- a/hw/xen/xen_frontend.c >>>> +++ b/hw/xen/xen_frontend.c >>>> @@ -39,7 +39,7 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >>>> return xenstore_read_uint64(xendev->fe, node, uval); >>>> } >>>> >>>> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node) >>>> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node) >>>> { >>>> int fe_state; >>>> >>>> @@ -85,6 +85,6 @@ void xenstore_update_fe(char *watch, struct XenDevice *xendev) >>>> } >>>> node = watch + len + 1; >>>> >>>> - xen_be_frontend_changed(xendev, node); >>>> + xen_fe_frontend_changed(xendev, node); >>>> xen_be_check_state(xendev); >>>> } >>>> diff --git a/include/hw/xen/xen_frontend.h b/include/hw/xen/xen_frontend.h >>>> index bb0bc23..2a5f03f 100644 >>>> --- a/include/hw/xen/xen_frontend.h >>>> +++ b/include/hw/xen/xen_frontend.h >>>> @@ -9,6 +9,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, >>>> uint64_t *uval); >>>> void xenstore_update_fe(char *watch, struct XenDevice *xendev); >>>> >>>> -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node); >>>> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node); >>>> >>>> #endif /* QEMU_HW_XEN_FRONTEND_H */ >>>> >> >>
On October 13, 2016 2:09 PM, Emil Condrea <emilcondrea@gmail.com> wrote: >As you suggested, I've dropped the all patches for xen_frontend. > >Emil > >On Wed, Oct 12, 2016 at 2:00 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 09/10/2016 21:50, Emil Condrea wrote: >>> On Tue, Oct 4, 2016 at 11:06 AM, Paolo Bonzini <pbonzini@redhat.com> >wrote: >>>> >>>> >>>> On 04/10/2016 08:43, Emil Condrea wrote: >>>>> xen_be_frontend_changed -> xen_fe_frontend_changed >>>> >>>> This is not correct. The front-end is implemented in the guest >>>> domain, while the back-end is implemented in the dom0 or stubdom. >>>> >>> Hi Paolo, Yes, the front-end is almost implemented in the guest This case (as mentioned in 00/15) is very particular. The frontend is actually implemented in QEMU, and the guest still run native driver tpm_tis.ko.. >>> You are right, thanks for the feedback! I will drop this patch >>> together with the hunk from 04/15 patch which moves this function to >>> xen_frontend.c >> >> Actually all of your new xen_frontend.c seems to be reading frontend >> information from XenStore, which is typically something that the >> backend does. So I suggest dropping the patch altogether. >> So we are better leave it as is. Maybe we need to rename some functions in that file. __iirc__ adding xen_frontend.c is one of Stefano's comments in previous v6 or v7.. Quan >> Thanks, >> >> Paolo >> >>>> This function processes *in the backed* a notification that the >>>> frontend state changed, hence the name should be >xen_be_frontend_changed. >>>> >>>> Paolo >>>> >>>>> Signed-off-by: Emil Condrea <emilcondrea@gmail.com> >>>>> --- >>>>> hw/xen/xen_backend.c | 2 +- >>>>> hw/xen/xen_frontend.c | 4 ++-- >>>>> include/hw/xen/xen_frontend.h | 2 +- >>>>> 3 files changed, 4 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c index >>>>> 30d3aaa..b79e83e 100644 >>>>> --- a/hw/xen/xen_backend.c >>>>> +++ b/hw/xen/xen_backend.c >>>>> @@ -213,7 +213,7 @@ static int xen_be_try_setup(struct XenDevice >*xendev) >>>>> xen_be_set_state(xendev, XenbusStateInitialising); >>>>> >>>>> xen_be_backend_changed(xendev, NULL); >>>>> - xen_be_frontend_changed(xendev, NULL); >>>>> + xen_fe_frontend_changed(xendev, NULL); >>>>> return 0; >>>>> } >>>>> >>>>> diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c index >>>>> 1407f5f..761688b 100644 >>>>> --- a/hw/xen/xen_frontend.c >>>>> +++ b/hw/xen/xen_frontend.c >>>>> @@ -39,7 +39,7 @@ int xenstore_read_fe_uint64(struct XenDevice >*xendev, const char *node, >>>>> return xenstore_read_uint64(xendev->fe, node, uval); } >>>>> >>>>> -void xen_be_frontend_changed(struct XenDevice *xendev, const char >>>>> *node) >>>>> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char >>>>> +*node) >>>>> { >>>>> int fe_state; >>>>> >>>>> @@ -85,6 +85,6 @@ void xenstore_update_fe(char *watch, struct >XenDevice *xendev) >>>>> } >>>>> node = watch + len + 1; >>>>> >>>>> - xen_be_frontend_changed(xendev, node); >>>>> + xen_fe_frontend_changed(xendev, node); >>>>> xen_be_check_state(xendev); >>>>> } >>>>> diff --git a/include/hw/xen/xen_frontend.h >>>>> b/include/hw/xen/xen_frontend.h index bb0bc23..2a5f03f 100644 >>>>> --- a/include/hw/xen/xen_frontend.h >>>>> +++ b/include/hw/xen/xen_frontend.h >>>>> @@ -9,6 +9,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, >const char *node, >>>>> >uint64_t >>>>> *uval); void xenstore_update_fe(char *watch, struct XenDevice >>>>> *xendev); >>>>> >>>>> -void xen_be_frontend_changed(struct XenDevice *xendev, const char >>>>> *node); >>>>> +void xen_fe_frontend_changed(struct XenDevice *xendev, const char >>>>> +*node); >>>>> >>>>> #endif /* QEMU_HW_XEN_FRONTEND_H */ >>>>> >>> >>>
> So we are better leave it as is. Maybe we need to rename some functions in > that file. > __iirc__ adding xen_frontend.c is one of Stefano's comments in previous v6 > or v7.. I agree; it's quite possible that code can be shared between backend and frontend (renaming functions as needed), and it's good to have a generic xen frontend mechanism instead of something specific to TPM. Paolo
diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c index 30d3aaa..b79e83e 100644 --- a/hw/xen/xen_backend.c +++ b/hw/xen/xen_backend.c @@ -213,7 +213,7 @@ static int xen_be_try_setup(struct XenDevice *xendev) xen_be_set_state(xendev, XenbusStateInitialising); xen_be_backend_changed(xendev, NULL); - xen_be_frontend_changed(xendev, NULL); + xen_fe_frontend_changed(xendev, NULL); return 0; } diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c index 1407f5f..761688b 100644 --- a/hw/xen/xen_frontend.c +++ b/hw/xen/xen_frontend.c @@ -39,7 +39,7 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, return xenstore_read_uint64(xendev->fe, node, uval); } -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node) +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node) { int fe_state; @@ -85,6 +85,6 @@ void xenstore_update_fe(char *watch, struct XenDevice *xendev) } node = watch + len + 1; - xen_be_frontend_changed(xendev, node); + xen_fe_frontend_changed(xendev, node); xen_be_check_state(xendev); } diff --git a/include/hw/xen/xen_frontend.h b/include/hw/xen/xen_frontend.h index bb0bc23..2a5f03f 100644 --- a/include/hw/xen/xen_frontend.h +++ b/include/hw/xen/xen_frontend.h @@ -9,6 +9,6 @@ int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, uint64_t *uval); void xenstore_update_fe(char *watch, struct XenDevice *xendev); -void xen_be_frontend_changed(struct XenDevice *xendev, const char *node); +void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node); #endif /* QEMU_HW_XEN_FRONTEND_H */
xen_be_frontend_changed -> xen_fe_frontend_changed Signed-off-by: Emil Condrea <emilcondrea@gmail.com> --- hw/xen/xen_backend.c | 2 +- hw/xen/xen_frontend.c | 4 ++-- include/hw/xen/xen_frontend.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)