All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xen/apic: implement io apic read with hypercall
@ 2012-04-20  9:25 Lin Ming
  2012-04-20  9:58 ` [Xen-devel] " Andrew Cooper
  0 siblings, 1 reply; 22+ messages in thread
From: Lin Ming @ 2012-04-20  9:25 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk; +Cc: xen-devel, linux-kernel

Implements xen_io_apic_read with hypercall, so it returns proper IO-APIC
information instead of fabricated one.

Signed-off-by: Lin Ming <mlin@ss.pku.edu.cn>
---
 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 <linux/init.h>
 #include <asm/x86_init.h>
+#include <asm/apic.h>
+#include <xen/interface/physdev.h>
+#include <asm/xen/hypercall.h>
 
 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;
 }
 
 void __init xen_init_apic(void)
-- 
1.7.2.5




^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20  9:25 [PATCH] xen/apic: implement io apic read with hypercall Lin Ming
@ 2012-04-20  9:58 ` Andrew Cooper
  2012-04-20 11:13   ` Lin Ming
  0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2012-04-20  9:58 UTC (permalink / raw)
  To: Lin Ming; +Cc: Konrad Rzeszutek Wilk, xen-devel, linux-kernel

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 <mlin@ss.pku.edu.cn>
> ---
>  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 <linux/init.h>
>  #include <asm/x86_init.h>
> +#include <asm/apic.h>
> +#include <xen/interface/physdev.h>
> +#include <asm/xen/hypercall.h>
>  
>  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.

>  }
>  
>  void __init xen_init_apic(void)

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20  9:58 ` [Xen-devel] " Andrew Cooper
@ 2012-04-20 11:13   ` Lin Ming
  2012-04-20 12:38     ` Ian Campbell
  0 siblings, 1 reply; 22+ messages in thread
From: Lin Ming @ 2012-04-20 11:13 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Konrad Rzeszutek Wilk, xen-devel, linux-kernel

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 <mlin@ss.pku.edu.cn>
> > ---
> >  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 <linux/init.h>
> >  #include <asm/x86_init.h>
> > +#include <asm/apic.h>
> > +#include <xen/interface/physdev.h>
> > +#include <asm/xen/hypercall.h>
> >  
> >  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. 

unsigned int ret = apic_read(...);
if (ret == -1)
	//handle error.

Thanks,
Lin Ming



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 11:13   ` Lin Ming
@ 2012-04-20 12:38     ` Ian Campbell
  2012-04-20 12:52       ` Jan Beulich
                         ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-20 12:38 UTC (permalink / raw)
  To: Lin Ming; +Cc: Andrew Cooper, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

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 <mlin@ss.pku.edu.cn>
> > > ---
> > >  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 <linux/init.h>
> > >  #include <asm/x86_init.h>
> > > +#include <asm/apic.h>
> > > +#include <xen/interface/physdev.h>
> > > +#include <asm/xen/hypercall.h>
> > >  
> > >  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?

Under what circumstances can these hypercalls fail? Would a BUG_ON be
appropriate/

> 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



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 12:38     ` Ian Campbell
@ 2012-04-20 12:52       ` Jan Beulich
  2012-04-20 12:53       ` Andrew Cooper
  2012-04-20 16:41       ` Konrad Rzeszutek Wilk
  2 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2012-04-20 12:52 UTC (permalink / raw)
  To: Ian Campbell, Lin Ming
  Cc: Andrew Cooper, xen-devel, Konrad Rzeszutek Wilk, linux-kernel

>>> On 20.04.12 at 14:38, Ian Campbell <Ian.Campbell@citrix.com> 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 <mlin@ss.pku.edu.cn>
>> > > ---
>> > >  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 <linux/init.h>
>> > >  #include <asm/x86_init.h>
>> > > +#include <asm/apic.h>
>> > > +#include <xen/interface/physdev.h>
>> > > +#include <asm/xen/hypercall.h>
>> > >  
>> > >  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?
> 
> Under what circumstances can these hypercalls fail?

Only when the input is wrong (or it's not a privileged domain).

> Would a BUG_ON be appropriate/

Probably.

Jan


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 12:38     ` Ian Campbell
  2012-04-20 12:52       ` Jan Beulich
@ 2012-04-20 12:53       ` Andrew Cooper
  2012-04-20 13:12         ` Ian Campbell
  2012-04-20 16:41       ` Konrad Rzeszutek Wilk
  2 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2012-04-20 12:53 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Lin Ming, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

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 <mlin@ss.pku.edu.cn>
>>>> ---
>>>>  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 <linux/init.h>
>>>>  #include <asm/x86_init.h>
>>>> +#include <asm/apic.h>
>>>> +#include <xen/interface/physdev.h>
>>>> +#include <asm/xen/hypercall.h>
>>>>  
>>>>  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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 12:53       ` Andrew Cooper
@ 2012-04-20 13:12         ` Ian Campbell
  2012-04-20 13:20           ` Andrew Cooper
  2012-04-20 14:50             ` Lin Ming
  0 siblings, 2 replies; 22+ messages in thread
