Re: [Xen-devel] [PATCH v2 07/16] x86: put XEN_DOMCTL_{set, get}_address_size under CONFIG_PV

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 31.10.18 at 10:54, <wei.liu2@xxxxxxxxxx> wrote:
> On Wed, Oct 31, 2018 at 03:47:25AM -0600, Jan Beulich wrote:
>> >>> On 31.10.18 at 10:33, <wei.liu2@xxxxxxxxxx> wrote:
>> > On Wed, Oct 31, 2018 at 03:00:17AM -0600, Jan Beulich wrote:
>> >> >>> On 30.10.18 at 21:50, <wei.liu2@xxxxxxxxxx> wrote:
>> >> > On Mon, Oct 29, 2018 at 08:28:07AM -0600, Jan Beulich wrote:
>> >> >> >>> On 19.10.18 at 16:28, <wei.liu2@xxxxxxxxxx> wrote:
>> >> >> > --- a/xen/arch/x86/domctl.c
>> >> >> > +++ b/xen/arch/x86/domctl.c
>> >> >> > @@ -608,6 +608,7 @@ long arch_do_domctl(
>> >> >> >              copyback = true;
>> >> >> >          break;
>> >> >> >  
>> >> >> > +#ifdef CONFIG_PV
>> >> >> >      case XEN_DOMCTL_set_address_size:
>> >> >> >          if ( ((domctl->u.address_size.size == 64) && 
>> >> >> > !d->arch.is_32bit_pv) ||
>> >> >> >               ((domctl->u.address_size.size == 32) && 
>> >> >> > d->arch.is_32bit_pv) )
>> >> >> > @@ -623,6 +624,7 @@ long arch_do_domctl(
>> >> >> >                                                                
>> >> >> > BITS_PER_LONG;
>> >> >> >          copyback = true;
>> >> >> >          break;
>> >> >> > +#endif
>> >> >> 
>> >> >> ... add such a check so that similar behavior will result with PV
>> >> >> enabled and disabled (error codes may differ, but success vs
>> >> >> error ought to match).
>> >> > 
>> >> > I don't follow. Do you mean adding is_pv_domain check somewhere?
>> >> 
>> >> Yes. Otherwise behavior differs between a PV and a !PV hypervisor.
>> > 
>> > I'm still at a loss to figure out what "similar behavior" you want. When
>> > you say behaviour differs, do you mean the lack of copyback in my patch?
>> > Why would that be relevant to a ENOSYS hypercall?
>> 
>> With !PV you'd get -ENOSYS (which is wrong in its own right, but
>> that a different topic). With PV you'd not get any error under at
>> least some conditions even for a HVM domain (if you don't make it
>> into switch_compat()), afaict.
> 
> Right, so you actually wanted me to fix two issues.
> 
> One is to change it to not return -ENOSYS. I think you would want
> -EOPNOTSUPP instead?

We've had that discussion in the past, and there was no consensus
reached as to changing pre-existing (but wrong) error codes. So no,
this wasn't part of what I've been asking for.

> The other is to tighten the hypercalls to be PV only. I think for HVM
> guests they should return -EINVAL.

Or indeed -EOPNOTSUPP, seeing that Arm does support them
despite not supporting PV guests.

> So instead of putting the pair under CONFIG_PV, I should be adding some
> checks to them individually to do what we agree on later. Does this make
> sense?

Yes.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel