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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Gao, Chao
> Sent: Wednesday, March 22, 2017 8:19 AM
> 
> 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.

I overlooked this part

> 
> >> 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?
> 

Just "Pointer to posted descriptor".

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