From: Ian Campbell @ 2012-04-20 13:12 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Lin Ming, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
> >
> > 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).

So either the guest has called a hypercall which it is not permitted to
or it has called it with invalid parameters of one sort or another. Both
of these would be a code bug in the guest and therefore asserting that
no failure occurred is reasonable?

What could the caller do with the error other than log it and collapse?

> 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.

I think you have successfully argued that it is ;-)

Ian.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 13:12         ` Ian Campbell
@ 2012-04-20 13:20           ` Andrew Cooper
  2012-04-20 14:50             ` Lin Ming
  1 sibling, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2012-04-20 13:20 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Lin Ming, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

On 20/04/12 14:12, Ian Campbell wrote:
> On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
>>> 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).
> So either the guest has called a hypercall which it is not permitted to
> or it has called it with invalid parameters of one sort or another. Both
> of these would be a code bug in the guest and therefore asserting that
> no failure occurred is reasonable?
>
> What could the caller do with the error other than log it and collapse?
>
>> 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.
> I think you have successfully argued that it is ;-)
>
> Ian.
>

So I have.  -EMoreCoffee

Yes - I meant to say that BUG_ON() was ok for domU.

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 13:12         ` Ian Campbell
@ 2012-04-20 14:50             ` Lin Ming
  2012-04-20 14:50             ` Lin Ming
  1 sibling, 0 replies; 22+ messages in thread
From: Lin Ming @ 2012-04-20 14:50 UTC (permalink / raw)
  To: Ian Campbell
  Cc: Andrew Cooper, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

On Fri, Apr 20, 2012 at 9:12 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
>> >
>> > 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).
>
> So either the guest has called a hypercall which it is not permitted to
> or it has called it with invalid parameters of one sort or another. Both
> of these would be a code bug in the guest and therefore asserting that
> no failure occurred is reasonable?
>
> What could the caller do with the error other than log it and collapse?
>
>> 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.
>
> I think you have successfully argued that it is ;-)

BUG_ON is too severe. How about WARN_ON?

ret = hypercall(...)

if (ret) {
   WARN_ON(1);
   return -1;
}


>
> Ian.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] xen/apic: implement io apic read with hypercall
@ 2012-04-20 14:50             ` Lin Ming
  0 siblings, 0 replies; 22+ messages in thread
From: Lin Ming @ 2012-04-20 14:50 UTC (permalink / raw)
  To: Ian Campbell
  Cc: Andrew Cooper, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

On Fri, Apr 20, 2012 at 9:12 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
>> >
>> > 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).
>
> So either the guest has called a hypercall which it is not permitted to
> or it has called it with invalid parameters of one sort or another. Both
> of these would be a code bug in the guest and therefore asserting that
> no failure occurred is reasonable?
>
> What could the caller do with the error other than log it and collapse?
>
>> 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.
>
> I think you have successfully argued that it is ;-)

BUG_ON is too severe. How about WARN_ON?

ret = hypercall(...)

if (ret) {
   WARN_ON(1);
   return -1;
}


>
> Ian.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 14:50             ` Lin Ming
  (?)
