Re: [PATCH v6 4/5] intel_iommu: allow Extended Interrupt Mode when using userspace APIC

[Bui Quang Minh <minhquangbui99@gmail.com>]

On 7/21/23 03:47, Peter Xu wrote:
> On Mon, Jul 17, 2023 at 11:29:56PM +0700, Bui Quang Minh wrote:
>> On 7/17/23 17:47, Joao Martins wrote:
>>> +Peter, +Jason (intel-iommu maintainer/reviewer)
> 
> Thanks for copying me, Joan.
> 
>>>
>>> On 15/07/2023 16:22, Bui Quang Minh wrote:
>>>> As userspace APIC now supports x2APIC, intel interrupt remapping
>>>> hardware can be set to EIM mode when userspace local APIC is used.
>>>>
>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Bui Quang Minh <minhquangbui99@gmail.com>
>>>> ---
>>>>    hw/i386/intel_iommu.c | 11 -----------
>>>>    1 file changed, 11 deletions(-)
>>>>
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index dcc334060c..5e576f6059 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -4043,17 +4043,6 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
>>>>                          && x86_iommu_ir_supported(x86_iommu) ?
>>>>                                                  ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>>>>        }
>>>> -    if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
>>>> -        if (!kvm_irqchip_is_split()) {
>>>> -            error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split");
>>>> -            return false;
>>>> -        }
>>>> -        if (!kvm_enable_x2apic()) {
>>>> -            error_setg(errp, "eim=on requires support on the KVM side"
>>>> -                             "(X2APIC_API, first shipped in v4.7)");
>>>> -            return false;
>>>> -        }
>>>> -    }
>>> Given commit 20ca47429e ('Revert "intel_iommu: Fix irqchip / X2APIC
>>> configuration checks"'), won't we regress behaviour again  for the accel=kvm
>>> case by dropping the kvm_enable_x2apic() call here?
>>>
>>> Perhaps if we support userspace APIC with TCG the check just needs to be redone
>>> to instead avoid always requiring kvm e.g.:
>>>
>>> if (kvm_irqchip_in_kernel()) {
>>>       error_setg(errp, "eim=on requires accel=kvm,kernel-irqchip=split"
>>>                  "(X2APIC_API, first shipped in v4.7)");
>>> }
>>>
>>> if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) {
>>>       error_setg(errp, "eim=on requires support on the KVM side"
>>>                  "(X2APIC_API, first shipped in v4.7)");
>>>       return false;
>>> }
>>
>> Thank you for your review. I think the check for kvm_irqchip_in_kernel() is
>> not correct, AFAIK, kvm_irqchip_is_split() == true also means
>> kvm_irqchip_in_kernel() == true on x86. To check if kernel-irqchip = on, we
>> need to do something like in x86_iommu_realize
>>
>>      bool irq_all_kernel = kvm_irqchip_in_kernel() &&
>> !kvm_irqchip_is_split();
>>
>> The original check for !kvm_irqchip_is_split means emulated/userspace APIC.
>> It's because to reach that check x86_iommu_ir_supported(...) == true and
>> x86_iommu_ir_supported(...) == true is not supported when kernel-irqchip =
>> on (there is a check for this in x86_iommu_realize)
>>
>> So I think we need to change the check to
>>
>>      if (s->intr_eim == ON_OFF_AUTO_ON && !s->buggy_eim) {
>>          if (kvm_irqchip_is_split() && !kvm_enable_x2apic()) {
>>              error_setg(errp, "eim=on requires support on the KVM side"
>>                               "(X2APIC_API, first shipped in v4.7)");
>>              return false;
>>          }
>>      }
>>
>> Is it OK?
> 
> Mostly to me, except that we may also want to keep failing if all irq chips
> are in kernel?

Yes, that behavior does not change after this patch. x86_iommu_realize 
in the parent type TYPE_X86_IOMMU_DEVICE fails when interrupt remapping 
is on and all irq chips are in kernel already.

     static void x86_iommu_realize(DeviceState *dev, Error **errp)
     {
         /* ... */
         /* Both Intel and AMD IOMMU IR only support "kernel-irqchip 
{off|split}" */
         if (x86_iommu_ir_supported(x86_iommu) && irq_all_kernel) {
             error_setg(errp, "Interrupt Remapping cannot work with "
                          "kernel-irqchip=on, please use 'split|off'.");
             return;
         }
         /* ... */
     }


So in case we reach here in with the interrupt remapping is on and 
decide whether eim is on or not, it cannot be that irq chips are all in 
kernel.

Thanks,
Quang Minh.