Re: [PATCH V3 6/29] tools/libxl: Add a user configurable parameter to control vIOMMU attributes

[Chao Gao <chao.gao@intel.com>]

On Thu, Oct 19, 2017 at 10:49:22AM +0100, Roger Pau Monné wrote:
>On Thu, Sep 21, 2017 at 11:01:47PM -0400, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> A field, viommu_info, is added to struct libxl_domain_build_info. Several
>> attributes can be specified by guest config file for virtual IOMMU. These
>> attributes are used for DMAR construction and vIOMMU creation.
>
>IMHO this should come much later in the series, ideally you would
>introduce the xl/libxl code in the last patches, together with the
>xl.cfg man page change.

It can be put to the end of this series. But I prefer to introduce the
vIOMMU from up to down (means the use interface goes first and then how
to implement a vIOMMU step by step) for it may be easier to understand. 

>
>> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
>> index 9123585..decd7a8 100644
>> --- a/tools/libxl/libxl_create.c
>> +++ b/tools/libxl/libxl_create.c
>> @@ -27,6 +27,8 @@
>>  
>>  #include <xen-xsm/flask/flask.h>
>>  
>> +#define VIOMMU_VTD_BASE_ADDR        0xfed90000ULL
>
>This should be in libxl_arch.h see LAPIC_BASE_ADDRESS.

Agree.

>
>> +
>>  int libxl__domain_create_info_setdefault(libxl__gc *gc,
>>                                           libxl_domain_create_info *c_info)
>>  {
>> @@ -59,6 +61,47 @@ void libxl__rdm_setdefault(libxl__gc *gc, libxl_domain_build_info *b_info)
>>                              LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT;
>>  }
>>  
>> +static int libxl__viommu_set_default(libxl__gc *gc,
>> +                                     libxl_domain_build_info *b_info)
>> +{
>> +    int i;
>> +
>> +    if (!b_info->num_viommus)
>> +        return 0;
>> +
>> +    for (i = 0; i < b_info->num_viommus; i++) {
>> +        libxl_viommu_info *viommu = &b_info->viommu[i];
>> +
>> +        if (libxl_defbool_is_default(viommu->intremap))
>> +            libxl_defbool_set(&viommu->intremap, true);
>> +
>> +        if (!libxl_defbool_val(viommu->intremap)) {
>> +            LOGE(ERROR, "Cannot create one virtual VTD without intremap");
>> +            return ERROR_INVAL;
>> +        }
>> +
>> +        if (viommu->type == LIBXL_VIOMMU_TYPE_INTEL_VTD) {
>> +            /*
>> +             * If there are multiple vIOMMUs, we need arrange all vIOMMUs to
>> +             * avoid overlap. Put a check here in case we get here for multiple
>> +             * vIOMMUs case.
>> +             */
>> +            if (b_info->num_viommus > 1) {
>> +                LOGE(ERROR, "Multiple vIOMMUs support is under implementation");
>
>s/LOGE/LOG/ LOGE should only be used when errno is set (which is not
>the case here).

yes.

>
>> +                return ERROR_INVAL;
>> +            }
>> +
>> +            /* Set default values to unexposed fields */
>> +            viommu->base_addr = VIOMMU_VTD_BASE_ADDR;
>> +
>> +            /* Set desired capbilities */
>> +            viommu->cap = VIOMMU_CAP_IRQ_REMAPPING;
>
>I'm not sure whether this code should be in libxl_x86.c, but
>libxl__domain_build_info_setdefault is already quite messed up, so I
>guess it's fine.
>
>> +        }
>
>Shouldn't this be:
>
>switch(viommu->type) {
>case LIBXL_VIOMMU_TYPE_INTEL_VTD:
>    ...
>    break;
>
>default:
>    return ERROR_INVAL;
>}
>
>So that you catch type being set to an invalid vIOMMU type?

sure. Will update.

>
>> +    if (d_config->b_info.num_viommus > 1) {
>> +        ret = ERROR_INVAL;
>> +        LOGD(ERROR, domid, "Cannot support multiple vIOMMUs");
>> +        goto error_out;
>> +    }
>
>Er, you already have this check in libxl__viommu_set_default, and in
>any case I would just rely on the hypervisor failing to create more
>than one vIOMMU per domain, rather than adding the same check here.

It is fine to me. Will remove all checks against viommu numbers in
toolstack.

Thanks
chao

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