[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 07 of 16] amd iommu: Add 2 hypercalls for libxc



>>> On 23.12.11 at 12:29, Wei Wang <wei.wang2@xxxxxxx> wrote:
> # HG changeset patch
> # User Wei Wang <wei.wang2@xxxxxxx>
> # Date 1324569392 -3600
> # Node ID 40d61d0390ec930cf53ce5cbf91faada8c7192bd
> # Parent  bf9f21ad9a0ec245b409f3862a5a36c0e070f333
> amd iommu: Add 2 hypercalls for libxc
> 
> iommu_set_msi: used by qemu to inform hypervisor iommu vector number in 
> guest
> space. Hypervisor needs this vector to inject msi into guest when PPR 
> logging
> happens.
> 
> iommu_bind_bdf: used by xl to bind guest bdf number to machine bdf number.
> IOMMU emulations codes receives commands from guest iommu driver and 
> forwards
> them to host iommu. But virtual device id from guest should be converted 
> into
> physical before sending to real hardware.
> 
> Signed -off-by: Wei Wang <wei.wang2@xxxxxxx>
> 
> diff -r bf9f21ad9a0e -r 40d61d0390ec xen/drivers/passthrough/amd/iommu_guest.c
> --- a/xen/drivers/passthrough/amd/iommu_guest.c       Thu Dec 22 16:56:28 
> 2011 +0100
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c       Thu Dec 22 16:56:32 
> 2011 
> +0100
> @@ -50,12 +50,27 @@
>  
>  static unsigned int machine_bdf(struct domain *d, uint16_t guest_bdf)
>  {
> -    return guest_bdf;
> +    struct pci_dev *pdev;
> +    uint16_t mbdf = 0;
> +
> +    for_each_pdev( d, pdev )
> +    {
> +        if ( pdev->gbdf == guest_bdf )
> +        {
> +            mbdf = PCI_BDF2(pdev->bus, pdev->devfn);
> +            break;
> +        }
> +    }
> +    return mbdf;
>  }
>  
>  static uint16_t guest_bdf(struct domain *d, uint16_t machine_bdf)
>  {
> -    return machine_bdf;
> +    struct pci_dev *pdev;
> +
> +    pdev = pci_get_pdev_by_domain(d, 0, PCI_BUS(machine_bdf),
> +                                  PCI_DEVFN2(machine_bdf));
> +    return pdev->gbdf;
>  }
>  
>  static inline struct guest_iommu *domain_iommu(struct domain *d)
> @@ -913,3 +928,43 @@ const struct hvm_mmio_handler iommu_mmio
>      .read_handler = guest_iommu_mmio_read,
>      .write_handler = guest_iommu_mmio_write
>  };
> +
> +/* iommu hypercall handler */
> +int iommu_bind_bdf(struct domain* d, uint16_t gbdf, uint16_t mbdf)
> +{
> +    struct pci_dev *pdev;
> +    int ret = -ENODEV;
> +
> +    if ( !iommu_found() )
> +        return 0;
> +
> +    spin_lock(&pcidevs_lock);
> +
> +    for_each_pdev( d, pdev )
> +    {
> +        if ( (pdev->bus != PCI_BUS(mbdf) ) || 
> +             (pdev->devfn != PCI_DEVFN2(mbdf)) )
> +            continue;
> +
> +        pdev->gbdf = gbdf;
> +        ret = 0;
> +    }
> +
> +    spin_unlock(&pcidevs_lock);
> +    return ret;
> +}
> +
> +void iommu_set_msi(struct domain* d, uint16_t vector, uint16_t dest,
> +                   uint16_t dest_mode, uint16_t delivery_mode, 
> +                   uint16_t trig_mode)
> +{
> +    struct guest_iommu *iommu = domain_iommu(d);
> +
> +    if ( !iommu_found() )
> +        return;
> +
> +    iommu->msi.vector = vector;
> +    iommu->msi.dest = dest;
> +    iommu->msi.dest_mode = dest_mode;
> +    iommu->msi.trig_mode = trig_mode;
> +}
> diff -r bf9f21ad9a0e -r 40d61d0390ec xen/drivers/passthrough/iommu.c
> --- a/xen/drivers/passthrough/iommu.c Thu Dec 22 16:56:28 2011 +0100
> +++ b/xen/drivers/passthrough/iommu.c Thu Dec 22 16:56:32 2011 +0100
> @@ -648,6 +648,40 @@ int iommu_do_domctl(
>          put_domain(d);
>          break;
>  
> +#ifndef __ia64__ 

While I understand your reasons for putting the #ifdef here, I would
like to see it removed in favor of a proper abstraction through vectors
in struct iommu_ops.

> +    case XEN_DOMCTL_guest_iommu_op:
> +    {
> +        xen_domctl_guest_iommu_op_t * guest_op;
> +
> +        if ( unlikely((d = get_domain_by_id(domctl->domain)) == NULL) )
> +        {
> +            gdprintk(XENLOG_ERR,
> +                    "XEN_DOMCTL_guest_iommu_op: get_domain_by_id() 
> failed\n");
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        guest_op = &(domctl->u.guest_iommu_op);
> +        switch ( guest_op->op )
> +        {
> +            case XEN_DOMCTL_GUEST_IOMMU_OP_SET_MSI:
> +                iommu_set_msi(d, guest_op->u.msi.vector, 
> +                              guest_op->u.msi.dest,
> +                              guest_op->u.msi.dest_mode, 
> +                              guest_op->u.msi.delivery_mode,
> +                              guest_op->u.msi.trig_mode);
> +                ret = 0;
> +                break;
> +            case XEN_DOMCTL_GUEST_IOMMU_OP_BIND_BDF:
> +                ret = iommu_bind_bdf(d, guest_op->u.bdf_bind.g_bdf,
> +                                     guest_op->u.bdf_bind.m_bdf);
> +                break;
> +        }
> +        put_domain(d);
> +        break;
> +    }
> +#endif
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff -r bf9f21ad9a0e -r 40d61d0390ec xen/include/public/domctl.h
> --- a/xen/include/public/domctl.h     Thu Dec 22 16:56:28 2011 +0100
> +++ b/xen/include/public/domctl.h     Thu Dec 22 16:56:32 2011 +0100
> @@ -848,6 +848,29 @@ struct xen_domctl_set_access_required {
>  typedef struct xen_domctl_set_access_required 
> xen_domctl_set_access_required_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_set_access_required_t);
>  
> +/* Support for guest iommu emulation */
> +struct xen_domctl_guest_iommu_op {
> +    /* XEN_DOMCTL_GUEST_IOMMU_OP_* */
> +#define XEN_DOMCTL_GUEST_IOMMU_OP_SET_MSI               0
> +#define XEN_DOMCTL_GUEST_IOMMU_OP_BIND_BDF              1
> +    uint8_t op;
> +    union {
> +        struct iommu_msi {
> +        uint8_t  vector;
> +        uint8_t  dest;
> +        uint8_t  dest_mode;
> +        uint8_t  delivery_mode;
> +        uint8_t  trig_mode;
> +        } msi;
> +        struct bdf_bind { 
> +            uint32_t            g_bdf;
> +            uint32_t            m_bdf;
> +        } bdf_bind;
> +    } u;
> +};
> +typedef struct xen_domctl_guest_iommu_op xen_domctl_guest_iommu_op_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_domctl_guest_iommu_op_t);
> +
>  struct xen_domctl {
>      uint32_t cmd;
>  #define XEN_DOMCTL_createdomain                   1
> @@ -912,6 +935,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_getvcpuextstate               63
>  #define XEN_DOMCTL_set_access_required           64
>  #define XEN_DOMCTL_audit_p2m                     65
> +#define XEN_DOMCTL_guest_iommu_op                66
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -960,6 +984,7 @@ struct xen_domctl {
>          struct xen_domctl_debug_op          debug_op;
>          struct xen_domctl_mem_event_op      mem_event_op;
>          struct xen_domctl_mem_sharing_op    mem_sharing_op;
> +        struct xen_domctl_guest_iommu_op    guest_iommu_op;
>  #if defined(__i386__) || defined(__x86_64__)
>          struct xen_domctl_cpuid             cpuid;
>          struct xen_domctl_vcpuextstate      vcpuextstate;
> diff -r bf9f21ad9a0e -r 40d61d0390ec xen/include/xen/iommu.h
> --- a/xen/include/xen/iommu.h Thu Dec 22 16:56:28 2011 +0100
> +++ b/xen/include/xen/iommu.h Thu Dec 22 16:56:32 2011 +0100
> @@ -164,6 +164,14 @@ int iommu_do_domctl(struct xen_domctl *,
>  void iommu_iotlb_flush(struct domain *d, unsigned long gfn, unsigned int 
> page_count);
>  void iommu_iotlb_flush_all(struct domain *d);
>  
> +#ifndef __ia64_
> +/* Only used by AMD IOMMU */

Even without said abstraction, the conditional is pointless here, and
the comment would seem unnecessary to me.

> +void iommu_set_msi(struct domain* d, uint16_t vector, uint16_t dest,
> +                   uint16_t dest_mode, uint16_t delivery_mode, 
> +                   uint16_t trig_mode);
> +int iommu_bind_bdf(struct domain* d, uint16_t gbdf, uint16_t mbdf);
> +#endif
> +
>  /*
>   * The purpose of the iommu_dont_flush_iotlb optional cpu flag is to
>   * avoid unecessary iotlb_flush in the low level IOMMU code.
> diff -r bf9f21ad9a0e -r 40d61d0390ec xen/include/xen/pci.h
> --- a/xen/include/xen/pci.h   Thu Dec 22 16:56:28 2011 +0100
> +++ b/xen/include/xen/pci.h   Thu Dec 22 16:56:32 2011 +0100
> @@ -63,6 +63,9 @@ struct pci_dev {
>      const u8 devfn;
>      struct pci_dev_info info;
>      u64 vf_rlen[6];
> +
> +    /* used by amd iomm to represent bdf value in guest space */
> +    u16 gbdf;

For one, this would better be placed immediately after devfn (on
64-bit there's a 32-bit hole there).

Second, what about the segment number - is that one always
going to be the physical one? Or always zero?

Finally, please correct the typo ("iommu") and remove "amd" as once
again the concept ought to be generic.

Jan

>  };
>  
>  #define for_each_pdev(domain, pdev) \



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.