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 Fri, Aug 18, 2017 at 01:45:50PM +0800, Chao Gao wrote:
> 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?
> 

Thinking a bit more about this, when I first said "conflicts" I didn't
mean to parse the content. I was referring to the code in
libxl_x86_apci.c which also seems to manipulate acpi_modules.

I would like the code to generate dmar take into consideration
libxl__dom_load_acpi.


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