Re: [Xen-devel] [v3 10/15] vt-d: Add API to update IRTE when VT-d PI is used

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, July 10, 2015 10:02 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [v3 10/15] vt-d: Add API to update IRTE when VT-d PI is used
> 
> >>> On 24.06.15 at 07:18, <feng.wu@xxxxxxxxx> wrote:
> > This patch adds an API which is used to update the IRTE
> > for posted-interrupt when guest changes MSI/MSI-X information.
> 
> This is again an example where adding a dead function complicates
> review: How will I know here why this statement is correct, namely
> why MSI/MSI-X are affected but IO-APIC isn't?
> 
> > +int pi_update_irte(struct vcpu *v, struct pirq *pirq, uint8_t gvec)
> > +{
> > +    struct irq_desc *desc;
> > +    struct msi_desc *msi_desc;
> > +    int remap_index;
> > +    int rc = 0;
> > +    struct pci_dev *pci_dev;
> > +    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;
> > +    struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> 
> I suppose some of the pointers above could become pointers to
> const?
> 
> > +    unsigned long flags;
> > +    uint128_t old_ire, ret;
> > +
> > +    desc = pirq_spin_lock_irq_desc(pirq, NULL);
> > +    if ( !desc )
> > +        return -ENOMEM;
> > +
> > +    msi_desc = desc->msi_desc;
> > +    if ( !msi_desc )
> > +    {
> > +        rc = -EBADSLT;
> > +        goto unlock_out;
> > +    }
> > +
> > +    pci_dev = msi_desc->dev;
> > +    if ( !pci_dev )
> > +    {
> > +        rc = -ENODEV;
> > +        goto unlock_out;
> > +    }
> > +
> > +    remap_index = msi_desc->remap_index;
> > +    drhd = acpi_find_matched_drhd_unit(pci_dev);
> > +    if ( !drhd )
> > +    {
> > +        rc = -ENODEV;
> > +        goto unlock_out;
> > +    }
> > +
> > +    iommu = drhd->iommu;
> > +    ir_ctrl = iommu_ir_ctrl(iommu);
> > +    if ( !ir_ctrl )
> > +    {
> > +        rc = -ENODEV;
> > +        goto unlock_out;
> > +    }
> > +
> > +    spin_lock_irqsave(&ir_ctrl->iremap_lock, flags);
> 
> Interrupts are unconditionally disabled here already. Question
> though is whether you really need to hold on to the IRQ descriptor
> lock across the entire function. Much of course depends on what
> other locks you maybe imply to be held by the caller.

I search lots of current code where the irq desc lock is used, and
I think the unlock operate can be placed before acquiring iremap_lock.

> 
> I'm particularly worried by the call to acpi_find_matched_drhd_unit()
> - is it maybe worth storing the iommu pointer in struct msi_desc?

I think it worth, Like Andrew also mentioned this point before. I tend
to make this a independent work and do it later, since the 4.6 release
is coming, I am still try my best to target it. Could you please share
your concern here, performance? Or other things? Thanks!

> 
> > +    GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, remap_index,
> iremap_entries, p);
> > +    new_ire = *p;
> > +
> > +    /* Setup/Update interrupt remapping table entry. */
> > +    setup_posted_irte(&new_ire, pi_desc, gvec);
> > +
> > +    do {
> > +        old_ire = *(uint128_t *)p;
> 
> This cast suggests that you might want to go beyond what Andrew
> said on cmpxchg16b()'s parameters: Perhaps they'd better be
> void * instead of uint128_t *.

In that case, I need to do the cast in __cmpxchg16b(), right?

> 
> > +        ret = cmpxchg16b(p, &old_ire, &new_ire);
> > +    } while ( memcmp(&ret, &old_ire, sizeof(old_ire)) );
> 
> Doesn't setup_posted_irte() need to move inside this loop, as it
> tries to preserve certain fields? Or else, what is the cmpxchg16b
> loop guarding against (i.e. why isn't this just a single one)?

Why need we move setup_posted_irte() inside the loop? "new_ire"
will not be changed after setup, right? Here we need to make sure
the 128b IRTE is updated atomically, especially for the high part
of posted-interrupt descriptor address and the low part of it.

Thanks,
Feng

> 
> > +    iommu_flush_cache_entry(p, sizeof(struct iremap_entry));
> 
> sizeof(*p)
> 
> Jan


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