| Message ID | 1502310866-10450-10-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > + } > + } > + } This still looks wrong to me. How do you know acpi_modules[0] is DMAR table? You should have a look at libxl_x86_acpi.c and work out a proper solution.
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).
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
On Fri, Aug 18, 2017 at 01:45:50PM +0800, Chao Gao wrote: > > > >> > + } > >> > + } > >> > + } > >> > >> 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; This is indeed how it is stated in various comments. I'm not sure why there is such restriction. Maybe x86 maintainers can shed more light on this? > 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? That's true. Ignore the comment about fixing up conflicts then.
>>> On 18.08.17 at 15:45, <wei.liu2@citrix.com> wrote: > On Fri, Aug 18, 2017 at 01:45:50PM +0800, Chao Gao wrote: >> > >> >> > + } >> >> > + } >> >> > + } >> >> >> >> 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; > > This is indeed how it is stated in various comments. I'm not sure why > there is such restriction. Maybe x86 maintainers can shed more light on > this? Not me, sorry. Maybe ask whoever has written that code? Jan
On Fri, Aug 18, 2017 at 07:56:36AM -0600, Jan Beulich wrote: > >>> On 18.08.17 at 15:45, <wei.liu2@citrix.com> wrote: > > On Fri, Aug 18, 2017 at 01:45:50PM +0800, Chao Gao wrote: > >> > > >> >> > + } > >> >> > + } > >> >> > + } > >> >> > >> >> 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; > > > > This is indeed how it is stated in various comments. I'm not sure why > > there is such restriction. Maybe x86 maintainers can shed more light on > > this? > > Not me, sorry. Maybe ask whoever has written that code? > OK. I have misunderstood the restriction was from hvmloader.
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.
On 2017年08月22日 21:48, Wei Liu wrote: >> > 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. Code in libxl_x86_acpi.c works for Hvmlite/PVHv2. The code we added is for hvm guest. > > I would like the code to generate dmar take into consideration > libxl__dom_load_acpi. > If add dmar table for hvmlite, we should combine dmar table with other ACPI table and populate into acpi_modules[2]. This is how hvmlite add other ACPI tables in libxl__dom_load_acpi().
On Wed, Aug 23, 2017 at 01:35:17PM +0800, Lan Tianyu wrote: > On 2017年08月22日 21:48, Wei Liu wrote: > >> > 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. > > Code in libxl_x86_acpi.c works for Hvmlite/PVHv2. The code we added is > for hvm guest. > That's correct for the code as-is but what is preventing the code there from working with HVM? Assuming correct checks and branches are added to appropriate places? I'm against having multiple locations doing things that could potentially clash with each other. In the foreseeable future PVH is going to get need similar functionality. My expectation is that if the existing code needs to be taken into consideration and the contributors need to figure out if and how it can be modified to suite their needs. If everyone is doing their own thing in their own little function Xen will eventually become unmaintainable. > > > > I would like the code to generate dmar take into consideration > > libxl__dom_load_acpi. > > > > If add dmar table for hvmlite, we should combine dmar table with other > ACPI table and populate into acpi_modules[2]. This is how hvmlite add > other ACPI tables in libxl__dom_load_acpi(). > Sure, that sounds plausible. What I would like to see is to have one entry point to manipulate APCI tables. Given the patch volume we're seeing now, we expect contributors to drive the discussion forward. If you're not sure, feel free to ask more questions. > > -- > Best regards > Tianyu Lan
On 2017年08月23日 16:34, Wei Liu wrote: > On Wed, Aug 23, 2017 at 01:35:17PM +0800, Lan Tianyu wrote: >> On 2017年08月22日 21:48, Wei Liu wrote: >>>>> 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. >> >> Code in libxl_x86_acpi.c works for Hvmlite/PVHv2. The code we added is >> for hvm guest. >> > > That's correct for the code as-is but what is preventing the code there > from working with HVM? Assuming correct checks and branches are added > to appropriate places? > > I'm against having multiple locations doing things that could > potentially clash with each other. In the foreseeable future PVH is > going to get need similar functionality. > > My expectation is that if the existing code needs to be taken into > consideration and the contributors need to figure out if and how it can > be modified to suite their needs. If everyone is doing their own thing > in their own little function Xen will eventually become unmaintainable. > >>> >>> I would like the code to generate dmar take into consideration >>> libxl__dom_load_acpi. >>> >> >> If add dmar table for hvmlite, we should combine dmar table with other >> ACPI table and populate into acpi_modules[2]. This is how hvmlite add >> other ACPI tables in libxl__dom_load_acpi(). >> > > Sure, that sounds plausible. > > What I would like to see is to have one entry point to manipulate APCI > tables. > > Given the patch volume we're seeing now, we expect contributors to drive > the discussion forward. If you're not sure, feel free to ask more questions. I am not sure whether I understood correctly. PVHv2 builds all ACPI table in tool stack and uses acpi_module[0, 1, 2] to pass related table content. HVM builds ACPI tables in hvmloader and just use acpi_module[0] to pass additional ACPI firmware or table. These two modes have different way to use acpi_modules[]. So I think we can't combine them, right? For build dmar table, we have introduced construct_dmar() in under libacpi to build dmar table and PVHv2 also can use it in libxl__dom_load_acpi().
On Thu, Aug 24, 2017 at 11:24:12AM +0800, Lan Tianyu wrote: > On 2017年08月23日 16:34, Wei Liu wrote: > >>> > >>> I would like the code to generate dmar take into consideration > >>> libxl__dom_load_acpi. > >>> > >> > >> If add dmar table for hvmlite, we should combine dmar table with other > >> ACPI table and populate into acpi_modules[2]. This is how hvmlite add > >> other ACPI tables in libxl__dom_load_acpi(). > >> > > > > Sure, that sounds plausible. > > > > What I would like to see is to have one entry point to manipulate APCI > > tables. > > > > Given the patch volume we're seeing now, we expect contributors to drive > > the discussion forward. If you're not sure, feel free to ask more questions. > > I am not sure whether I understood correctly. > > PVHv2 builds all ACPI table in tool stack and uses acpi_module[0, 1, 2] > to pass related table content. > > HVM builds ACPI tables in hvmloader and just use acpi_module[0] to pass > additional ACPI firmware or table. > > These two modes have different way to use acpi_modules[]. So I think we > can't combine them, right? > There might be some misunderstanding. We probably don't want to manipulate the content of the tables in libxl. > For build dmar table, we have introduced construct_dmar() in under > libacpi to build dmar table and PVHv2 also can use it in > libxl__dom_load_acpi(). > My major complain is now there are two functions and in two different locations, in two different phases of domain construction that would manipulate ACPI tables. I would like to have only one. The function you're currently modifying libxl__domain_firmware is not the right place. It's primary function is to load files from disks. You should be able to call the function you introduced in libxl__dom_load_acpi, provided appropriate checks are added.
On 2017年08月24日 19:08, Wei Liu wrote: >>>> If add dmar table for hvmlite, we should combine dmar table with other >>>> > >> ACPI table and populate into acpi_modules[2]. This is how hvmlite add >>>> > >> other ACPI tables in libxl__dom_load_acpi(). >>>> > >> >>> > > >>> > > Sure, that sounds plausible. >>> > > >>> > > What I would like to see is to have one entry point to manipulate APCI >>> > > tables. >>> > > >>> > > Given the patch volume we're seeing now, we expect contributors to drive >>> > > the discussion forward. If you're not sure, feel free to ask more questions. >> > >> > I am not sure whether I understood correctly. >> > >> > PVHv2 builds all ACPI table in tool stack and uses acpi_module[0, 1, 2] >> > to pass related table content. >> > >> > HVM builds ACPI tables in hvmloader and just use acpi_module[0] to pass >> > additional ACPI firmware or table. >> > >> > These two modes have different way to use acpi_modules[]. So I think we >> > can't combine them, right? >> > > There might be some misunderstanding. We probably don't want to > manipulate the content of the tables in libxl. > >> > For build dmar table, we have introduced construct_dmar() in under >> > libacpi to build dmar table and PVHv2 also can use it in >> > libxl__dom_load_acpi(). >> > > My major complain is now there are two functions and in two different > locations, in two different phases of domain construction that would > manipulate ACPI tables. I would like to have only one. > > The function you're currently modifying libxl__domain_firmware is not > the right place. It's primary function is to load files from disks. > > You should be able to call the function you introduced in > libxl__dom_load_acpi, provided appropriate checks are added. But libxl__dom_load_acpi() isn't called on hvm guest code path. It just works for PVHv2/HVMlite and have some conflict with hvm guest configuration(i.e, acpi_module). int libxl__arch_domain_finalise_hw_description(libxl__gc *gc, libxl_domain_build_info *info, struct xc_dom_image *dom) { int rc = 0; if ((info->type == LIBXL_DOMAIN_TYPE_HVM) && (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE)) { rc = libxl__dom_load_acpi(gc, info, dom); if (rc != 0) LOGE(ERROR, "libxl_dom_load_acpi failed"); } return rc; }
On 2017年08月25日 11:19, Lan Tianyu wrote: > On 2017年08月24日 19:08, Wei Liu wrote: >>>>> If add dmar table for hvmlite, we should combine dmar table with other >>>>>>>> ACPI table and populate into acpi_modules[2]. This is how hvmlite add >>>>>>>> other ACPI tables in libxl__dom_load_acpi(). >>>>>>>> >>>>>> >>>>>> Sure, that sounds plausible. >>>>>> >>>>>> What I would like to see is to have one entry point to manipulate APCI >>>>>> tables. >>>>>> >>>>>> Given the patch volume we're seeing now, we expect contributors to drive >>>>>> the discussion forward. If you're not sure, feel free to ask more questions. >>>> >>>> I am not sure whether I understood correctly. >>>> >>>> PVHv2 builds all ACPI table in tool stack and uses acpi_module[0, 1, 2] >>>> to pass related table content. >>>> >>>> HVM builds ACPI tables in hvmloader and just use acpi_module[0] to pass >>>> additional ACPI firmware or table. >>>> >>>> These two modes have different way to use acpi_modules[]. So I think we >>>> can't combine them, right? >>>> >> There might be some misunderstanding. We probably don't want to >> manipulate the content of the tables in libxl. >> >>>> For build dmar table, we have introduced construct_dmar() in under >>>> libacpi to build dmar table and PVHv2 also can use it in >>>> libxl__dom_load_acpi(). >>>> >> My major complain is now there are two functions and in two different >> locations, in two different phases of domain construction that would >> manipulate ACPI tables. I would like to have only one. >> >> The function you're currently modifying libxl__domain_firmware is not >> the right place. It's primary function is to load files from disks. >> >> You should be able to call the function you introduced in >> libxl__dom_load_acpi, provided appropriate checks are added. > > But libxl__dom_load_acpi() isn't called on hvm guest code path. It just > works for PVHv2/HVMlite and have some conflict with hvm guest > configuration(i.e, acpi_module). > > > int libxl__arch_domain_finalise_hw_description(libxl__gc *gc, > libxl_domain_build_info > *info, > struct xc_dom_image *dom) > { > int rc = 0; > > if ((info->type == LIBXL_DOMAIN_TYPE_HVM) && > (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE)) { > rc = libxl__dom_load_acpi(gc, info, dom); > if (rc != 0) > LOGE(ERROR, "libxl_dom_load_acpi failed"); > } > > return rc; > } We may remove the check and move introduced code in libxl__dom_load_acpi(). Run new code just for hvm guest. Does this make sense?
On Fri, Aug 25, 2017 at 03:33:47PM +0800, Lan Tianyu wrote: > > > > int libxl__arch_domain_finalise_hw_description(libxl__gc *gc, > > libxl_domain_build_info > > *info, > > struct xc_dom_image *dom) > > { > > int rc = 0; > > > > if ((info->type == LIBXL_DOMAIN_TYPE_HVM) && > > (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE)) { > > rc = libxl__dom_load_acpi(gc, info, dom); > > if (rc != 0) > > LOGE(ERROR, "libxl_dom_load_acpi failed"); > > } > > > > return rc; > > } > > We may remove the check and move introduced code in > libxl__dom_load_acpi(). Run new code just for hvm guest. Does this make > sense? > More or less. Push the check down to libxl__dom_load_acpi > -- > Best regards > Tianyu Lan
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h index 5e1fc60..d8ddd60 100644 --- a/tools/libxl/libxl_arch.h +++ b/tools/libxl/libxl_arch.h @@ -78,6 +78,11 @@ int libxl__arch_extra_memory(libxl__gc *gc, int libxl__dom_load_acpi(libxl__gc *gc, const libxl_domain_build_info *b_info, struct xc_dom_image *dom); + +int libxl__dom_build_dmar(libxl__gc *gc, + const libxl_domain_build_info *b_info, + struct xc_dom_image *dom, + void **data, int *len); #endif #endif 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); + 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; + } + } + } + return 0; out: assert(rc != 0); diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c index c0a6e32..1fa97ff 100644 --- a/tools/libxl/libxl_x86_acpi.c +++ b/tools/libxl/libxl_x86_acpi.c @@ -16,6 +16,7 @@ #include "libxl_arch.h" #include <xen/hvm/hvm_info_table.h> #include <xen/hvm/e820.h> +#include "libacpi/acpi2_0.h" #include "libacpi/libacpi.h" #include <xc_dom.h> @@ -236,6 +237,53 @@ out: return rc; } +static void *acpi_memalign(struct acpi_ctxt *ctxt, uint32_t size, + uint32_t align) +{ + int ret; + void *ptr; + + ret = posix_memalign(&ptr, align, size); + if (ret != 0 || !ptr) + return NULL; + + return ptr; +} + +int libxl__dom_build_dmar(libxl__gc *gc, + const libxl_domain_build_info *b_info, + struct xc_dom_image *dom, + void **data, int *len) +{ + struct acpi_config config = { 0 }; + struct acpi_ctxt ctxt; + void *table; + + if ((b_info->type != LIBXL_DOMAIN_TYPE_HVM) || + (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE) || + (b_info->viommu.type != LIBXL_VIOMMU_TYPE_INTEL_VTD)) + return 0; + + ctxt.mem_ops.alloc = acpi_memalign; + ctxt.mem_ops.v2p = virt_to_phys; + ctxt.mem_ops.free = acpi_mem_free; + + if (libxl_defbool_val(b_info->viommu.intremap)) + config.iommu_intremap_supported = true; + if (libxl_defbool_val(b_info->viommu.u.intel_vtd.x2apic)) + config.iommu_x2apic_supported = true; + config.iommu_base_addr = b_info->viommu.base_addr; + + config.ioapic_id = 1; /* the IOAPIC_ID used by HVM */ + + table = construct_dmar(&ctxt, &config); + if ( !table ) + return ERROR_NOMEM; + *data = table; + *len = ((struct acpi_header *)table)->length; + return 0; +} + /* * Local variables: * mode: C