Re: [RFC PATCH 2/5] hw/intc/gicv3: add support for setting KVM vGIC maintenance IRQ

[Miguel Luis]

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