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

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

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@xxxxxxxxx>
>> ---
>>  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.

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

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


> 
>> +    uint64_t caps;
>> +};
>> +
>>  struct xen_dm_op {
>>      uint32_t op;
>>      uint32_t pad;
>> @@ -336,6 +372,9 @@ struct xen_dm_op {
>>          struct xen_dm_op_set_mem_type set_mem_type;
>>          struct xen_dm_op_inject_event inject_event;
>>          struct xen_dm_op_inject_msi inject_msi;
>> +        struct xen_dm_op_create_viommu create_viommu;
>> +        struct xen_dm_op_destroy_viommu destroy_viommu;
>> +        struct xen_dm_op_query_viommu_caps query_viommu_caps;
>>      } u;
>>  };
>>  
>> -- 
>> 1.8.3.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxx
>> https://lists.xen.org/xen-devel


-- 
Best regards
Tianyu Lan

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