From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall Date: Fri, 20 Apr 2012 13:53:23 +0100 Message-ID: <4F915C43.4020207@citrix.com> References: <1334913957.2863.1.camel@hp6530s> <4F913340.4000202@citrix.com> <1334920396.2863.16.camel@hp6530s> <1334925508.28331.63.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1334925508.28331.63.camel@zakaz.uk.xensource.com> Sender: linux-kernel-owner@vger.kernel.org To: Ian Campbell Cc: Lin Ming , "xen-devel@lists.xensource.com" , "linux-kernel@vger.kernel.org" , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On 20/04/12 13:38, Ian Campbell wrote: > On Fri, 2012-04-20 at 12:13 +0100, Lin Ming wrote: >> On Fri, 2012-04-20 at 10:58 +0100, Andrew Cooper wrote: >>> On 20/04/12 10:25, Lin Ming wrote: >>>> Implements xen_io_apic_read with hypercall, so it returns proper IO-APIC >>>> information instead of fabricated one. >>>> >>>> Signed-off-by: Lin Ming >>>> --- >>>> arch/x86/xen/apic.c | 16 +++++++++++----- >>>> 1 files changed, 11 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c >>>> index aee16ab..f1f392d 100644 >>>> --- a/arch/x86/xen/apic.c >>>> +++ b/arch/x86/xen/apic.c >>>> @@ -1,14 +1,20 @@ >>>> #include >>>> #include >>>> +#include >>>> +#include >>>> +#include >>>> >>>> unsigned int xen_io_apic_read(unsigned apic, unsigned reg) >>>> { >>>> - if (reg == 0x1) >>>> - return 0x00170020; >>>> - else if (reg == 0x0) >>>> - return apic << 24; >>>> + struct physdev_apic apic_op; >>>> + int ret; >>>> >>>> - return 0xff; >>>> + apic_op.apic_physbase = mpc_ioapic_addr(apic); >>>> + apic_op.reg = reg; >>>> + ret = HYPERVISOR_physdev_op(PHYSDEVOP_apic_read, &apic_op); >>>> + if (ret) >>>> + return ret; >>>> + return apic_op.value; >>> Hypercall ret errors are negative, yet this function is unsigned. Given >>> that the previous function had no possible way to fail, perhaps on error >>> you should fake up the values as before. >> How about return -1 on error? >> The calling function can check -1 for error. > Isn't -1 potentially (at least theoretically) a valid value to read from > one of these registers? Theoretically yes, but practically it would only be buggy hardware returning -1. > > Under what circumstances can these hypercalls fail? Would a BUG_ON be > appropriate/ -EFAULT, -EPERM, anything xsm_apic() could return (which looks only to be -EPERM). The call into Xen itself will return 0 as a value if an invalid physbase is passed in the hypercall. So a BUG_ON() is not safe/sensible for domU. > >> unsigned int ret = apic_read(...); >> if (ret == -1) >> //handle error. >> >> Thanks, >> Lin Ming >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com