| Message ID | 20230227163718.62003-3-miguel.luis@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU v7.2.0 aarch64 Nested Virtualization Support | expand |
On Mon, 27 Feb 2023 at 16:37, Miguel Luis <miguel.luis@oracle.com> wrote: > > From: Haibo Xu <haibo.xu@linaro.org> > > Use the VGIC maintenance IRQ if VHE is requested. As per the ARM GIC > Architecture Specification for GICv3 and GICv4 Arm strongly recommends that > maintenance interrupts are configured to use INTID 25 matching the > Server Base System Architecture (SBSA) recomendation. What does this mean for QEMU, though? If the issue is "KVM doesn't support the maintenance interrupt being anything other than INTID 25" then we should say so (and have our code error out if the board tries to use some other value). If the issue is "the *host* has to be using the right INTID" then I would hope that KVM simply doesn't expose the capability if the host h/w won't let it work correctly. If KVM can happily use any maintenance interrupt ID that the board model wants, then we should make that work, rather than hardcoding 25 into our gicv3 code. > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index b871350856..377181e009 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -759,6 +759,11 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem) > qdev_prop_set_uint32(vms->gic, "redist-region-count[1]", > MIN(smp_cpus - redist0_count, redist1_capacity)); > } > + > + if (kvm_irqchip_in_kernel()) { > + qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", > + vms->virt); > + } > } else { > if (!kvm_irqchip_in_kernel()) { > qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", > diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c > index 351843db4a..e2a6ff1b49 100644 > --- a/hw/intc/arm_gicv3_common.c > +++ b/hw/intc/arm_gicv3_common.c > @@ -563,6 +563,7 @@ static Property arm_gicv3_common_properties[] = { > DEFINE_PROP_UINT32("revision", GICv3State, revision, 3), > DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0), > DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0), > + DEFINE_PROP_BOOL("has-virtualization-extensions", GICv3State, virt_extn, 0), > /* > * Compatibility property: force 8 bits of physical priority, even > * if the CPU being emulated should have fewer. > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c > index 3ca643ecba..ce924753bb 100644 > --- a/hw/intc/arm_gicv3_kvm.c > +++ b/hw/intc/arm_gicv3_kvm.c > @@ -22,6 +22,7 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "hw/intc/arm_gicv3_common.h" > +#include "hw/arm/virt.h" > #include "qemu/error-report.h" > #include "qemu/module.h" > #include "sysemu/kvm.h" > @@ -803,6 +804,30 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) > return; > } > > + > + if (s->virt_extn) { > + /* > + * Arm strongly recommends that maintenance interrupts are configured > + * to use INTID 25. For more information, see Server Base System > + * Architecture (SBSA) > + */ > + uint32_t maint_irq = PPI(ARCH_GIC_MAINT_IRQ); > + > + struct kvm_device_attr kdevattr = { > + .group = KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, > + .addr = (uint64_t)&maint_irq > + }; > + > + if (!kvm_device_ioctl(s->dev_fd, KVM_GET_DEVICE_ATTR, &kdevattr)) { > + error_setg(errp, "VGICv3 setting maintenance IRQ are not " > + "supported by this host kernel"); > + return; > + } > + > + kvm_device_ioctl(s->dev_fd, KVM_SET_DEVICE_ATTR, &kdevattr); > + } So if I understand this correctly, the requirement here is basically "tell the kernel which IRQ line is being used by the board code for the maintenance interrupt", right? It seems to me that the straightforward way to do that is to have an integer QOM property on the GICv3 device like "maintenance-interrupt-id", and make the board code set it (whether using KVM or not). The TCG implementation doesn't care, and the KVM implementation can set it up in kvm_arm_gicv3_realize(). Then you don't need to specifically tell the GIC device that the guest is using the virtualization extensions. thanks -- PMM
On Mon, 06 Mar 2023 14:02:33 +0000, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 27 Feb 2023 at 16:37, Miguel Luis <miguel.luis@oracle.com> wrote: > > > > From: Haibo Xu <haibo.xu@linaro.org> > > > > Use the VGIC maintenance IRQ if VHE is requested. As per the ARM GIC > > Architecture Specification for GICv3 and GICv4 Arm strongly recommends that > > maintenance interrupts are configured to use INTID 25 matching the > > Server Base System Architecture (SBSA) recomendation. > > What does this mean for QEMU, though? If the issue is > "KVM doesn't support the maintenance interrupt being anything > other than INTID 25" then we should say so (and have our code > error out if the board tries to use some other value). No, KVM doesn't give two hoots about the INTID, as long as this is a PPI that is otherwise unused. > If the > issue is "the *host* has to be using the right INTID" then I > would hope that KVM simply doesn't expose the capability if > the host h/w won't let it work correctly. No host maintenance interrupt, no NV. This is specially mandatory as the L1 guest is in (almost) complete control of the ICH_*_EL2 registers and expects MIs to be delivered. > If KVM can happily > use any maintenance interrupt ID that the board model wants, > then we should make that work, rather than hardcoding 25 into > our gicv3 code. +1. I'd eliminate any reference to SBSA, as it has no bearing on either KVM nor the QEMU GIC code. I also question the "if VHE is requested". Not having VHE doesn't preclude virtualisation. Was that supposed to be "virtualisation extension" instead? Thanks, M.
Hi Peter, > On 6 Mar 2023, at 13:02, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 27 Feb 2023 at 16:37, Miguel Luis <miguel.luis@oracle.com> wrote: >> >> From: Haibo Xu <haibo.xu@linaro.org> >> >> Use the VGIC maintenance IRQ if VHE is requested. As per the ARM GIC >> Architecture Specification for GICv3 and GICv4 Arm strongly recommends that >> maintenance interrupts are configured to use INTID 25 matching the >> Server Base System Architecture (SBSA) recomendation. > > What does this mean for QEMU, though? If the issue is > "KVM doesn't support the maintenance interrupt being anything > other than INTID 25" then we should say so (and have our code > error out if the board tries to use some other value). From the previous work I wondered where did the 25 would come from and I'm in total agreement that this needs a better and meaningful commit message. > If the > issue is "the *host* has to be using the right INTID" then I > would hope that KVM simply doesn't expose the capability if > the host h/w won't let it work correctly. If KVM can happily > use any maintenance interrupt ID that the board model wants, > then we should make that work, rather than hardcoding 25 into > our gicv3 code. > I agree. >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index b871350856..377181e009 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -759,6 +759,11 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem) >> qdev_prop_set_uint32(vms->gic, "redist-region-count[1]", >> MIN(smp_cpus - redist0_count, redist1_capacity)); >> } >> + >> + if (kvm_irqchip_in_kernel()) { >> + qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", >> + vms->virt); >> + } >> } else { >> if (!kvm_irqchip_in_kernel()) { >> qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", >> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c >> index 351843db4a..e2a6ff1b49 100644 >> --- a/hw/intc/arm_gicv3_common.c >> +++ b/hw/intc/arm_gicv3_common.c >> @@ -563,6 +563,7 @@ static Property arm_gicv3_common_properties[] = { >> DEFINE_PROP_UINT32("revision", GICv3State, revision, 3), >> DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0), >> DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0), >> + DEFINE_PROP_BOOL("has-virtualization-extensions", GICv3State, virt_extn, 0), >> /* >> * Compatibility property: force 8 bits of physical priority, even >> * if the CPU being emulated should have fewer. >> diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c >> index 3ca643ecba..ce924753bb 100644 >> --- a/hw/intc/arm_gicv3_kvm.c >> +++ b/hw/intc/arm_gicv3_kvm.c >> @@ -22,6 +22,7 @@ >> #include "qemu/osdep.h" >> #include "qapi/error.h" >> #include "hw/intc/arm_gicv3_common.h" >> +#include "hw/arm/virt.h" >> #include "qemu/error-report.h" >> #include "qemu/module.h" >> #include "sysemu/kvm.h" >> @@ -803,6 +804,30 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> + >> + if (s->virt_extn) { >> + /* >> + * Arm strongly recommends that maintenance interrupts are configured >> + * to use INTID 25. For more information, see Server Base System >> + * Architecture (SBSA) >> + */ >> + uint32_t maint_irq = PPI(ARCH_GIC_MAINT_IRQ); >> + >> + struct kvm_device_attr kdevattr = { >> + .group = KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, >> + .addr = (uint64_t)&maint_irq >> + }; >> + >> + if (!kvm_device_ioctl(s->dev_fd, KVM_GET_DEVICE_ATTR, &kdevattr)) { >> + error_setg(errp, "VGICv3 setting maintenance IRQ are not " >> + "supported by this host kernel"); >> + return; >> + } >> + >> + kvm_device_ioctl(s->dev_fd, KVM_SET_DEVICE_ATTR, &kdevattr); >> + } > > So if I understand this correctly, the requirement here is basically > "tell the kernel which IRQ line is being used by the board code > for the maintenance interrupt", right? From the previous statement I understood it would be better to consider the board code. So, yes. > It seems to me that the > straightforward way to do that is to have an integer QOM property on > the GICv3 device like "maintenance-interrupt-id", and make the > board code set it (whether using KVM or not). Thanks, I’ll look into it. > The TCG implementation > doesn't care, and the KVM implementation can set it up in > kvm_arm_gicv3_realize(). Then you don't need to specifically tell > the GIC device that the guest is using the virtualization extensions. > Yes, that seems better suited. Thank you, Miguel > thanks > -- PMM
Hi Marc, > On 6 Mar 2023, at 13:32, Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 06 Mar 2023 14:02:33 +0000, > Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Mon, 27 Feb 2023 at 16:37, Miguel Luis <miguel.luis@oracle.com> wrote: >>> >>> From: Haibo Xu <haibo.xu@linaro.org> >>> >>> Use the VGIC maintenance IRQ if VHE is requested. As per the ARM GIC >>> Architecture Specification for GICv3 and GICv4 Arm strongly recommends that >>> maintenance interrupts are configured to use INTID 25 matching the >>> Server Base System Architecture (SBSA) recomendation. >> >> What does this mean for QEMU, though? If the issue is >> "KVM doesn't support the maintenance interrupt being anything >> other than INTID 25" then we should say so (and have our code >> error out if the board tries to use some other value). > > No, KVM doesn't give two hoots about the INTID, as long as this is a > PPI that is otherwise unused. > >> If the >> issue is "the *host* has to be using the right INTID" then I >> would hope that KVM simply doesn't expose the capability if >> the host h/w won't let it work correctly. > > No host maintenance interrupt, no NV. This is specially mandatory as > the L1 guest is in (almost) complete control of the ICH_*_EL2 > registers and expects MIs to be delivered. > >> If KVM can happily >> use any maintenance interrupt ID that the board model wants, >> then we should make that work, rather than hardcoding 25 into >> our gicv3 code. > > +1. > > I'd eliminate any reference to SBSA, as it has no bearing on either > KVM nor the QEMU GIC code. > > I also question the "if VHE is requested". Not having VHE doesn't > preclude virtualisation. Was that supposed to be "virtualisation > extension" instead? > s/VHE/virtualization extension/ I’ve noted it has been a recurring confusion on my part. Will fix. :) Thank you, Miguel > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index b871350856..377181e009 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -759,6 +759,11 @@ static void create_gic(VirtMachineState *vms, MemoryRegion *mem) qdev_prop_set_uint32(vms->gic, "redist-region-count[1]", MIN(smp_cpus - redist0_count, redist1_capacity)); } + + if (kvm_irqchip_in_kernel()) { + qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", + vms->virt); + } } else { if (!kvm_irqchip_in_kernel()) { qdev_prop_set_bit(vms->gic, "has-virtualization-extensions", diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c index 351843db4a..e2a6ff1b49 100644 --- a/hw/intc/arm_gicv3_common.c +++ b/hw/intc/arm_gicv3_common.c @@ -563,6 +563,7 @@ static Property arm_gicv3_common_properties[] = { DEFINE_PROP_UINT32("revision", GICv3State, revision, 3), DEFINE_PROP_BOOL("has-lpi", GICv3State, lpi_enable, 0), DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0), + DEFINE_PROP_BOOL("has-virtualization-extensions", GICv3State, virt_extn, 0), /* * Compatibility property: force 8 bits of physical priority, even * if the CPU being emulated should have fewer. diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c index 3ca643ecba..ce924753bb 100644 --- a/hw/intc/arm_gicv3_kvm.c +++ b/hw/intc/arm_gicv3_kvm.c @@ -22,6 +22,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "hw/intc/arm_gicv3_common.h" +#include "hw/arm/virt.h" #include "qemu/error-report.h" #include "qemu/module.h" #include "sysemu/kvm.h" @@ -803,6 +804,30 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp) return; } + + if (s->virt_extn) { + /* + * Arm strongly recommends that maintenance interrupts are configured + * to use INTID 25. For more information, see Server Base System + * Architecture (SBSA) + */ + uint32_t maint_irq = PPI(ARCH_GIC_MAINT_IRQ); + + struct kvm_device_attr kdevattr = { + .group = KVM_DEV_ARM_VGIC_GRP_MAINT_IRQ, + .addr = (uint64_t)&maint_irq + }; + + if (!kvm_device_ioctl(s->dev_fd, KVM_GET_DEVICE_ATTR, &kdevattr)) { + error_setg(errp, "VGICv3 setting maintenance IRQ are not " + "supported by this host kernel"); + return; + } + + kvm_device_ioctl(s->dev_fd, KVM_SET_DEVICE_ATTR, &kdevattr); + } + + gicv3_init_irqs_and_mmio(s, kvm_arm_gicv3_set_irq, NULL); for (i = 0; i < s->num_cpu; i++) { diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h index ab5182a28a..91e1c1a45a 100644 --- a/include/hw/intc/arm_gicv3_common.h +++ b/include/hw/intc/arm_gicv3_common.h @@ -248,6 +248,7 @@ struct GICv3State { uint32_t revision; bool lpi_enable; bool security_extn; + bool virt_extn; bool force_8bit_prio; bool irq_reset_nonsecure; bool gicd_no_migration_shift_bug;