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