| Message ID | 20230715152224.54757-5-minhquangbui99@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/5] i386/tcg: implement x2APIC registers MSR access | expand |
+Peter, +Jason (intel-iommu maintainer/reviewer) 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; }
On 7/17/23 17:47, Joao Martins wrote: > +Peter, +Jason (intel-iommu maintainer/reviewer) > > 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? Thanks, Quang Minh.
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? Thanks,
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.
On Fri, Jul 21, 2023 at 10:35:01PM +0700, Bui Quang Minh wrote: > 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. Ah that's okay then, thanks.
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; - } - } /* Currently only address widths supported are 39 and 48 bits */ if ((s->aw_bits != VTD_HOST_AW_39BIT) &&