Re: [PART2 RFC v2 00/10] iommu/AMD: Introduce IOMMU AVIC support

[Joerg Roedel]

On Mon, Jun 13, 2016 at 05:06:40PM -0500, Suravee Suthikulpanit wrote:
> Suravee Suthikulpanit (10):
> iommu/amd: Detect and enable guest vAPIC support
> iommu/amd: Add support for 128-bit IRTE
> iommu/amd: Detect and initialize guest vAPIC log
> iommu/amd: Adding GALOG interrupt handler
> iommu/amd: Introduce amd_iommu_update_ga()
> iommu/amd: Implements irq_set_vcpu_affinity() hook to setup vapic mode
> for pass-through devices
> iommu/amd: Enable vAPIC interrupt remapping mode by default
> svm: Introduce AMD IOMMU avic_ga_log_notifier
> svm: Implements update_pi_irte hook to setup posted interrupt
> svm: Update AMD IOMMU IRTE with vcpu scheduling information when
> enable AVIC
>
> arch/x86/include/asm/kvm_host.h | 2 +
> arch/x86/kvm/svm.c | 195 +++++++++++++++++-
> drivers/iommu/amd_iommu.c | 432 +++++++++++++++++++++++++++++++++++-----
> drivers/iommu/amd_iommu_init.c | 163 ++++++++++++++-
> drivers/iommu/amd_iommu_proto.h | 1 +
> drivers/iommu/amd_iommu_types.h | 138 +++++++++++++
> include/linux/amd-iommu.h | 24 +++
> 7 files changed, 896 insertions(+), 59 deletions(-)

The code has a few style issues (thing I'd implemented differently), but
it looks functional. Anyway, before merging this the last 3 patches need
to be acked by the KVM maintainers.

Paolo?


Thanks,

Joerg

[Joerg Roedel]

On Tue, Jun 21, 2016 at 09:27:27AM -0500, Suthikulpanit, Suravee wrote:

> On 6/21/2016 8:50 AM, Joerg Roedel wrote:
> >The code has a few style issues (thing I'd implemented differently), but
> >it looks functional.
>

> Anything in particular that you think I should improve or/and change?

One thing I noticed were all the if (small IRTE) ... else (big IRTE)
that would better be hidden behind function pointers. Also a few magic
numbers are in there that could be replaced by proper defines.



Joerg

[Paolo Bonzini]

On 21/06/2016 15:50, Joerg Roedel wrote:
> The code has a few style issues (thing I'd implemented differently), but
> it looks functional. Anyway, before merging this the last 3 patches need
> to be acked by the KVM maintainers.
>
> Paolo?

I think patches 9 and 10 should be squashed because the code after patch
9 is only partly functional.

Likewise, I think this:


+
+ if (AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
+ amd_iommu_irq_ops.capability |= (1 << IRQ_POSTING_CAP);

from patch 1 should be moved to patch 6.

Otherwise the KVM bits are sane.

Paolo