Re: [RFC PATCH 09/11] kvm: simplify processor compat check
On Sun, Sep 29, 2013 at 08:35:16PM +0530, Aneesh Kumar K.V wrote:
> Gleb Natapov writes:
>
> > On Sat, Sep 28, 2013 at 09:06:47PM +0530, Aneesh Kumar K.V wrote:
> >> Paolo Bonzini writes:
> >>
> >> > Il 27/09/2013 15:13, Aneesh Kumar K.V ha scritto:
> >> >> Alexander Graf writes:
> >> >>
> >> >>> On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote:
> >> >>>
> >> From: "Aneesh Kumar K.V"
> >> >>>
> >> >>> Missing patch description.
> >> >>>
> >> Signed-off-by: Aneesh Kumar K.V
> >> >>>
> >> >>> I fail to see how this really simplifies things, but at the end of the
> >> >>> day it's Gleb's and Paolo's call.
> >> >>
> >> >> will do. It avoid calling
> >> >>
> >> >> for_each_online_cpu(cpu) {
> >> >> smp_call_function_single()
> >> >>
> >> >> on multiple architecture.
> >> >
> >> > I agree with Alex.
> >> >
> >> > The current code is not specially awesome; having
> >> > kvm_arch_check_processor_compat take an int* disguised as a void* is a
> >> > bit ugly indeed.
> >> >
> >> > However, the API makes sense and tells you that it is being passed as a
> >> > callback (to smp_call_function_single in this case).
> >>
> >> But whether to check on all cpus or not is arch dependent right?.
> >> IIUC only x86 and ppc64 need to do that. Also on ppc64 it really
> >> depends on whether HV or PR. We need to check on all cpus only if it is
> >> HV.
> >>
> >> >
> >> > You are making the API more complicated to use on the arch layer
> >> > (because arch maintainers now have to think "do I need to check this on
> >> > all online CPUs?") and making the "leaf" POWER code less legible because
> >> > it still has the weird void()(void *) calling convention.
> >> >
> >>
> >> IIUC what we wanted to check is to find out whether kvm can run on this
> >> system. That is really an arch specific check. So for core kvm the call
> >> should be a simple
> >>
> >> if (kvm_arch_check_process_compat() < 0)
> >> error;
> > We have that already, just return error from kvm_arch_hardware_setup. This
> > is specific processor compatibility check and you are arguing that the
> > processor check should be part of kvm_arch_hardware_setup().
>
>
> What about the success case ?. ie, on arch like arm we do
>
> void kvm_arch_check_processor_compat(void *rtn)
> {
> *(int *)rtn = 0;
> }
>
> for_each_online_cpu(cpu) {
As I said they opted out from doing the check. They may reconsider after
first bad HW will be discovered.
> smp_call_function_single(cpu,
> kvm_arch_check_processor_compat,
> &r, 1);
> if (r < 0)
> goto out_free_1;
> }
>
> There is no need to do that for loop for arm.
It's done once during module initialisation. Why is this a big deal?
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 09/11] kvm: simplify processor compat check
Gleb Natapov writes:
> On Sat, Sep 28, 2013 at 09:06:47PM +0530, Aneesh Kumar K.V wrote:
>> Paolo Bonzini writes:
>>
>> > Il 27/09/2013 15:13, Aneesh Kumar K.V ha scritto:
>> >> Alexander Graf writes:
>> >>
>> >>> On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote:
>> >>>
>> From: "Aneesh Kumar K.V"
>> >>>
>> >>> Missing patch description.
>> >>>
>> Signed-off-by: Aneesh Kumar K.V
>> >>>
>> >>> I fail to see how this really simplifies things, but at the end of the
>> >>> day it's Gleb's and Paolo's call.
>> >>
>> >> will do. It avoid calling
>> >>
>> >> for_each_online_cpu(cpu) {
>> >> smp_call_function_single()
>> >>
>> >> on multiple architecture.
>> >
>> > I agree with Alex.
>> >
>> > The current code is not specially awesome; having
>> > kvm_arch_check_processor_compat take an int* disguised as a void* is a
>> > bit ugly indeed.
>> >
>> > However, the API makes sense and tells you that it is being passed as a
>> > callback (to smp_call_function_single in this case).
>>
>> But whether to check on all cpus or not is arch dependent right?.
>> IIUC only x86 and ppc64 need to do that. Also on ppc64 it really
>> depends on whether HV or PR. We need to check on all cpus only if it is
>> HV.
>>
>> >
>> > You are making the API more complicated to use on the arch layer
>> > (because arch maintainers now have to think "do I need to check this on
>> > all online CPUs?") and making the "leaf" POWER code less legible because
>> > it still has the weird void()(void *) calling convention.
>> >
>>
>> IIUC what we wanted to check is to find out whether kvm can run on this
>> system. That is really an arch specific check. So for core kvm the call
>> should be a simple
>>
>> if (kvm_arch_check_process_compat() < 0)
>> error;
> We have that already, just return error from kvm_arch_hardware_setup. This
> is specific processor compatibility check and you are arguing that the
> processor check should be part of kvm_arch_hardware_setup().
What about the success case ?. ie, on arch like arm we do
void kvm_arch_check_processor_compat(void *rtn)
{
*(int *)rtn = 0;
}
for_each_online_cpu(cpu) {
smp_call_function_single(cpu,
kvm_arch_check_processor_compat,
&r, 1);
if (r < 0)
goto out_free_1;
}
There is no need to do that for loop for arm.
The only reason I wanted this patch in the series is to make
kvm_arch_check_processor_compat take additional argument opaque.
I am dropping that requirement in the last patch. Considering
that we have objection to this one, I will drop this patch in
the next posting by rearranging the patches.
-aneesh
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] KVM: Implement default IRQ routing
On Thu, Sep 26, 2013 at 10:00:59AM +1000, Paul Mackerras wrote: > On Mon, Sep 23, 2013 at 09:34:01PM +0300, Gleb Natapov wrote: > > On Mon, Sep 23, 2013 at 09:24:14PM +1000, Paul Mackerras wrote: > > > On Sun, Sep 22, 2013 at 03:32:53PM +0300, Gleb Natapov wrote: > > > > On Tue, Sep 17, 2013 at 07:18:40PM +1000, Paul Mackerras wrote: > > > > > This implements a simple way to express the case of IRQ routing where > > > > > there is one in-kernel PIC and the system interrupts (GSIs) are routed > > > > > 1-1 to the corresponding pins of the PIC. This is expressed by having > > > > > kvm->irq_routing == NULL with a skeleton irq routing entry in the new > > > > > kvm->default_irq_route field. > > > > > > > > > > This provides a convenient way to provide backwards compatibility when > > > > > adding IRQ routing capability to an existing in-kernel PIC, such as > > > > > the > > > > > XICS emulation on powerpc. > > > > > > > > > Why not create simple 1-1 irq routing table? It will take a little bit > > > > more memory, but there will be no need for kvm->irq_routing == NULL > > > > special handling. > > > > > > The short answer is that userspace wants to use interrupt source > > > numbers (i.e. pin numbers for the inputs to the emulated XICS) that > > > are scattered throughout a large space, since that mirrors what real > > > hardware does. More specifically, hardware divides up the interrupt > > > source number into two fields, each of typically 12 bits, where the > > > more significant field identifies an "interrupt source unit" (ISU) and > > > the less significant field identifies an interrupt within the ISU. > > > Each PCI host bridge would have an ISU, for example, and there can be > > > ISUs associated with other things that attach directly to the > > > interconnect fabric (coprocessors, cluster interconnects, etc.). > > > > > > Today, QEMU creates a virtual ISU numbered 1 for the emulated PCI host > > > bridge, which means for example that virtio devices get interrupt pin > > > numbers starting at 4096. > > > > > > So, I could have increased KVM_IRQCHIP_NUM_PINS to some multiple of > > > 4096, say 16384, which would allow for 3 ISUs. But that would bloat > > > out struct kvm_irq_routing_table to over 64kB, and if I wanted 1-1 > > > mappings between GSI and pins for all of them, the routing table would > > > be over 960kB. > > > > > Yes, this is not an option. GSI is just a cookie for anything but x86 > > non MSI interrupts. So the way to use irq routing table to deliver XICS irqs > > is to register GSI->XICS irq mapping and by triggering "GSI", which is > > just an arbitrary number, userspace tells kernel that XICS irq, that was > > registered for that GSI, should be injected. > > Yes, that's fine as far as it goes, but the trouble is that the > existing data structures (specifically the chip[][] array in struct > kvm_irq_routing_table) don't handle well the case where the pin > numbers are large and/or sparse. > > In other words, using a small compact set of GSI numbers wouldn't > help, because it's not the GSI -> pin mapping that is the problem, it > is the reverse pin -> GSI mapping. > That's internal implementation detail that can be redesigned if needed, we can let architecture define how irq is mapped back to a GSI (cookie). Buts since you mentioned chip[][] here I have a question about patch 2: in kvm_set_routing_entry() you check irq against KVM_IRQCHIP_NUM_PINS which is defined to be 256 for powerpc, but according to what you described above about 12 bit ISU/irq devision this is not enough to inject any interrupt with ISU 1. You would have had at least 256 irqs in each ISU if you reused irqchip to specify ISU, but you didn't do it. Why not extend a union in kvm_irq_routing_entry with xics specific structure that will specify ISU/irq withing ISU? Who irq numbers inside ISU are assigned? -- Gleb. -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 09/11] kvm: simplify processor compat check
On Sat, Sep 28, 2013 at 09:06:47PM +0530, Aneesh Kumar K.V wrote:
> Paolo Bonzini writes:
>
> > Il 27/09/2013 15:13, Aneesh Kumar K.V ha scritto:
> >> Alexander Graf writes:
> >>
> >>> On 27.09.2013, at 12:03, Aneesh Kumar K.V wrote:
> >>>
> From: "Aneesh Kumar K.V"
> >>>
> >>> Missing patch description.
> >>>
> Signed-off-by: Aneesh Kumar K.V
> >>>
> >>> I fail to see how this really simplifies things, but at the end of the
> >>> day it's Gleb's and Paolo's call.
> >>
> >> will do. It avoid calling
> >>
> >>for_each_online_cpu(cpu) {
> >>smp_call_function_single()
> >>
> >> on multiple architecture.
> >
> > I agree with Alex.
> >
> > The current code is not specially awesome; having
> > kvm_arch_check_processor_compat take an int* disguised as a void* is a
> > bit ugly indeed.
> >
> > However, the API makes sense and tells you that it is being passed as a
> > callback (to smp_call_function_single in this case).
>
> But whether to check on all cpus or not is arch dependent right?.
> IIUC only x86 and ppc64 need to do that. Also on ppc64 it really
> depends on whether HV or PR. We need to check on all cpus only if it is
> HV.
>
> >
> > You are making the API more complicated to use on the arch layer
> > (because arch maintainers now have to think "do I need to check this on
> > all online CPUs?") and making the "leaf" POWER code less legible because
> > it still has the weird void()(void *) calling convention.
> >
>
> IIUC what we wanted to check is to find out whether kvm can run on this
> system. That is really an arch specific check. So for core kvm the call
> should be a simple
>
> if (kvm_arch_check_process_compat() < 0)
> error;
We have that already, just return error from kvm_arch_hardware_setup. This
is specific processor compatibility check and you are arguing that the
processor check should be part of kvm_arch_hardware_setup().
>
> Now how each arch figure out whether kvm can run on this system should
> be arch specific. For x86 we do need to check all the cpus. On ppc64 for
> HV we need to. For other archs we always allow kvm.
>
This is really a sanity check. Theoretically on x86 we also should
not need to check all cpus since SMP configuration with different cpu
models is not supported by the architecture (AFAIK), but bugs happen
(BIOS bugs may cause difference in capabilities for instance). So some
arches opted out from this sanity check for now and this is their choice,
but the code makes it explicit what are we checking here.
>
> > If anything, you could change kvm_arch_check_processor_compat to return
> > an int and accept no argument, and introduce a wrapper that kvm_init
> > passes to smp_call_function_single.
>
> What i am suggesting in the patch is to avoid calling
> smp_call_function_single from kvm_init and let arch decide whether to
> check on all cpus or not.
>
> -aneesh
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
