| Message ID | 20250404193923.1413163-6-seanjc@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: iommu: Overhaul device posted IRQs support | expand |
On 4/5/2025 1:08 AM, Sean Christopherson wrote: > Return -EINVAL instead of success if amd_ir_set_vcpu_affinity() is > invoked without use_vapic; lying to KVM about whether or not the IRTE was > configured to post IRQs is all kinds of bad. > > Fixes: d98de49a53e4 ("iommu/amd: Enable vAPIC interrupt remapping mode by default") > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > drivers/iommu/amd/iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index cd5116d8c3b2..b3a01b7757ee 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -3850,7 +3850,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info) > * we should not modify the IRTE > */ > if (!dev_data || !dev_data->use_vapic) > - return 0; > + return -EINVAL; > Hi Sean, you can update following functions as well to return error when IOMMU is using legacy interrupt mode. 1. amd_iommu_update_ga 2. amd_iommu_activate_guest_mode 3. amd_iommu_deactivate_guest_mode Currently these functions return 0 to the kvm layer when they fail to set the IRTE. Regards Sairaj Kodilkar
On Fri, Apr 11, 2025, Sairaj Kodilkar wrote: > On 4/5/2025 1:08 AM, Sean Christopherson wrote: > > Return -EINVAL instead of success if amd_ir_set_vcpu_affinity() is > > invoked without use_vapic; lying to KVM about whether or not the IRTE was > > configured to post IRQs is all kinds of bad. > > > > Fixes: d98de49a53e4 ("iommu/amd: Enable vAPIC interrupt remapping mode by default") > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > drivers/iommu/amd/iommu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > > index cd5116d8c3b2..b3a01b7757ee 100644 > > --- a/drivers/iommu/amd/iommu.c > > +++ b/drivers/iommu/amd/iommu.c > > @@ -3850,7 +3850,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info) > > * we should not modify the IRTE > > */ > > if (!dev_data || !dev_data->use_vapic) > > - return 0; > > + return -EINVAL; > > Hi Sean, > you can update following functions as well to return error when > IOMMU is using legacy interrupt mode. > 1. amd_iommu_update_ga > 2. amd_iommu_activate_guest_mode > 3. amd_iommu_deactivate_guest_mode Heh, I'm well aware, and this series gets there eventually (the end product WARNs and returns an error in all three functions). I fixed amd_ir_set_vcpu_affinity() early in the series because it's the initial API that KVM will use to configure an IRTE for posting to a vCPU. I.e. to reach the other helpers, KVM would need to ignore the error returned by amd_ir_set_vcpu_affinity(). > Currently these functions return 0 to the kvm layer when they fail to > set the IRTE. > > Regards > Sairaj Kodilkar
On 4/11/2025 7:35 PM, Sean Christopherson wrote: > On Fri, Apr 11, 2025, Sairaj Kodilkar wrote: >> On 4/5/2025 1:08 AM, Sean Christopherson wrote: >>> Return -EINVAL instead of success if amd_ir_set_vcpu_affinity() is >>> invoked without use_vapic; lying to KVM about whether or not the IRTE was >>> configured to post IRQs is all kinds of bad. >>> >>> Fixes: d98de49a53e4 ("iommu/amd: Enable vAPIC interrupt remapping mode by default") >>> Signed-off-by: Sean Christopherson <seanjc@google.com> >>> --- >>> drivers/iommu/amd/iommu.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c >>> index cd5116d8c3b2..b3a01b7757ee 100644 >>> --- a/drivers/iommu/amd/iommu.c >>> +++ b/drivers/iommu/amd/iommu.c >>> @@ -3850,7 +3850,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info) >>> * we should not modify the IRTE >>> */ >>> if (!dev_data || !dev_data->use_vapic) >>> - return 0; >>> + return -EINVAL; >> >> Hi Sean, >> you can update following functions as well to return error when >> IOMMU is using legacy interrupt mode. >> 1. amd_iommu_update_ga >> 2. amd_iommu_activate_guest_mode >> 3. amd_iommu_deactivate_guest_mode > > Heh, I'm well aware, and this series gets there eventually (the end product WARNs > and returns an error in all three functions). I fixed amd_ir_set_vcpu_affinity() > early in the series because it's the initial API that KVM will use to configure > an IRTE for posting to a vCPU. I.e. to reach the other helpers, KVM would need > to ignore the error returned by amd_ir_set_vcpu_affinity(). > Ohh sorry about that. Since I was reviewing patches sequentially, I did come across those changes. Regards Sairaj Kodilkar >> Currently these functions return 0 to the kvm layer when they fail to >> set the IRTE. >> >> Regards >> Sairaj Kodilkar
On 4/5/2025 1:08 AM, Sean Christopherson wrote: > Return -EINVAL instead of success if amd_ir_set_vcpu_affinity() is > invoked without use_vapic; lying to KVM about whether or not the IRTE was > configured to post IRQs is all kinds of bad. > > Fixes: d98de49a53e4 ("iommu/amd: Enable vAPIC interrupt remapping mode by default") > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Vasant Hegde <vasant.hegde@amd.com> -Vasant
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index cd5116d8c3b2..b3a01b7757ee 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -3850,7 +3850,7 @@ static int amd_ir_set_vcpu_affinity(struct irq_data *data, void *vcpu_info) * we should not modify the IRTE */ if (!dev_data || !dev_data->use_vapic) - return 0; + return -EINVAL; ir_data->cfg = irqd_cfg(data); pi_data->ir_data = ir_data;
Return -EINVAL instead of success if amd_ir_set_vcpu_affinity() is invoked without use_vapic; lying to KVM about whether or not the IRTE was configured to post IRQs is all kinds of bad. Fixes: d98de49a53e4 ("iommu/amd: Enable vAPIC interrupt remapping mode by default") Signed-off-by: Sean Christopherson <seanjc@google.com> --- drivers/iommu/amd/iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)