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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Mar 22, 2017 at 01:59:19PM +0800, Tian, Kevin wrote:
>> From: Gao, Chao
>> Sent: Wednesday, March 15, 2017 1:11 PM
>> 
>> Currently, msi_msg_to_remap_entry is buggy when the live IRTE is in posted
>> format. Straightforwardly, we can let caller specify which format of IRTE 
>> they
>> want update to. But the problem is current callers are not aware of the
>> binding with guest interrupt. Making all callers be aware of the binding with
>> guest interrupt will cause a far more complicated change. Also some callings
>> happen in interrupt context where they can't acquire d->event_lock to read
>> struct hvm_pirq_dpci.
>
>Above text is unclear to me. Are you trying to explain why current 
>code is buggy (which I don't get the point) or simply for mitigation
>options which were once considered but dropped for some reasons?
>

the latter. I will divide them into two sections and add more description about
why current code is buggy.

>> 
>> diff --git a/xen/drivers/passthrough/vtd/intremap.c
>> b/xen/drivers/passthrough/vtd/intremap.c
>> index bfd468b..6202ece 100644
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -552,11 +552,12 @@ static int msi_msg_to_remap_entry(
>>      struct msi_desc *msi_desc, struct msi_msg *msg)  {
>>      struct iremap_entry *iremap_entry = NULL, *iremap_entries;
>> -    struct iremap_entry new_ire;
>> +    struct iremap_entry new_ire = {{0}};
>>      struct msi_msg_remap_entry *remap_rte;
>>      unsigned int index, i, nr = 1;
>>      unsigned long flags;
>>      struct ir_ctrl *ir_ctrl = iommu_ir_ctrl(iommu);
>> +    const struct pi_desc *pi_desc = msi_desc->pi_desc;
>> 
>>      if ( msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
>>          nr = msi_desc->msi.nvec;
>> @@ -595,33 +596,35 @@ static int msi_msg_to_remap_entry(
>>      GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, index,
>>                       iremap_entries, iremap_entry);
>> 
>> -    memcpy(&new_ire, iremap_entry, sizeof(struct iremap_entry));
>> -
>> -    /* Set interrupt remapping table entry */
>> -    new_ire.remap.fpd = 0;
>> -    new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
>> & 0x1;
>> -    new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
>> -    new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT)
>> & 0x1;
>> -    /* Hardware require RH = 1 for LPR delivery mode */
>> -    new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
>> -    new_ire.remap.avail = 0;
>> -    new_ire.remap.res_1 = 0;
>> -    new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
>> -                            MSI_DATA_VECTOR_MASK;
>> -    new_ire.remap.res_2 = 0;
>> -    if ( x2apic_enabled )
>> -        new_ire.remap.dst = msg->dest32;
>> +    if ( !pi_desc )
>> +    {
>> +        new_ire.remap.dm = msg->address_lo >>
>> MSI_ADDR_DESTMODE_SHIFT;
>> +        new_ire.remap.tm = msg->data >> MSI_DATA_TRIGGER_SHIFT;
>> +        new_ire.remap.dlm = msg->data >>
>> MSI_DATA_DELIVERY_MODE_SHIFT;
>> +        /* Hardware require RH = 1 for LPR delivery mode */
>> +        new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
>> +        new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
>> +                                MSI_DATA_VECTOR_MASK;
>> +        if ( x2apic_enabled )
>> +            new_ire.remap.dst = msg->dest32;
>> +        else
>> +            new_ire.remap.dst =
>> +                MASK_EXTR(msg->address_lo, MSI_ADDR_DEST_ID_MASK) << 8;
>> +        new_ire.remap.p = 1;
>
>Old code also touches fpd, res_1/2/3/4, which are abandoned
>above. Can you elaborate?
>

We have initialized new_ire to zero so I remove all the lines that assign 0 to 
fields.

>> diff --git a/xen/include/asm-x86/msi.h b/xen/include/asm-x86/msi.h index
>> 9c02945..3286692 100644
>> --- a/xen/include/asm-x86/msi.h
>> +++ b/xen/include/asm-x86/msi.h
>> @@ -118,6 +118,8 @@ struct msi_desc {
>>      struct msi_msg msg;             /* Last set MSI message */
>> 
>>      int remap_index;                /* index in interrupt remapping table
>> */
>> +    const void *pi_desc;            /* PDA, indicates msi is delivered via
>> VT-d PI */
>
>what's PDA?

Posted Descriptor Address which is recorded in posted format IRTE.

How about just use "Indicates msi is delivered via VT-d PI" because of the line 
width
limitation?

>
>> +    uint8_t gvec;                   /* guest vector. valid when pi_desc
>> isn't NULL */
>>  };
>> 
>>  /*
>> --
>> 1.8.3.1
>

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