| Message ID | 1663d06172cffa723e00893837ba04634f061fc8.1596768588.git.haibo.xu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Add vSPE support to KVM guest | expand |
On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote: > Add a virtual SPE device for virt machine while using PPI > 5 for SPE overflow interrupt number. Any reason PPI 5 was selected? > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > --- > hw/arm/virt-acpi-build.c | 3 +++ > hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++ > include/hw/acpi/acpi-defs.h | 1 + > include/hw/arm/virt.h | 1 + > target/arm/cpu.c | 2 ++ > target/arm/cpu.h | 2 ++ > target/arm/kvm.c | 6 ++++++ > 7 files changed, 57 insertions(+) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 91f0df7b13..5073ba22a5 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > } > + if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > + gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ)); > + } > if (vms->virt) { > gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ)); > } > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index ecfee362a1..c40819705d 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms) > } > } > > +static void fdt_add_spe_nodes(const VirtMachineState *vms) > +{ > + CPUState *cpu; > + ARMCPU *armcpu; > + uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI; > + > + CPU_FOREACH(cpu) { > + armcpu = ARM_CPU(cpu); > + if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > + return; > + } > + if (kvm_enabled()) { > + if (kvm_irqchip_in_kernel()) { > + kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ)); > + } > + kvm_arm_spe_init(cpu); > + } > + } > + > + if (vms->gic_version == VIRT_GIC_VERSION_2) { > + irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START, > + GIC_FDT_IRQ_PPI_CPU_WIDTH, > + (1 << vms->smp_cpus) - 1); > + } > + > + armcpu = ARM_CPU(qemu_get_cpu(0)); > + qemu_fdt_add_subnode(vms->fdt, "/spe"); > + if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) { > + const char compat[] = "arm,statistical-profiling-extension-v1"; > + qemu_fdt_setprop(vms->fdt, "/spe", "compatible", > + compat, sizeof(compat)); > + qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts", > + GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags); > + } > +} Please look at [1]. I've refactored fdt_add_pmu_nodes(), so it no longer makes a good pattern for this function. [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727588.html > + > static inline DeviceState *create_acpi_ged(VirtMachineState *vms) > { > DeviceState *dev; > @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms) > qdev_get_gpio_in(vms->gic, ppibase > + VIRTUAL_PMU_IRQ)); > > + qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0, > + qdev_get_gpio_in(vms->gic, ppibase > + + VIRTUAL_SPE_IRQ)); > + > sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > sysbus_connect_irq(gicbusdev, i + smp_cpus, > qdev_get_gpio_in(cpudev, ARM_CPU_FIQ)); > @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine) > > fdt_add_pmu_nodes(vms); > > + fdt_add_spe_nodes(vms); > + You didn't add any compat code, which means all virt machine types are now getting an SPE FDT node, ACPI table change, and, most importantly, PPI 5 has gone from unallocated to allocated. We definitely need compat code. > create_uart(vms, VIRT_UART, sysmem, serial_hd(0)); > > if (vms->secure) { > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index 38a42f409a..56a7f38ae4 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface { > uint32_t vgic_interrupt; > uint64_t gicr_base_address; > uint64_t arm_mpidr; > + uint16_t spe_interrupt; /* ACPI 6.3 */ > } QEMU_PACKED; > > typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface; > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index dff67e1bef..56c83224d2 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -49,6 +49,7 @@ > #define ARCH_TIMER_NS_EL1_IRQ 14 > #define ARCH_TIMER_NS_EL2_IRQ 10 > > +#define VIRTUAL_SPE_IRQ 5 > #define VIRTUAL_PMU_IRQ 7 > > #define PPI(irq) ((irq) + 16) > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 40768b4d19..67ab0089fd 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj) > "gicv3-maintenance-interrupt", 1); > qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt, > "pmu-interrupt", 1); > + qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt, > + "spe-interrupt", 1); > #endif > > /* DTB consumers generally don't in fact care what the 'compatible' > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index fe0ac14386..4bf8591df8 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -790,6 +790,8 @@ struct ARMCPU { > qemu_irq gicv3_maintenance_interrupt; > /* GPIO output for the PMU interrupt */ > qemu_irq pmu_interrupt; > + /* GPIO output for the SPE interrupt */ > + qemu_irq spe_interrupt; > > /* MemoryRegion to use for secure physical accesses */ > MemoryRegion *secure_memory; > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index 58f991e890..ecafdda364 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run) > switched_level &= ~KVM_ARM_DEV_PMU; > } > > + if (switched_level & KVM_ARM_DEV_SPE) { > + qemu_set_irq(cpu->spe_interrupt, > + !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE)); > + switched_level &= ~KVM_ARM_DEV_SPE; > + } > + Did you test with a userspace irqchip? > if (switched_level) { > qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n", > __func__, switched_level); > -- > 2.17.1 > > Thanks, drew
On Mon, 10 Aug 2020 at 19:05, Andrew Jones <drjones@redhat.com> wrote: > > On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote: > > Add a virtual SPE device for virt machine while using PPI > > 5 for SPE overflow interrupt number. > > Any reason PPI 5 was selected? > No special reason to choose PPI 5. Just re-use the setting in kvmtool. > > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > > --- > > hw/arm/virt-acpi-build.c | 3 +++ > > hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++ > > include/hw/acpi/acpi-defs.h | 1 + > > include/hw/arm/virt.h | 1 + > > target/arm/cpu.c | 2 ++ > > target/arm/cpu.h | 2 ++ > > target/arm/kvm.c | 6 ++++++ > > 7 files changed, 57 insertions(+) > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index 91f0df7b13..5073ba22a5 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > > if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { > > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > > } > > + if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > > + gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ)); > > + } > > if (vms->virt) { > > gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ)); > > } > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index ecfee362a1..c40819705d 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms) > > } > > } > > > > +static void fdt_add_spe_nodes(const VirtMachineState *vms) > > +{ > > + CPUState *cpu; > > + ARMCPU *armcpu; > > + uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI; > > + > > + CPU_FOREACH(cpu) { > > + armcpu = ARM_CPU(cpu); > > + if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > > + return; > > + } > > + if (kvm_enabled()) { > > + if (kvm_irqchip_in_kernel()) { > > + kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ)); > > + } > > + kvm_arm_spe_init(cpu); > > + } > > + } > > + > > + if (vms->gic_version == VIRT_GIC_VERSION_2) { > > + irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START, > > + GIC_FDT_IRQ_PPI_CPU_WIDTH, > > + (1 << vms->smp_cpus) - 1); > > + } > > + > > + armcpu = ARM_CPU(qemu_get_cpu(0)); > > + qemu_fdt_add_subnode(vms->fdt, "/spe"); > > + if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) { > > + const char compat[] = "arm,statistical-profiling-extension-v1"; > > + qemu_fdt_setprop(vms->fdt, "/spe", "compatible", > > + compat, sizeof(compat)); > > + qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts", > > + GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags); > > + } > > +} > > Please look at [1]. I've refactored fdt_add_pmu_nodes(), so it no longer > makes a good pattern for this function. > > [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg727588.html > > Ok, I will spend some time studying your patches, and rework the related patches in the next version. > > + > > static inline DeviceState *create_acpi_ged(VirtMachineState *vms) > > { > > DeviceState *dev; > > @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms) > > qdev_get_gpio_in(vms->gic, ppibase > > + VIRTUAL_PMU_IRQ)); > > > > + qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0, > > + qdev_get_gpio_in(vms->gic, ppibase > > + + VIRTUAL_SPE_IRQ)); > > + > > sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > > sysbus_connect_irq(gicbusdev, i + smp_cpus, > > qdev_get_gpio_in(cpudev, ARM_CPU_FIQ)); > > @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine) > > > > fdt_add_pmu_nodes(vms); > > > > + fdt_add_spe_nodes(vms); > > + > > You didn't add any compat code, which means all virt machine types are now > getting an SPE FDT node, ACPI table change, and, most importantly, PPI 5 > has gone from unallocated to allocated. We definitely need compat code. > So the 'compat code' here means to only add the SPE node in KVM mode? > > create_uart(vms, VIRT_UART, sysmem, serial_hd(0)); > > > > if (vms->secure) { > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > index 38a42f409a..56a7f38ae4 100644 > > --- a/include/hw/acpi/acpi-defs.h > > +++ b/include/hw/acpi/acpi-defs.h > > @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface { > > uint32_t vgic_interrupt; > > uint64_t gicr_base_address; > > uint64_t arm_mpidr; > > + uint16_t spe_interrupt; /* ACPI 6.3 */ > > } QEMU_PACKED; > > > > typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface; > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > > index dff67e1bef..56c83224d2 100644 > > --- a/include/hw/arm/virt.h > > +++ b/include/hw/arm/virt.h > > @@ -49,6 +49,7 @@ > > #define ARCH_TIMER_NS_EL1_IRQ 14 > > #define ARCH_TIMER_NS_EL2_IRQ 10 > > > > +#define VIRTUAL_SPE_IRQ 5 > > #define VIRTUAL_PMU_IRQ 7 > > > > #define PPI(irq) ((irq) + 16) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 40768b4d19..67ab0089fd 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj) > > "gicv3-maintenance-interrupt", 1); > > qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt, > > "pmu-interrupt", 1); > > + qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt, > > + "spe-interrupt", 1); > > #endif > > > > /* DTB consumers generally don't in fact care what the 'compatible' > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index fe0ac14386..4bf8591df8 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -790,6 +790,8 @@ struct ARMCPU { > > qemu_irq gicv3_maintenance_interrupt; > > /* GPIO output for the PMU interrupt */ > > qemu_irq pmu_interrupt; > > + /* GPIO output for the SPE interrupt */ > > + qemu_irq spe_interrupt; > > > > /* MemoryRegion to use for secure physical accesses */ > > MemoryRegion *secure_memory; > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > > index 58f991e890..ecafdda364 100644 > > --- a/target/arm/kvm.c > > +++ b/target/arm/kvm.c > > @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run) > > switched_level &= ~KVM_ARM_DEV_PMU; > > } > > > > + if (switched_level & KVM_ARM_DEV_SPE) { > > + qemu_set_irq(cpu->spe_interrupt, > > + !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE)); > > + switched_level &= ~KVM_ARM_DEV_SPE; > > + } > > + > > Did you test with a userspace irqchip? No, I just tested with an in-kernel irqchip. Actually, the current kernel vSPE patch doesn't support a userspace irqchip. AFAIK, the userspace irqchip support should be ready in the next kernel patch series which will be sent out for review in the middle of September. > > > if (switched_level) { > > qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n", > > __func__, switched_level); > > -- > > 2.17.1 > > > > > > Thanks, > drew >
On Tue, Aug 11, 2020 at 10:38:02AM +0800, Haibo Xu wrote: > On Mon, 10 Aug 2020 at 19:05, Andrew Jones <drjones@redhat.com> wrote: > > > > On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote: > > > Add a virtual SPE device for virt machine while using PPI > > > 5 for SPE overflow interrupt number. > > > > Any reason PPI 5 was selected? > > > > No special reason to choose PPI 5. Just re-use the setting in kvmtool. Please write in the commit message that kvmtool has already selected PPI 5 for this purpose. > > > + fdt_add_spe_nodes(vms); > > > + > > > > You didn't add any compat code, which means all virt machine types are now > > getting an SPE FDT node, ACPI table change, and, most importantly, PPI 5 > > has gone from unallocated to allocated. We definitely need compat code. > > > > So the 'compat code' here means to only add the SPE node in KVM mode? No, it means only add it for the 5.2 and later machine types. You'll see what I mean when you study the patchset I pointed out, which is also only for 5.2 and later machine types. > > > + if (switched_level & KVM_ARM_DEV_SPE) { > > > + qemu_set_irq(cpu->spe_interrupt, > > > + !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE)); > > > + switched_level &= ~KVM_ARM_DEV_SPE; > > > + } > > > + > > > > Did you test with a userspace irqchip? > > No, I just tested with an in-kernel irqchip. > Actually, the current kernel vSPE patch doesn't support a userspace irqchip. > AFAIK, the userspace irqchip support should be ready in the next > kernel patch series > which will be sent out for review in the middle of September. It probably doesn't hurt to do the above hunk already, hoping it will just work when it's possible to test, but I generally prefer only adding tested code. Maybe this hunk should be a separate patch with a commit message explaining that it's untested? Thanks, drew
On Wed, 12 Aug 2020 at 00:40, Andrew Jones <drjones@redhat.com> wrote: > On Tue, Aug 11, 2020 at 10:38:02AM +0800, Haibo Xu wrote: > > On Mon, 10 Aug 2020 at 19:05, Andrew Jones <drjones@redhat.com> wrote: > > > > > > On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote: > > > > Add a virtual SPE device for virt machine while using PPI > > > > 5 for SPE overflow interrupt number. > > > > > > Any reason PPI 5 was selected? > > > > > > > No special reason to choose PPI 5. Just re-use the setting in kvmtool. > > Please write in the commit message that kvmtool has already selected PPI 5 > for this purpose. > Ok, will fix it. > > > > + fdt_add_spe_nodes(vms); > > > > + > > > > > > You didn't add any compat code, which means all virt machine types are > now > > > getting an SPE FDT node, ACPI table change, and, most importantly, PPI > 5 > > > has gone from unallocated to allocated. We definitely need compat code. > > > > > > > So the 'compat code' here means to only add the SPE node in KVM mode? > > No, it means only add it for the 5.2 and later machine types. You'll see > what I mean when you study the patchset I pointed out, which is also only > for 5.2 and later machine types. > Ok, thanks for the clarification! > > > > + if (switched_level & KVM_ARM_DEV_SPE) { > > > > + qemu_set_irq(cpu->spe_interrupt, > > > > + !!(run->s.regs.device_irq_level & > KVM_ARM_DEV_SPE)); > > > > + switched_level &= ~KVM_ARM_DEV_SPE; > > > > + } > > > > + > > > > > > Did you test with a userspace irqchip? > > > > No, I just tested with an in-kernel irqchip. > > Actually, the current kernel vSPE patch doesn't support a userspace > irqchip. > > AFAIK, the userspace irqchip support should be ready in the next > > kernel patch series > > which will be sent out for review in the middle of September. > > It probably doesn't hurt to do the above hunk already, hoping it will just > work when it's possible to test, but I generally prefer only adding tested > code. Maybe this hunk should be a separate patch with a commit message > explaining that it's untested? > Good idea! I will drop the hunk in this series, and send out a separate patch to enable it once the kernel support is ready! > Thanks, > drew > > <div dir="ltr"><div dir="ltr">On Wed, 12 Aug 2020 at 00:40, Andrew Jones <<a href="mailto:drjones@redhat.com">drjones@redhat.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Aug 11, 2020 at 10:38:02AM +0800, Haibo Xu wrote:<br> > On Mon, 10 Aug 2020 at 19:05, Andrew Jones <<a href="mailto:drjones@redhat.com" target="_blank">drjones@redhat.com</a>> wrote:<br> > ><br> > > On Fri, Aug 07, 2020 at 08:10:36AM +0000, Haibo Xu wrote:<br> > > > Add a virtual SPE device for virt machine while using PPI<br> > > > 5 for SPE overflow interrupt number.<br> > ><br> > > Any reason PPI 5 was selected?<br> > ><br> > <br> > No special reason to choose PPI 5. Just re-use the setting in kvmtool.<br> <br> Please write in the commit message that kvmtool has already selected PPI 5<br> for this purpose.<br></blockquote><div><br></div><div>Ok, will fix it.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > + fdt_add_spe_nodes(vms);<br> > > > +<br> > ><br> > > You didn't add any compat code, which means all virt machine types are now<br> > > getting an SPE FDT node, ACPI table change, and, most importantly, PPI 5<br> > > has gone from unallocated to allocated. We definitely need compat code.<br> > ><br> > <br> > So the 'compat code' here means to only add the SPE node in KVM mode?<br> <br> No, it means only add it for the 5.2 and later machine types. You'll see<br> what I mean when you study the patchset I pointed out, which is also only<br> for 5.2 and later machine types.<br></blockquote><div><br></div><div>Ok, thanks for the clarification!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > > + if (switched_level & KVM_ARM_DEV_SPE) {<br> > > > + qemu_set_irq(cpu->spe_interrupt,<br> > > > + !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE));<br> > > > + switched_level &= ~KVM_ARM_DEV_SPE;<br> > > > + }<br> > > > +<br> > ><br> > > Did you test with a userspace irqchip?<br> > <br> > No, I just tested with an in-kernel irqchip.<br> > Actually, the current kernel vSPE patch doesn't support a userspace irqchip.<br> > AFAIK, the userspace irqchip support should be ready in the next<br> > kernel patch series<br> > which will be sent out for review in the middle of September.<br> <br> It probably doesn't hurt to do the above hunk already, hoping it will just<br> work when it's possible to test, but I generally prefer only adding tested<br> code. Maybe this hunk should be a separate patch with a commit message<br> explaining that it's untested?<br></blockquote><div><br></div><div>Good idea! I will drop the hunk in this series, and send out a separate patch to enable it</div><div>once the kernel support is ready!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Thanks,<br> drew<br> <br> </blockquote></div></div>
Hi Haibo, On 8/7/20 10:10 AM, Haibo Xu wrote: > Add a virtual SPE device for virt machine while using PPI > 5 for SPE overflow interrupt number. > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > --- > hw/arm/virt-acpi-build.c | 3 +++ > hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++ > include/hw/acpi/acpi-defs.h | 1 + > include/hw/arm/virt.h | 1 + > target/arm/cpu.c | 2 ++ > target/arm/cpu.h | 2 ++ > target/arm/kvm.c | 6 ++++++ > 7 files changed, 57 insertions(+) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 91f0df7b13..5073ba22a5 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > } > + if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > + gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ)); > + } > if (vms->virt) { > gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ)); > } > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index ecfee362a1..c40819705d 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms) > } > } > > +static void fdt_add_spe_nodes(const VirtMachineState *vms) > +{ > + CPUState *cpu; > + ARMCPU *armcpu; > + uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI; > + > + CPU_FOREACH(cpu) { > + armcpu = ARM_CPU(cpu); > + if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > + return; > + } > + if (kvm_enabled()) { > + if (kvm_irqchip_in_kernel()) { > + kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ)); > + } > + kvm_arm_spe_init(cpu); > + } > + } > + > + if (vms->gic_version == VIRT_GIC_VERSION_2) { > + irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START, > + GIC_FDT_IRQ_PPI_CPU_WIDTH, > + (1 << vms->smp_cpus) - 1); > + } > + > + armcpu = ARM_CPU(qemu_get_cpu(0)); > + qemu_fdt_add_subnode(vms->fdt, "/spe"); > + if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) { > + const char compat[] = "arm,statistical-profiling-extension-v1"; > + qemu_fdt_setprop(vms->fdt, "/spe", "compatible", > + compat, sizeof(compat)); > + qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts", > + GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags); > + } > +} > + > static inline DeviceState *create_acpi_ged(VirtMachineState *vms) > { > DeviceState *dev; > @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms) > qdev_get_gpio_in(vms->gic, ppibase > + VIRTUAL_PMU_IRQ)); > > + qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0, > + qdev_get_gpio_in(vms->gic, ppibase > + + VIRTUAL_SPE_IRQ)); > + > sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > sysbus_connect_irq(gicbusdev, i + smp_cpus, > qdev_get_gpio_in(cpudev, ARM_CPU_FIQ)); > @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine) > > fdt_add_pmu_nodes(vms); > > + fdt_add_spe_nodes(vms); > + > create_uart(vms, VIRT_UART, sysmem, serial_hd(0)); > > if (vms->secure) { > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index 38a42f409a..56a7f38ae4 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface { > uint32_t vgic_interrupt; > uint64_t gicr_base_address; > uint64_t arm_mpidr; > + uint16_t spe_interrupt; /* ACPI 6.3 */ This does not work for me. You miss 2 uint8_t fields inbetween arm_mpdir and spe_interrupt: Processor Power Efficiency Class and Reserved. At the moment arm_spe_acpi_register_device() silently fails on guest side since gicc->header.length < ACPI_MADT_GICC_SPE Thanks Eric > } QEMU_PACKED; > > typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface; > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index dff67e1bef..56c83224d2 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -49,6 +49,7 @@ > #define ARCH_TIMER_NS_EL1_IRQ 14 > #define ARCH_TIMER_NS_EL2_IRQ 10 > > +#define VIRTUAL_SPE_IRQ 5 > #define VIRTUAL_PMU_IRQ 7 > > #define PPI(irq) ((irq) + 16) > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 40768b4d19..67ab0089fd 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj) > "gicv3-maintenance-interrupt", 1); > qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt, > "pmu-interrupt", 1); > + qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt, > + "spe-interrupt", 1); > #endif > > /* DTB consumers generally don't in fact care what the 'compatible' > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index fe0ac14386..4bf8591df8 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -790,6 +790,8 @@ struct ARMCPU { > qemu_irq gicv3_maintenance_interrupt; > /* GPIO output for the PMU interrupt */ > qemu_irq pmu_interrupt; > + /* GPIO output for the SPE interrupt */ > + qemu_irq spe_interrupt; > > /* MemoryRegion to use for secure physical accesses */ > MemoryRegion *secure_memory; > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index 58f991e890..ecafdda364 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run) > switched_level &= ~KVM_ARM_DEV_PMU; > } > > + if (switched_level & KVM_ARM_DEV_SPE) { > + qemu_set_irq(cpu->spe_interrupt, > + !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE)); > + switched_level &= ~KVM_ARM_DEV_SPE; > + } > + > if (switched_level) { > qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n", > __func__, switched_level); >
On Sat, 29 Aug 2020 at 23:22, Auger Eric <eric.auger@redhat.com> wrote: > > Hi Haibo, > > On 8/7/20 10:10 AM, Haibo Xu wrote: > > Add a virtual SPE device for virt machine while using PPI > > 5 for SPE overflow interrupt number. > > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org> > > --- > > hw/arm/virt-acpi-build.c | 3 +++ > > hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++ > > include/hw/acpi/acpi-defs.h | 1 + > > include/hw/arm/virt.h | 1 + > > target/arm/cpu.c | 2 ++ > > target/arm/cpu.h | 2 ++ > > target/arm/kvm.c | 6 ++++++ > > 7 files changed, 57 insertions(+) > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index 91f0df7b13..5073ba22a5 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > > if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { > > gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); > > } > > + if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > > + gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ)); > > + } > > if (vms->virt) { > > gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ)); > > } > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index ecfee362a1..c40819705d 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms) > > } > > } > > > > +static void fdt_add_spe_nodes(const VirtMachineState *vms) > > +{ > > + CPUState *cpu; > > + ARMCPU *armcpu; > > + uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI; > > + > > + CPU_FOREACH(cpu) { > > + armcpu = ARM_CPU(cpu); > > + if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { > > + return; > > + } > > + if (kvm_enabled()) { > > + if (kvm_irqchip_in_kernel()) { > > + kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ)); > > + } > > + kvm_arm_spe_init(cpu); > > + } > > + } > > + > > + if (vms->gic_version == VIRT_GIC_VERSION_2) { > > + irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START, > > + GIC_FDT_IRQ_PPI_CPU_WIDTH, > > + (1 << vms->smp_cpus) - 1); > > + } > > + > > + armcpu = ARM_CPU(qemu_get_cpu(0)); > > + qemu_fdt_add_subnode(vms->fdt, "/spe"); > > + if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) { > > + const char compat[] = "arm,statistical-profiling-extension-v1"; > > + qemu_fdt_setprop(vms->fdt, "/spe", "compatible", > > + compat, sizeof(compat)); > > + qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts", > > + GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags); > > + } > > +} > > + > > static inline DeviceState *create_acpi_ged(VirtMachineState *vms) > > { > > DeviceState *dev; > > @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms) > > qdev_get_gpio_in(vms->gic, ppibase > > + VIRTUAL_PMU_IRQ)); > > > > + qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0, > > + qdev_get_gpio_in(vms->gic, ppibase > > + + VIRTUAL_SPE_IRQ)); > > + > > sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); > > sysbus_connect_irq(gicbusdev, i + smp_cpus, > > qdev_get_gpio_in(cpudev, ARM_CPU_FIQ)); > > @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine) > > > > fdt_add_pmu_nodes(vms); > > > > + fdt_add_spe_nodes(vms); > > + > > create_uart(vms, VIRT_UART, sysmem, serial_hd(0)); > > > > if (vms->secure) { > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > index 38a42f409a..56a7f38ae4 100644 > > --- a/include/hw/acpi/acpi-defs.h > > +++ b/include/hw/acpi/acpi-defs.h > > @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface { > > uint32_t vgic_interrupt; > > uint64_t gicr_base_address; > > uint64_t arm_mpidr; > > + uint16_t spe_interrupt; /* ACPI 6.3 */ > This does not work for me. > You miss 2 uint8_t fields inbetween arm_mpdir and spe_interrupt: > Processor Power Efficiency Class and Reserved. > > At the moment arm_spe_acpi_register_device() silently fails on guest > side since > gicc->header.length < ACPI_MADT_GICC_SPE > > Thanks > > Eric > Thanks for pointing out this. I will fix it in the next version. > > } QEMU_PACKED; > > > > typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface; > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > > index dff67e1bef..56c83224d2 100644 > > --- a/include/hw/arm/virt.h > > +++ b/include/hw/arm/virt.h > > @@ -49,6 +49,7 @@ > > #define ARCH_TIMER_NS_EL1_IRQ 14 > > #define ARCH_TIMER_NS_EL2_IRQ 10 > > > > +#define VIRTUAL_SPE_IRQ 5 > > #define VIRTUAL_PMU_IRQ 7 > > > > #define PPI(irq) ((irq) + 16) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 40768b4d19..67ab0089fd 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj) > > "gicv3-maintenance-interrupt", 1); > > qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt, > > "pmu-interrupt", 1); > > + qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt, > > + "spe-interrupt", 1); > > #endif > > > > /* DTB consumers generally don't in fact care what the 'compatible' > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index fe0ac14386..4bf8591df8 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -790,6 +790,8 @@ struct ARMCPU { > > qemu_irq gicv3_maintenance_interrupt; > > /* GPIO output for the PMU interrupt */ > > qemu_irq pmu_interrupt; > > + /* GPIO output for the SPE interrupt */ > > + qemu_irq spe_interrupt; > > > > /* MemoryRegion to use for secure physical accesses */ > > MemoryRegion *secure_memory; > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > > index 58f991e890..ecafdda364 100644 > > --- a/target/arm/kvm.c > > +++ b/target/arm/kvm.c > > @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run) > > switched_level &= ~KVM_ARM_DEV_PMU; > > } > > > > + if (switched_level & KVM_ARM_DEV_SPE) { > > + qemu_set_irq(cpu->spe_interrupt, > > + !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE)); > > + switched_level &= ~KVM_ARM_DEV_SPE; > > + } > > + > > if (switched_level) { > > qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n", > > __func__, switched_level); > > >
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 91f0df7b13..5073ba22a5 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -666,6 +666,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) { gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ)); } + if (arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { + gicc->spe_interrupt = cpu_to_le32(PPI(VIRTUAL_SPE_IRQ)); + } if (vms->virt) { gicc->vgic_interrupt = cpu_to_le32(PPI(ARCH_GIC_MAINT_IRQ)); } diff --git a/hw/arm/virt.c b/hw/arm/virt.c index ecfee362a1..c40819705d 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -555,6 +555,42 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms) } } +static void fdt_add_spe_nodes(const VirtMachineState *vms) +{ + CPUState *cpu; + ARMCPU *armcpu; + uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI; + + CPU_FOREACH(cpu) { + armcpu = ARM_CPU(cpu); + if (!arm_feature(&armcpu->env, ARM_FEATURE_SPE)) { + return; + } + if (kvm_enabled()) { + if (kvm_irqchip_in_kernel()) { + kvm_arm_spe_set_irq(cpu, PPI(VIRTUAL_SPE_IRQ)); + } + kvm_arm_spe_init(cpu); + } + } + + if (vms->gic_version == VIRT_GIC_VERSION_2) { + irqflags = deposit32(irqflags, GIC_FDT_IRQ_PPI_CPU_START, + GIC_FDT_IRQ_PPI_CPU_WIDTH, + (1 << vms->smp_cpus) - 1); + } + + armcpu = ARM_CPU(qemu_get_cpu(0)); + qemu_fdt_add_subnode(vms->fdt, "/spe"); + if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) { + const char compat[] = "arm,statistical-profiling-extension-v1"; + qemu_fdt_setprop(vms->fdt, "/spe", "compatible", + compat, sizeof(compat)); + qemu_fdt_setprop_cells(vms->fdt, "/spe", "interrupts", + GIC_FDT_IRQ_TYPE_PPI, VIRTUAL_SPE_IRQ, irqflags); + } +} + static inline DeviceState *create_acpi_ged(VirtMachineState *vms) { DeviceState *dev; @@ -727,6 +763,10 @@ static void create_gic(VirtMachineState *vms) qdev_get_gpio_in(vms->gic, ppibase + VIRTUAL_PMU_IRQ)); + qdev_connect_gpio_out_named(cpudev, "spe-interrupt", 0, + qdev_get_gpio_in(vms->gic, ppibase + + VIRTUAL_SPE_IRQ)); + sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ)); sysbus_connect_irq(gicbusdev, i + smp_cpus, qdev_get_gpio_in(cpudev, ARM_CPU_FIQ)); @@ -1915,6 +1955,8 @@ static void machvirt_init(MachineState *machine) fdt_add_pmu_nodes(vms); + fdt_add_spe_nodes(vms); + create_uart(vms, VIRT_UART, sysmem, serial_hd(0)); if (vms->secure) { diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h index 38a42f409a..56a7f38ae4 100644 --- a/include/hw/acpi/acpi-defs.h +++ b/include/hw/acpi/acpi-defs.h @@ -302,6 +302,7 @@ struct AcpiMadtGenericCpuInterface { uint32_t vgic_interrupt; uint64_t gicr_base_address; uint64_t arm_mpidr; + uint16_t spe_interrupt; /* ACPI 6.3 */ } QEMU_PACKED; typedef struct AcpiMadtGenericCpuInterface AcpiMadtGenericCpuInterface; diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index dff67e1bef..56c83224d2 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -49,6 +49,7 @@ #define ARCH_TIMER_NS_EL1_IRQ 14 #define ARCH_TIMER_NS_EL2_IRQ 10 +#define VIRTUAL_SPE_IRQ 5 #define VIRTUAL_PMU_IRQ 7 #define PPI(irq) ((irq) + 16) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 40768b4d19..67ab0089fd 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1038,6 +1038,8 @@ static void arm_cpu_initfn(Object *obj) "gicv3-maintenance-interrupt", 1); qdev_init_gpio_out_named(DEVICE(cpu), &cpu->pmu_interrupt, "pmu-interrupt", 1); + qdev_init_gpio_out_named(DEVICE(cpu), &cpu->spe_interrupt, + "spe-interrupt", 1); #endif /* DTB consumers generally don't in fact care what the 'compatible' diff --git a/target/arm/cpu.h b/target/arm/cpu.h index fe0ac14386..4bf8591df8 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -790,6 +790,8 @@ struct ARMCPU { qemu_irq gicv3_maintenance_interrupt; /* GPIO output for the PMU interrupt */ qemu_irq pmu_interrupt; + /* GPIO output for the SPE interrupt */ + qemu_irq spe_interrupt; /* MemoryRegion to use for secure physical accesses */ MemoryRegion *secure_memory; diff --git a/target/arm/kvm.c b/target/arm/kvm.c index 58f991e890..ecafdda364 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -820,6 +820,12 @@ MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run) switched_level &= ~KVM_ARM_DEV_PMU; } + if (switched_level & KVM_ARM_DEV_SPE) { + qemu_set_irq(cpu->spe_interrupt, + !!(run->s.regs.device_irq_level & KVM_ARM_DEV_SPE)); + switched_level &= ~KVM_ARM_DEV_SPE; + } + if (switched_level) { qemu_log_mask(LOG_UNIMP, "%s: unhandled in-kernel device IRQ %x\n", __func__, switched_level);
Add a virtual SPE device for virt machine while using PPI 5 for SPE overflow interrupt number. Signed-off-by: Haibo Xu <haibo.xu@linaro.org> --- hw/arm/virt-acpi-build.c | 3 +++ hw/arm/virt.c | 42 +++++++++++++++++++++++++++++++++++++ include/hw/acpi/acpi-defs.h | 1 + include/hw/arm/virt.h | 1 + target/arm/cpu.c | 2 ++ target/arm/cpu.h | 2 ++ target/arm/kvm.c | 6 ++++++ 7 files changed, 57 insertions(+) -- 2.17.1