Re: CVS commit: src/sys/arch/xen/xen
> Module Name:src > Committed By: jdolecek > Date: Fri Apr 10 18:03:06 UTC 2020 > > Modified Files: > src/sys/arch/xen/xen: if_xennet_xenbus.c > > Log Message: > convert to bus_dma(9), remove now not necessary XENPVHVM redefines > [...] > + KASSERT(req->rxreq_gntref = GRANT_INVALID_REF); I don't think this does quite what you meant it to do! I can fix it by adding the missing `=', but it may cause the kassert to fire because I don't see anywhere in xennet_rx_free_req that sets it to GRANT_INVALID_REF and I don't know the surrounding logic well enough to be confident that just changing it there is correct.
Re: CVS commit: src/sys/arch/xen/xen
On Mon, Apr 13, 2020 at 08:09:13PM +, Jaromir Dolecek wrote: > Module Name: src > Committed By: jdolecek > Date: Mon Apr 13 20:09:13 UTC 2020 > > Modified Files: > src/sys/arch/xen/xen: xbd_xenbus.c > > Log Message: > KASSERT() that requested I/O size is <= XBD_MAX_XFER - this can happen > e.g. with custom DomU kernel which doesn't have the value for MAXPHYS > reduced to 32k like the XEN3_DOMU config I though we were now splitting the requests ? -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch/xen/xen
HI, I am not sur whether it is the commit below, but 2 out 4 times my xen-DOMU from today (20200409/9.99.55) panics with following locking botch: [ 29.9301379] panic: kernel diagnostic assertion "IFNET_LOCKED(ifp)" failed: file "/usr/src/sys/arch/xen/xen/if_xennet_xenbus.c", line 1120 [ 29.9301379] cpu2: Begin traceback... [ 29.9301379] vpanic() at netbsd:vpanic+0x146 [ 29.9301379] kern_assert() at netbsd:kern_assert+0x48 [ 29.9301379] xennet_ioctl() at netbsd:xennet_ioctl+0x6d [ 29.9301379] if_mcast_op() at netbsd:if_mcast_op+0x6a [ 29.9301379] in6_addmulti() at netbsd:in6_addmulti+0x153 [ 29.9301379] in6_joingroup() at netbsd:in6_joingroup+0x45 [ 29.9301379] ip6_ctloutput() at netbsd:ip6_ctloutput+0x141c [ 29.9301379] udp6_ctloutput() at netbsd:udp6_ctloutput+0xa2 [ 29.9301379] udp6_ctloutput_wrapper() at netbsd:udp6_ctloutput_wrapper+0x2c [ 29.9301379] sosetopt() at netbsd:sosetopt+0x5c [ 29.9301379] sys_setsockopt() at netbsd:sys_setsockopt+0x8e [ 29.9301379] syscall() at netbsd:syscall+0x9c [ 29.9301379] --- syscall (number 105) --- [ 29.9301379] 75d934d3469a: [ 29.9301379] cpu2: End traceback... [ 29.9301379] rebooting... Best regards, Frank On 04/06/20 20:23, Jaromir Dolecek wrote: Module Name:src Committed By: jdolecek Date: Mon Apr 6 18:23:21 UTC 2020 Modified Files: src/sys/arch/xen/xen: if_xennet_xenbus.c Log Message: convert to IFEF_MPSAFE, also enable interrupt handler without biglock no performance difference observed compared to version before change, for neither UP nor MP DomU To generate a diff of this commit: cvs rdiff -u -r1.105 -r1.106 src/sys/arch/xen/xen/if_xennet_xenbus.c Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files.
Re: CVS commit: src/sys/arch/xen/xen
On Sun, Apr 05, 2020 at 05:26:47PM +, Jaromir Dolecek wrote: > Module Name: src > Committed By: jdolecek > Date: Sun Apr 5 17:26:47 UTC 2020 > > Modified Files: > src/sys/arch/xen/xen: if_xennet_xenbus.c xennetback_xenbus.c > > Log Message: > remove support for legacy rx-flip mode for xennet(4)/xvif(4), making > rx-copy (first shipped in NetBSD 6.0 in 2012) the only supported > mode did you actually read my reply to your proposal to port-xen ? The example provided in the xen 4.11 sources still use rx-flip, so I'm not sure it's not supported any more by linux dom0. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch/xen/xen
On Oct 17, 6:02pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/arch/xen/xen | Don't you think that you should also revert r1.46 [1]? Now that | privcmd_map_obj() frees everything correctly... Yes, I was going to and I forgot; done now. christos
Re: CVS commit: src/sys/arch/xen/xen
Le 10/10/2014 18:42, Maxime Villard a écrit :
Le 03/10/2014 22:56, Christos Zoulas a écrit :
Module Name:src
Committed By:christos
Date:Fri Oct 3 20:56:24 UTC 2014
Modified Files:
src/sys/arch/xen/xen: privcmd.c
Log Message:
correct error paths; still need to verify that the didn't give us back...
case is correct.
To generate a diff of this commit:
cvs rdiff -u -r1.47 -r1.48 src/sys/arch/xen/xen/privcmd.c
Don't you think that you should also revert r1.46 [1]? Now that
privcmd_map_obj() frees everything correctly...
[1]:
===
RCS file: /ftp/cvs/cvsroot/src/sys/arch/xen/xen/privcmd.c,v
rcsdiff: /ftp/cvs/cvsroot/src/sys/arch/xen/xen/privcmd.c,v: warning: Unknown
phrases like `commitid ...;' are present.
retrieving revision 1.45
retrieving revision 1.46
diff -u -p -r1.45 -r1.46
--- src/sys/arch/xen/xen/privcmd.c2013/11/06 06:23:151.45
+++ src/sys/arch/xen/xen/privcmd.c2014/09/21 16:53:381.46
@@ -1,4 +1,4 @@
-/* $NetBSD: privcmd.c,v 1.45 2013/11/06 06:23:15 mrg Exp $ */
+/* $NetBSD: privcmd.c,v 1.46 2014/09/21 16:53:38 christos Exp $ */
/*-
* Copyright (c) 2004 Christian Limpach.
@@ -27,7 +27,7 @@
#include sys/cdefs.h
-__KERNEL_RCSID(0, $NetBSD: privcmd.c,v 1.45 2013/11/06 06:23:15 mrg Exp $);
+__KERNEL_RCSID(0, $NetBSD: privcmd.c,v 1.46 2014/09/21 16:53:38 christos Exp
$);
#include sys/param.h
#include sys/systm.h
@@ -360,8 +360,11 @@ privcmd_ioctl(void *v)
}
error = privcmd_map_obj(vmm, va, maddr,
mentry.npages, mcmd-dom);
-if (error)
+if (error) {
+kmem_free(maddr,
+sizeof(paddr_t) * mentry.npages);
return error;
+}
}
break;
}
??
Re: CVS commit: src/sys/arch/xen/xen
Le 03/10/2014 22:56, Christos Zoulas a écrit :
Module Name:src
Committed By: christos
Date: Fri Oct 3 20:56:24 UTC 2014
Modified Files:
src/sys/arch/xen/xen: privcmd.c
Log Message:
correct error paths; still need to verify that the didn't give us back...
case is correct.
To generate a diff of this commit:
cvs rdiff -u -r1.47 -r1.48 src/sys/arch/xen/xen/privcmd.c
Don't you think that you should also revert r1.46 [1]? Now that
privcmd_map_obj() frees everything correctly...
[1]:
===
RCS file: /ftp/cvs/cvsroot/src/sys/arch/xen/xen/privcmd.c,v
rcsdiff: /ftp/cvs/cvsroot/src/sys/arch/xen/xen/privcmd.c,v: warning: Unknown
phrases like `commitid ...;' are present.
retrieving revision 1.45
retrieving revision 1.46
diff -u -p -r1.45 -r1.46
--- src/sys/arch/xen/xen/privcmd.c 2013/11/06 06:23:15 1.45
+++ src/sys/arch/xen/xen/privcmd.c 2014/09/21 16:53:38 1.46
@@ -1,4 +1,4 @@
-/* $NetBSD: privcmd.c,v 1.45 2013/11/06 06:23:15 mrg Exp $ */
+/* $NetBSD: privcmd.c,v 1.46 2014/09/21 16:53:38 christos Exp $ */
/*-
* Copyright (c) 2004 Christian Limpach.
@@ -27,7 +27,7 @@
#include sys/cdefs.h
-__KERNEL_RCSID(0, $NetBSD: privcmd.c,v 1.45 2013/11/06 06:23:15 mrg Exp $);
+__KERNEL_RCSID(0, $NetBSD: privcmd.c,v 1.46 2014/09/21 16:53:38 christos Exp
$);
#include sys/param.h
#include sys/systm.h
@@ -360,8 +360,11 @@ privcmd_ioctl(void *v)
}
error = privcmd_map_obj(vmm, va, maddr,
mentry.npages, mcmd-dom);
- if (error)
+ if (error) {
+ kmem_free(maddr,
+ sizeof(paddr_t) * mentry.npages);
return error;
+ }
}
break;
}
Re: CVS commit: src/sys/arch/xen/xen
Le 27/09/2014 17:51, Christos Zoulas a écrit :
On Sep 27, 8:36am, m...@m00nbsd.net (Maxime Villard) wrote:
-- Subject: Re: CVS commit: src/sys/arch/xen/xen
| One however returns an error without freeing:
|
| if (newstart != start) {
| printf(uvm_map didn't give us back our vm space\n);
| return EINVAL;
| }
|
| I think this one is the real bug; isn't it?
So the fix should be this, right?
Yes perhaps. But you should ask someone else...
christos
Index: privcmd.c
===
RCS file: /cvsroot/src/sys/arch/xen/xen/privcmd.c,v
retrieving revision 1.47
diff -u -u -r1.47 privcmd.c
--- privcmd.c 21 Sep 2014 16:56:44 - 1.47
+++ privcmd.c 27 Sep 2014 15:50:56 -
@@ -576,12 +576,13 @@
if (error) {
if (obj)
obj-uobj.pgops-pgo_detach(obj-uobj);
- kmem_free(maddr, sizeof(paddr_t) * npages);
- kmem_free(obj, sizeof(*obj));
return error;
}
if (newstart != start) {
printf(uvm_map didn't give us back our vm space\n);
+ uvm_unmap1(map, newstart, newstart + size, 0);
+ if (obj)
+ obj-uobj.pgops-pgo_detach(obj-uobj);
return EINVAL;
}
return 0;
Re: CVS commit: src/sys/arch/xen/xen
Le 21/09/2014 20:22, Christos Zoulas a écrit :
On Sep 21, 7:57pm, m...@m00nbsd.net (Maxime Villard) wrote:
-- Subject: Re: CVS commit: src/sys/arch/xen/xen
| Did you test this change? Verily I'm not sure it's a proper bug, but I
| kept it in my list so that someone (bouyer@?) could investigate.
This is why I committed the fix in two pieces. The first one is obviously
right. The second one is probably correct too, and if you notice there is
also another if statement below that does not free either, but returns
EINVAL. Didn't your code checker flag that?
The only change I did not apply was a false positive
(jumping inside a loop!?!?) which was disgusting code.
christos
- 1 -
error = privcmd_map_obj(vmm, va, maddr,
mentry.npages, mcmd-dom);
- if (error)
+ if (error) {
+ kmem_free(maddr,
+ sizeof(paddr_t) * mentry.npages);
return error;
+ }
}
That's a fix I don't feel comfortable with: in privcmd_map_obj(), there
are two
kmem_free(maddr, sizeof(paddr_t) * npages);
return EINVAL;
One however returns an error without freeing:
if (newstart != start) {
printf(uvm_map didn't give us back our vm space\n);
return EINVAL;
}
I think this one is the real bug; isn't it?
- 2 -
I see you committed this:
#include sys/param.h
#include sys/systm.h
@@ -556,7 +556,7 @@ privcmd_map_obj(struct vm_map *map, vadd
/* remove current entries */
uvm_unmap1(map, start, start + size, 0);
- obj = kmem_alloc(sizeof(struct privcmd_object), KM_SLEEP);
+ obj = kmem_alloc(sizeof(*obj), KM_SLEEP);
if (obj == NULL) {
kmem_free(maddr, sizeof(paddr_t) * npages);
return ENOMEM;
@@ -576,6 +576,8 @@ privcmd_map_obj(struct vm_map *map, vadd
if (error) {
if (obj)
obj-uobj.pgops-pgo_detach(obj-uobj);
+ kmem_free(maddr, sizeof(paddr_t) * npages);
+ kmem_free(obj, sizeof(*obj));
return error;
}
But if I understand correctly, pgo_detach is linked to privpgop_detach
which already frees 'obj' and 'maddr'. Doesn't it?
Re: CVS commit: src/sys/arch/xen/xen
On Sep 27, 8:36am, m...@m00nbsd.net (Maxime Villard) wrote:
-- Subject: Re: CVS commit: src/sys/arch/xen/xen
| One however returns an error without freeing:
|
| if (newstart != start) {
| printf(uvm_map didn't give us back our vm space\n);
| return EINVAL;
| }
|
| I think this one is the real bug; isn't it?
So the fix should be this, right?
christos
Index: privcmd.c
===
RCS file: /cvsroot/src/sys/arch/xen/xen/privcmd.c,v
retrieving revision 1.47
diff -u -u -r1.47 privcmd.c
--- privcmd.c 21 Sep 2014 16:56:44 - 1.47
+++ privcmd.c 27 Sep 2014 15:50:56 -
@@ -576,12 +576,13 @@
if (error) {
if (obj)
obj-uobj.pgops-pgo_detach(obj-uobj);
- kmem_free(maddr, sizeof(paddr_t) * npages);
- kmem_free(obj, sizeof(*obj));
return error;
}
if (newstart != start) {
printf(uvm_map didn't give us back our vm space\n);
+ uvm_unmap1(map, newstart, newstart + size, 0);
+ if (obj)
+ obj-uobj.pgops-pgo_detach(obj-uobj);
return EINVAL;
}
return 0;
Re: CVS commit: src/sys/arch/xen/xen
On Sat, Sep 27, 2014 at 11:51:59AM -0400, Christos Zoulas wrote:
On Sep 27, 8:36am, m...@m00nbsd.net (Maxime Villard) wrote:
-- Subject: Re: CVS commit: src/sys/arch/xen/xen
| One however returns an error without freeing:
|
| if (newstart != start) {
| printf(uvm_map didn't give us back our vm space\n);
| return EINVAL;
| }
|
| I think this one is the real bug; isn't it?
So the fix should be this, right?
christos
Index: privcmd.c
===
RCS file: /cvsroot/src/sys/arch/xen/xen/privcmd.c,v
retrieving revision 1.47
diff -u -u -r1.47 privcmd.c
--- privcmd.c 21 Sep 2014 16:56:44 - 1.47
+++ privcmd.c 27 Sep 2014 15:50:56 -
@@ -576,12 +576,13 @@
if (error) {
if (obj)
obj-uobj.pgops-pgo_detach(obj-uobj);
- kmem_free(maddr, sizeof(paddr_t) * npages);
- kmem_free(obj, sizeof(*obj));
return error;
}
if (newstart != start) {
printf(uvm_map didn't give us back our vm space\n);
+ uvm_unmap1(map, newstart, newstart + size, 0);
+ if (obj)
+ obj-uobj.pgops-pgo_detach(obj-uobj);
return EINVAL;
}
where is obj freed then ?
--
Manuel Bouyer bou...@antioche.eu.org
NetBSD: 26 ans d'experience feront toujours la difference
--
Re: CVS commit: src/sys/arch/xen/xen
On Sep 27, 10:15pm, bou...@antioche.eu.org (Manuel Bouyer) wrote:
-- Subject: Re: CVS commit: src/sys/arch/xen/xen
| if (newstart != start) {
| printf(uvm_map didn't give us back our vm space\n);
| + uvm_unmap1(map, newstart, newstart + size, 0);
| + if (obj)
| + obj-uobj.pgops-pgo_detach(obj-uobj);
| return EINVAL;
| }
|
| where is obj freed then ?
In detach.
christos
Re: CVS commit: src/sys/arch/xen/xen
On Sep 21, 7:57pm, m...@m00nbsd.net (Maxime Villard) wrote: -- Subject: Re: CVS commit: src/sys/arch/xen/xen | Did you test this change? Verily I'm not sure it's a proper bug, but I | kept it in my list so that someone (bouyer@?) could investigate. This is why I committed the fix in two pieces. The first one is obviously right. The second one is probably correct too, and if you notice there is also another if statement below that does not free either, but returns EINVAL. Didn't your code checker flag that? The only change I did not apply was a false positive (jumping inside a loop!?!?) which was disgusting code. christos
Re: CVS commit: src/sys/arch/xen/xen
On Sat, Feb 01, 2014 at 06:09:04PM -0800, John Nemeth wrote: } } XEN might need the bits of fpuinit() that don't play with cr0. } In particular a fninit and setting TS. } Although I'm not 100% sure the TS is needed on a single cpu system. Xen domU is SMP capable. NetBSD dom0 currently aren't, but hopefully it is in the works. Probably best to make fpuinit() xen-save then. David -- David Laight: da...@l8s.co.uk
Re: CVS commit: src/sys/arch/xen/xen
On Sun, Feb 02, 2014 at 01:27:45PM +0100, Manuel Bouyer wrote: On Sat, Feb 01, 2014 at 10:49:44PM +, David Laight wrote: On Sat, Feb 01, 2014 at 08:07:07PM +, Manuel Bouyer wrote: Module Name: src Committed By: bouyer Date: Sat Feb 1 20:07:07 UTC 2014 Modified Files: src/sys/arch/xen/xen: hypervisor.c Log Message: Revert previous: calling fpuinit() leads to a panic, as a domU is not allowed to manipulate cr0 directly. Xen doesn't need this, the fpu is handled by the hypervisor. I probably remember seeing that panic as well. XEN might need the bits of fpuinit() that don't play with cr0. In particular a fninit and setting TS. Although I'm not 100% sure the TS is needed on a single cpu system. It isn't there on amd64, but really it does no harm. Without it one process will have an indererminate fp state. It's been a while since I looked at this and I don't remmeber the details, but I don't think anything of fpuinit() is neeed for Xen. in netbsd-6 npxattach() doens't do anything either for Xen (only set i386_fpu_present to 1 and init npxdna_func, which seems to be done at compile time now). I think the FPU is initialised by the hypervisor. Or it's done by npxdna() on the first FPU use. With the code as is in HEAD now, all FPU-related atf tests pass on a Xen guest with 4 CPUs, so I assume it's working as expected. Something needs to set the TS (task switched) flag when a new cpu is added. Both amd64 and i386 'bare metal' have direct calls to fpuinit() to do this. If TS isn't set the first FP process that gets migrated to that cpu won't fault on its first FP instruction. If it has just forked it probably won't care, but otherwise it will all go horribly wrong. It probably doesn't matter for the boot cpu - except that you (effectively) clone 'random' FP registers instead of known 'initially zero' ones. David -- David Laight: da...@l8s.co.uk
Re: CVS commit: src/sys/arch/xen/xen
On Sun, Feb 02, 2014 at 06:53:55PM +, David Laight wrote: It's been a while since I looked at this and I don't remmeber the details, but I don't think anything of fpuinit() is neeed for Xen. in netbsd-6 npxattach() doens't do anything either for Xen (only set i386_fpu_present to 1 and init npxdna_func, which seems to be done at compile time now). I think the FPU is initialised by the hypervisor. Or it's done by npxdna() on the first FPU use. With the code as is in HEAD now, all FPU-related atf tests pass on a Xen guest with 4 CPUs, so I assume it's working as expected. Something needs to set the TS (task switched) flag when a new cpu is added. Both amd64 and i386 'bare metal' have direct calls to fpuinit() to do this. If TS isn't set the first FP process that gets migrated to that cpu won't fault on its first FP instruction. If it has just forked it probably won't care, but otherwise it will all go horribly wrong. This is the clts/stts macros/functions, isn't it ? For Xen, this is done with the HYPERVISOR_fpu_taskswitch() hypercall. For the boot CPU it's done in i386_proc0_tss_ldt_init(). For other CPUs it will be done when a process is created/migrated to this CPU in i386_tls_switch(), because (l != ci-ci_fpcurlwp) will be true. It probably doesn't matter for the boot cpu - except that you (effectively) clone 'random' FP registers instead of known 'initially zero' ones. But these are zeroed on first call to the first npxdna() call, because the lwp won't have MDL_USEDFPU set at this time. -- Manuel Bouyer bou...@antioche.eu.org NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch/xen/xen
On Sun, Feb 02, 2014 at 08:41:29PM +0100, Manuel Bouyer wrote: On Sun, Feb 02, 2014 at 06:53:55PM +, David Laight wrote: Something needs to set the TS (task switched) flag when a new cpu is added. Both amd64 and i386 'bare metal' have direct calls to fpuinit() to do this. If TS isn't set the first FP process that gets migrated to that cpu won't fault on its first FP instruction. If it has just forked it probably won't care, but otherwise it will all go horribly wrong. This is the clts/stts macros/functions, isn't it ? For Xen, this is done with the HYPERVISOR_fpu_taskswitch() hypercall. Yes. For the boot CPU it's done in i386_proc0_tss_ldt_init(). For other CPUs it will be done when a process is created/migrated to this CPU in i386_tls_switch(), because (l != ci-ci_fpcurlwp) will be true. Yes, probably true. I'd assumed that the call to fpuinit() served a purpose. It probably doesn't matter for the boot cpu - except that you (effectively) clone 'random' FP registers instead of known 'initially zero' ones. But these are zeroed on first call to the first npxdna() call, because the lwp won't have MDL_USEDFPU set at this time. fork() ought to replicate the FP registers of the parent. I think it saves the fpu state, and then copies it. On bare metal it could base that descision of the state of the TS flag (I don't think it does). But for XEN the equivalent isn't readable. I'm about to commit code that inverts the default and assumes that the fpu is present until fpuinit() detects it isn't. That way it matters much less if it isn't called. (No one runs 496SX) David -- David Laight: da...@l8s.co.uk
Re: CVS commit: src/sys/arch/xen/xen
On Sun, Feb 02, 2014 at 08:24:08PM +, David Laight wrote: On Sun, Feb 02, 2014 at 08:41:29PM +0100, Manuel Bouyer wrote: On Sun, Feb 02, 2014 at 06:53:55PM +, David Laight wrote: Something needs to set the TS (task switched) flag when a new cpu is added. Both amd64 and i386 'bare metal' have direct calls to fpuinit() to do this. If TS isn't set the first FP process that gets migrated to that cpu won't fault on its first FP instruction. If it has just forked it probably won't care, but otherwise it will all go horribly wrong. This is the clts/stts macros/functions, isn't it ? For Xen, this is done with the HYPERVISOR_fpu_taskswitch() hypercall. Yes. For the boot CPU it's done in i386_proc0_tss_ldt_init(). For other CPUs it will be done when a process is created/migrated to this CPU in i386_tls_switch(), because (l != ci-ci_fpcurlwp) will be true. Yes, probably true. I'd assumed that the call to fpuinit() served a purpose. It probably does for native. For Xen things are done a bit differently. It probably doesn't matter for the boot cpu - except that you (effectively) clone 'random' FP registers instead of known 'initially zero' ones. But these are zeroed on first call to the first npxdna() call, because the lwp won't have MDL_USEDFPU set at this time. fork() ought to replicate the FP registers of the parent. I think it saves the fpu state, and then copies it. On bare metal it could base that descision of the state of the TS flag (I don't think it does). But for XEN the equivalent isn't readable. I wouldn't expect native or Xen to be different in this case. -- Manuel Bouyer bou...@antioche.eu.org NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch/xen/xen
On Sat, Feb 01, 2014 at 05:48:04PM +, David Laight wrote: Module Name: src Committed By: dsl Date: Sat Feb 1 17:48:04 UTC 2014 Modified Files: src/sys/arch/xen/xen: hypervisor.c Log Message: Add a direct call to fpuinit(). I'm not sure this is architecturally the best place, but I think it is where npxattach() used to get called. Might fix FP support in domu Actually this broke it: xencons0 at hypervisor0: Xen Virtual Console Driver panic: XXX lcr0 not supported fpuinit() should *not* be called for Xen (and it was not before). This is handled by the hypervisor. I didn't notice i386_fpu_present was set to 1 in fpuinit() (I don't know how I missed it), so setting i386_fpu_present to 1 unconditionally for Xen is probably the right thing to do. -- Manuel Bouyer bou...@antioche.eu.org NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch/xen/xen
On Sat, Feb 01, 2014 at 08:07:07PM +, Manuel Bouyer wrote: Module Name: src Committed By: bouyer Date: Sat Feb 1 20:07:07 UTC 2014 Modified Files: src/sys/arch/xen/xen: hypervisor.c Log Message: Revert previous: calling fpuinit() leads to a panic, as a domU is not allowed to manipulate cr0 directly. Xen doesn't need this, the fpu is handled by the hypervisor. I probably remember seeing that panic as well. XEN might need the bits of fpuinit() that don't play with cr0. In particular a fninit and setting TS. Although I'm not 100% sure the TS is needed on a single cpu system. It isn't there on amd64, but really it does no harm. Without it one process will have an indererminate fp state. I can't say I've actuallly looked a xen. Presumably we somewhere compile the hypervisor as part of a kernel? David -- David Laight: da...@l8s.co.uk
Re: CVS commit: src/sys/arch/xen/xen
On 2/1/14, 2:49 PM, David Laight wrote: On Sat, Feb 01, 2014 at 08:07:07PM +, Manuel Bouyer wrote: I can't say I've actuallly looked a xen. Presumably we somewhere compile the hypervisor as part of a kernel? The hypervisor is in pkgsrc - sysutils/xenkernel42 (and the accompanying xentools42). It's not NetBSD-based. +j
Re: CVS commit: src/sys/arch/xen/xen
On 12/08/11 04:34, Cherry G. Mathew wrote: Module Name:src Committed By: cherry Date: Thu Dec 8 03:34:48 UTC 2011 Modified Files: src/sys/arch/xen/xen: evtchn.c Log Message: kmem_free() the appropriate size. Thanks cegger@ This is still not enough: In pirq_establish() you allocate sizeof(struct pintrhand) but in event_remove_handler you free sizeof(struct intrhand). And sizeof(struct pintrhand) != sizeof(intrhand). We need a ih_size field to track the right size. Christoph
Re: CVS commit: src/sys/arch/xen/xen
On Thu, Dec 08, 2011 at 11:09:19AM +0100, Christoph Egger wrote: On 12/08/11 04:34, Cherry G. Mathew wrote: Module Name: src Committed By:cherry Date:Thu Dec 8 03:34:48 UTC 2011 Modified Files: src/sys/arch/xen/xen: evtchn.c Log Message: kmem_free() the appropriate size. Thanks cegger@ This is still not enough: In pirq_establish() you allocate sizeof(struct pintrhand) but in event_remove_handler you free sizeof(struct intrhand). And sizeof(struct pintrhand) != sizeof(intrhand). We need a ih_size field to track the right size. No, in event_remove_handler() is't really a intrhand which is freed, not pintrhand (the pintrhand is ih-ih_arg, not ih itself). At this time, pintrhand is never freed. It should be freed in pci_intr_disestablish() when this one will do something usefull. -- Manuel Bouyer bou...@antioche.eu.org NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch/xen/xen
On 7 December 2011 19:19, Christoph Egger ceg...@netbsd.org wrote: Module Name: src Committed By: cegger Date: Wed Dec 7 13:49:04 UTC 2011 Modified Files: src/sys/arch/xen/xen: evtchn.c Log Message: build fix: add back sys/malloc.h. malloc(9) is still in use. Hi, sorry about this, I missed it - can you move the malloc() in pirq_establish() to kmem_alloc() instead ? iiuc, we're moving away from malloc() so this is regress. Cheers, -- ~Cherry
Re: CVS commit: src/sys/arch/xen/xen
Hello, Jean-Yves Migeon j...@netbsd.org wrote: Module Name: src Committed By: jym Date: Sun May 15 07:24:15 UTC 2011 Modified Files: src/sys/arch/xen/xen: xbdback_xenbus.c Log Message: Use atomic_ops(3) for ref counting. + atomic_dec_uint((xbdip)-xbdi_refcnt); \ + if (0 == (xbdip)-xbdi_refcnt) \ xbdback_finish_disconnect(xbdip); \ This is not correct. Atomic op might decrease the reference count to 1 while other thread executes xbdi_put() before xbdi_refcnt is fetched, thus decreasing it to 0. In such case, both threads would fetch 0. The following is a correct way: if (atomic_dec_uint_nv(xbdip-xbdi_refcnt) == 0) xbdback_finish_disconnect(xbdip); Also, it seems there is no need for xbdi_refcnt to be volatile. -- Mindaugas
Re: CVS commit: src/sys/arch/xen/xen
On 15.05.2011 21:11, Mindaugas Rasiukevicius wrote: This is not correct. Atomic op might decrease the reference count to 1 while other thread executes xbdi_put() before xbdi_refcnt is fetched, thus decreasing it to 0. In such case, both threads would fetch 0. The following is a correct way: if (atomic_dec_uint_nv(xbdip-xbdi_refcnt) == 0) xbdback_finish_disconnect(xbdip); Also, it seems there is no need for xbdi_refcnt to be volatile. Good point; I was pondering about the _nv() version when I made the change, but forgot about the possible concurrency regarding refcnt fetch (not actually possible as port-xen is not MP, but will become soonish) I'll remove the volatile declaration too, only xbdi_put/_get use the refcnt for G/C anyway. Thanks for pointing that out! -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: CVS commit: src/sys/arch/xen/xen
On 18 April 2011 04:21, Mindaugas Rasiukevicius rm...@netbsd.org wrote: Masao Uebayashi uebay...@tombi.co.jp wrote: On Mon, Apr 18, 2011 at 03:04:32AM +, Mindaugas Rasiukevicius wrote: Module Name: src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. And, according to yamt@, KM_SLEEP can fail in the current design... IIRC yamt@ fixed it a year or few ago. And in the more specific immediate context: http://mail-index.netbsd.org/port-xen/2011/04/07/msg006613.html -- ~Cherry
Re: CVS commit: src/sys/arch/xen/xen
On 18 April 2011 08:00, Cherry G. Mathew cherry.g.mat...@gmail.com wrote: On 18 April 2011 04:21, Mindaugas Rasiukevicius rm...@netbsd.org wrote: Masao Uebayashi uebay...@tombi.co.jp wrote: On Mon, Apr 18, 2011 at 03:04:32AM +, Mindaugas Rasiukevicius wrote: Module Name: src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. And, according to yamt@, KM_SLEEP can fail in the current design... IIRC yamt@ fixed it a year or few ago. And in the more specific immediate context: http://mail-index.netbsd.org/port-xen/2011/04/07/msg006613.html Hi, PS: Can you please revert ? Unless it's breaking anything else, or you have a fix for the problem mentioned in the thread above, ie; Cheers, -- ~Cherry
Re: CVS commit: src/sys/arch/xen/xen
On 18.04.2011 09:23, Cherry G. Mathew wrote: On 18 April 2011 08:00, Cherry G. Mathew cherry.g.mat...@gmail.com wrote: On 18 April 2011 04:21, Mindaugas Rasiukevicius rm...@netbsd.org wrote: Masao Uebayashi uebay...@tombi.co.jp wrote: On Mon, Apr 18, 2011 at 03:04:32AM +, Mindaugas Rasiukevicius wrote: Module Name:src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. And, according to yamt@, KM_SLEEP can fail in the current design... IIRC yamt@ fixed it a year or few ago. And in the more specific immediate context: http://mail-index.netbsd.org/port-xen/2011/04/07/msg006613.html PS: Can you please revert ? Unless it's breaking anything else, or you have a fix for the problem mentioned in the thread above, ie; Uho, I forgot to mention in my commit log that I fixed it. I am allocating bpg_entries via pool_cache(9), and the constructor bpge_ctor() will return an error if uvm(9) fails to find a free page. In that case, the thread will just bail out and start waiting again. -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: CVS commit: src/sys/arch/xen/xen
On 18 April 2011 09:04, Jean-Yves Migeon jeanyves.mig...@free.fr wrote: On 18.04.2011 09:23, Cherry G. Mathew wrote: On 18 April 2011 08:00, Cherry G. Mathew cherry.g.mat...@gmail.com wrote: On 18 April 2011 04:21, Mindaugas Rasiukevicius rm...@netbsd.org wrote: Masao Uebayashi uebay...@tombi.co.jp wrote: On Mon, Apr 18, 2011 at 03:04:32AM +, Mindaugas Rasiukevicius wrote: Module Name: src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. And, according to yamt@, KM_SLEEP can fail in the current design... IIRC yamt@ fixed it a year or few ago. And in the more specific immediate context: http://mail-index.netbsd.org/port-xen/2011/04/07/msg006613.html PS: Can you please revert ? Unless it's breaking anything else, or you have a fix for the problem mentioned in the thread above, ie; Uho, I forgot to mention in my commit log that I fixed it. I am allocating bpg_entries via pool_cache(9), and the constructor bpge_ctor() will return an error if uvm(9) fails to find a free page. In that case, the thread will just bail out and start waiting again. Ah right, I missed your commit, sorry. Thanks, -- ~Cherry
Re: CVS commit: src/sys/arch/xen/xen
On 18.04.2011 05:04, Mindaugas Rasiukevicius wrote: Module Name: src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. Ah yes, forgot about this one, thanks. Although I am still unsure about the check, in-kernel NULL deref is... problematic. I am not so sure whether it is safe to assume non-NULL return if caller can sleep. It's something that ought to be specified for all available memoryallocators(9) especially as the code behind can evolve (hey, Lars :) ). -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: CVS commit: src/sys/arch/xen/xen
Cherry G. Mathew cherry.g.mat...@gmail.com wrote: On Mon, Apr 18, 2011 at 03:04:32AM +, Mindaugas Rasiukevicius wrote: Module Name: src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. And, according to yamt@, KM_SLEEP can fail in the current design... IIRC yamt@ fixed it a year or few ago. And in the more specific immediate context: http://mail-index.netbsd.org/port-xen/2011/04/07/msg006613.html Hi, PS: Can you please revert ? Unless it's breaking anything else, or you have a fix for the problem mentioned in the thread above, ie; Change is for balloon_xenbus_attach(), which is irrelevant to the issue described in that email. Are you aware that KM_NOSLEEP might fail not only because there is no memory (or KVA) available, though? -- Mindaugas
Re: CVS commit: src/sys/arch/xen/xen
On 18.04.2011 10:35, Mindaugas Rasiukevicius wrote: We used to check the return of big size allocations, when kmem(9) could fail even with KM_SLEEP due to KVA starvation. However, pretty much all kernel does not perform checks for smaller allocations and since the bug was fixed - we are no longer checking for big ones as well. This applies to all allocators. So, any thread sleeping for an allocation cannot be interrupted? -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: CVS commit: src/sys/arch/xen/xen
On Apr,Monday 18 2011, at 10:26 AM, Mindaugas Rasiukevicius wrote: Cherry G. Mathew cherry.g.mat...@gmail.com wrote: On Mon, Apr 18, 2011 at 03:04:32AM +, Mindaugas Rasiukevicius wrote: Module Name:src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. And, according to yamt@, KM_SLEEP can fail in the current design... IIRC yamt@ fixed it a year or few ago. And in the more specific immediate context: http://mail-index.netbsd.org/port-xen/2011/04/07/msg006613.html Hi, PS: Can you please revert ? Unless it's breaking anything else, or you have a fix for the problem mentioned in the thread above, ie; Change is for balloon_xenbus_attach(), which is irrelevant to the issue described in that email. Are you aware that KM_NOSLEEP might fail not only because there is no memory (or KVA) available, though? I had several problems with memory hungry zfs, where allocation failed just because some lock in uvm was hold when KM_NOSLEEP allocation was issued. We should avoid of using KM_NOSLEEP whenever is it possible. Regards Adam.
Re: CVS commit: src/sys/arch/xen/xen
Jean-Yves Migeon jeanyves.mig...@free.fr wrote: On 18.04.2011 10:35, Mindaugas Rasiukevicius wrote: We used to check the return of big size allocations, when kmem(9) could fail even with KM_SLEEP due to KVA starvation. However, pretty much all kernel does not perform checks for smaller allocations and since the bug was fixed - we are no longer checking for big ones as well. This applies to all allocators. So, any thread sleeping for an allocation cannot be interrupted? Nope. However, for nearly all cases there is no need for that. For the previous reserve_pages() case, pointed out by Cherry, code should rather be restructured to pre-allocate memory before acquiring the locks. That would have avoided the problem. -- Mindaugas
Re: CVS commit: src/sys/arch/xen/xen
Masao Uebayashi uebay...@tombi.co.jp wrote: On Mon, Apr 18, 2011 at 03:04:32AM +, Mindaugas Rasiukevicius wrote: Module Name:src Committed By: rmind Date: Mon Apr 18 03:04:31 UTC 2011 Modified Files: src/sys/arch/xen/xen: balloon.c Log Message: balloon_xenbus_attach: use KM_SLEEP for allocation. Note: please do not use KM_NOSLEEP. And, according to yamt@, KM_SLEEP can fail in the current design... IIRC yamt@ fixed it a year or few ago. -- Mindaugas
Re: CVS commit: src/sys/arch/xen/xen
Date: Mon, 04 Apr 2011 23:46:19 +0200 From: Jean-Yves Migeon jeanyves.mig...@free.fr The newer scripts for Xen read the interface value from the vifname entry in Xenstore, so changing the name is not that critical. But this should be stabilized sooner than later. Personally, I find '_' rather ugly in an interface name... but I am open to suggestions, even fixing rc.d/network if it needs to. One could declare that ifnames must match [a-z][-a-z0-9]*[0-9], and then in /etc/rc.d/network pass them through `tr - _' before evaluating `args=\$ifconfig_$int'. This guarantees that setting ifconfig_ifN in /etc/rc.conf always works, and depends only on agreeing to the ifname policy and ensuring that all the existing ifnames actually follow it. Of course, tr might not be available yet in /etc/rc.d/network, but someone wizardlier (or more patient) with sh than I can probably work around that...
Re: CVS commit: src/sys/arch/xen/xen
On 06.04.2011 20:01, Manuel Bouyer wrote: You could also use xvifxiy (e.g. xvif5i2, where i stands for 'index'). or any other letter ... Huh hmm indeed... I wonder why I did not think about this approach before... -- Jean-Yves Migeon jeanyves.mig...@free.fr
Re: CVS commit: src/sys/arch/xen/xen
Date: Sun, 3 Apr 2011 23:21:39 + From: Jean-Yves Migeon j...@netbsd.org Now that pkgsrc-2011Q1 has arrived, and before -6 chimes in, change ifxname for xvif(4) from xvif%d.%d to xvif%d-%d. This is needed to avoid sysctl(9) EINVAL errors when creating interface nodes. This change came awfully close to fixing PR misc/39376 too... (Hyphens may be valid in sysctl nodes while dots are not, but neither hyphens nor dots are valid in sh variables, for /etc/rc.d/network's ifconfig_ifN.)
Re: CVS commit: src/sys/arch/xen/xen
On Wed, Mar 30, 2011 at 12:17:05AM +, Jean-Yves Migeon wrote: Module Name: src Committed By: jym Date: Wed Mar 30 00:17:04 UTC 2011 Modified Files: src/sys/arch/xen/xen: if_xennet_xenbus.c Log Message: printf(xennet: ...) = aprint_error_ifnet() It's silly, I know, but aprint_error_ifnet() is only supposed to be used during autoconfiguration. Dave -- David Young OJC Technologies dyo...@ojctech.com Urbana, IL * (217) 344-0444 x24
