Re: [Xen-devel] [PATCH V2 9/25] tools/libxl: build DMAR table for a guest with one virtual VTD

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Aug 17, 2017 at 01:28:21PM +0100, Wei Liu wrote:
>On Thu, Aug 17, 2017 at 12:32:17PM +0100, Wei Liu wrote:
>> On Wed, Aug 09, 2017 at 04:34:10PM -0400, Lan Tianyu wrote:
>> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> > index f54fd49..94c9196 100644
>> > --- a/tools/libxl/libxl_dom.c
>> > +++ b/tools/libxl/libxl_dom.c
>> > @@ -1060,6 +1060,42 @@ static int libxl__domain_firmware(libxl__gc *gc,
>> >          }
>> >      }
>> >  
>> > +    /*
>> > +     * If a guest has one virtual VTD, build DMAR table for it and joint 
>> > this
>> > +     * table with existing content in acpi_modules in order to employ HVM
>> > +     * firmware pass-through mechanism to pass-through DMAR table.
>> > +     */
>> > +    if (info->viommu.type == LIBXL_VIOMMU_TYPE_INTEL_VTD) {
>> > +        datalen = 0;
>> > +        e = libxl__dom_build_dmar(gc, info, dom, &data, &datalen);
>> > +        if (e) {
>> > +            LOGEV(ERROR, e, "failed to build DMAR table");
>> > +            rc = ERROR_FAIL;
>> > +            goto out;
>> > +        }
>> > +        if (datalen) {
>> > +            libxl__ptr_add(gc, data);
>> > +            if (!dom->acpi_modules[0].data) {
>> > +                dom->acpi_modules[0].data = data;
>> > +                dom->acpi_modules[0].length = (uint32_t)datalen;
>> > +            } else {
>> > +                /* joint tables */
>> > +                void *newdata;
>> > +                newdata = malloc(datalen + dom->acpi_modules[0].length);
>> 
>> All memory allocations in libxl should use libxl__*lloc wrappers.
>> 
>> > +                if (!newdata) {
>> > +                    LOGE(ERROR, "failed to joint DMAR table to acpi 
>> > modules");
>> > +                    rc = ERROR_FAIL;
>> > +                    goto out;
>> > +                }
>> > +                memcpy(newdata, dom->acpi_modules[0].data,
>> > +                       dom->acpi_modules[0].length);
>> > +                memcpy(newdata + dom->acpi_modules[0].length, data, 
>> > datalen);
>> > +                dom->acpi_modules[0].data = newdata;
>> > +                dom->acpi_modules[0].length += (uint32_t)datalen;
>
>Also, this leaks the old pointer, right?

Yes. Will fix this.

>
>> > +            }
>> > +        }
>> > +    }
>> 
>> This still looks wrong to me. How do you know acpi_modules[0] is DMAR
>> table?
>> 
>
>Oh, I sorta see why you do this, but I still think this is wrong. The
>DMAR should either be a new module or be joined to the existing one (and
>with all conflicts resolved).

Hi, Wei
Thanks for your comments.

iirc, HVM only supports one module; DMAR cannot be a new module. Joining to
the existing one is the approach we are taking. 

Which kind of conflicts you think should be resolved? If you mean I
forget to free the old buf, I will fix this. If you mean the potential
overlap between the binary passed by admin and DMAR table built here, I
don't have much idea on this. Even without the DMAR table, the binary
may contains MADT or other tables and tool stacks don't intrepret the
binary and check whether there are conflicts, right?

Thanks
Chao

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