@ 2012-04-20 14:59             ` Jan Beulich
  -1 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2012-04-20 14:59 UTC (permalink / raw)
  To: Lin Ming
  Cc: Andrew Cooper, Ian Campbell, xen-devel, Konrad Rzeszutek Wilk,
	linux-kernel

>>> On 20.04.12 at 16:50, Lin Ming <mlin@ss.pku.edu.cn> wrote:
> On Fri, Apr 20, 2012 at 9:12 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
>>> >
>>> > 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).
>>
>> So either the guest has called a hypercall which it is not permitted to
>> or it has called it with invalid parameters of one sort or another. Both
>> of these would be a code bug in the guest and therefore asserting that
>> no failure occurred is reasonable?
>>
>> What could the caller do with the error other than log it and collapse?
>>
>>> 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.
>>
>> I think you have successfully argued that it is ;-)
> 
> BUG_ON is too severe. How about WARN_ON?
> 
> ret = hypercall(...)
> 
> if (ret) {
>    WARN_ON(1);
>    return -1;
> }

But if you go with this, please use the more modern

if (WARN_ON(ret))
	return -1;

Jan


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 14:50             ` Lin Ming
  (?)
  (?)
@ 2012-04-20 15:06             ` Ian Campbell
  2012-04-20 15:39               ` Lin Ming
  -1 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2012-04-20 15:06 UTC (permalink / raw)
  To: Lin Ming; +Cc: Andrew Cooper, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

On Fri, 2012-04-20 at 15:50 +0100, Lin Ming wrote:
> On Fri, Apr 20, 2012 at 9:12 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
> >> >
> >> > 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).
> >
> > So either the guest has called a hypercall which it is not permitted to
> > or it has called it with invalid parameters of one sort or another. Both
> > of these would be a code bug in the guest and therefore asserting that
> > no failure occurred is reasonable?
> >
> > What could the caller do with the error other than log it and collapse?
> >
> >> 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.
> >
> > I think you have successfully argued that it is ;-)
> 
> BUG_ON is too severe.

Why? Under what circumstances can this be correctly called in a way
which would result in the hypercall failing?

>  How about WARN_ON?
> 
> ret = hypercall(...)
> 
> if (ret) {
>    WARN_ON(1);
>    return -1;
> }
> 
> 
> >
> > Ian.



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 15:06             ` Ian Campbell
@ 2012-04-20 15:39               ` Lin Ming
  2012-04-20 16:43                 ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 22+ messages in thread
From: Lin Ming @ 2012-04-20 15:39 UTC (permalink / raw)
  To: Ian Campbell
  Cc: Andrew Cooper, xen-devel, linux-kernel, Konrad Rzeszutek Wilk

On Fri, 2012-04-20 at 16:06 +0100, Ian Campbell wrote:
> On Fri, 2012-04-20 at 15:50 +0100, Lin Ming wrote:
> > On Fri, Apr 20, 2012 at 9:12 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
> > >> >
> > >> > 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).
> > >
> > > So either the guest has called a hypercall which it is not permitted to
> > > or it has called it with invalid parameters of one sort or another. Both
> > > of these would be a code bug in the guest and therefore asserting that
> > > no failure occurred is reasonable?
> > >
> > > What could the caller do with the error other than log it and collapse?
> > >
> > >> The call into Xen itself will return 0 as a value if an
> > >> invalid physbase is passed in the hypercall.

Just checked ioapic_guest_read.
It will return -EINVAL if an invalid physbase is passed in.

> > >
> > >> So a BUG_ON() is not safe/sensible for domU.
> > >
> > > I think you have successfully argued that it is ;-)
> > 
> > BUG_ON is too severe.
> 
> Why? Under what circumstances can this be correctly called in a way
> which would result in the hypercall failing?

Is BUG_ON() reasonable if invalid physbase passed in?

