Hi Peter,
> On 6 Mar 2023, at 13:02, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
> On Mon, 27 Feb 2023 at 16:37, Miguel Luis <miguel.l...@oracle.com> wrote:
>>
>> From: Haibo Xu <haibo...@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
