Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[Andrew Cooper]

On 06/09/16 09:03, Jan Beulich wrote:
 On 12.08.16 at 13:49,  wrote:
>> On 12/08/16 11:58, Jan Beulich wrote:
>> On 11.08.16 at 20:10,  wrote:
 On 09/08/16 11:40, Jan Beulich wrote:
> --- a/xen/arch/x86/cpu/mtrr/main.c
> +++ b/xen/arch/x86/cpu/mtrr/main.c
> @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
>   if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
>   printk(KERN_WARNING
>  "mtrr: your processor doesn't support 
> write-combining\n");
> - return -ENOSYS;
> + return -EOPNOTSUPP;
 Will this break the classic-xen MTRR code?  ISTR it was very picky, from
 the cpuid work.
>>> There are no -ENOSYS checks in there afaics, and I also can't
>>> otherwise see any way for this change to break it.
>>>
  Also, as some further cleanup, that printk should
 become a print-once.
>>> Well, for a message that presumably would never actually get
>>> issued (as I'm unaware of 64-bit capable CPUs not supporting
>>> WC) I don't think this sort of cleanup has a really high priority.
>>> Certainly not in this patch.
>> Agreed.  This was a TODO note, rather than a request for this patch.  I
>> have noticed a few other printk()'s which should become print once.
> Btw., with the MTRR concern hopefully addressed, any chance of
> getting an ack on the x86 pieces here?

Acked-by: Andrew Cooper 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[Jan Beulich]

>>> On 12.08.16 at 13:49,  wrote:
> On 12/08/16 11:58, Jan Beulich wrote:
> On 11.08.16 at 20:10,  wrote:
>>> On 09/08/16 11:40, Jan Beulich wrote:
 --- a/xen/arch/x86/cpu/mtrr/main.c
 +++ b/xen/arch/x86/cpu/mtrr/main.c
 @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
printk(KERN_WARNING
   "mtrr: your processor doesn't support 
 write-combining\n");
 -  return -ENOSYS;
 +  return -EOPNOTSUPP;
>>> Will this break the classic-xen MTRR code?  ISTR it was very picky, from
>>> the cpuid work.
>> There are no -ENOSYS checks in there afaics, and I also can't
>> otherwise see any way for this change to break it.
>>
>>>  Also, as some further cleanup, that printk should
>>> become a print-once.
>> Well, for a message that presumably would never actually get
>> issued (as I'm unaware of 64-bit capable CPUs not supporting
>> WC) I don't think this sort of cleanup has a really high priority.
>> Certainly not in this patch.
> 
> Agreed.  This was a TODO note, rather than a request for this patch.  I
> have noticed a few other printk()'s which should become print once.

Btw., with the MTRR concern hopefully addressed, any chance of
getting an ack on the x86 pieces here?

Thanks, Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[George Dunlap]

On Tue, Aug 9, 2016 at 11:40 AM, Jan Beulich  wrote:
> This doesn't cover all of them, just the ones that I think would most
> obviously better be -EINVAL or -EOPNOTSUPP.
>
> Signed-off-by: Jan Beulich 

FWIW:

Reviewed-by: George Dunlap 

>
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -440,7 +440,7 @@ int unmmap_broken_page(struct domain *d,
>  return -EINVAL;
>
>  if ( !has_hvm_container_domain(d) || !paging_mode_hap(d) )
> -return -ENOSYS;
> +return -EOPNOTSUPP;
>
>  rc = -1;
>  r_mfn = get_gfn_query(d, gfn, );
> --- a/xen/arch/x86/cpu/mtrr/main.c
> +++ b/xen/arch/x86/cpu/mtrr/main.c
> @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
> if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
> printk(KERN_WARNING
>"mtrr: your processor doesn't support 
> write-combining\n");
> -   return -ENOSYS;
> +   return -EOPNOTSUPP;
> }
>
> if (!size) {
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5592,7 +5592,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
>  break;
>
>  case HVMOP_flush_tlbs:
> -rc = guest_handle_is_null(arg) ? hvmop_flush_tlb_all() : -ENOSYS;
> +rc = guest_handle_is_null(arg) ? hvmop_flush_tlb_all() : -EINVAL;
>  break;
>
>  case HVMOP_track_dirty_vram:
> @@ -5832,7 +5832,7 @@ int hvm_debug_op(struct vcpu *v, int32_t
>  {
>  case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
>  case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> -rc = -ENOSYS;
> +rc = -EOPNOTSUPP;
>  if ( !cpu_has_monitor_trap_flag )
>  break;
>  rc = 0;
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3565,7 +3565,7 @@ long do_mmuext_op(
>  if ( !opt_allow_superpage )
>  {
>  MEM_LOG("Superpages disallowed");
> -rc = -ENOSYS;
> +rc = -EOPNOTSUPP;
>  }
>  else if ( unlikely(d != pg_owner) )
>  rc = -EPERM;
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -353,7 +353,7 @@ int compat_memory_op(unsigned int cmd, X
>  struct get_reserved_device_memory grdm;
>
>  if ( unlikely(start_extent) )
> -return -ENOSYS;
> +return -EINVAL;
>
>  if ( copy_from_guest(, compat, 1) ||
>   !compat_handle_okay(grdm.map.buffer, grdm.map.nr_entries) )
> --- a/xen/common/event_fifo.c
> +++ b/xen/common/event_fifo.c
> @@ -621,7 +621,7 @@ int evtchn_fifo_expand_array(const struc
>  int rc;
>
>  if ( !d->evtchn_fifo )
> -return -ENOSYS;
> +return -EOPNOTSUPP;
>
>  spin_lock(>event_lock);
>  rc = add_page_to_event_array(d, expand_array->array_gfn);
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3025,7 +3025,7 @@ do_grant_table_op(
>  return -EINVAL;
>
>  if ( (cmd &= GNTTABOP_CMD_MASK) != GNTTABOP_cache_flush && opaque_in )
> -return -ENOSYS;
> +return -EINVAL;
>
>  rc = -EFAULT;
>  switch ( cmd )
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -937,14 +937,14 @@ long do_memory_op(unsigned long cmd, XEN
>
>  case XENMEM_exchange:
>  if ( unlikely(start_extent) )
> -return -ENOSYS;
> +return -EINVAL;
>
>  rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
>  break;
>
>  case XENMEM_maximum_ram_page:
>  if ( unlikely(start_extent) )
> -return -ENOSYS;
> +return -EINVAL;
>
>  rc = max_page;
>  break;
> @@ -953,7 +953,7 @@ long do_memory_op(unsigned long cmd, XEN
>  case XENMEM_maximum_reservation:
>  case XENMEM_maximum_gpfn:
>  if ( unlikely(start_extent) )
> -return -ENOSYS;
> +return -EINVAL;
>
>  if ( copy_from_guest(, arg, 1) )
>  return -EFAULT;
> @@ -1077,7 +1077,7 @@ long do_memory_op(unsigned long cmd, XEN
>  struct page_info *page;
>
>  if ( unlikely(start_extent) )
> -return -ENOSYS;
> +return -EINVAL;
>
>  if ( copy_from_guest(, arg, 1) )
>  return -EFAULT;
> @@ -1114,7 +1114,7 @@ long do_memory_op(unsigned long cmd, XEN
>
>  case XENMEM_claim_pages:
>  if ( unlikely(start_extent) )
> -return -ENOSYS;
> +return -EINVAL;
>
>  if ( copy_from_guest(, arg, 1) )
>  return -EFAULT;
> @@ -1148,7 +1148,7 @@ long do_memory_op(unsigned long cmd, XEN
>  struct vnuma_info tmp;
>
>  if ( unlikely(start_extent) )
> -return -ENOSYS;
> +return -EINVAL;
>
>  /*
>   * Guest passes nr_vnodes, number of regions and nr_vcpus thus
> @@ 

Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[Andrew Cooper]

On 12/08/16 11:58, Jan Beulich wrote:
 On 11.08.16 at 20:10,  wrote:
>> On 09/08/16 11:40, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpu/mtrr/main.c
>>> +++ b/xen/arch/x86/cpu/mtrr/main.c
>>> @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
>>> if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
>>> printk(KERN_WARNING
>>>"mtrr: your processor doesn't support 
>>> write-combining\n");
>>> -   return -ENOSYS;
>>> +   return -EOPNOTSUPP;
>> Will this break the classic-xen MTRR code?  ISTR it was very picky, from
>> the cpuid work.
> There are no -ENOSYS checks in there afaics, and I also can't
> otherwise see any way for this change to break it.
>
>>  Also, as some further cleanup, that printk should
>> become a print-once.
> Well, for a message that presumably would never actually get
> issued (as I'm unaware of 64-bit capable CPUs not supporting
> WC) I don't think this sort of cleanup has a really high priority.
> Certainly not in this patch.

Agreed.  This was a TODO note, rather than a request for this patch.  I
have noticed a few other printk()'s which should become print once.

Sorry for the confusion.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[Jan Beulich]

>>> On 12.08.16 at 12:34,  wrote:
> On 11/08/16 19:10, Andrew Cooper wrote:
>> On 09/08/16 11:40, Jan Beulich wrote:
>>> --- a/xen/arch/x86/cpu/mtrr/main.c
>>> +++ b/xen/arch/x86/cpu/mtrr/main.c
>>> @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
>>> if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
>>> printk(KERN_WARNING
>>>"mtrr: your processor doesn't support 
>>> write-combining\n");
>>> -   return -ENOSYS;
>>> +   return -EOPNOTSUPP;
>> 
>> Will this break the classic-xen MTRR code?  ISTR it was very picky, from
>> the cpuid work.  Also, as some further cleanup, that printk should
>> become a print-once.
>> 
>> The others look ok.
> 
> That does bring up a good general point though -- the return value is
> part of the ABI.  Are you reasonably confident that none of the callers
> will be confused when this return value changes?  If so, a note in the
> commit message justifying this confidence would be helpful I think.

I don't think specific return values can be considered part of the
ABI, or else we couldn't e.g. change the order in which certain
checks get performed.

And then please also consider a hypothetical future hypervisor with
the MTRR operations simply ripped out - that would return -ENOSYS
or -EOPNOTSUPP then too, without a way for the caller to tell that
more generic error condition from the more specific one here.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[Jan Beulich]

>>> On 11.08.16 at 20:10,  wrote:
> On 09/08/16 11:40, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/mtrr/main.c
>> +++ b/xen/arch/x86/cpu/mtrr/main.c
>> @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
>>  if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
>>  printk(KERN_WARNING
>> "mtrr: your processor doesn't support 
>> write-combining\n");
>> -return -ENOSYS;
>> +return -EOPNOTSUPP;
> 
> Will this break the classic-xen MTRR code?  ISTR it was very picky, from
> the cpuid work.

There are no -ENOSYS checks in there afaics, and I also can't
otherwise see any way for this change to break it.

>  Also, as some further cleanup, that printk should
> become a print-once.

Well, for a message that presumably would never actually get
issued (as I'm unaware of 64-bit capable CPUs not supporting
WC) I don't think this sort of cleanup has a really high priority.
Certainly not in this patch.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[George Dunlap]

On 11/08/16 19:10, Andrew Cooper wrote:
> On 09/08/16 11:40, Jan Beulich wrote:
>> --- a/xen/arch/x86/cpu/mtrr/main.c
>> +++ b/xen/arch/x86/cpu/mtrr/main.c
>> @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
>>  if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
>>  printk(KERN_WARNING
>> "mtrr: your processor doesn't support 
>> write-combining\n");
>> -return -ENOSYS;
>> +return -EOPNOTSUPP;
> 
> Will this break the classic-xen MTRR code?  ISTR it was very picky, from
> the cpuid work.  Also, as some further cleanup, that printk should
> become a print-once.
> 
> The others look ok.

That does bring up a good general point though -- the return value is
part of the ABI.  Are you reasonably confident that none of the callers
will be confused when this return value changes?  If so, a note in the
commit message justifying this confidence would be helpful I think.

 -George


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] replace bogus -ENOSYS uses

[Andrew Cooper]

On 09/08/16 11:40, Jan Beulich wrote:
> --- a/xen/arch/x86/cpu/mtrr/main.c
> +++ b/xen/arch/x86/cpu/mtrr/main.c
> @@ -332,7 +332,7 @@ int mtrr_add_page(unsigned long base, un
>   if ((type == MTRR_TYPE_WRCOMB) && !have_wrcomb()) {
>   printk(KERN_WARNING
>  "mtrr: your processor doesn't support 
> write-combining\n");
> - return -ENOSYS;
> + return -EOPNOTSUPP;

Will this break the classic-xen MTRR code?  ISTR it was very picky, from
the cpuid work.  Also, as some further cleanup, that printk should
become a print-once.

The others look ok.

~Andrew


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel