Re: [PATCH 3/25][V3] irq_flags / halt routines
Jeremy Fitzhardinge escreveu: Glauber de Oliveira Costa wrote: Thanks for the explanation, Andi. I understand it much better now, and agree with you. As alternatives what we have now, we can either keep the paravirt_ops as it is now for the native case, just hooking the vsmp functions in place of the normal one, (there are just three ops anyway), refill the paravirt_ops entirely in somewhere like vsmp.c, or similar (or maybe even assigning paravirt_ops.fn = vsmp_fn on the fly, but early enough). One thing to note is that current code assumes the IF flag is always in bit 9, so if you paravirtualize this, you need to either a) make the vsmp version copy AC into IF to satisfy the interface, or b) add a new op meaning "tell me if this eflags has interrupts enabled or not". I went for option a), and it seems to work OK (using bit 9 for "interrupt enabled" is pretty arbitrary from a Xen perspective, but not very hard to implement, and more localized than making all eflags tests a pvop). J It is implemented like a) in the latest patch I send, following chris' suggestion. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
Glauber de Oliveira Costa wrote: > Thanks for the explanation, Andi. I understand it much better now, and > agree with you. > > As alternatives what we have now, we can either keep the paravirt_ops > as it is now for the native case, just hooking the vsmp functions in > place of the normal one, (there are just three ops anyway), refill the > paravirt_ops entirely in somewhere like vsmp.c, or similar (or maybe > even assigning paravirt_ops.fn = vsmp_fn on the fly, but early enough). One thing to note is that current code assumes the IF flag is always in bit 9, so if you paravirtualize this, you need to either a) make the vsmp version copy AC into IF to satisfy the interface, or b) add a new op meaning "tell me if this eflags has interrupts enabled or not". I went for option a), and it seems to work OK (using bit 9 for "interrupt enabled" is pretty arbitrary from a Xen perspective, but not very hard to implement, and more localized than making all eflags tests a pvop). J - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
> between __cacheline_aligned_in_smp and other compile time bits based on > VSMP specific INTERNODE_CACHE, etc. I think compile time the way to go. Yes you're right they'll need an additional build option for that. It would be too wasteful to have the big cache line for all paravirt kernels. But it can be below paravirt ops and at least clean up the interrupt saving code. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
On 8/15/07, Chris Wright <[EMAIL PROTECTED]> wrote: > * Glauber de Oliveira Costa ([EMAIL PROTECTED]) wrote: > > Only caveat, is that it has to be done before smp gets in the game, and > > with interrupts disabled. (which makes the function in vsmp.c not eligible). > > > > My current option is to force VSMP to use PARAVIRT, as said before, and > > then fill paravirt_arch_setup, which is currently unused, with code to > > replace the needed paravirt_ops.fn. > > > > I don't know if there is any method to dynamically determine (at this > > point) that we are in a vsmp arch, and if there are not, it will have to > > get ifdefs anyway. But at least, they are far more local. > > between __cacheline_aligned_in_smp and other compile time bits based on > VSMP specific INTERNODE_CACHE, etc. I think compile time the way to go. > > > I am okay with both, but after all the explanation, I don't think that > > adding a new pvops is a bad idea. It would make things less cumbersome > > in this case. Also, hacks like this save_fl may require changes to the > > hypervisor, right? I don't even know where such hypervisor is, and how > > easy it is to replace it (it may be deeply hidden in firmware) > > No hypervisor change needed. Just the pv backend needs to return 0 or > X86_EFLAGS_IF for save_flags (and similar translation on restore_flags). > Xen uses a simple shared memory flag and does something which you could > roughly translate into this: > > xen_save_flags() > if (xen_vcpu_interrupts_enabled) > return X86_EFLAGS_IF; > else > return 0; > > This doesn't require any hypervisor changes. Similarly, VSMP could do > something along the lines of: > > vsmp_save_flags() > flags = native_save_flags(); > if (flags & X86_EFLAGS_IF) || (flags & X86_EFLAGS_AC) > return X86_EFLAGS_IF; > else > return 0; > I'm attaching to this message my idea on how would it work. This is just for comments/considerations. If you all ack this, I'll spread the changes over the patch series as needed, and then resend the patches. -- Glauber de Oliveira Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act." diff --git a/arch/x86_64/Kconfig b/arch/x86_64/Kconfig index 00b2fc9..1204b08 100644 --- a/arch/x86_64/Kconfig +++ b/arch/x86_64/Kconfig @@ -150,6 +150,7 @@ config X86_PC config X86_VSMP bool "Support for ScaleMP vSMP" depends on PCI + select PARAVIRT help Support for ScaleMP vSMP systems. Say 'Y' here if this kernel is supposed to run on these EM64T-based machines. Only choose this option diff --git a/arch/x86_64/kernel/paravirt.c b/arch/x86_64/kernel/paravirt.c index dcd9919..23a8786 100644 --- a/arch/x86_64/kernel/paravirt.c +++ b/arch/x86_64/kernel/paravirt.c @@ -22,6 +22,8 @@ #include #include #include +#include +#include #include #include @@ -40,15 +42,30 @@ #include #include #include +#include /* nop stub */ void _paravirt_nop(void) { } +int arch_is_vsmp = 0; + /* natively, we do normal setup, but we still need to return something */ static int native_arch_setup(void) { + if (!early_pci_allowed()) + goto out; + + if ((read_pci_config_16(0, 0x1f, 0, PCI_VENDOR_ID) == PCI_VENDOR_ID_SCALEMP) && +(read_pci_config_16(0, 0x1f, 0, PCI_DEVICE_ID) == PCI_DEVICE_ID_SCALEMP_VSMP_CTL)) { + paravirt_ops.irq_disable = vsmp_irq_disable; + paravirt_ops.irq_enable = vsmp_irq_enable; + paravirt_ops.save_fl = vsmp_save_flags; + arch_is_vsmp = 1; + } + +out: return 0; } @@ -103,8 +120,6 @@ static unsigned native_patch(u8 type, u16 clobbers, void *insns, unsigned len) switch(type) { #define SITE(x) case PARAVIRT_PATCH(x): start = start_##x; end = end_##x; goto patch_site - SITE(irq_disable); - SITE(irq_enable); SITE(restore_fl); SITE(save_fl); SITE(iret); @@ -117,7 +132,23 @@ static unsigned native_patch(u8 type, u16 clobbers, void *insns, unsigned len) SITE(flush_tlb_single); SITE(wbinvd); #undef SITE - + case PARAVIRT_PATCH(irq_disable): + case PARAVIRT_PATCH(irq_enable): + start = start_irq_disable; + end = end_irq_disable; + + if (type == PARAVIRT_PATCH(irq_enable)) { + start = start_irq_enable; + end = end_irq_enable; + } + + if (arch_is_vsmp) { + ret = paravirt_patch_default(type, + clobbers, + insns, + len); + break; + } patch_site: ret = paravirt_patch_insns(insns, len, start, end); break; @@ -214,30 +245,6 @@ void init_IRQ(void) paravirt_ops.init_IRQ(); } -static unsigned long native_save_fl(void) -{ - unsigned long f; - asm volatile("pushfq ; popq %0":"=g" (f): /* no input */); - return f; -} - -static void native_restore_fl(unsigned long f) -{ - asm volatile("pushq %0 ; popfq": /* no output */ - :"g" (f) - :"memory", "cc"); -} - -static void native_irq_disable(void) -{ - asm volatile("cli": : :"memory"); -
Re: [PATCH 3/25][V3] irq_flags / halt routines
* Glauber de Oliveira Costa ([EMAIL PROTECTED]) wrote: > Only caveat, is that it has to be done before smp gets in the game, and > with interrupts disabled. (which makes the function in vsmp.c not eligible). > > My current option is to force VSMP to use PARAVIRT, as said before, and > then fill paravirt_arch_setup, which is currently unused, with code to > replace the needed paravirt_ops.fn. > > I don't know if there is any method to dynamically determine (at this > point) that we are in a vsmp arch, and if there are not, it will have to > get ifdefs anyway. But at least, they are far more local. between __cacheline_aligned_in_smp and other compile time bits based on VSMP specific INTERNODE_CACHE, etc. I think compile time the way to go. > I am okay with both, but after all the explanation, I don't think that > adding a new pvops is a bad idea. It would make things less cumbersome > in this case. Also, hacks like this save_fl may require changes to the > hypervisor, right? I don't even know where such hypervisor is, and how > easy it is to replace it (it may be deeply hidden in firmware) No hypervisor change needed. Just the pv backend needs to return 0 or X86_EFLAGS_IF for save_flags (and similar translation on restore_flags). Xen uses a simple shared memory flag and does something which you could roughly translate into this: xen_save_flags() if (xen_vcpu_interrupts_enabled) return X86_EFLAGS_IF; else return 0; This doesn't require any hypervisor changes. Similarly, VSMP could do something along the lines of: vsmp_save_flags() flags = native_save_flags(); if (flags & X86_EFLAGS_IF) || (flags & X86_EFLAGS_AC) return X86_EFLAGS_IF; else return 0; > A question raises here: Would vsmp turn paravirt_enabled to 1 ? Probably not. It's mostly native and I'm not sure it would want the bits disabled from if (paravirt_enabled()) tests. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
Chris Wright escreveu: * Glauber de Oliveira Costa ([EMAIL PROTECTED]) wrote: As alternatives what we have now, we can either keep the paravirt_ops as it is now for the native case, just hooking the vsmp functions in place of the normal one, (there are just three ops anyway), refill the paravirt_ops entirely in somewhere like vsmp.c, or similar (or maybe even assigning paravirt_ops.fn = vsmp_fn on the fly, but early enough). It will definitely keep the code shorter, and to be honest, I'd feel more confortable with (since I don't know the subtles of the architecture). Only caveat, is that it has to be done before smp gets in the game, and with interrupts disabled. (which makes the function in vsmp.c not eligible). My current option is to force VSMP to use PARAVIRT, as said before, and then fill paravirt_arch_setup, which is currently unused, with code to replace the needed paravirt_ops.fn. I don't know if there is any method to dynamically determine (at this point) that we are in a vsmp arch, and if there are not, it will have to get ifdefs anyway. But at least, they are far more local. This is the best (just override pvops.fn for the few needed for VSMP). The irq_disabled_flags() is the only problem. For i386 we dropped it (disabled_flags) as a pvop and forced the backend to provide a flags (via save_flags) that conforms to IF only. I am okay with both, but after all the explanation, I don't think that adding a new pvops is a bad idea. It would make things less cumbersome in this case. Also, hacks like this save_fl may require changes to the hypervisor, right? I don't even know where such hypervisor is, and how easy it is to replace it (it may be deeply hidden in firmware) A question raises here: Would vsmp turn paravirt_enabled to 1 ? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
* Glauber de Oliveira Costa ([EMAIL PROTECTED]) wrote: > As alternatives what we have now, we can either keep the paravirt_ops as > it is now for the native case, just hooking the vsmp functions in place > of the normal one, (there are just three ops anyway), refill the > paravirt_ops entirely in somewhere like vsmp.c, or similar (or maybe > even assigning paravirt_ops.fn = vsmp_fn on the fly, but early enough). This is the best (just override pvops.fn for the few needed for VSMP). The irq_disabled_flags() is the only problem. For i386 we dropped it (disabled_flags) as a pvop and forced the backend to provide a flags (via save_flags) that conforms to IF only. thanks, -chris - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
> Maybe we could even make VSMP depend on PARAVIRT, to make it sure it is > completely a paravirt client. That's the right thing to do I think. Remove the existing ifdefs and hook vsmp in only using paravirt ops. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
Andi Kleen escreveu: On Wed, Aug 15, 2007 at 12:09:42PM -0300, Glauber de Oliveira Costa wrote: Again, this is the code of such function: static inline int raw_irqs_disabled_flags(unsigned long flags) { return !(flags & X86_EFLAGS_IF); } so all it is doing is getting a parameter (flags), and bitmasking it. It is not talking to any hypervisor. I can't see your point. Unless you are arguing that it _should_ be talking to a hypervisor. Is that your point? vSMP is a hypervisor based architecture. For some reason that is not 100% clear to me, but Kiran or Shai can probably explain, it needs this additional bit in EFLAGS when interrupts are disabled. That gives it some hints and then it goes somehow faster. That is clearly paravirtualization. Since paravirtops is designed to handle such hooks cleanly I request that you move vSMP over to it or work with the vSMP maintainers to do that. Otherwise we have two different ways to do paravirtualization which is wrong. Thanks for the explanation, Andi. I understand it much better now, and agree with you. As alternatives what we have now, we can either keep the paravirt_ops as it is now for the native case, just hooking the vsmp functions in place of the normal one, (there are just three ops anyway), refill the paravirt_ops entirely in somewhere like vsmp.c, or similar (or maybe even assigning paravirt_ops.fn = vsmp_fn on the fly, but early enough). Maybe we could even make VSMP depend on PARAVIRT, to make it sure it is completely a paravirt client. But as you could see, my knowledge of vsmp does not go that far, and I would really like to have input from the vsmp guys prior to touch anything here. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
On Wed, Aug 15, 2007 at 12:09:42PM -0300, Glauber de Oliveira Costa wrote: > Again, this is the code of such function: > > static inline int raw_irqs_disabled_flags(unsigned long flags) > { > return !(flags & X86_EFLAGS_IF); > } > so all it is doing is getting a parameter (flags), and bitmasking it. It > is not talking to any hypervisor. I can't see your point. Unless you are > arguing that it _should_ be talking to a hypervisor. Is that your point? vSMP is a hypervisor based architecture. For some reason that is not 100% clear to me, but Kiran or Shai can probably explain, it needs this additional bit in EFLAGS when interrupts are disabled. That gives it some hints and then it goes somehow faster. That is clearly paravirtualization. Since paravirtops is designed to handle such hooks cleanly I request that you move vSMP over to it or work with the vSMP maintainers to do that. Otherwise we have two different ways to do paravirtualization which is wrong. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
Avi Kivity escreveu: Glauber de Oliveira Costa wrote: Andi Kleen escreveu: On Wed, Aug 15, 2007 at 11:18:25AM -0300, Glauber de Oliveira Costa wrote: Didn't we agree this should be a pvops client? -Andi No. I exposed my reasoning, asked you back, but got no answer. I'll do it again: This operations are just manipulating bits, and are doing no privileged operations at all. Nothing that can be paravirtualized, in It's talking to a Hypervisor. That is privileged enough. Please do that change. If you add so many more ifdefs it's your duty to keep the overall number low. Again, this is the code of such function: static inline int raw_irqs_disabled_flags(unsigned long flags) { return !(flags & X86_EFLAGS_IF); } so all it is doing is getting a parameter (flags), and bitmasking it. It is not talking to any hypervisor. I can't see your point. Unless you are arguing that it _should_ be talking to a hypervisor. Is that your point? It is talking to a hypervisor. This hypervisor does full virtualization, except that it allows the guest to hide eflags.IF inside eflags.AC as an optimization (otherwise you need to do binary translation to overcome popf silently disregarding IF on the stack). You can regard eflags.AC as the paravirtualized eflags.IF (Xen for example has a per-vcpu memory flag for the same). Thanks Avi, I understand it now. Andi, I will update it and resend shortly. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
Glauber de Oliveira Costa wrote: Andi Kleen escreveu: On Wed, Aug 15, 2007 at 11:18:25AM -0300, Glauber de Oliveira Costa wrote: Didn't we agree this should be a pvops client? -Andi No. I exposed my reasoning, asked you back, but got no answer. I'll do it again: This operations are just manipulating bits, and are doing no privileged operations at all. Nothing that can be paravirtualized, in It's talking to a Hypervisor. That is privileged enough. Please do that change. If you add so many more ifdefs it's your duty to keep the overall number low. Again, this is the code of such function: static inline int raw_irqs_disabled_flags(unsigned long flags) { return !(flags & X86_EFLAGS_IF); } so all it is doing is getting a parameter (flags), and bitmasking it. It is not talking to any hypervisor. I can't see your point. Unless you are arguing that it _should_ be talking to a hypervisor. Is that your point? It is talking to a hypervisor. This hypervisor does full virtualization, except that it allows the guest to hide eflags.IF inside eflags.AC as an optimization (otherwise you need to do binary translation to overcome popf silently disregarding IF on the stack). You can regard eflags.AC as the paravirtualized eflags.IF (Xen for example has a per-vcpu memory flag for the same). -- error compiling committee.c: too many arguments to function - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
Andi Kleen escreveu: On Wed, Aug 15, 2007 at 11:18:25AM -0300, Glauber de Oliveira Costa wrote: Didn't we agree this should be a pvops client? -Andi No. I exposed my reasoning, asked you back, but got no answer. I'll do it again: This operations are just manipulating bits, and are doing no privileged operations at all. Nothing that can be paravirtualized, in It's talking to a Hypervisor. That is privileged enough. Please do that change. If you add so many more ifdefs it's your duty to keep the overall number low. Again, this is the code of such function: static inline int raw_irqs_disabled_flags(unsigned long flags) { return !(flags & X86_EFLAGS_IF); } so all it is doing is getting a parameter (flags), and bitmasking it. It is not talking to any hypervisor. I can't see your point. Unless you are arguing that it _should_ be talking to a hypervisor. Is that your point? If it is the case, please tell me why. My current understanding is that we want to keep few changes from the normal kernel. So there is not too much reason for it. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
On Wed, Aug 15, 2007 at 11:18:25AM -0300, Glauber de Oliveira Costa wrote: > > Didn't we agree this should be a pvops client? > > > > -Andi > > > No. I exposed my reasoning, asked you back, but got no answer. > I'll do it again: > > This operations are just manipulating bits, and are doing no > privileged operations at all. Nothing that can be paravirtualized, in It's talking to a Hypervisor. That is privileged enough. Please do that change. If you add so many more ifdefs it's your duty to keep the overall number low. Various other paravirt ops also do things which are not strictly privileged. -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
> Didn't we agree this should be a pvops client? > > -Andi > No. I exposed my reasoning, asked you back, but got no answer. I'll do it again: This operations are just manipulating bits, and are doing no privileged operations at all. Nothing that can be paravirtualized, in the proper sense. Altough we do can introduce such operations for clarity of code, I personally believe it is not the way to go. What I did, then, was move this outside the PARAVIRT ifdef, which lead to a much cleaner code. -- Glauber de Oliveira Costa. "Free as in Freedom" http://glommer.net "The less confident you are, the more serious you have to act." - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/25][V3] irq_flags / halt routines
> +#ifdef CONFIG_X86_VSMP > +static inline int raw_irqs_disabled_flags(unsigned long flags) > +{ > + return !(flags & X86_EFLAGS_IF) || (flags & X86_EFLAGS_AC); > +} > + > +#else > static inline int raw_irqs_disabled_flags(unsigned long flags) > { > return !(flags & X86_EFLAGS_IF); > } > > -#endif > +#endif /* CONFIG_X86_VSMP */ Didn't we agree this should be a pvops client? -Andi - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/