| Message ID | 1468835505-7278-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote: > set_segment_base is the only hypercall exists in only one of the two modes > guests might run in; all other hypercalls are either implemented, or > unimplemented in both modes. > > Remove this split, by allowing do_set_segment_base() to be called in the > compat hypercall path. This change will simplify the verification logic in a > later change. > > No behavioural change from a guests point of view. Nevertheless I don't view this as a reasonable change on its own: > --- a/xen/arch/x86/x86_64/compat/entry.S > +++ b/xen/arch/x86/x86_64/compat/entry.S > @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table) > .quad compat_update_va_mapping_otherdomain > .quad compat_iret > .quad compat_vcpu_op > - .quad compat_ni_hypercall /* 25 */ > + .quad do_set_segment_base /* 25 */ This part will (I suppose) be deleted by a later change. > --- a/xen/arch/x86/x86_64/mm.c > +++ b/xen/arch/x86/x86_64/mm.c > @@ -1031,6 +1031,9 @@ long do_set_segment_base(unsigned int which, unsigned long base) > struct vcpu *v = current; > long ret = 0; > > + if ( is_pv_32bit_vcpu(v) ) > + return -ENOSYS; /* x86/64 only. */ And this addition could as well happen when that later change does the re-org. Jan
On 02/08/16 13:52, Jan Beulich wrote: >>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote: >> set_segment_base is the only hypercall exists in only one of the two modes >> guests might run in; all other hypercalls are either implemented, or >> unimplemented in both modes. >> >> Remove this split, by allowing do_set_segment_base() to be called in the >> compat hypercall path. This change will simplify the verification logic in a >> later change. >> >> No behavioural change from a guests point of view. > Nevertheless I don't view this as a reasonable change on its own: Why not? It is far better to call it out in isolation, than to mix it in with an already-existing complicated change. > >> --- a/xen/arch/x86/x86_64/compat/entry.S >> +++ b/xen/arch/x86/x86_64/compat/entry.S >> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table) >> .quad compat_update_va_mapping_otherdomain >> .quad compat_iret >> .quad compat_vcpu_op >> - .quad compat_ni_hypercall /* 25 */ >> + .quad do_set_segment_base /* 25 */ > This part will (I suppose) be deleted by a later change. When moved into C, it will be hidden behind the macros used to construct both table values at once. > >> --- a/xen/arch/x86/x86_64/mm.c >> +++ b/xen/arch/x86/x86_64/mm.c >> @@ -1031,6 +1031,9 @@ long do_set_segment_base(unsigned int which, unsigned long base) >> struct vcpu *v = current; >> long ret = 0; >> >> + if ( is_pv_32bit_vcpu(v) ) >> + return -ENOSYS; /* x86/64 only. */ > And this addition could as well happen when that later change > does the re-org. Again, that would further complicate an already-complicated patch. ~Andrew
>>> On 02.08.16 at 15:25, <andrew.cooper3@citrix.com> wrote: > On 02/08/16 13:52, Jan Beulich wrote: >>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote: >>> set_segment_base is the only hypercall exists in only one of the two modes >>> guests might run in; all other hypercalls are either implemented, or >>> unimplemented in both modes. >>> >>> Remove this split, by allowing do_set_segment_base() to be called in the >>> compat hypercall path. This change will simplify the verification logic in a >>> later change. >>> >>> No behavioural change from a guests point of view. >> Nevertheless I don't view this as a reasonable change on its own: > > Why not? It is far better to call it out in isolation, than to mix it > in with an already-existing complicated change. If the changes here were anywhere near being themselves complicated, I'd agree. >>> --- a/xen/arch/x86/x86_64/compat/entry.S >>> +++ b/xen/arch/x86/x86_64/compat/entry.S >>> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table) >>> .quad compat_update_va_mapping_otherdomain >>> .quad compat_iret >>> .quad compat_vcpu_op >>> - .quad compat_ni_hypercall /* 25 */ >>> + .quad do_set_segment_base /* 25 */ >> This part will (I suppose) be deleted by a later change. > > When moved into C, it will be hidden behind the macros used to construct > both table values at once. And compat_set_segment_base could then just be #define-d to NULL. Jan
On 02/08/16 14:31, Jan Beulich wrote: >>>> On 02.08.16 at 15:25, <andrew.cooper3@citrix.com> wrote: >> On 02/08/16 13:52, Jan Beulich wrote: >>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote: >>>> set_segment_base is the only hypercall exists in only one of the two modes >>>> guests might run in; all other hypercalls are either implemented, or >>>> unimplemented in both modes. >>>> >>>> Remove this split, by allowing do_set_segment_base() to be called in the >>>> compat hypercall path. This change will simplify the verification logic in a >>>> later change. >>>> >>>> No behavioural change from a guests point of view. >>> Nevertheless I don't view this as a reasonable change on its own: >> Why not? It is far better to call it out in isolation, than to mix it >> in with an already-existing complicated change. > If the changes here were anywhere near being themselves > complicated, I'd agree. > >>>> --- a/xen/arch/x86/x86_64/compat/entry.S >>>> +++ b/xen/arch/x86/x86_64/compat/entry.S >>>> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table) >>>> .quad compat_update_va_mapping_otherdomain >>>> .quad compat_iret >>>> .quad compat_vcpu_op >>>> - .quad compat_ni_hypercall /* 25 */ >>>> + .quad do_set_segment_base /* 25 */ >>> This part will (I suppose) be deleted by a later change. >> When moved into C, it will be hidden behind the macros used to construct >> both table values at once. > And compat_set_segment_base could then just be #define-d to NULL. The entire purpose of making this change is so we don't end up with NULL in one half a pair of function pointers, and have to extend the runtime logic to check different pointers. This is the only hypercall with this problem, so I have opted to make it not a problem, rather than causing an extra runtime hit on all hypercalls. A 32bit PV guest doesn't have any reason to make this hypercall. It it does, I really don't care that it takes a few instructions extra to return -ENOSYS. ~Andrew
>>> On 02.08.16 at 15:39, <andrew.cooper3@citrix.com> wrote: > On 02/08/16 14:31, Jan Beulich wrote: >>>>> On 02.08.16 at 15:25, <andrew.cooper3@citrix.com> wrote: >>> On 02/08/16 13:52, Jan Beulich wrote: >>>>>>> On 18.07.16 at 11:51, <andrew.cooper3@citrix.com> wrote: >>>>> --- a/xen/arch/x86/x86_64/compat/entry.S >>>>> +++ b/xen/arch/x86/x86_64/compat/entry.S >>>>> @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table) >>>>> .quad compat_update_va_mapping_otherdomain >>>>> .quad compat_iret >>>>> .quad compat_vcpu_op >>>>> - .quad compat_ni_hypercall /* 25 */ >>>>> + .quad do_set_segment_base /* 25 */ >>>> This part will (I suppose) be deleted by a later change. >>> When moved into C, it will be hidden behind the macros used to construct >>> both table values at once. >> And compat_set_segment_base could then just be #define-d to NULL. > > The entire purpose of making this change is so we don't end up with NULL > in one half a pair of function pointers, and have to extend the runtime > logic to check different pointers. Well, okay then - feel free to add by ack. Jan
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S index 7f02afd..89673c4 100644 --- a/xen/arch/x86/x86_64/compat/entry.S +++ b/xen/arch/x86/x86_64/compat/entry.S @@ -453,7 +453,7 @@ ENTRY(compat_hypercall_table) .quad compat_update_va_mapping_otherdomain .quad compat_iret .quad compat_vcpu_op - .quad compat_ni_hypercall /* 25 */ + .quad do_set_segment_base /* 25 */ .quad compat_mmuext_op .quad compat_xsm_op .quad compat_nmi_op diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c index ff9fc43..512d855 100644 --- a/xen/arch/x86/x86_64/mm.c +++ b/xen/arch/x86/x86_64/mm.c @@ -1031,6 +1031,9 @@ long do_set_segment_base(unsigned int which, unsigned long base) struct vcpu *v = current; long ret = 0; + if ( is_pv_32bit_vcpu(v) ) + return -ENOSYS; /* x86/64 only. */ + switch ( which ) { case SEGBASE_FS:
set_segment_base is the only hypercall exists in only one of the two modes guests might run in; all other hypercalls are either implemented, or unimplemented in both modes. Remove this split, by allowing do_set_segment_base() to be called in the compat hypercall path. This change will simplify the verification logic in a later change. No behavioural change from a guests point of view. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/x86_64/compat/entry.S | 2 +- xen/arch/x86/x86_64/mm.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)