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
Andrew Cooper
2012-Apr-20 09:58 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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
Lin Ming
2012-Apr-20 11:13 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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
Ian Campbell
2012-Apr-20 12:38 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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
Jan Beulich
2012-Apr-20 12:52 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
>>> 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
Andrew Cooper
2012-Apr-20 12:53 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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
Ian Campbell
2012-Apr-20 13:12 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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.
Andrew Cooper
2012-Apr-20 13:20 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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
Lin Ming
2012-Apr-20 14:50 UTC
Re: [PATCH] xen/apic: implement io apic read with hypercall
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.
Jan Beulich
2012-Apr-20 14:59 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
>>> 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
Ian Campbell
2012-Apr-20 15:06 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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.
Lin Ming
2012-Apr-20 15:39 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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. > >
Konrad Rzeszutek Wilk
2012-Apr-20 16:41 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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
Konrad Rzeszutek Wilk
2012-Apr-20 16:43 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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. > > > > >
Lin Ming
2012-Apr-23 08:42 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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
Konrad Rzeszutek Wilk
2012-Apr-23 15:11 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
> >> > 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..
Lin Ming
2012-Apr-24 14:43 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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; }
Konrad Rzeszutek Wilk
2012-Apr-24 16:23 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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;> }
Lin Ming
2012-Apr-25 10:06 UTC
Re: [Xen-devel] [PATCH] xen/apic: implement io apic read with hypercall
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.
Konrad Rzeszutek Wilk
2012-Apr-26 15:33 UTC
Re: [PATCH] xen/apic: implement io apic read with hypercall
> >> > >> 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.