| Message ID | 7711f68d-394e-a74f-81fa-51f8447174ce@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: compat header generation and checking adjustments | expand |
On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote: > Translation macros aren't needed at all (or else a devicetree_label > entry would have been missing), and userlist has been removed quite some > time ago. > > No functional change. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/include/xlat.lst > +++ b/xen/include/xlat.lst > @@ -148,14 +148,11 @@ > ? xenoprof_init xenoprof.h > ? xenoprof_passive xenoprof.h > ? flask_access xsm/flask_op.h > -! flask_boolean xsm/flask_op.h > ? flask_cache_stats xsm/flask_op.h > ? flask_hash_stats xsm/flask_op.h > -! flask_load xsm/flask_op.h > ? flask_ocontext xsm/flask_op.h > ? flask_peersid xsm/flask_op.h > ? flask_relabel xsm/flask_op.h > ? flask_setavc_threshold xsm/flask_op.h > ? flask_setenforce xsm/flask_op.h > -! flask_sid_context xsm/flask_op.h > ? flask_transition xsm/flask_op.h Shouldn't those become checks then? Sorry I realize my knowledge of all this compat stuff is very poor. Thanks.
On 14.07.2020 16:58, Roger Pau Monné wrote: > On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote: >> Translation macros aren't needed at all (or else a devicetree_label >> entry would have been missing), and userlist has been removed quite some >> time ago. >> >> No functional change. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/include/xlat.lst >> +++ b/xen/include/xlat.lst >> @@ -148,14 +148,11 @@ >> ? xenoprof_init xenoprof.h >> ? xenoprof_passive xenoprof.h >> ? flask_access xsm/flask_op.h >> -! flask_boolean xsm/flask_op.h >> ? flask_cache_stats xsm/flask_op.h >> ? flask_hash_stats xsm/flask_op.h >> -! flask_load xsm/flask_op.h >> ? flask_ocontext xsm/flask_op.h >> ? flask_peersid xsm/flask_op.h >> ? flask_relabel xsm/flask_op.h >> ? flask_setavc_threshold xsm/flask_op.h >> ? flask_setenforce xsm/flask_op.h >> -! flask_sid_context xsm/flask_op.h >> ? flask_transition xsm/flask_op.h > > Shouldn't those become checks then? No, checking will never succeed for structures containing XEN_GUEST_HANDLE(). But there's no point in generating xlat macros when they're never used. There are two fundamentally different strategies for handling the compat hypercalls: One is to wrap a translation layer around the native hypercall. That's where the xlat macros come into play. The other, used here, is to compile the entire hypercall function a second time, arranging for the compat structures to get used in place of the native ones. There are no xlat macros involved here, all that's needed are correctly translated structures. (For completeness, x86's MCA hypercall uses yet another, quite adhoc strategy for handling, but also not involving any xlat macro use. Hence the consideration there to possibly drop the respective lines from the file here.) Jan
On Wed, Jul 15, 2020 at 08:42:44AM +0200, Jan Beulich wrote: > On 14.07.2020 16:58, Roger Pau Monné wrote: > > On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote: > >> Translation macros aren't needed at all (or else a devicetree_label > >> entry would have been missing), and userlist has been removed quite some > >> time ago. > >> > >> No functional change. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> > >> --- a/xen/include/xlat.lst > >> +++ b/xen/include/xlat.lst > >> @@ -148,14 +148,11 @@ > >> ? xenoprof_init xenoprof.h > >> ? xenoprof_passive xenoprof.h > >> ? flask_access xsm/flask_op.h > >> -! flask_boolean xsm/flask_op.h > >> ? flask_cache_stats xsm/flask_op.h > >> ? flask_hash_stats xsm/flask_op.h > >> -! flask_load xsm/flask_op.h > >> ? flask_ocontext xsm/flask_op.h > >> ? flask_peersid xsm/flask_op.h > >> ? flask_relabel xsm/flask_op.h > >> ? flask_setavc_threshold xsm/flask_op.h > >> ? flask_setenforce xsm/flask_op.h > >> -! flask_sid_context xsm/flask_op.h > >> ? flask_transition xsm/flask_op.h > > > > Shouldn't those become checks then? > > No, checking will never succeed for structures containing > XEN_GUEST_HANDLE(). But there's no point in generating xlat macros > when they're never used. There are two fundamentally different > strategies for handling the compat hypercalls: One is to wrap a > translation layer around the native hypercall. That's where the > xlat macros come into play. The other, used here, is to compile > the entire hypercall function a second time, arranging for the > compat structures to get used in place of the native ones. There > are no xlat macros involved here, all that's needed are correctly > translated structures. (For completeness, x86's MCA hypercall > uses yet another, quite adhoc strategy for handling, but also not > involving any xlat macro use. Hence the consideration there to > possibly drop the respective lines from the file here.) Thanks, I think this explanation is helpful and I wonder whether it would be possible to have something along this lines in a file or as a comment somewhere, maybe at the top of xlat.lst? Also could you add a line to the commit message noting that flask code doesn't use any of the translation macros because it follows a different approach to compat handling? IMO the compat code is complicated to understand, and it also seems to be mostly undocumented. For the patch: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks.
On 15.07.2020 10:41, Roger Pau Monné wrote: > On Wed, Jul 15, 2020 at 08:42:44AM +0200, Jan Beulich wrote: >> On 14.07.2020 16:58, Roger Pau Monné wrote: >>> On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote: >>>> Translation macros aren't needed at all (or else a devicetree_label >>>> entry would have been missing), and userlist has been removed quite some >>>> time ago. >>>> >>>> No functional change. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> >>>> --- a/xen/include/xlat.lst >>>> +++ b/xen/include/xlat.lst >>>> @@ -148,14 +148,11 @@ >>>> ? xenoprof_init xenoprof.h >>>> ? xenoprof_passive xenoprof.h >>>> ? flask_access xsm/flask_op.h >>>> -! flask_boolean xsm/flask_op.h >>>> ? flask_cache_stats xsm/flask_op.h >>>> ? flask_hash_stats xsm/flask_op.h >>>> -! flask_load xsm/flask_op.h >>>> ? flask_ocontext xsm/flask_op.h >>>> ? flask_peersid xsm/flask_op.h >>>> ? flask_relabel xsm/flask_op.h >>>> ? flask_setavc_threshold xsm/flask_op.h >>>> ? flask_setenforce xsm/flask_op.h >>>> -! flask_sid_context xsm/flask_op.h >>>> ? flask_transition xsm/flask_op.h >>> >>> Shouldn't those become checks then? >> >> No, checking will never succeed for structures containing >> XEN_GUEST_HANDLE(). But there's no point in generating xlat macros >> when they're never used. There are two fundamentally different >> strategies for handling the compat hypercalls: One is to wrap a >> translation layer around the native hypercall. That's where the >> xlat macros come into play. The other, used here, is to compile >> the entire hypercall function a second time, arranging for the >> compat structures to get used in place of the native ones. There >> are no xlat macros involved here, all that's needed are correctly >> translated structures. (For completeness, x86's MCA hypercall >> uses yet another, quite adhoc strategy for handling, but also not >> involving any xlat macro use. Hence the consideration there to >> possibly drop the respective lines from the file here.) > > Thanks, I think this explanation is helpful and I wonder whether it > would be possible to have something along this lines in a file or as a > comment somewhere, maybe at the top of xlat.lst? To be honest - I'm not sure: Such a comment may indeed be helpful to have, but I don't think I can see any single good place for it to live. For people editing xlat.lst (a file the existence of which many aren't even aware of), this would be a good place. But how would others have any chance of running into this comment? > Also could you add a line to the commit message noting that flask code > doesn't use any of the translation macros because it follows a > different approach to compat handling? I've made the sentence start "Translation macros aren't used (and hence needed) at all ..." - is that enough of an adjustment? > For the patch: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan
On Wed, Jul 15, 2020 at 10:52:28AM +0200, Jan Beulich wrote: > On 15.07.2020 10:41, Roger Pau Monné wrote: > > On Wed, Jul 15, 2020 at 08:42:44AM +0200, Jan Beulich wrote: > >> On 14.07.2020 16:58, Roger Pau Monné wrote: > >>> On Wed, Jul 01, 2020 at 12:28:07PM +0200, Jan Beulich wrote: > >>>> Translation macros aren't needed at all (or else a devicetree_label > >>>> entry would have been missing), and userlist has been removed quite some > >>>> time ago. > >>>> > >>>> No functional change. > >>>> > >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>>> > >>>> --- a/xen/include/xlat.lst > >>>> +++ b/xen/include/xlat.lst > >>>> @@ -148,14 +148,11 @@ > >>>> ? xenoprof_init xenoprof.h > >>>> ? xenoprof_passive xenoprof.h > >>>> ? flask_access xsm/flask_op.h > >>>> -! flask_boolean xsm/flask_op.h > >>>> ? flask_cache_stats xsm/flask_op.h > >>>> ? flask_hash_stats xsm/flask_op.h > >>>> -! flask_load xsm/flask_op.h > >>>> ? flask_ocontext xsm/flask_op.h > >>>> ? flask_peersid xsm/flask_op.h > >>>> ? flask_relabel xsm/flask_op.h > >>>> ? flask_setavc_threshold xsm/flask_op.h > >>>> ? flask_setenforce xsm/flask_op.h > >>>> -! flask_sid_context xsm/flask_op.h > >>>> ? flask_transition xsm/flask_op.h > >>> > >>> Shouldn't those become checks then? > >> > >> No, checking will never succeed for structures containing > >> XEN_GUEST_HANDLE(). But there's no point in generating xlat macros > >> when they're never used. There are two fundamentally different > >> strategies for handling the compat hypercalls: One is to wrap a > >> translation layer around the native hypercall. That's where the > >> xlat macros come into play. The other, used here, is to compile > >> the entire hypercall function a second time, arranging for the > >> compat structures to get used in place of the native ones. There > >> are no xlat macros involved here, all that's needed are correctly > >> translated structures. (For completeness, x86's MCA hypercall > >> uses yet another, quite adhoc strategy for handling, but also not > >> involving any xlat macro use. Hence the consideration there to > >> possibly drop the respective lines from the file here.) > > > > Thanks, I think this explanation is helpful and I wonder whether it > > would be possible to have something along this lines in a file or as a > > comment somewhere, maybe at the top of xlat.lst? > > To be honest - I'm not sure: Such a comment may indeed be helpful > to have, but I don't think I can see any single good place for it > to live. For people editing xlat.lst (a file the existence of which > many aren't even aware of), this would be a good place. But how > would others have any chance of running into this comment? I would add it to xlat.lst rather than not adding it at all. If we can find a better place to add it I'm all in, but as said I would rather add it somewhere right now than just defer adding it until the perfect placement is found. > > Also could you add a line to the commit message noting that flask code > > doesn't use any of the translation macros because it follows a > > different approach to compat handling? > > I've made the sentence start "Translation macros aren't used (and > hence needed) at all ..." - is that enough of an adjustment? Yes, that's fine. Thanks.
--- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -148,14 +148,11 @@ ? xenoprof_init xenoprof.h ? xenoprof_passive xenoprof.h ? flask_access xsm/flask_op.h -! flask_boolean xsm/flask_op.h ? flask_cache_stats xsm/flask_op.h ? flask_hash_stats xsm/flask_op.h -! flask_load xsm/flask_op.h ? flask_ocontext xsm/flask_op.h ? flask_peersid xsm/flask_op.h ? flask_relabel xsm/flask_op.h ? flask_setavc_threshold xsm/flask_op.h ? flask_setenforce xsm/flask_op.h -! flask_sid_context xsm/flask_op.h ? flask_transition xsm/flask_op.h --- a/xen/xsm/flask/flask_op.c +++ b/xen/xsm/flask/flask_op.c @@ -790,8 +790,6 @@ CHECK_flask_transition; #define xen_flask_load compat_flask_load #define flask_security_load compat_security_load -#define xen_flask_userlist compat_flask_userlist - #define xen_flask_sid_context compat_flask_sid_context #define flask_security_context compat_security_context #define flask_security_sid compat_security_sid
Translation macros aren't needed at all (or else a devicetree_label entry would have been missing), and userlist has been removed quite some time ago. No functional change. Signed-off-by: Jan Beulich <jbeulich@suse.com>