Re: [PART2 PATCH v3 07/11] iommu/amd: Introduce amd_iommu_update_ga()

[Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>]

Hi Radim,

I have a feeling that there might be some confusion in the use of 
parameter ga_tag here in various places. My apology. I am in the process 
of cleaning up this, and will send out the V4.

In the meantime, let me try to clarify a couple design detail that might 
be missed here.

On 07/12/2016 01:59 AM, Radim Krčmář wrote:
> 2016-07-11 05:11-0500, Suravee Suthikulpanit:
>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>
>> Introduces a new IOMMU API, amd_iommu_update_ga(), which allows
>> KVM (SVM) to update existing posted interrupt IOMMU IRTE when
>> load/unload vcpu.
>>
>> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> ---
>> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
>> @@ -4481,4 +4481,67 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu)
>> +int amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 ga_tag,
>> +			u64 base, bool is_run)
>
> Not just in this function does the interface between svm and iommu split
> ga_tag into its two components (vcpu_id and ga_tag), but it seems that
> the combined value could always be used instead ...
> Is there an advantage to passing two values?

Basically, the amd_iommu_update_ga() function is designed to achieve two 
things:

1. Communicate from SVM to AMD IOMMU driver the interrupt routing 
information (e.g. the physical CPU to route the guest interrupt to) in 
case the vcpu is running.

2. In case of vcpu is not running, the IOMMU driver should decode the 
GATAG field of the GA log entry to find out which VCPU of which VM need 
to be scheduled in, and notify KVM/SVM. The GATAG is encode as ((VM_ID 
<< 8) | VCPU_ID).

Here, the amd_iommu_update_ga() takes the two separate value for input 
parameters. Mainly the ga_tag (which is really the vm_id) and vcpu_id. 
This allow IOMMU driver to decide how to encode the GATAG to be 
programmed into the IRTE. Currently, the actual GATAG is a 16-bit value, 
<vm_id><vcpu_id>. This keeps the interface independent from how we 
encode the GATAG.

>> +{
>> +	unsigned long flags;
>> +	struct amd_iommu *iommu;
>> +
>> +	if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir))
>> +		return 0;
>> +
>> +	for_each_iommu(iommu) {
>> +		struct amd_ir_data *ir_data;
>> +
>> +		spin_lock_irqsave(&iommu->ga_hash_lock, flags);
>> +
>> +		/* Note: Update all possible ir_data for a particular
>> +		 * vcpu in a particular vm.
>> +		 */
>> +		hash_for_each_possible(iommu->ga_hash, ir_data, hnode,
>> +				       AMD_IOMMU_GATAG(ga_tag, vcpu_id)) {
>> +			struct irte_ga *irte = (struct irte_ga *) ir_data->entry;
>
> (The ga_tag check is missing here too.)

Here, the intention is to update all interrupt remapping entries in the 
bucket w/ the same GATAG (i.e. vm_id + vcpu_id), where GATAG = 
AMD_IOMMU_GATAG(vm_id, vcpu_id).

>> +			if (!irte->lo.fields_vapic.guest_mode)
>> +				continue;
>> +
>> +			update_irte_ga((struct irte_ga *)ir_data->ref,
>> +					ir_data->irq_2_irte.devid,
>> +					base, cpu, is_run);
>
> (The lookup leading up to here is avoidable -- svm, the caller, has the
>   ability to map ga_tag into irte/ir_data directly with a pointer.
>   I'm not sure if the lookup is slow enough to pardon optimization, but
>   it might make the code simpler as well.)

I might have mislead you up to this point. Not sure if the assumption 
here still hold with my explanation above. Sorry for confusion.

Thanks,
Suravee