Re: [PATCH V3 11/29] x86/hvm: Introduce a emulated VTD for HVM

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

On Thu, Oct 19, 2017 at 12:20:35PM +0100, Roger Pau Monné wrote:
>On Thu, Sep 21, 2017 at 11:01:52PM -0400, Lan Tianyu wrote:
>> From: Chao Gao <chao.gao@intel.com>
>> 
>> This patch adds create/destroy function for the emulated VTD
>> and adapts it to the common VIOMMU abstraction.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
>> ---
>>  
>> -obj-y += iommu.o
>>  obj-y += dmar.o
>> -obj-y += utils.o
>> -obj-y += qinval.o
>>  obj-y += intremap.o
>> +obj-y += iommu.o
>> +obj-y += qinval.o
>>  obj-y += quirks.o
>> +obj-y += utils.o
>
>Why do you need to shuffle the list above?

I placed them in alphabetic order.

>
>Also I'm not sure the Intel vIOMMU implementation should live here. As
>you can see the path is:
>
>xen/drivers/passthrough/vtd/
>
>The vIOMMU is not tied to passthrough at all, so I would rather place
>it in:
>
>xen/drivers/vvtd/
>
>Or maybe you can create something like:
>
>xen/drivers/viommu/
>
>So that all vIOMMU implementations can share some code.
>

vvtd and vtd use the same header files (i.g. vtd.h). That is why we put
it there.  If that, we shoule move the related header files to a public
directory.

>>  #define cap_isoch(c)        (((c) >> 23) & 1)
>>  #define cap_qos(c)        (((c) >> 22) & 1)
>>  #define cap_mgaw(c)        ((((c) >> 16) & 0x3f) + 1)
>> -#define cap_sagaw(c)        (((c) >> 8) & 0x1f)
>> +#define cap_set_mgaw(c)     ((((c) - 1) & 0x3f) << 16)
>> +#define cap_sagaw(c)        (((c) >> DMA_CAP_SAGAW_SHIFT) & 0x1f)
>>  #define cap_caching_mode(c)    (((c) >> 7) & 1)
>>  #define cap_phmr(c)        (((c) >> 6) & 1)
>>  #define cap_plmr(c)        (((c) >> 5) & 1)
>> @@ -104,10 +113,16 @@
>>  #define ecap_niotlb_iunits(e)    ((((e) >> 24) & 0xff) + 1)
>>  #define ecap_iotlb_offset(e)     ((((e) >> 8) & 0x3ff) * 16)
>>  #define ecap_coherent(e)         ((e >> 0) & 0x1)
>> -#define ecap_queued_inval(e)     ((e >> 1) & 0x1)
>> +#define DMA_ECAP_QI_SHIFT        1
>> +#define DMA_ECAP_QI              (1ULL << DMA_ECAP_QI_SHIFT)
>> +#define ecap_queued_inval(e)     ((e >> DMA_ECAP_QI_SHIFT) & 0x1)
>
>Looks like this could be based on MASK_EXTR instead, but seeing how
>the file is full of open-coded mask extracts I'm not sure it's worth
>it anymore.
>
>>  #define ecap_dev_iotlb(e)        ((e >> 2) & 0x1)
>> -#define ecap_intr_remap(e)       ((e >> 3) & 0x1)
>> -#define ecap_eim(e)              ((e >> 4) & 0x1)
>> +#define DMA_ECAP_IR_SHIFT        3
>> +#define DMA_ECAP_IR              (1ULL << DMA_ECAP_IR_SHIFT)
>> +#define ecap_intr_remap(e)       ((e >> DMA_ECAP_IR_SHIFT) & 0x1)
>> +#define DMA_ECAP_EIM_SHIFT       4
>> +#define DMA_ECAP_EIM             (1ULL << DMA_ECAP_EIM_SHIFT)
>> +#define ecap_eim(e)              ((e >> DMA_ECAP_EIM_SHIFT) & 0x1)
>
>Maybe worth placing all the DMA_ECAP_* defines in a separate section?
>Seems like how it's done for other features like DMA_FSTS or
>DMA_CCMD.

Got it.

>> +
>> +/* Supported capabilities by vvtd */
>> +unsigned int vvtd_caps = VIOMMU_CAP_IRQ_REMAPPING;
>
>static?
>
>Or even better, why is this not a define like VIOMMU_MAX_CAPS or
>similar.

Yeah. It should be renamed to VVTD_MAX_CAPS.

>
>> +
>> +union hvm_hw_vvtd_regs {
>> +    uint32_t data32[256];
>> +    uint64_t data64[128];
>> +};
>
>Do you really need to store all the register space instead of only
>storing specific registers?

I prefer to store all the registers for we don't need a trick to map
the real offset in hardware to the index in the array.

>
>> +
>> +struct vvtd {
>> +    /* Address range of remapping hardware register-set */
>> +    uint64_t base_addr;
>> +    uint64_t length;
>
>The length field doesn't seem to be used below.

will remove it.

>
>> +    /* Point back to the owner domain */
>> +    struct domain *domain;
>> +    union hvm_hw_vvtd_regs *regs;
>
>Does this need to be a pointer?

Seems not.
>
>> +    struct page_info *regs_page;
>> +};
>> +
>> +static int vvtd_create(struct domain *d, struct viommu *viommu)
>> +{
>> +    struct vvtd *vvtd;
>> +    int ret;
>> +
>> +    if ( !is_hvm_domain(d) || (viommu->base_address & (PAGE_SIZE - 1)) ||
>> +        (~vvtd_caps & viommu->caps) )
>> +        return -EINVAL;
>> +
>> +    ret = -ENOMEM;
>> +    vvtd = xzalloc_bytes(sizeof(struct vvtd));
>> +    if ( !vvtd )
>> +        return ret;
>> +
>> +    vvtd->regs_page = alloc_domheap_page(d, MEMF_no_owner);
>> +    if ( !vvtd->regs_page )
>> +        goto out1;
>> +
>> +    vvtd->regs = __map_domain_page_global(vvtd->regs_page);
>> +    if ( !vvtd->regs )
>> +        goto out2;
>> +    clear_page(vvtd->regs);
>
>Not sure why vvtd->regs needs to be a pointer, and why it needs to use
>a full page. AFAICT the size of hvm_hw_vvtd_regs is 1024B, so you are
>wasting 3/4 of a page.

I will define registers as an array directly and 
shrink the size to the number we are really used now.

>> +struct viommu_ops vvtd_hvm_vmx_ops = {
>> +    .create = vvtd_create,
>> +    .destroy = vvtd_destroy
>> +};
>> +
>> +static int vvtd_register(void)
>> +{
>> +    viommu_register_type(VIOMMU_TYPE_INTEL_VTD, &vvtd_hvm_vmx_ops);
>> +    return 0;
>> +}
>> +__initcall(vvtd_register);
>
>As commented in another patch I think the vIOMMU types should be
>registered using a method similar to REGISTER_SCHEDULER.

Both are ok to me. Will follow your suggestion.

Thanks
Chao

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