> 
> >  How about WARN_ON?
> > 
> > ret = hypercall(...)
> > 
> > if (ret) {
> >    WARN_ON(1);
> >    return -1;
> > }
> > 
> > 
> > >
> > > Ian.
> 
> 



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 12:38     ` Ian Campbell
  2012-04-20 12:52       ` Jan Beulich
  2012-04-20 12:53       ` Andrew Cooper
@ 2012-04-20 16:41       ` Konrad Rzeszutek Wilk
  2012-04-23  8:42         ` Lin Ming
  2 siblings, 1 reply; 22+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-04-20 16:41 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Lin Ming, Andrew Cooper, xen-devel, linux-kernel

On Fri, Apr 20, 2012 at 01:38:28PM +0100, 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 <mlin@ss.pku.edu.cn>
> > > > ---
> > > >  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 <linux/init.h>
> > > >  #include <asm/x86_init.h>
> > > > +#include <asm/apic.h>
> > > > +#include <xen/interface/physdev.h>
> > > > +#include <asm/xen/hypercall.h>
> > > >  
> > > >  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?

Yeah, but then we are back to crashing at bootup (with dom0) :-)

Perhaps the fallback is to emulate (so retain some of the original code)
as we have been since .. uh 3.0?

> 
> Under what circumstances can these hypercalls fail? Would a BUG_ON be
> appropriate/
> 
> > 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
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 15:39               ` Lin Ming
@ 2012-04-20 16:43                 ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 22+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-04-20 16:43 UTC (permalink / raw)
  To: Lin Ming; +Cc: Ian Campbell, Andrew Cooper, xen-devel, linux-kernel

On Fri, Apr 20, 2012 at 11:39:24PM +0800, Lin Ming wrote:
> On Fri, 2012-04-20 at 16:06 +0100, Ian Campbell wrote:
> > On Fri, 2012-04-20 at 15:50 +0100, Lin Ming wrote:
> > > On Fri, Apr 20, 2012 at 9:12 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > > On Fri, 2012-04-20 at 13:53 +0100, Andrew Cooper wrote:
> > > >> >
> > > >> > 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).
> > > >
> > > > So either the guest has called a hypercall which it is not permitted to
> > > > or it has called it with invalid parameters of one sort or another. Both
> > > > of these would be a code bug in the guest and therefore asserting that
> > > > no failure occurred is reasonable?
> > > >
> > > > What could the caller do with the error other than log it and collapse?
> > > >
> > > >> The call into Xen itself will return 0 as a value if an
> > > >> invalid physbase is passed in the hypercall.
> 
> Just checked ioapic_guest_read.
> It will return -EINVAL if an invalid physbase is passed in.
> 
> > > >
> > > >> So a BUG_ON() is not safe/sensible for domU.
> > > >
> > > > I think you have successfully argued that it is ;-)
> > > 
> > > BUG_ON is too severe.
> > 
> > Why? Under what circumstances can this be correctly called in a way
> > which would result in the hypercall failing?
> 
> Is BUG_ON() reasonable if invalid physbase passed in?

Just emulate the values in the error case. We don't _need_ them per say - except
to emulate some sensible values.

> 
> > 
> > >  How about WARN_ON?
> > > 
> > > ret = hypercall(...)
> > > 
> > > if (ret) {
> > >    WARN_ON(1);
> > >    return -1;
> > > }
> > > 
> > > 
> > > >
> > > > Ian.
> > 
> > 
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-20 16:41       ` Konrad Rzeszutek Wilk
@ 2012-04-23  8:42         ` Lin Ming
  2012-04-23 15:11           ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 22+ messages in thread
From: Lin Ming @ 2012-04-23  8:42 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Ian Campbell, Andrew Cooper, xen-devel, linux-kernel

On Sat, Apr 21, 2012 at 12:41 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Fri, Apr 20, 2012 at 01:38:28PM +0100, 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 <mlin@ss.pku.edu.cn>
>> > > > ---
>> > > >  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 <linux/init.h>
>> > > >  #include <asm/x86_init.h>
>> > > > +#include <asm/apic.h>
>> > > > +#include <xen/interface/physdev.h>
>> > > > +#include <asm/xen/hypercall.h>
>> > > >
>> > > >  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?
>
> Yeah, but then we are back to crashing at bootup (with dom0) :-)
>
> Perhaps the fallback is to emulate (so retain some of the original code)
> as we have been since .. uh 3.0?

