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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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?

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

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