Re: [Xen-devel] [PATCH V2 1/25] DOMCTL: Introduce new DOMCTL commands for vIOMMU support

[Lan Tianyu <tianyu.lan@xxxxxxxxx>]

Hi Roger:
        Thanks for your review.

On 2017年08月22日 22:32, Roger Pau Monné wrote:
> On Wed, Aug 09, 2017 at 04:34:02PM -0400, 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/common/domctl.c         |  3 +++
>>  xen/common/viommu.c         | 43 +++++++++++++++++++++++++++++++++++++
> 
> I'm confused, I don't see this file in the repo, and the cover letter
> doesn't mention this being based on top of any other series, where
> does this viommu.c file come from?
> 
>>  xen/include/public/domctl.h | 52 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  xen/include/xen/viommu.h    |  6 ++++++
>>  4 files changed, 104 insertions(+)
>>
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index d80488b..01c3024 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -1144,6 +1144,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>> u_domctl)
>>          if ( !ret )
>>              copyback = 1;
>>          break;
>> +    case XEN_DOMCTL_viommu_op:
>> +        ret = viommu_domctl(d, &op->u.viommu_op, &copyback);
>> +        break;
> 
> Hm, shouldn't this be protected with #ifdef CONFIG_VIOMMU?
> 

Added viommu_domctl() always returns -ENODEV when CONFIG_VIOMMU is unset.

>>      default:
>>          ret = arch_do_domctl(op, d, u_domctl);
>> diff --git a/xen/common/viommu.c b/xen/common/viommu.c
>> index 6874d9f..a4d004d 100644
>> --- a/xen/common/viommu.c
>> +++ b/xen/common/viommu.c
>> @@ -148,6 +148,49 @@ static u64 viommu_query_caps(struct domain *d, u64 type)
>>      return viommu_type->ops->query_caps(d);
>>  }
>>  
>> +int viommu_domctl(struct domain *d, struct xen_domctl_viommu_op *op,
>> +                  bool *need_copy)
>> +{
>> +    int rc = -EINVAL, ret;
> 
> Do you really need both ret and rc?
> 
>> +    if ( !viommu_enabled() )
>> +        return rc;
> 
> EINVAL? Maybe ENODEV?

OK.

> 
>> +
>> +    switch ( op->cmd )
>> +    {
>> +    case XEN_DOMCTL_create_viommu:
>> +        ret = viommu_create(d, op->u.create_viommu.viommu_type,
>> +            op->u.create_viommu.base_address,
>> +            op->u.create_viommu.length,
>> +            op->u.create_viommu.capabilities);
> 
> I would rather prefer for viommu_create to simply return an error or
> 0, and store the viommu_id by passing a pointer parameter to viommu_create, 
> ie:
> 
> rc = viommu_create(d, op->u.create_viommu.viommu_type,
>                    op->u.create_viommu.base_address,
>                    op->u.create_viommu.length,
>                    op->u.create_viommu.capabilities,
>                    &op->u.create_viommu.viommu_id);
> 

Got it. Will update in the next version.

>> +        if ( ret >= 0 ) {
>                            ^ coding style
>> +            op->u.create_viommu.viommu_id = ret;
>> +            *need_copy = true;
>> +            rc = 0; /* return 0 if success */
>> +        }
>> +        break;
>> +
>> +    case XEN_DOMCTL_destroy_viommu:
>> +        rc = viommu_destroy(d, op->u.destroy_viommu.viommu_id);
>> +        break;
>> +
>> +    case XEN_DOMCTL_query_viommu_caps:
>> +        ret = viommu_query_caps(d, op->u.query_caps.viommu_type);
> 
> Same here, I would rather pass another parameter and use the return
> for error only.
> 
>> +        if ( ret >= 0 )
>> +        {
>> +            op->u.query_caps.capabilities = ret;
>> +            rc = 0;
>> +        }
>> +        *need_copy = true;
>> +        break;
>> +
>> +    default:
>> +        break;
> 
> Here you should return ENOSYS.


OK.

> 
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>>  int __init viommu_setup(void)
>>  {
>>      INIT_LIST_HEAD(&type_list);
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index ff39762..4b10f26 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1149,6 +1149,56 @@ struct xen_domctl_psr_cat_op {
>>  typedef struct xen_domctl_psr_cat_op xen_domctl_psr_cat_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cat_op_t);
>>  
>> +/*  vIOMMU helper
>> + *
>> + *  vIOMMU interface can be used to create/destroy vIOMMU and
>> + *  query vIOMMU capabilities.
>> + */
>> +
>> +/* vIOMMU type - specify vendor vIOMMU device model */
>> +#define VIOMMU_TYPE_INTEL_VTD     (1u << 0)
> 
> If this going to be used to specify the vendor only, it doesn't need
> to be a bitfield, because it doesn't make sense to specify for
> example VIOMMU_TYPE_INTEL_VTD | VIOMMU_TYPE_AMD, it's either Intel or
> AMD. Do you have plans to expand this with other uses? In which case
> the comment should be fixed.

Wei suggested to replace this bitfield with a number directly. Will update.

> 
>> +
>> +/* vIOMMU capabilities */
>> +#define VIOMMU_CAP_IRQ_REMAPPING  (1u << 0)
>> +
>> +struct xen_domctl_viommu_op {
>> +    uint32_t cmd;
>> +#define XEN_DOMCTL_create_viommu          0
>> +#define XEN_DOMCTL_destroy_viommu         1
>> +#define XEN_DOMCTL_query_viommu_caps      2
>> +    union {
>> +        struct {
>> +            /* IN - vIOMMU type */
>> +            uint64_t viommu_type;
>> +            /* 
>> +             * IN - MMIO base address of vIOMMU. vIOMMU device models
>> +             * are in charge of to check base_address and length.
>> +             */
>> +            uint64_t base_address;
>> +            /* IN - Length of MMIO region */
>> +            uint64_t length;
> 
> It seems weird that you can specify the length, is that something
> that a user would like to set? Isn't the length of the IOMMU MMIO
> region fixed by the hardware spec?

Different vendor may have different IOMMU register region sizes. (e.g,
VTD has one page size for register region). The length field is to make
vIOMMU device model not to abuse address space. Some registers' offsets
are reported by other register and these offsets are emulated by vIOMMU
device model. If it's not necessary, we can remove it and add it when
there is real such requirement.

> 
>> +            /* IN - Capabilities with which we want to create */
>> +            uint64_t capabilities;
>> +            /* OUT - vIOMMU identity */
>> +            uint32_t viommu_id;
>> +        } create_viommu;
>> +
>> +        struct {
>> +            /* IN - vIOMMU identity */
>> +            uint32_t viommu_id;
>> +        } destroy_viommu;
>> +
>> +        struct {
>> +            /* IN - vIOMMU type */
>> +            uint64_t viommu_type;
>> +            /* OUT - vIOMMU Capabilities */
>> +            uint64_t capabilities;
>> +        } query_caps;
> 
> This also seems weird, shouldn't you query the capabilities of an
> already created vIOMMU, rather than a vIOMMU type? Shouldn't the first
> field be viommu_id?
> 

Query interface here is to check what capabilities the vIOMMU device
model specified by viommu_type can support before create vIOMMU (suppose
user may select different capabilities). If capabilities returned by
query interface doesn't meet user configuration, tool stack should
return error. So it just accepts viommu_type.

-- 
Best regards
Tianyu Lan

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