Do you mean the return value of io_apic_read in 3.0?
It's 0xffffffff.

Lin Ming

>
>>
>> Under what circumstances can these hypercalls fail? Would a BUG_ON be
>> appropriate/
>>
>> > unsigned int ret = apic_read(...);
>> > if (ret == -1)
>> >     //handle error.
>> >
>> > Thanks,
>> > Lin Ming

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-23  8:42         ` Lin Ming
@ 2012-04-23 15:11           ` Konrad Rzeszutek Wilk
  2012-04-24 14:43             ` Lin Ming
  0 siblings, 1 reply; 22+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-04-23 15:11 UTC (permalink / raw)
  To: Lin Ming; +Cc: Ian Campbell, Andrew Cooper, xen-devel, linux-kernel

> >> > 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?
> >
> > Yeah, but then we are back to crashing at bootup (with dom0) :-)
> >
> > Perhaps the fallback is to emulate (so retain some of the original code)
> > as we have been since .. uh 3.0?
> 
> Do you mean the return value of io_apic_read in 3.0?

No. I meant that we would continue to emulate. The improvement
is that now we do:

      if (reg == 0x1)
              return 0x00170020;
      else if (reg == 0x0)
              return apic << 24;

instead of 0xfdfdfdfd.

> It's 0xffffffff.

Now it is 0xfdfdfdfd.

I am suggesting that instead of BUG_ON(), we fallback to do returning
an emulatated IO_APIC values - like the ones that this original patch
cooked up..

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-23 15:11           ` Konrad Rzeszutek Wilk
@ 2012-04-24 14:43             ` Lin Ming
  2012-04-24 16:23               ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 22+ messages in thread
From: Lin Ming @ 2012-04-24 14:43 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Ian Campbell, Andrew Cooper, xen-devel, linux-kernel

On Mon, Apr 23, 2012 at 11:11 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
>> >> > 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?
>> >
>> > Yeah, but then we are back to crashing at bootup (with dom0) :-)
>> >
>> > Perhaps the fallback is to emulate (so retain some of the original code)
>> > as we have been since .. uh 3.0?
>>
>> Do you mean the return value of io_apic_read in 3.0?
>
> No. I meant that we would continue to emulate. The improvement
> is that now we do:
>
>       if (reg == 0x1)
>               return 0x00170020;
>       else if (reg == 0x0)
>               return apic << 24;
>
> instead of 0xfdfdfdfd.
>
>> It's 0xffffffff.
>
> Now it is 0xfdfdfdfd.
>
> I am suggesting that instead of BUG_ON(), we fallback to do returning
> an emulatated IO_APIC values - like the ones that this original patch
> cooked up..

But we still need to return some value if the register is not emulated.

How about below?

unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
{
        struct physdev_apic apic_op;
        int ret;

        apic_op.apic_physbase = mpc_ioapic_addr(apic);
        apic_op.reg = reg;
        ret = HYPERVISOR_physdev_op(PHYSDEVOP_apic_read, &apic_op);
        if (!ret)
                return apic_op.value;

        /* emulate register */
        if (reg == 0x1)
                return 0x00170020;
        else if (reg == 0x0)
                return apic << 24;
        else
                return -1;
}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-24 14:43             ` Lin Ming
@ 2012-04-24 16:23               ` Konrad Rzeszutek Wilk
  2012-04-25 10:06                 ` Lin Ming
  0 siblings, 1 reply; 22+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-04-24 16:23 UTC (permalink / raw)
  To: Lin Ming; +Cc: Ian Campbell, Andrew Cooper, xen-devel, linux-kernel

