Re: [Xen-devel] [RFC v2 07/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: Tuesday, June 09, 2015 10:33 PM
> To: Wu, Feng
> Cc: andrew.cooper3@xxxxxxxxxx; george.dunlap@xxxxxxxxxxxxx; Tian, Kevin;
> Zhang, Yang Z; xen-devel@xxxxxxxxxxxxx; keir@xxxxxxx
> Subject: Re: [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used
> 
> >>> 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?

So what kind of message do you think I need to show here?

Thanks,
Feng

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