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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.15 at 11:07, <feng.wu@xxxxxxxxx> wrote:
> +bool_t pi_update_irte(struct vcpu *v, struct pirq *pirq, uint8_t gvec)

Without seeing the caller right away it's hard to judge, but generally I'd
prefer functions to return -E... values as error indicators, i.e.

> +{
> +    struct irq_desc *desc;
> +    struct msi_desc *msi_desc;
> +    int remap_index;
> +    bool_t 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;
> +    unsigned long flags;
> +
> +    desc = pirq_spin_lock_irq_desc(pirq, NULL);
> +    if ( !desc )
> +        return 0;

-ENOMEM

> +    msi_desc = desc->msi_desc;
> +    if ( !msi_desc )
> +        goto unlock_out;

-EBADSLT

> +    pci_dev = msi_desc->dev;
> +    if ( !pci_dev )
> +        goto unlock_out;

-ENODEV

> +    remap_index = msi_desc->remap_index;
> +    drhd = acpi_find_matched_drhd_unit(pci_dev);
> +    if ( !drhd )
> +    {
> +        dprintk(XENLOG_INFO VTDPREFIX,
> +                "%pv: failed to get drhd, pci device: "
> +                "%04x:%02x:%02x.%u, guest vector: %u\n",
> +                v, pci_dev->seg, pci_dev->bus, PCI_SLOT(pci_dev->devfn),
> +                PCI_FUNC(pci_dev->devfn), gvec);
> +        goto unlock_out;
> +    }
> +
> +    iommu = drhd->iommu;
> +    ir_ctrl = iommu_ir_ctrl(iommu);
> +    if ( !ir_ctrl )
> +    {
> +        dprintk(XENLOG_INFO VTDPREFIX,
> +                "%pv: failed to get ir_ctrl, pci device: "
> +                "%04x:%02x:%02x.%u, guest vector: %u\n",
> +                v, pci_dev->seg, pci_dev->bus, PCI_SLOT(pci_dev->devfn),
> +                PCI_FUNC(pci_dev->devfn), gvec);
> +        goto unlock_out;
> +    }

Do you think these log messages are useful beyond your bringup
purposes?

> +    spin_lock_irqsave(&ir_ctrl->iremap_lock, flags);
> +
> +    GET_IREMAP_ENTRY(ir_ctrl->iremap_maddr, remap_index, iremap_entries, p);
> +
> +    memcpy(&new_ire, p, sizeof(new_ire));

Please use structure assignment (being type safe) in preference to
memcpy() (not being type safe).

> --- a/xen/drivers/passthrough/vtd/iommu.h
> +++ b/xen/drivers/passthrough/vtd/iommu.h
> @@ -327,6 +327,10 @@ struct iremap_entry {
>    };
>  };
>  
> +#define PDA_LOW_BIT    26
> +#define PDA_HIGH_BIT   32
> +#define PDA_MASK(XX)   (~(-1UL << PDA_##XX##_BIT))

To me it would look more natural if you used ~0UL.

Jan


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