On Tue, Apr 24, 2012 at 10:43:53PM +0800, Lin Ming wrote:
> On Mon, Apr 23, 2012 at 11:11 PM, Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com> wrote:
> >> >> > 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?
> >> >
> >> > Yeah, but then we are back to crashing at bootup (with dom0) :-)
> >> >
> >> > Perhaps the fallback is to emulate (so retain some of the original code)
> >> > as we have been since .. uh 3.0?
> >>
> >> Do you mean the return value of io_apic_read in 3.0?
> >
> > No. I meant that we would continue to emulate. The improvement
> > is that now we do:
> >
> >       if (reg == 0x1)
> >               return 0x00170020;
> >       else if (reg == 0x0)
> >               return apic << 24;
> >
> > instead of 0xfdfdfdfd.
> >
> >> It's 0xffffffff.
> >
> > Now it is 0xfdfdfdfd.
> >
> > I am suggesting that instead of BUG_ON(), we fallback to do returning
> > an emulatated IO_APIC values - like the ones that this original patch
> > cooked up..
> 
> But we still need to return some value if the register is not emulated.

Right. 0xfd;
> 
> How about below?


Almost perfect.
> 
> unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
> {
>         struct physdev_apic apic_op;
>         int ret;
> 
>         apic_op.apic_physbase = mpc_ioapic_addr(apic);
>         apic_op.reg = reg;
>         ret = HYPERVISOR_physdev_op(PHYSDEVOP_apic_read, &apic_op);
>         if (!ret)
>                 return apic_op.value;
> 
>         /* emulate register */
>         if (reg == 0x1)
>                 return 0x00170020;
>         else if (reg == 0x0)
>                 return apic << 24;
>         else
>                 return -1;

	return 0xfd;
> }

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-24 16:23               ` Konrad Rzeszutek Wilk
@ 2012-04-25 10:06                 ` Lin Ming
  2012-04-26 15:33                     ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 22+ messages in thread
From: Lin Ming @ 2012-04-25 10:06 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: Ian Campbell, Andrew Cooper, xen-devel, linux-kernel

