[PATCH] xen/apic: implement io apic read with hypercall

[PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Ian Campbell]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Jan Beulich]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Ian Campbell]

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.

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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.

Re: [PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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.

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Jan Beulich]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Ian Campbell]

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.

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Konrad Rzeszutek Wilk]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Konrad Rzeszutek Wilk]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Konrad Rzeszutek Wilk]

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

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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;
}

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Konrad Rzeszutek Wilk]

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;
> }

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Lin Ming]

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.

Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall

[Konrad Rzeszutek Wilk]

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

Re: [PATCH] xen/apic: implement io apic read with hypercall

[Konrad Rzeszutek Wilk]

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