Re: [Xen-devel] [PATCH v11 2/6] VT-d: Introduce new fields in msi_desc to track binding with guest interrupt

[Chao Gao <chao.gao@xxxxxxxxx>]

On Fri, Mar 31, 2017 at 01:46:33PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Wednesday, March 29, 2017 1:12 PM
>> 
>>      return entry;
>> diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
>> index d53976c..5dbfe53 100644
>> --- a/xen/drivers/passthrough/io.c
>> +++ b/xen/drivers/passthrough/io.c
>> @@ -618,6 +618,8 @@ int pt_irq_destroy_bind(
>>          else
>>              what = "bogus";
>>      }
>> +    else if ( iommu_intpost && pirq && pirq_dpci->gmsi.posted )
>
>No need to check iommu_intpost. Just keep later conditions.

ok. Is it for if posted is set, the iommu_intpost should be true?

>
>btw isn't the condition be " (pirq_dpci && pirq_dpci->gmsi.posted)"?

I don't think so. pirq, not pirq_dpci, is consumed by pi_update_irte.
Furthermore if pirq_dpci is null, pirq is also null. But it is not true
in turn.

>
>> +        pi_update_irte(NULL, pirq, 0);
>> 
>>      if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) &&
>>           list_empty(&pirq_dpci->digl_list) )
>>  /*
>>   * This function is used to update the IRTE for posted-interrupt
>>   * when guest changes MSI/MSI-X information.
>> @@ -946,17 +912,9 @@ int pi_update_irte(const struct vcpu *v, const struct
>> pirq *pirq,
>>      const uint8_t gvec)
>>  {
>>      struct irq_desc *desc;
>> -    const struct msi_desc *msi_desc;
>> -    int remap_index;
>> -    int rc = 0;
>> -    const struct pci_dev *pci_dev;
>> -    const struct acpi_drhd_unit *drhd;
>> -    struct iommu *iommu;
>> -    struct ir_ctrl *ir_ctrl;
>> -    struct iremap_entry *iremap_entries = NULL, *p = NULL;
>> -    struct iremap_entry new_ire, old_ire;
>> -    const struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
>> -    __uint128_t ret;
>> +    struct msi_desc *msi_desc;
>> +    const struct pi_desc *pi_desc = v ? &v->arch.hvm_vmx.pi_desc : NULL;
>
>will there be a case that pi_update_desc is invoked with v==NULL?

The line behind your last comment above. It is to clear 'pi_desc' and 'gvec'
fields in msi_desc when guest destroies the binding with pt-irq.

>
>> +    int rc;
>> 
>>      desc = pirq_spin_lock_irq_desc(pirq, NULL);
>>      if ( !desc )
>> @@ -968,59 +926,13 @@ int pi_update_irte(const struct vcpu *v, const
>> struct pirq *pirq,
>>          rc = -ENODEV;
>>          goto unlock_out;
>>      }
>> -
>> -    pci_dev = msi_desc->dev;
>> -    if ( !pci_dev )
>> -    {
>> -        rc = -ENODEV;
>> -        goto unlock_out;
>> -    }
>> -
>> -    remap_index = msi_desc->remap_index;
>> +    msi_desc->pi_desc = pi_desc;
>> +    msi_desc->gvec = gvec;
>
>Is it possible to see pi_desc already assigned? the name of pi_update_irte
>looks it may be invoked multiple times. If we assume a new update should
>happen only when previous one is cleaned up, then some check code
>should be added here to prove that assumption.

We don't make this assumption. I just want to track the msi's binding with
guest. If guest updates it, we updates here. If guest clears it, then we clear
it accordingly.

Thanks
Chao

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel