xen/apic: implement io apic read with hypercall
Subsystems (2)
PATCH 1/1 xen/apic: implement io apic read with hypercall
Implements xen_io_apic_read with hypercall, so it returns proper IO-APIC information instead of fabricated one.
--- arch/x86/xen/apic.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)
arch/x86/xen/apic.c +11 -5
@@ -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 )
Signature · 1 line
1.7.2.5
Quoted 25 lines · Lin Ming
Implements
xen_io_apic_readwith hypercall, so it returns proper IO-APIC information instead of fabricated one.Signed-off-by Lin Ming <[email protected]>--- arch/x86/xen/apic.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)arch/x86/xen/apic.c +3
-4@@ -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 ;
arch/x86/xen/apic.c line 17
Quoted 10 lines
arch/x86/xen/apic.c +8
-1+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.
Quoted 3 lines
arch/x86/xen/apic.c +0
-0}void __init xen_init_apic (void )
Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
Quoted 29 lines · Andrew Cooper
Quoted 28 lines · Lin Ming
Implements
xen_io_apic_readwith hypercall, so it returns proper IO-APIC information instead of fabricated one.Signed-off-by Lin Ming <[email protected]>--- arch/x86/xen/apic.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)arch/x86/xen/apic.c +5
-4@@ -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;
Quoted 11 lines
arch/x86/xen/apic.c line 17
Quoted 7 lines
arch/x86/xen/apic.c +6
-1-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
Quoted 42 lines · Lin Ming
Quoted 29 lines · Andrew Cooper
Quoted 28 lines · Lin Ming
Implements
xen_io_apic_readwith hypercall, so it returns proper IO-APIC information instead of fabricated one.Signed-off-by Lin Ming <[email protected]>--- arch/x86/xen/apic.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)arch/x86/xen/apic.c +5
-4@@ -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;Quoted 11 lines
arch/x86/xen/apic.c line 17
Quoted 7 lines
arch/x86/xen/apic.c +6
-1-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.
Quoted 2 lines
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/
Quoted 13 lines
unsigned int ret =
apic_read(...);
if (ret == -1)//handle error.Thanks,
Lin Ming_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Quoted 44 lines
Quoted 1 line
Quoted 1 line · Ian Campbell
Quoted 42 lines · Lin Ming
Quoted 40 lines · Andrew Cooper
Quoted 25 lines · Lin Ming
Implements
xen_io_apic_readwith hypercall, so it returns proper IO-APIC information instead of fabricated one.Signed-off-by Lin Ming <[email protected]>--- arch/x86/xen/apic.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)arch/x86/xen/apic.c +3
-4@@ -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 ;arch/x86/xen/apic.c line 17
Quoted 10 lines
arch/x86/xen/apic.c +8
-1+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.
Quoted 7 lines
Quoted 2 lines
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).
Quoted 1 line
Would a
BUG_ONbe appropriate/
Probably.
Jan
Quoted 41 lines · Ian Campbell
Quoted 40 lines · Lin Ming
Quoted 39 lines · Andrew Cooper
Quoted 25 lines · Lin Ming
Implements
xen_io_apic_readwith hypercall, so it returns proper IO-APIC information instead of fabricated one.Signed-off-by Lin Ming <[email protected]>--- arch/x86/xen/apic.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)arch/x86/xen/apic.c +3
-4@@ -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 ;arch/x86/xen/apic.c line 17
Quoted 10 lines
arch/x86/xen/apic.c +8
-1+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.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 3 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe 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.
Quoted 15 lines
Quoted 14 lines
unsigned int ret =
apic_read(...);
if (ret == -1)//handle error.Thanks,
Lin Ming_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
Quoted 6 lines · Andrew Cooper
Quoted 3 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/
-EFAULT,-EPERM, anythingxsm_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?
Quoted 4 lines
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.
Quoted 10 lines · Ian Campbell
Quoted 4 lines · Andrew Cooper
Quoted 2 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/
-EFAULT,-EPERM, anythingxsm_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?
Quoted 9 lines
What could the caller do with the error other than log it and collapse?
Quoted 3 lines
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.
Signature · 2 lines
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com
Quoted 13 lines · Ian Campbell
Quoted 6 lines · Andrew Cooper
Quoted 3 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/
-EFAULT,-EPERM, anythingxsm_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?
Quoted 8 lines
What could the caller do with the error other than log it and collapse?
Quoted 4 lines
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;
}
Quoted 2 lines
Ian.
Quoted 24 lines
Quoted 1 line
Quoted 1 line · Lin Ming
Quoted 13 lines · Ian Campbell
Quoted 6 lines · Andrew Cooper
Quoted 3 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/
-EFAULT,-EPERM, anythingxsm_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?
Quoted 8 lines
What could the caller do with the error other than log it and collapse?
Quoted 4 lines
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 ;-)
Quoted 8 lines
BUG_ONis too severe. How aboutWARN_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
Quoted 16 lines · Lin Ming
Quoted 15 lines · Ian Campbell
Quoted 6 lines · Andrew Cooper
Quoted 3 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/
-EFAULT,-EPERM, anythingxsm_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?
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
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_ONis too severe.
Why? Under what circumstances can this be correctly called in a way which would result in the hypercall failing?
Quoted 12 lines
How about
WARN_ON?ret = hypercall(...)
if (ret) { WARN_ON(1); return -1; }Quoted 2 lines
Ian.
Quoted 15 lines · Ian Campbell
Quoted 14 lines · Lin Ming
Quoted 13 lines · Ian Campbell
Quoted 6 lines · Andrew Cooper
Quoted 3 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/
-EFAULT,-EPERM, anythingxsm_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?
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
What could the caller do with the error other than log it and collapse?
Quoted 2 lines
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.
Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
So a
BUG_ON()is not safe/sensible for domU.I think you have successfully argued that it is ;-)
BUG_ONis 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?
Quoted 15 lines
Quoted 13 lines
How about
WARN_ON?ret = hypercall(...)
if (ret) { WARN_ON(1); return -1; }Quoted 2 lines
Ian.
Quoted 27 lines · Lin Ming
Quoted 15 lines · Ian Campbell
Quoted 14 lines · Lin Ming
Quoted 13 lines · Ian Campbell
Quoted 6 lines · Andrew Cooper
Quoted 3 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/
-EFAULT,-EPERM, anythingxsm_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?
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
What could the caller do with the error other than log it and collapse?
Quoted 2 lines
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-EINVALif an invalid physbase is passed in.Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
So a
BUG_ON()is not safe/sensible for domU.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
I think you have successfully argued that it is ;-)
BUG_ONis 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.
Quoted 17 lines
Quoted 16 lines
Quoted 14 lines
How about
WARN_ON?ret = hypercall(...)
if (ret) { WARN_ON(1); return -1; }Quoted 2 lines
Ian.
Quoted 43 lines · Ian Campbell
Quoted 42 lines · Lin Ming
Quoted 40 lines · Andrew Cooper
Quoted 25 lines · Lin Ming
Implements
xen_io_apic_readwith hypercall, so it returns proper IO-APIC information instead of fabricated one.Signed-off-by Lin Ming <[email protected]>--- arch/x86/xen/apic.c | 16 +++++++++++----- 1 files changed, 11 insertions(+), 5 deletions(-)arch/x86/xen/apic.c +3
-4@@ -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 ;arch/x86/xen/apic.c line 17
Quoted 10 lines
arch/x86/xen/apic.c +8
-1+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.
Quoted 5 lines
Quoted 2 lines
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?
Quoted 24 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/Quoted 9 lines
unsigned int ret =
apic_read(...);
if (ret == -1)//handle error.Thanks,
Lin MingQuoted 4 lines
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
Quoted 47 lines · Konrad Rzeszutek Wilk
Quoted 46 lines · Ian Campbell
Quoted 44 lines · Lin Ming
Quoted 29 lines · Andrew Cooper
Quoted 28 lines · Lin Ming
Implements
xen_io_apic_readwith hypercall, so it returns proper IO-APIC information instead of fabricated one.Signed-off-by Lin Ming <[email protected]>---
arch/x86/xen/apic.c | 16 +++++++++++-----1 files changed, 11 insertions(+), 5 deletions(-)arch/x86/xen/apic.c +5
-4@@ -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;Quoted 11 lines
arch/x86/xen/apic.c line 17
Quoted 7 lines
arch/x86/xen/apic.c +6
-1- 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.
Quoted 7 lines
Quoted 2 lines
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
Quoted 11 lines
Quoted 11 lines
Under what circumstances can these hypercalls fail? Would a
BUG_ONbe appropriate/Quoted 6 lines
unsigned int ret =
apic_read(...);
if (ret == -1)
//handle error.Thanks,
Lin Ming
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 6 lines
Quoted 4 lines
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_readin 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.
Quoted 1 line
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..
Quoted 23 lines · Konrad Rzeszutek Wilk
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 6 lines
Quoted 4 lines
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_readin 3.0?No. I meant that we would continue to emulate. The improvement is that now we do:
if (reg ==
0x1)
return0x00170020;
else if (reg ==0x0)
return apic << 24;instead of
0xfdfdfdfd.
Quoted 7 lines
Quoted 1 line
It's
0xffffffff.Now it is
0xfdfdfdfd.I am suggesting that instead of
BUG_ON(), we fallback to do returning an emulatatedIO_APICvalues - 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;
}Quoted 27 lines · Lin Ming
Quoted 25 lines · Konrad Rzeszutek Wilk
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 6 lines
Quoted 4 lines
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_readin 3.0?No. I meant that we would continue to emulate. The improvement is that now we do:
if (reg ==
0x1)
return0x00170020;
else if (reg ==0x0)
return apic << 24;instead of
0xfdfdfdfd.Quoted 1 line
It's
0xffffffff.
Quoted 7 lines
Quoted 5 lines
Now it is
0xfdfdfdfd.I am suggesting that instead of
BUG_ON(), we fallback to do returning an emulatatedIO_APICvalues - 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;
Quoted 2 lines
How about below?
Almost perfect.
Quoted 12 lines
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;
Quoted 7 lines
/* emulate register */
if (reg == 0x1)
return 0x00170020;
else if (reg == 0x0)
return apic << 24;
else
return -1;return 0xfd;
Quoted 1 line
}
Quoted 62 lines · Konrad Rzeszutek Wilk
Quoted 27 lines · Lin Ming
Quoted 25 lines · Konrad Rzeszutek Wilk
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 6 lines
Quoted 4 lines
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_readin 3.0?No. I meant that we would continue to emulate. The improvement is that now we do:
if (reg ==
0x1)
return0x00170020;
else if (reg ==0x0)
return apic << 24;instead of
0xfdfdfdfd.Quoted 1 line
It's
0xffffffff.Quoted 7 lines
Quoted 5 lines
Now it is
0xfdfdfdfd.I am suggesting that instead of
BUG_ON(), we fallback to do returning an emulatatedIO_APICvalues - 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;Quoted 2 lines
How about below?
Almost perfect.
Quoted 12 lines
unsigned int
xen_io_apic_read(unsigned apic, unsigned reg){
structphysdev_apicapic_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;Quoted 7 lines
/* emulate register */
if (reg ==0x1)
return0x00170020;
else if (reg ==0x0)
return apic << 24;
else
return -1;
Quoted 1 line
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.
Quoted 20 lines
Quoted 20 lines
Quoted 19 lines
unsigned int
xen_io_apic_read(unsigned apic, unsigned reg){
structphysdev_apicapic_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)
return0x00170020;
else if (reg ==0x0)
return apic << 24;
else
return -1;
Quoted 5 lines
Quoted 1 line
return
0xfd;Where does this magic number
0xfdcome from?Both
native_io_apic_readandxen_io_apic_readdoes not return0xfdon 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.