On Wed, Apr 25, 2012 at 12:23 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Tue, Apr 24, 2012 at 10:43:53PM +0800, Lin Ming wrote:
>> On Mon, Apr 23, 2012 at 11:11 PM, Konrad Rzeszutek Wilk
>> <konrad.wilk@oracle.com> wrote:
>> >> >> > 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?
>> >> >
>> >> > Yeah, but then we are back to crashing at bootup (with dom0) :-)
>> >> >
>> >> > Perhaps the fallback is to emulate (so retain some of the original code)
>> >> > as we have been since .. uh 3.0?
>> >>
>> >> Do you mean the return value of io_apic_read in 3.0?
>> >
>> > No. I meant that we would continue to emulate. The improvement
>> > is that now we do:
>> >
>> >       if (reg == 0x1)
>> >               return 0x00170020;
>> >       else if (reg == 0x0)
>> >               return apic << 24;
>> >
>> > instead of 0xfdfdfdfd.
>> >
>> >> It's 0xffffffff.
>> >
>> > Now it is 0xfdfdfdfd.
>> >
>> > I am suggesting that instead of BUG_ON(), we fallback to do returning
>> > an emulatated IO_APIC values - like the ones that this original patch
>> > cooked up..
>>
>> But we still need to return some value if the register is not emulated.
>
> Right. 0xfd;
>>
>> How about below?
>
>
> Almost perfect.
>>
>> unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
>> {
>>         struct physdev_apic apic_op;
>>         int ret;
>>
>>         apic_op.apic_physbase = mpc_ioapic_addr(apic);
>>         apic_op.reg = reg;
>>         ret = HYPERVISOR_physdev_op(PHYSDEVOP_apic_read, &apic_op);
>>         if (!ret)
>>                 return apic_op.value;
>>
>>         /* emulate register */
>>         if (reg == 0x1)
>>                 return 0x00170020;
>>         else if (reg == 0x0)
>>                 return apic << 24;
>>         else
>>                 return -1;
>
>        return 0xfd;

Where does this magic number 0xfd come from?

Both native_io_apic_read and xen_io_apic_read does not return 0xfd on error.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
  2012-04-25 10:06                 ` Lin Ming
@ 2012-04-26 15:33                     ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 22+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-04-26 15:33 UTC (permalink / raw)
  To: Lin Ming; +Cc: Andrew Cooper, xen-devel, Ian Campbell, linux-kernel

> >>
> >> unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
> >> {
> >>         struct physdev_apic apic_op;
> >>         int ret;
> >>
> >>         apic_op.apic_physbase = mpc_ioapic_addr(apic);
> >>         apic_op.reg = reg;
> >>         ret = HYPERVISOR_physdev_op(PHYSDEVOP_apic_read, &apic_op);
> >>         if (!ret)
> >>                 return apic_op.value;
> >>
> >>         /* emulate register */
> >>         if (reg == 0x1)
> >>                 return 0x00170020;
> >>         else if (reg == 0x0)
> >>                 return apic << 24;
> >>         else
> >>                 return -1;
> >
> >        return 0xfd;
> 
> Where does this magic number 0xfd come from?
> 
> Both native_io_apic_read and xen_io_apic_read does not return 0xfd on error.

That is correct. But that is what it should have been. Suresh
pointed that out sometime and I managed to lose that part in one of the
commits. The earlier patch of this version did that.

Thought thinking about it this some I am not sure if 0xff is a better
choice... In the end it probably does not matter the slighest.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH] xen/apic: implement io apic read with hypercall
@ 2012-04-26 15:33                     ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 22+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-04-26 15:33 UTC (permalink / raw)
  To: Lin Ming; +Cc: Andrew Cooper, xen-devel, Ian Campbell, linux-kernel

> >>
> >> unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
> >> {
> >>         struct physdev_apic apic_op;
> >>         int ret;
> >>
> >>         apic_op.apic_physbase = mpc_ioapic_addr(apic);
> >>         apic_op.reg = reg;
> >>         ret = HYPERVISOR_physdev_op(PHYSDEVOP_apic_read, &apic_op);
> >>         if (!ret)
> >>                 return apic_op.value;
> >>
> >>         /* emulate register */
> >>         if (reg == 0x1)
> >>                 return 0x00170020;
> >>         else if (reg == 0x0)
> >>                 return apic << 24;
> >>         else
> >>                 return -1;
> >
> >        return 0xfd;
> 
> Where does this magic number 0xfd come from?
> 
> Both native_io_apic_read and xen_io_apic_read does not return 0xfd on error.

That is correct. But that is what it should have been. Suresh
pointed that out sometime and I managed to lose that part in one of the
commits. The earlier patch of this version did that.

Thought thinking about it this some I am not sure if 0xff is a better
choice... In the end it probably does not matter the slighest.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2012-04-26 15:39 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-20  9:25 [PATCH] xen/apic: implement io apic read with hypercall Lin Ming
2012-04-20  9:58 ` [Xen-devel] " Andrew Cooper
2012-04-20 11:13   ` Lin Ming
2012-04-20 12:38     ` Ian Campbell
2012-04-20 12:52       ` Jan Beulich
2012-04-20 12:53       ` Andrew Cooper
2012-04-20 13:12         ` Ian Campbell
2012-04-20 13:20           ` Andrew Cooper
2012-04-20 14:50           ` Lin Ming
2012-04-20 14:50             ` Lin Ming
2012-04-20 14:59             ` [Xen-devel] " Jan Beulich
2012-04-20 15:06             ` Ian Campbell
2012-04-20 15:39               ` Lin Ming
2012-04-20 16:43                 ` Konrad Rzeszutek Wilk
2012-04-20 16:41       ` Konrad Rzeszutek Wilk
2012-04-23  8:42         ` Lin Ming
2012-04-23 15:11           ` Konrad Rzeszutek Wilk
2012-04-24 14:43             ` Lin Ming
2012-04-24 16:23               ` Konrad Rzeszutek Wilk
2012-04-25 10:06                 ` Lin Ming
2012-04-26 15:33                   ` Konrad Rzeszutek Wilk
2012-04-26 15:33                     ` Konrad Rzeszutek Wilk

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.