Re: [RFC PATCH 2/23] DMOP: Introduce new DMOP commands for vIOMMU support

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Apr 18, 2017 at 03:24:35PM +0800, Lan Tianyu wrote:
> Hi Konrad:
> 	Thanks for your review.
> 
> On 2017年04月17日 22:36, Konrad Rzeszutek Wilk wrote:
> > On Fri, Mar 17, 2017 at 07:27:02PM +0800, Lan Tianyu wrote:
> >> This patch is to introduce create, destroy and query capabilities
> >> command for vIOMMU. vIOMMU layer will deal with requests and call
> >> arch vIOMMU ops.
> >>
> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >> ---
> >>  xen/arch/x86/hvm/dm.c          | 29 +++++++++++++++++++++++++++++
> >>  xen/include/public/hvm/dm_op.h | 39 +++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 68 insertions(+)
> >>
> >> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> >> index 2122c45..2b28f70 100644
> >> --- a/xen/arch/x86/hvm/dm.c
> >> +++ b/xen/arch/x86/hvm/dm.c
> >> @@ -491,6 +491,35 @@ static int dm_op(domid_t domid,
> >>          break;
> >>      }
> >>  
> >> +    case XEN_DMOP_create_viommu:
> >> +    {
> >> +        struct xen_dm_op_create_viommu *data =
> >> +            &op.u.create_viommu;
> >> +
> >> +        rc = viommu_create(d, data->base_address, data->length, data->capabilities);
> >> +        if (rc >= 0) {
> > 
> > The style guide is is to have a space here and { on a newline.
> 
> Yes, will fix.
> 
> > 
> >> +            data->viommu_id = rc;
> >> +            rc = 0;
> >> +        }
> >> +        break;
> >> +    }
> > 
> > Newline here..
> > 
> > 
> >> +    case XEN_DMOP_destroy_viommu:
> >> +    {
> >> +        const struct xen_dm_op_destroy_viommu *data =
> >> +            &op.u.destroy_viommu;
> >> +
> >> +        rc = viommu_destroy(d, data->viommu_id);
> >> +        break;
> >> +    }
> > 
> > Ahem?
> >> +    case XEN_DMOP_query_viommu_caps:
> >> +    {
> >> +        struct xen_dm_op_query_viommu_caps *data =
> >> +            &op.u.query_viommu_caps;
> >> +
> >> +        data->caps = viommu_query_caps(d);
> >> +        rc = 0;
> >> +        break;
> >> +    }
> > 
> > And here.
> >>      default:
> >>          rc = -EOPNOTSUPP;
> >>          break;
> >> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h
> >> index f54cece..b8c7359 100644
> >> --- a/xen/include/public/hvm/dm_op.h
> >> +++ b/xen/include/public/hvm/dm_op.h
> >> @@ -318,6 +318,42 @@ struct xen_dm_op_inject_msi {
> >>      uint64_aligned_t addr;
> >>  };
> >>  
> >> +/*
> >> + * XEN_DMOP_create_viommu: Create vIOMMU device.
> >> + */
> >> +#define XEN_DMOP_create_viommu 15
> >> +
> >> +struct xen_dm_op_create_viommu {
> >> +    /* IN - MMIO base address of vIOMMU */
> > 
> > Any limit? Can it be zero?
> 
> In current patchset, base address is allocated by toolstack and passed
> to Qemu to create vIOMMU in hyervisor. Toolstack should make sure the
> range won't be conflicted with other resource.

Sure, but the hypervisor should also do some sanity checking. Having some idea
of limits/sizes would be quite helpfull. Either in the code or in this
comment.
> 
> > 
> >> +    uint64_t base_address;
> >> +    /* IN - Length of MMIO region */
> > 
> > Any restrictions? Can it be say 2 bytes? Or is this in page-size granularity?
> 
> >From the VTD spec, register size must be an integer multiple of 4KB and
> I think the vIOMMU device model(E,G vvtd) in hypervisor should check the
> lengh. Different vendor may have different restriction.

Right, but I thinking you should document this, or at least make it clear
what the expectations are. 
> 
> > 
> >> +    uint64_t length;
> >> +    /* IN - Capabilities with which we want to create */
> >> +    uint64_t capabilities;
> > 
> > That sounds like some form of flags?
> 
> Yes, this patchset just introduces interrupt remapping flag and other
> vendor also can use it to add new features.
> 
> > 
> >> +    /* OUT - vIOMMU identity */
> >> +    uint32_t viommu_id;
> >> +};
> >> +
> >> +/*
> >> + * XEN_DMOP_destroy_viommu: Destroy vIOMMU device.
> >> + */
> >> +#define XEN_DMOP_destroy_viommu 16
> >> +
> >> +struct xen_dm_op_destroy_viommu {
> >> +    /* OUT - vIOMMU identity */
> > 
> > Out? Not in?
> 
> Sorry, it should be OUT parameter.
> 
> > 
> >> +    uint32_t viommu_id;
> >> +};
> >> +
> >> +/*
> >> + * XEN_DMOP_q_viommu: Query vIOMMU capabilities.
> >> + */
> >> +#define XEN_DMOP_query_viommu_caps 17
> >> +
> >> +struct xen_dm_op_query_viommu_caps {
> >> +    /* OUT - vIOMMU Capabilities*/
> > 
> > Don't you need to also mention which vIOMMU? As you
> > could have potentially many of them?
> 
> If we want to support different vendors' vIOMMU, it's necessary to do
> that and we need to introduce a new field "vIOMMU type" (E,G Intel, AMD
> and ARM IOMMU).

Right, but the 'xen_dm_op_create_viommu' has the capabilities
and retuirns the vIOMMU identity. It looks like it could be called
multiple times which means you could have multiple vIOMMU and
each could have a different capability.

As such this call should also have as IN the vIOMMU identity
to at least be symmetrical with the other hypercalls.

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