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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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'm particularly worried by the call to acpi_find_matched_drhd_unit()
- is it maybe worth storing the iommu pointer in struct msi_desc?

> +    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 *.

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

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