| Message ID | 1467745519-9868-21-git-send-email-boris.ostrovsky@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(CC Stefano) Hi Boris, On 05/07/16 20:05, Boris Ostrovsky wrote: > diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h > index 34a853c..7c6536b 100644 > --- a/tools/libxl/libxl_arch.h > +++ b/tools/libxl/libxl_arch.h > @@ -62,4 +62,7 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc, > uint32_t domid, > struct xc_dom_image *dom); > > +int libxl__dom_load_acpi(libxl__gc *gc, > + libxl_domain_build_info *info, > + struct xc_dom_image *dom); This file contains arch specific function with are called by the generic libxl code. I don't think this is the right file for an x86 specific function which has a generic name. IHMO, this should go in libxl_x86_acpi.h with "x86" in the name and with '_hidden' attribute. > #endif [...] > +static int populate_acpi_pages(struct xc_dom_image *dom, > + xen_pfn_t *extents, > + unsigned int num_pages, > + struct acpi_ctxt *ctxt) > +{ > + int rc; > + xc_interface *xch = dom->xch; > + uint32_t domid = dom->guest_domid; > + unsigned long idx, first_high_idx = (1ull << (32 - ctxt->page_shift)); > + > + for (; num_pages; num_pages--, extents++) { > + > + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) == 1) It looks like this is working because libxl is setting the maximum size of the domain with some slack (1MB). You might get in trouble if the slack is reduced or used by someone or the ACPI blob is increasing. > + continue; > + > + if (dom->highmem_end) { > + idx = --dom->highmem_end; > + if ( idx == first_high_idx ) > + dom->highmem_end = 0; > + } else > + idx = --dom->lowmem_end; > + > + rc = xc_domain_add_to_physmap(xch, domid, > + XENMAPSPACE_gmfn, > + idx, *extents); > + if (rc) > + return rc; > + } > + > + return 0; > +} > + > +int libxl__dom_load_acpi(libxl__gc *gc, > + libxl_domain_build_info *info, > + struct xc_dom_image *dom) > +{ > + struct acpi_config config = {0}; > + struct acpi_ctxt ctxt; > + uint32_t domid = dom->guest_domid; > + xc_interface *xch = dom->xch; > + int rc, i, acpi_pages_num; > + xen_pfn_t extent, *extents; > + void *acpi_pages, *guest_acpi_pages = NULL; > + unsigned long page_mask; > + > + if ((info->type != LIBXL_DOMAIN_TYPE_HVM) || > + (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE)) > + return 0; > + > + ctxt.page_size = XC_DOM_PAGE_SIZE(dom); > + ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom); > + page_mask = (1UL << ctxt.page_shift) - 1; > + > + ctxt.mem_ops.alloc = mem_alloc; > + ctxt.mem_ops.v2p = virt_to_phys; > + > + rc = init_acpi_config(gc, dom, info, &config); > + if (rc) { > + LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc); > + return rc; > + } > + > + /* Map page that will hold RSDP */ > + extent = RSDP_ADDRESS >> ctxt.page_shift; > + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > + if (rc) { > + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", > + __FUNCTION__, rc); > + goto out; > + } > + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, > + ctxt.page_size, > + PROT_READ | PROT_WRITE, > + RSDP_ADDRESS >> ctxt.page_shift); > + if (!config.rsdp) { > + LOG(ERROR, "%s: Can't map acpi_physical", __FUNCTION__); > + rc = -1; > + goto out; > + } > + > + /* Map acpi_info page */ > + extent = ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift; > + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > + if (rc) { > + LOG(ERROR, "%s: populate_acpi_pages for acpi_info failed with %d", > + __FUNCTION__, rc); > + goto out; > + } > + > + config.ainfop = (unsigned long)xc_map_foreign_range(xch, domid, > + ctxt.page_size, > + PROT_READ | PROT_WRITE, > + ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift); Loading the ACPI blob on ARM will be very similar except for the base address. So it would be nice to share some code with it. However, as mentioned in the ACPI thread [1], all the blobs are generally loaded by libxc and not libxl. This is more true on ARM because the guest address space is controlled by libxc (the position of all the blob are decided by it). Regards, [1] https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00040.html
On 07/06/2016 07:05 AM, Julien Grall wrote: > (CC Stefano) > > Hi Boris, > > On 05/07/16 20:05, Boris Ostrovsky wrote: >> diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h >> index 34a853c..7c6536b 100644 >> --- a/tools/libxl/libxl_arch.h >> +++ b/tools/libxl/libxl_arch.h >> @@ -62,4 +62,7 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc, >> uint32_t domid, >> struct xc_dom_image *dom); >> >> +int libxl__dom_load_acpi(libxl__gc *gc, >> + libxl_domain_build_info *info, >> + struct xc_dom_image *dom); > > This file contains arch specific function with are called by the > generic libxl code. > > I don't think this is the right file for an x86 specific function > which has a generic name. IHMO, this should go in libxl_x86_acpi.h > with "x86" in the name and with '_hidden' attribute. Right. I used to call this routine from libxl_dom.c. I moved the call site to libxl_x86.c (as you suggested earlier) but forgot to move the declaration. > >> #endif > > [...] > >> +static int populate_acpi_pages(struct xc_dom_image *dom, >> + xen_pfn_t *extents, >> + unsigned int num_pages, >> + struct acpi_ctxt *ctxt) >> +{ >> + int rc; >> + xc_interface *xch = dom->xch; >> + uint32_t domid = dom->guest_domid; >> + unsigned long idx, first_high_idx = (1ull << (32 - >> ctxt->page_shift)); >> + >> + for (; num_pages; num_pages--, extents++) { >> + >> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) >> == 1) > > It looks like this is working because libxl is setting the maximum > size of the domain with some slack (1MB). You might get in trouble if > the slack is reduced or used by someone or the ACPI blob is increasing. I saw your conversation about slack with Stefano and I am not sure I understood what it was about. If this was about padding guest's memory to be able to put there some additional data (such ACPI tables) then this is not my intention here: if I can't populate (because it is already populated, I guess) then I try to move memory around with code below. That's what mem_hole_populate_ram() does as well. > >> + continue; >> + >> + if (dom->highmem_end) { >> + idx = --dom->highmem_end; >> + if ( idx == first_high_idx ) >> + dom->highmem_end = 0; >> + } else >> + idx = --dom->lowmem_end; >> + >> + rc = xc_domain_add_to_physmap(xch, domid, >> + XENMAPSPACE_gmfn, >> + idx, *extents); >> + if (rc) >> + return rc; >> + } >> + >> + return 0; >> +} >> + >> +int libxl__dom_load_acpi(libxl__gc *gc, >> + libxl_domain_build_info *info, >> + struct xc_dom_image *dom) >> +{ >> + struct acpi_config config = {0}; >> + struct acpi_ctxt ctxt; >> + uint32_t domid = dom->guest_domid; >> + xc_interface *xch = dom->xch; >> + int rc, i, acpi_pages_num; >> + xen_pfn_t extent, *extents; >> + void *acpi_pages, *guest_acpi_pages = NULL; >> + unsigned long page_mask; >> + >> + if ((info->type != LIBXL_DOMAIN_TYPE_HVM) || >> + (info->device_model_version != >> LIBXL_DEVICE_MODEL_VERSION_NONE)) >> + return 0; >> + >> + ctxt.page_size = XC_DOM_PAGE_SIZE(dom); >> + ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom); >> + page_mask = (1UL << ctxt.page_shift) - 1; >> + >> + ctxt.mem_ops.alloc = mem_alloc; >> + ctxt.mem_ops.v2p = virt_to_phys; >> + >> + rc = init_acpi_config(gc, dom, info, &config); >> + if (rc) { >> + LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", >> __FUNCTION__, rc); >> + return rc; >> + } >> + >> + /* Map page that will hold RSDP */ >> + extent = RSDP_ADDRESS >> ctxt.page_shift; >> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >> + if (rc) { >> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", >> + __FUNCTION__, rc); >> + goto out; >> + } >> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, >> + ctxt.page_size, >> + PROT_READ | >> PROT_WRITE, >> + RSDP_ADDRESS >> >> ctxt.page_shift); >> + if (!config.rsdp) { >> + LOG(ERROR, "%s: Can't map acpi_physical", __FUNCTION__); >> + rc = -1; >> + goto out; >> + } >> + >> + /* Map acpi_info page */ >> + extent = ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift; >> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >> + if (rc) { >> + LOG(ERROR, "%s: populate_acpi_pages for acpi_info failed >> with %d", >> + __FUNCTION__, rc); >> + goto out; >> + } >> + >> + config.ainfop = (unsigned long)xc_map_foreign_range(xch, domid, >> + ctxt.page_size, >> + PROT_READ | >> PROT_WRITE, >> + >> ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift); > > Loading the ACPI blob on ARM will be very similar except for the base > address. So it would be nice to share some code with it. > > However, as mentioned in the ACPI thread [1], all the blobs are > generally loaded by libxc and not libxl. This is more true on ARM > because the guest address space is controlled by libxc (the position > of all the blob are decided by it). The difference is that I not only load the tables here but also build them. Which may or may not be the right thing to do in libxc. I suppose I can defer loading (and then keep pointer to tables in acpitable_blob) but the then I need to keep RSDP descriptor somewhere else (it is not part of the blob since it lives in lower MB of the guest). -boris > > Regards, > > [1] > https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00040.html >
Hi Boris, On 06/07/16 16:50, Boris Ostrovsky wrote: > On 07/06/2016 07:05 AM, Julien Grall wrote: >>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>> + xen_pfn_t *extents, >>> + unsigned int num_pages, >>> + struct acpi_ctxt *ctxt) >>> +{ >>> + int rc; >>> + xc_interface *xch = dom->xch; >>> + uint32_t domid = dom->guest_domid; >>> + unsigned long idx, first_high_idx = (1ull << (32 - >>> ctxt->page_shift)); >>> + >>> + for (; num_pages; num_pages--, extents++) { >>> + >>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) >>> == 1) >> >> It looks like this is working because libxl is setting the maximum >> size of the domain with some slack (1MB). You might get in trouble if >> the slack is reduced or used by someone or the ACPI blob is increasing. > > > I saw your conversation about slack with Stefano and I am not sure I > understood what it was about. If this was about padding guest's memory > to be able to put there some additional data (such ACPI tables) then > this is not my intention here: if I can't populate (because it is > already populated, I guess) then I try to move memory around with code > below. That's what mem_hole_populate_ram() does as well. The maximum amount of memory that could be assigned to a domain is fixed per-domain. This maximum amount does not take into account the size of the ACPI blob. So you may end up to fail because all the memory was assigned somewhere else. > >> >>> + continue; >>> + >>> + if (dom->highmem_end) { >>> + idx = --dom->highmem_end; >>> + if ( idx == first_high_idx ) >>> + dom->highmem_end = 0; >>> + } else >>> + idx = --dom->lowmem_end; >>> + >>> + rc = xc_domain_add_to_physmap(xch, domid, >>> + XENMAPSPACE_gmfn, >>> + idx, *extents); >>> + if (rc) >>> + return rc; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +int libxl__dom_load_acpi(libxl__gc *gc, >>> + libxl_domain_build_info *info, >>> + struct xc_dom_image *dom) >>> +{ >>> + struct acpi_config config = {0}; >>> + struct acpi_ctxt ctxt; >>> + uint32_t domid = dom->guest_domid; >>> + xc_interface *xch = dom->xch; >>> + int rc, i, acpi_pages_num; >>> + xen_pfn_t extent, *extents; >>> + void *acpi_pages, *guest_acpi_pages = NULL; >>> + unsigned long page_mask; >>> + >>> + if ((info->type != LIBXL_DOMAIN_TYPE_HVM) || >>> + (info->device_model_version != >>> LIBXL_DEVICE_MODEL_VERSION_NONE)) >>> + return 0; >>> + >>> + ctxt.page_size = XC_DOM_PAGE_SIZE(dom); >>> + ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom); >>> + page_mask = (1UL << ctxt.page_shift) - 1; >>> + >>> + ctxt.mem_ops.alloc = mem_alloc; >>> + ctxt.mem_ops.v2p = virt_to_phys; >>> + >>> + rc = init_acpi_config(gc, dom, info, &config); >>> + if (rc) { >>> + LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", >>> __FUNCTION__, rc); >>> + return rc; >>> + } >>> + >>> + /* Map page that will hold RSDP */ >>> + extent = RSDP_ADDRESS >> ctxt.page_shift; >>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >>> + if (rc) { >>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", >>> + __FUNCTION__, rc); >>> + goto out; >>> + } >>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, >>> + ctxt.page_size, >>> + PROT_READ | >>> PROT_WRITE, >>> + RSDP_ADDRESS >>>>> ctxt.page_shift); >>> + if (!config.rsdp) { >>> + LOG(ERROR, "%s: Can't map acpi_physical", __FUNCTION__); >>> + rc = -1; >>> + goto out; >>> + } >>> + >>> + /* Map acpi_info page */ >>> + extent = ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift; >>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >>> + if (rc) { >>> + LOG(ERROR, "%s: populate_acpi_pages for acpi_info failed >>> with %d", >>> + __FUNCTION__, rc); >>> + goto out; >>> + } >>> + >>> + config.ainfop = (unsigned long)xc_map_foreign_range(xch, domid, >>> + ctxt.page_size, >>> + PROT_READ | >>> PROT_WRITE, >>> + >>> ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift); >> >> Loading the ACPI blob on ARM will be very similar except for the base >> address. So it would be nice to share some code with it. >> >> However, as mentioned in the ACPI thread [1], all the blobs are >> generally loaded by libxc and not libxl. This is more true on ARM >> because the guest address space is controlled by libxc (the position >> of all the blob are decided by it). > > The difference is that I not only load the tables here but also build > them. Which may or may not be the right thing to do in libxc. > > I suppose I can defer loading (and then keep pointer to tables in > acpitable_blob) but the then I need to keep RSDP descriptor somewhere > else (it is not part of the blob since it lives in lower MB of the guest). The device tree for ARM are built in libxl and loaded for libxc. IHMO, it would be strange to have a different pattern for ACPI. Regards,
On 07/06/2016 12:04 PM, Julien Grall wrote: > Hi Boris, > > On 06/07/16 16:50, Boris Ostrovsky wrote: >> On 07/06/2016 07:05 AM, Julien Grall wrote: >>>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>>> + xen_pfn_t *extents, >>>> + unsigned int num_pages, >>>> + struct acpi_ctxt *ctxt) >>>> +{ >>>> + int rc; >>>> + xc_interface *xch = dom->xch; >>>> + uint32_t domid = dom->guest_domid; >>>> + unsigned long idx, first_high_idx = (1ull << (32 - >>>> ctxt->page_shift)); >>>> + >>>> + for (; num_pages; num_pages--, extents++) { >>>> + >>>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) >>>> == 1) >>> >>> It looks like this is working because libxl is setting the maximum >>> size of the domain with some slack (1MB). You might get in trouble if >>> the slack is reduced or used by someone or the ACPI blob is increasing. >> >> >> I saw your conversation about slack with Stefano and I am not sure I >> understood what it was about. If this was about padding guest's memory >> to be able to put there some additional data (such ACPI tables) then >> this is not my intention here: if I can't populate (because it is >> already populated, I guess) then I try to move memory around with code >> below. That's what mem_hole_populate_ram() does as well. > > The maximum amount of memory that could be assigned to a domain is > fixed per-domain. This maximum amount does not take into account the > size of the ACPI blob. So you may end up to fail because all the > memory was assigned somewhere else. Why shouldn't max amount of guest's memory include ACPI tables? I think we should fail and not rely on this slack if no more memory is available. ... > >> >>> However, as mentioned in the ACPI thread [1], all the blobs are >>> generally loaded by libxc and not libxl. This is more true on ARM >>> because the guest address space is controlled by libxc (the position >>> of all the blob are decided by it). >> >> The difference is that I not only load the tables here but also build >> them. Which may or may not be the right thing to do in libxc. >> >> I suppose I can defer loading (and then keep pointer to tables in >> acpitable_blob) but the then I need to keep RSDP descriptor somewhere >> else (it is not part of the blob since it lives in lower MB of the >> guest). > > The device tree for ARM are built in libxl and loaded for libxc. IHMO, > it would be strange to have a different pattern for ACPI. Is RSDP part of the ACPI blob for ARM? If not, how do you load it? -boris
On 06/07/16 17:30, Boris Ostrovsky wrote: > On 07/06/2016 12:04 PM, Julien Grall wrote: >> Hi Boris, >> >> On 06/07/16 16:50, Boris Ostrovsky wrote: >>> On 07/06/2016 07:05 AM, Julien Grall wrote: >>>>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>>>> + xen_pfn_t *extents, >>>>> + unsigned int num_pages, >>>>> + struct acpi_ctxt *ctxt) >>>>> +{ >>>>> + int rc; >>>>> + xc_interface *xch = dom->xch; >>>>> + uint32_t domid = dom->guest_domid; >>>>> + unsigned long idx, first_high_idx = (1ull << (32 - >>>>> ctxt->page_shift)); >>>>> + >>>>> + for (; num_pages; num_pages--, extents++) { >>>>> + >>>>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) >>>>> == 1) >>>> >>>> It looks like this is working because libxl is setting the maximum >>>> size of the domain with some slack (1MB). You might get in trouble if >>>> the slack is reduced or used by someone or the ACPI blob is increasing. >>> >>> >>> I saw your conversation about slack with Stefano and I am not sure I >>> understood what it was about. If this was about padding guest's memory >>> to be able to put there some additional data (such ACPI tables) then >>> this is not my intention here: if I can't populate (because it is >>> already populated, I guess) then I try to move memory around with code >>> below. That's what mem_hole_populate_ram() does as well. >> >> The maximum amount of memory that could be assigned to a domain is >> fixed per-domain. This maximum amount does not take into account the >> size of the ACPI blob. So you may end up to fail because all the >> memory was assigned somewhere else. > > Why shouldn't max amount of guest's memory include ACPI tables? I think > we should fail and not rely on this slack if no more memory is available. > > ... Because, at least for ARM, the ACPI memory region is not part of the "real" RAM. So this value is not taken into account into the current max mem. My point is we need to add the size of the ACPI blob to max mem to avoid any error here. > >> >>> >>>> However, as mentioned in the ACPI thread [1], all the blobs are >>>> generally loaded by libxc and not libxl. This is more true on ARM >>>> because the guest address space is controlled by libxc (the position >>>> of all the blob are decided by it). >>> >>> The difference is that I not only load the tables here but also build >>> them. Which may or may not be the right thing to do in libxc. >>> >>> I suppose I can defer loading (and then keep pointer to tables in >>> acpitable_blob) but the then I need to keep RSDP descriptor somewhere >>> else (it is not part of the blob since it lives in lower MB of the >>> guest). >> >> The device tree for ARM are built in libxl and loaded for libxc. IHMO, >> it would be strange to have a different pattern for ACPI. > > Is RSDP part of the ACPI blob for ARM? If not, how do you load it? RSDP is part of the ACPI blob for ARM. Regards,
On 07/06/2016 01:03 PM, Julien Grall wrote: > > > On 06/07/16 17:30, Boris Ostrovsky wrote: >> On 07/06/2016 12:04 PM, Julien Grall wrote: >>> Hi Boris, >>> >>> On 06/07/16 16:50, Boris Ostrovsky wrote: >>>> On 07/06/2016 07:05 AM, Julien Grall wrote: >>>>>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>>>>> + xen_pfn_t *extents, >>>>>> + unsigned int num_pages, >>>>>> + struct acpi_ctxt *ctxt) >>>>>> +{ >>>>>> + int rc; >>>>>> + xc_interface *xch = dom->xch; >>>>>> + uint32_t domid = dom->guest_domid; >>>>>> + unsigned long idx, first_high_idx = (1ull << (32 - >>>>>> ctxt->page_shift)); >>>>>> + >>>>>> + for (; num_pages; num_pages--, extents++) { >>>>>> + >>>>>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, >>>>>> extents) >>>>>> == 1) >>>>> >>>>> It looks like this is working because libxl is setting the maximum >>>>> size of the domain with some slack (1MB). You might get in trouble if >>>>> the slack is reduced or used by someone or the ACPI blob is >>>>> increasing. >>>> >>>> >>>> I saw your conversation about slack with Stefano and I am not sure I >>>> understood what it was about. If this was about padding guest's memory >>>> to be able to put there some additional data (such ACPI tables) then >>>> this is not my intention here: if I can't populate (because it is >>>> already populated, I guess) then I try to move memory around with code >>>> below. That's what mem_hole_populate_ram() does as well. >>> >>> The maximum amount of memory that could be assigned to a domain is >>> fixed per-domain. This maximum amount does not take into account the >>> size of the ACPI blob. So you may end up to fail because all the >>> memory was assigned somewhere else. >> >> Why shouldn't max amount of guest's memory include ACPI tables? I think >> we should fail and not rely on this slack if no more memory is >> available. >> >> ... > > Because, at least for ARM, the ACPI memory region is not part of the > "real" RAM. So this value is not taken into account into the current > max mem. Hmm.. I've always assumed it is part of memory but marked as a special type in e820 map on x86. Maybe Andrew or Jan can comment on this. > My point is we need to add the size of the ACPI blob to max mem to > avoid any error here. So you (or Shannon) plan on doing this for ARM, right (it's not done with the current version of patches)? > >> >>> >>>> >>>>> However, as mentioned in the ACPI thread [1], all the blobs are >>>>> generally loaded by libxc and not libxl. This is more true on ARM >>>>> because the guest address space is controlled by libxc (the position >>>>> of all the blob are decided by it). >>>> >>>> The difference is that I not only load the tables here but also build >>>> them. Which may or may not be the right thing to do in libxc. >>>> >>>> I suppose I can defer loading (and then keep pointer to tables in >>>> acpitable_blob) but the then I need to keep RSDP descriptor somewhere >>>> else (it is not part of the blob since it lives in lower MB of the >>>> guest). >>> >>> The device tree for ARM are built in libxl and loaded for libxc. IHMO, >>> it would be strange to have a different pattern for ACPI. >> >> Is RSDP part of the ACPI blob for ARM? If not, how do you load it? > > RSDP is part of the ACPI blob for ARM. And it's not for x86. So to separate building and loading the tables would require two blobs. I suppose we cal loop over blobs in xc_dom_load_acpi in https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00309.html -boris
>>> On 06.07.16 at 19:33, <boris.ostrovsky@oracle.com> wrote: > On 07/06/2016 01:03 PM, Julien Grall wrote: >> >> >> On 06/07/16 17:30, Boris Ostrovsky wrote: >>> On 07/06/2016 12:04 PM, Julien Grall wrote: >>>> Hi Boris, >>>> >>>> On 06/07/16 16:50, Boris Ostrovsky wrote: >>>>> On 07/06/2016 07:05 AM, Julien Grall wrote: >>>>>>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>>>>>> + xen_pfn_t *extents, >>>>>>> + unsigned int num_pages, >>>>>>> + struct acpi_ctxt *ctxt) >>>>>>> +{ >>>>>>> + int rc; >>>>>>> + xc_interface *xch = dom->xch; >>>>>>> + uint32_t domid = dom->guest_domid; >>>>>>> + unsigned long idx, first_high_idx = (1ull << (32 - >>>>>>> ctxt->page_shift)); >>>>>>> + >>>>>>> + for (; num_pages; num_pages--, extents++) { >>>>>>> + >>>>>>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, >>>>>>> extents) >>>>>>> == 1) >>>>>> >>>>>> It looks like this is working because libxl is setting the maximum >>>>>> size of the domain with some slack (1MB). You might get in trouble if >>>>>> the slack is reduced or used by someone or the ACPI blob is >>>>>> increasing. >>>>> >>>>> >>>>> I saw your conversation about slack with Stefano and I am not sure I >>>>> understood what it was about. If this was about padding guest's memory >>>>> to be able to put there some additional data (such ACPI tables) then >>>>> this is not my intention here: if I can't populate (because it is >>>>> already populated, I guess) then I try to move memory around with code >>>>> below. That's what mem_hole_populate_ram() does as well. >>>> >>>> The maximum amount of memory that could be assigned to a domain is >>>> fixed per-domain. This maximum amount does not take into account the >>>> size of the ACPI blob. So you may end up to fail because all the >>>> memory was assigned somewhere else. >>> >>> Why shouldn't max amount of guest's memory include ACPI tables? I think >>> we should fail and not rely on this slack if no more memory is >>> available. >>> >>> ... >> >> Because, at least for ARM, the ACPI memory region is not part of the >> "real" RAM. So this value is not taken into account into the current >> max mem. > > > Hmm.. I've always assumed it is part of memory but marked as a special > type in e820 map on x86. Maybe Andrew or Jan can comment on this. I guess you both mean the same - note how Julien says _"real" RAM_. So from a hypervisor resource consumption view this ought to be considered RAM; from a guest view it wouldn't be. Jan
On 07/07/2016 04:38 AM, Jan Beulich wrote: >>>> On 06.07.16 at 19:33, <boris.ostrovsky@oracle.com> wrote: >> On 07/06/2016 01:03 PM, Julien Grall wrote: >>> >>> On 06/07/16 17:30, Boris Ostrovsky wrote: >>>> On 07/06/2016 12:04 PM, Julien Grall wrote: >>>>> Hi Boris, >>>>> >>>>> On 06/07/16 16:50, Boris Ostrovsky wrote: >>>>>> On 07/06/2016 07:05 AM, Julien Grall wrote: >>>>>>>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>>>>>>> + xen_pfn_t *extents, >>>>>>>> + unsigned int num_pages, >>>>>>>> + struct acpi_ctxt *ctxt) >>>>>>>> +{ >>>>>>>> + int rc; >>>>>>>> + xc_interface *xch = dom->xch; >>>>>>>> + uint32_t domid = dom->guest_domid; >>>>>>>> + unsigned long idx, first_high_idx = (1ull << (32 - >>>>>>>> ctxt->page_shift)); >>>>>>>> + >>>>>>>> + for (; num_pages; num_pages--, extents++) { >>>>>>>> + >>>>>>>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, >>>>>>>> extents) >>>>>>>> == 1) >>>>>>> It looks like this is working because libxl is setting the maximum >>>>>>> size of the domain with some slack (1MB). You might get in trouble if >>>>>>> the slack is reduced or used by someone or the ACPI blob is >>>>>>> increasing. >>>>>> >>>>>> I saw your conversation about slack with Stefano and I am not sure I >>>>>> understood what it was about. If this was about padding guest's memory >>>>>> to be able to put there some additional data (such ACPI tables) then >>>>>> this is not my intention here: if I can't populate (because it is >>>>>> already populated, I guess) then I try to move memory around with code >>>>>> below. That's what mem_hole_populate_ram() does as well. >>>>> The maximum amount of memory that could be assigned to a domain is >>>>> fixed per-domain. This maximum amount does not take into account the >>>>> size of the ACPI blob. So you may end up to fail because all the >>>>> memory was assigned somewhere else. >>>> Why shouldn't max amount of guest's memory include ACPI tables? I think >>>> we should fail and not rely on this slack if no more memory is >>>> available. >>>> >>>> ... >>> Because, at least for ARM, the ACPI memory region is not part of the >>> "real" RAM. So this value is not taken into account into the current >>> max mem. >> >> Hmm.. I've always assumed it is part of memory but marked as a special >> type in e820 map on x86. Maybe Andrew or Jan can comment on this. > I guess you both mean the same - note how Julien says _"real" RAM_. > So from a hypervisor resource consumption view this ought to be > considered RAM; from a guest view it wouldn't be. In which case we shouldn't pad maxmem specified by guest's config file. Space to put ACPI tables must come from requested resources. If the tables don't fit then more memory should have been asked for. -boris
On 07/07/16 16:08, Boris Ostrovsky wrote: > On 07/07/2016 04:38 AM, Jan Beulich wrote: >>>>> On 06.07.16 at 19:33, <boris.ostrovsky@oracle.com> wrote: >>> On 07/06/2016 01:03 PM, Julien Grall wrote: >>>> >>>> On 06/07/16 17:30, Boris Ostrovsky wrote: >>>>> On 07/06/2016 12:04 PM, Julien Grall wrote: >>>>>> Hi Boris, >>>>>> >>>>>> On 06/07/16 16:50, Boris Ostrovsky wrote: >>>>>>> On 07/06/2016 07:05 AM, Julien Grall wrote: >>>>>>>>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>>>>>>>> + xen_pfn_t *extents, >>>>>>>>> + unsigned int num_pages, >>>>>>>>> + struct acpi_ctxt *ctxt) >>>>>>>>> +{ >>>>>>>>> + int rc; >>>>>>>>> + xc_interface *xch = dom->xch; >>>>>>>>> + uint32_t domid = dom->guest_domid; >>>>>>>>> + unsigned long idx, first_high_idx = (1ull << (32 - >>>>>>>>> ctxt->page_shift)); >>>>>>>>> + >>>>>>>>> + for (; num_pages; num_pages--, extents++) { >>>>>>>>> + >>>>>>>>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, >>>>>>>>> extents) >>>>>>>>> == 1) >>>>>>>> It looks like this is working because libxl is setting the maximum >>>>>>>> size of the domain with some slack (1MB). You might get in trouble if >>>>>>>> the slack is reduced or used by someone or the ACPI blob is >>>>>>>> increasing. >>>>>>> >>>>>>> I saw your conversation about slack with Stefano and I am not sure I >>>>>>> understood what it was about. If this was about padding guest's memory >>>>>>> to be able to put there some additional data (such ACPI tables) then >>>>>>> this is not my intention here: if I can't populate (because it is >>>>>>> already populated, I guess) then I try to move memory around with code >>>>>>> below. That's what mem_hole_populate_ram() does as well. >>>>>> The maximum amount of memory that could be assigned to a domain is >>>>>> fixed per-domain. This maximum amount does not take into account the >>>>>> size of the ACPI blob. So you may end up to fail because all the >>>>>> memory was assigned somewhere else. >>>>> Why shouldn't max amount of guest's memory include ACPI tables? I think >>>>> we should fail and not rely on this slack if no more memory is >>>>> available. >>>>> >>>>> ... >>>> Because, at least for ARM, the ACPI memory region is not part of the >>>> "real" RAM. So this value is not taken into account into the current >>>> max mem. >>> >>> Hmm.. I've always assumed it is part of memory but marked as a special >>> type in e820 map on x86. Maybe Andrew or Jan can comment on this. >> I guess you both mean the same - note how Julien says _"real" RAM_. >> So from a hypervisor resource consumption view this ought to be >> considered RAM; from a guest view it wouldn't be. > > In which case we shouldn't pad maxmem specified by guest's config file. > Space to put ACPI tables must come from requested resources. If the > tables don't fit then more memory should have been asked for. Why? In the case of ARM, the ACPI tables lives outside the guest RAM in a special region. I would have expect to be the same in x86. If so, I don't think this should be part of the maxmem requested by the user. IIRC, it is the same of the PCI ROM. The maxmem is incremented by the toolstack. Regards,
>>> On 07.07.16 at 17:12, <julien.grall@arm.com> wrote: > > On 07/07/16 16:08, Boris Ostrovsky wrote: >> On 07/07/2016 04:38 AM, Jan Beulich wrote: >>>>>> On 06.07.16 at 19:33, <boris.ostrovsky@oracle.com> wrote: >>>> On 07/06/2016 01:03 PM, Julien Grall wrote: >>>>> >>>>> On 06/07/16 17:30, Boris Ostrovsky wrote: >>>>>> On 07/06/2016 12:04 PM, Julien Grall wrote: >>>>>>> Hi Boris, >>>>>>> >>>>>>> On 06/07/16 16:50, Boris Ostrovsky wrote: >>>>>>>> On 07/06/2016 07:05 AM, Julien Grall wrote: >>>>>>>>>> +static int populate_acpi_pages(struct xc_dom_image *dom, >>>>>>>>>> + xen_pfn_t *extents, >>>>>>>>>> + unsigned int num_pages, >>>>>>>>>> + struct acpi_ctxt *ctxt) >>>>>>>>>> +{ >>>>>>>>>> + int rc; >>>>>>>>>> + xc_interface *xch = dom->xch; >>>>>>>>>> + uint32_t domid = dom->guest_domid; >>>>>>>>>> + unsigned long idx, first_high_idx = (1ull << (32 - >>>>>>>>>> ctxt->page_shift)); >>>>>>>>>> + >>>>>>>>>> + for (; num_pages; num_pages--, extents++) { >>>>>>>>>> + >>>>>>>>>> + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, >>>>>>>>>> extents) >>>>>>>>>> == 1) >>>>>>>>> It looks like this is working because libxl is setting the maximum >>>>>>>>> size of the domain with some slack (1MB). You might get in trouble if >>>>>>>>> the slack is reduced or used by someone or the ACPI blob is >>>>>>>>> increasing. >>>>>>>> >>>>>>>> I saw your conversation about slack with Stefano and I am not sure I >>>>>>>> understood what it was about. If this was about padding guest's memory >>>>>>>> to be able to put there some additional data (such ACPI tables) then >>>>>>>> this is not my intention here: if I can't populate (because it is >>>>>>>> already populated, I guess) then I try to move memory around with code >>>>>>>> below. That's what mem_hole_populate_ram() does as well. >>>>>>> The maximum amount of memory that could be assigned to a domain is >>>>>>> fixed per-domain. This maximum amount does not take into account the >>>>>>> size of the ACPI blob. So you may end up to fail because all the >>>>>>> memory was assigned somewhere else. >>>>>> Why shouldn't max amount of guest's memory include ACPI tables? I think >>>>>> we should fail and not rely on this slack if no more memory is >>>>>> available. >>>>>> >>>>>> ... >>>>> Because, at least for ARM, the ACPI memory region is not part of the >>>>> "real" RAM. So this value is not taken into account into the current >>>>> max mem. >>>> >>>> Hmm.. I've always assumed it is part of memory but marked as a special >>>> type in e820 map on x86. Maybe Andrew or Jan can comment on this. >>> I guess you both mean the same - note how Julien says _"real" RAM_. >>> So from a hypervisor resource consumption view this ought to be >>> considered RAM; from a guest view it wouldn't be. >> >> In which case we shouldn't pad maxmem specified by guest's config file. >> Space to put ACPI tables must come from requested resources. If the >> tables don't fit then more memory should have been asked for. > > Why? In the case of ARM, the ACPI tables lives outside the guest RAM in > a special region. I would have expect to be the same in x86. This is still RAM from a host resource accounting POV. > If so, I don't think this should be part of the maxmem requested by the > user. IIRC, it is the same of the PCI ROM. The maxmem is incremented by > the toolstack. For some parts iirc, and not for others. See also (for example) the recent discussion George had with PGNet Dev (https://lists.xenproject.org/archives/html/xen-devel/2016-07/msg00353.html). Jan
On Tue, Jul 05, 2016 at 03:05:19PM -0400, Boris Ostrovsky wrote: > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> > --- > > Changes in v1: > * Move to libxl > * Added populate_acpi_pages() > * Stash location/size of tables in xc_dom_image (to be used in constructing e820 map) > * Use libxl allocator > * Only set XEN_X86_EMU_LAPIC flag if 'apic' option is set. > * Make acpi_build_tables() return error code > > .gitignore | 4 + > tools/libacpi/build.c | 7 +- > tools/libacpi/libacpi.h | 15 ++- > tools/libxl/Makefile | 17 +++- > tools/libxl/libxl_arch.h | 3 + > tools/libxl/libxl_dom.c | 1 + > tools/libxl/libxl_x86.c | 29 +++-- > tools/libxl/libxl_x86_acpi.c | 292 ++++++++++++++++++++++++++++++++++++++++++ > tools/libxl/libxl_x86_acpi.h | 21 +++ > 9 files changed, 373 insertions(+), 16 deletions(-) > create mode 100644 tools/libxl/libxl_x86_acpi.c > create mode 100644 tools/libxl/libxl_x86_acpi.h > > diff --git a/.gitignore b/.gitignore > index 9dd2086..d4da37f 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -179,6 +179,10 @@ tools/libxl/testenum.c > tools/libxl/tmp.* > tools/libxl/_libxl.api-for-check > tools/libxl/*.api-ok > +tools/libxl/mk_dsdt > +tools/libxl/dsdt*.c > +tools/libxl/dsdt_*.asl > +tools/libxl/ssdt_*.h Please sort these alphabetically. > tools/misc/cpuperf/cpuperf-perfcntr > tools/misc/cpuperf/cpuperf-xen > tools/misc/xc_shadow > diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c > index 290f005..a6ddf53 100644 > --- a/tools/libacpi/build.c > +++ b/tools/libacpi/build.c > @@ -23,6 +23,7 @@ > #include "ssdt_tpm.h" > #include "ssdt_pm.h" > #include "x86.h" > +#include <xen/hvm/hvm_info_table.h> > #include <xen/hvm/hvm_xs_strings.h> > #include <xen/hvm/params.h> > [...] > +int libxl__dom_load_acpi(libxl__gc *gc, > + libxl_domain_build_info *info, > + struct xc_dom_image *dom); Indentation. > #endif > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index ba3472e..7e4e289 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -657,6 +657,7 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid, > LOGE(ERROR, "xc_dom_build_image failed"); > goto out; > } > + Stray blank line. > if ( (ret = xc_dom_boot_image(dom)) != 0 ) { > LOGE(ERROR, "xc_dom_boot_image failed"); > goto out; > diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c > index 32ce1d2..7201dbb 100644 > --- a/tools/libxl/libxl_x86.c > +++ b/tools/libxl/libxl_x86.c [...] > +int libxl__dom_load_acpi(libxl__gc *gc, > + libxl_domain_build_info *info, > + struct xc_dom_image *dom) > +{ > + struct acpi_config config = {0}; > + struct acpi_ctxt ctxt; > + uint32_t domid = dom->guest_domid; > + xc_interface *xch = dom->xch; > + int rc, i, acpi_pages_num; > + xen_pfn_t extent, *extents; > + void *acpi_pages, *guest_acpi_pages = NULL; > + unsigned long page_mask; > + > + if ((info->type != LIBXL_DOMAIN_TYPE_HVM) || > + (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE)) > + return 0; > + > + ctxt.page_size = XC_DOM_PAGE_SIZE(dom); > + ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom); > + page_mask = (1UL << ctxt.page_shift) - 1; > + > + ctxt.mem_ops.alloc = mem_alloc; > + ctxt.mem_ops.v2p = virt_to_phys; > + > + rc = init_acpi_config(gc, dom, info, &config); > + if (rc) { > + LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc); > + return rc; > + } > + > + /* Map page that will hold RSDP */ > + extent = RSDP_ADDRESS >> ctxt.page_shift; > + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > + if (rc) { > + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", > + __FUNCTION__, rc); > + goto out; > + } > + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, > + ctxt.page_size, > + PROT_READ | PROT_WRITE, > + RSDP_ADDRESS >> ctxt.page_shift); I think with Anthony's on-going work you should be more flexible for all you tables. I haven't really looked into details for this patch. Let's sort out the linkage issue between GPLv2 cod and LGPL code first. Wei.
On 07/08/2016 06:55 AM, Wei Liu wrote: > >> + >> + /* Map page that will hold RSDP */ >> + extent = RSDP_ADDRESS >> ctxt.page_shift; >> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >> + if (rc) { >> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", >> + __FUNCTION__, rc); >> + goto out; >> + } >> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, >> + ctxt.page_size, >> + PROT_READ | PROT_WRITE, >> + RSDP_ADDRESS >> ctxt.page_shift); > I think with Anthony's on-going work you should be more flexible for all > you tables. Not sure I understand what you mean here. You want the address (RSDP_ADDRESS) to be a variable as opposed to a macro? > > I haven't really looked into details for this patch. Let's sort out > the linkage issue between GPLv2 cod and LGPL code first. Ugh.. Yes, this one didn't even cross my mind until you brought it up yesterday. What are the options? Can we dual-license those files as GPLv2/LGPL, assuming copyright holders --- Keir (or Citrix?) and Intel --- agree? -boris
On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: > On 07/08/2016 06:55 AM, Wei Liu wrote: > > > >> + > >> + /* Map page that will hold RSDP */ > >> + extent = RSDP_ADDRESS >> ctxt.page_shift; > >> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > >> + if (rc) { > >> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", > >> + __FUNCTION__, rc); > >> + goto out; > >> + } > >> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, > >> + ctxt.page_size, > >> + PROT_READ | PROT_WRITE, > >> + RSDP_ADDRESS >> ctxt.page_shift); > > I think with Anthony's on-going work you should be more flexible for all > > you tables. > > Not sure I understand what you mean here. You want the address > (RSDP_ADDRESS) to be a variable as opposed to a macro? > I'm still trying to wrap my head around the possible interaction between Anthony's work and your work. Anthony's work allows dynamically loading of firmware blobs. If you use a fixed address, theoretically it can clash with firmware blobs among other things libxc can load. The high address is a safe bet so that probably won't happen in practice. Anthony's work allows loading arbitrary blobs actually. Can you take advantage of that mechanism as well? That is, to specify all your tables as modules and let libxc handle actually loading them into guest memory. Does this make sense? Also CC Anthony here. > > > > I haven't really looked into details for this patch. Let's sort out > > the linkage issue between GPLv2 cod and LGPL code first. > > > Ugh.. Yes, this one didn't even cross my mind until you brought it up > yesterday. > > What are the options? Can we dual-license those files as GPLv2/LGPL, > assuming copyright holders --- Keir (or Citrix?) and Intel --- agree? > I don't claim I know much about licenses. Let's discuss this issue in the other thread instead of having two threads. I think what Ian said makes sense FWIW. Wei. > -boris
On 07/08/2016 12:07 PM, Wei Liu wrote: > On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: >> On 07/08/2016 06:55 AM, Wei Liu wrote: >>>> + >>>> + /* Map page that will hold RSDP */ >>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; >>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >>>> + if (rc) { >>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", >>>> + __FUNCTION__, rc); >>>> + goto out; >>>> + } >>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, >>>> + ctxt.page_size, >>>> + PROT_READ | PROT_WRITE, >>>> + RSDP_ADDRESS >> ctxt.page_shift); >>> I think with Anthony's on-going work you should be more flexible for all >>> you tables. >> Not sure I understand what you mean here. You want the address >> (RSDP_ADDRESS) to be a variable as opposed to a macro? >> > I'm still trying to wrap my head around the possible interaction between > Anthony's work and your work. > > Anthony's work allows dynamically loading of firmware blobs. If you use > a fixed address, theoretically it can clash with firmware blobs among > other things libxc can load. The high address is a safe bet so that > probably won't happen in practice. > > Anthony's work allows loading arbitrary blobs actually. Can you take > advantage of that mechanism as well? That is, to specify all your tables > as modules and let libxc handle actually loading them into guest > memory. > > Does this make sense? > > Also CC Anthony here. My understanding of Anthony's series is that its goal was to provide an interface to pass DSDT (and maybe some other tables) from the toolstack to hvmloader. Here we don't have hvmloader, we are loading the tables directly into guest's memory. -boris
On Fri, Jul 08, 2016 at 01:20:46PM -0400, Boris Ostrovsky wrote: > On 07/08/2016 12:07 PM, Wei Liu wrote: > > On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: > >> On 07/08/2016 06:55 AM, Wei Liu wrote: > >>>> + > >>>> + /* Map page that will hold RSDP */ > >>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; > >>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > >>>> + if (rc) { > >>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", > >>>> + __FUNCTION__, rc); > >>>> + goto out; > >>>> + } > >>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, > >>>> + ctxt.page_size, > >>>> + PROT_READ | PROT_WRITE, > >>>> + RSDP_ADDRESS >> ctxt.page_shift); > >>> I think with Anthony's on-going work you should be more flexible for all > >>> you tables. > >> Not sure I understand what you mean here. You want the address > >> (RSDP_ADDRESS) to be a variable as opposed to a macro? > >> > > I'm still trying to wrap my head around the possible interaction between > > Anthony's work and your work. > > > > Anthony's work allows dynamically loading of firmware blobs. If you use > > a fixed address, theoretically it can clash with firmware blobs among > > other things libxc can load. The high address is a safe bet so that > > probably won't happen in practice. > > > > Anthony's work allows loading arbitrary blobs actually. Can you take > > advantage of that mechanism as well? That is, to specify all your tables > > as modules and let libxc handle actually loading them into guest > > memory. > > > > Does this make sense? > > > > Also CC Anthony here. > > > My understanding of Anthony's series is that its goal was to provide an > interface to pass DSDT (and maybe some other tables) from the toolstack > to hvmloader. > > Here we don't have hvmloader, we are loading the tables directly into > guest's memory. > Do you use the same hvm_start_info structure? I don't think that structure is restricted to hvmloader. Wei. > -boris > >
On 07/11/2016 06:47 AM, Wei Liu wrote: > On Fri, Jul 08, 2016 at 01:20:46PM -0400, Boris Ostrovsky wrote: >> On 07/08/2016 12:07 PM, Wei Liu wrote: >>> On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: >>>> On 07/08/2016 06:55 AM, Wei Liu wrote: >>>>>> + >>>>>> + /* Map page that will hold RSDP */ >>>>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; >>>>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >>>>>> + if (rc) { >>>>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", >>>>>> + __FUNCTION__, rc); >>>>>> + goto out; >>>>>> + } >>>>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, >>>>>> + ctxt.page_size, >>>>>> + PROT_READ | PROT_WRITE, >>>>>> + RSDP_ADDRESS >> ctxt.page_shift); >>>>> I think with Anthony's on-going work you should be more flexible for all >>>>> you tables. >>>> Not sure I understand what you mean here. You want the address >>>> (RSDP_ADDRESS) to be a variable as opposed to a macro? >>>> >>> I'm still trying to wrap my head around the possible interaction between >>> Anthony's work and your work. >>> >>> Anthony's work allows dynamically loading of firmware blobs. If you use >>> a fixed address, theoretically it can clash with firmware blobs among >>> other things libxc can load. The high address is a safe bet so that >>> probably won't happen in practice. >>> >>> Anthony's work allows loading arbitrary blobs actually. Can you take >>> advantage of that mechanism as well? That is, to specify all your tables >>> as modules and let libxc handle actually loading them into guest >>> memory. >>> >>> Does this make sense? >>> >>> Also CC Anthony here. >> >> My understanding of Anthony's series is that its goal was to provide an >> interface to pass DSDT (and maybe some other tables) from the toolstack >> to hvmloader. >> >> Here we don't have hvmloader, we are loading the tables directly into >> guest's memory. >> > Do you use the same hvm_start_info structure? I don't think that > structure is restricted to hvmloader. Yes, we do. However, in PVH(v2) case it will be seen next by the guest who will expect the tables to already be in memory. I.e. there is no intermediate Xen component, such as hvmloader, who can load the blobs. Having said that, I wonder whether we (both x86 and ARM) could use Anthony's xc_dom_image.full_acpi_module instead of acpitables_blob that Shannon's series added. (even if we can't, I think xc_hvm_firmware_module is the right datastructure to store the blob since it has both toolstack's virtual and guest's physical addresses). -boris
On 11/07/16 14:33, Boris Ostrovsky wrote: > On 07/11/2016 06:47 AM, Wei Liu wrote: >> On Fri, Jul 08, 2016 at 01:20:46PM -0400, Boris Ostrovsky wrote: >>> On 07/08/2016 12:07 PM, Wei Liu wrote: >>>> On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: >>>>> On 07/08/2016 06:55 AM, Wei Liu wrote: >>>>>>> + >>>>>>> + /* Map page that will hold RSDP */ >>>>>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; >>>>>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >>>>>>> + if (rc) { >>>>>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", >>>>>>> + __FUNCTION__, rc); >>>>>>> + goto out; >>>>>>> + } >>>>>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, >>>>>>> + ctxt.page_size, >>>>>>> + PROT_READ | PROT_WRITE, >>>>>>> + RSDP_ADDRESS >> ctxt.page_shift); >>>>>> I think with Anthony's on-going work you should be more flexible for all >>>>>> you tables. >>>>> Not sure I understand what you mean here. You want the address >>>>> (RSDP_ADDRESS) to be a variable as opposed to a macro? >>>>> >>>> I'm still trying to wrap my head around the possible interaction between >>>> Anthony's work and your work. >>>> >>>> Anthony's work allows dynamically loading of firmware blobs. If you use >>>> a fixed address, theoretically it can clash with firmware blobs among >>>> other things libxc can load. The high address is a safe bet so that >>>> probably won't happen in practice. >>>> >>>> Anthony's work allows loading arbitrary blobs actually. Can you take >>>> advantage of that mechanism as well? That is, to specify all your tables >>>> as modules and let libxc handle actually loading them into guest >>>> memory. >>>> >>>> Does this make sense? >>>> >>>> Also CC Anthony here. >>> >>> My understanding of Anthony's series is that its goal was to provide an >>> interface to pass DSDT (and maybe some other tables) from the toolstack >>> to hvmloader. >>> >>> Here we don't have hvmloader, we are loading the tables directly into >>> guest's memory. >>> >> Do you use the same hvm_start_info structure? I don't think that >> structure is restricted to hvmloader. > > > Yes, we do. However, in PVH(v2) case it will be seen next by the guest > who will expect the tables to already be in memory. I.e. there is no > intermediate Xen component, such as hvmloader, who can load the blobs. > > Having said that, I wonder whether we (both x86 and ARM) could use > Anthony's xc_dom_image.full_acpi_module instead of acpitables_blob that > Shannon's series added. (even if we can't, I think > xc_hvm_firmware_module is the right datastructure to store the blob > since it has both toolstack's virtual and guest's physical addresses). In this case, xc_hvm_firmware_module would need to be renamed as ARM guests are neither HVM nor PV. FWIW, from the toolstack point of view, ARM guests is considered as PV guest. Regards,
On Mon, Jul 11, 2016 at 09:33:21AM -0400, Boris Ostrovsky wrote: > On 07/11/2016 06:47 AM, Wei Liu wrote: > > On Fri, Jul 08, 2016 at 01:20:46PM -0400, Boris Ostrovsky wrote: > >> On 07/08/2016 12:07 PM, Wei Liu wrote: > >>> On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: > >>>> On 07/08/2016 06:55 AM, Wei Liu wrote: > >>>>>> + > >>>>>> + /* Map page that will hold RSDP */ > >>>>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; > >>>>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > >>>>>> + if (rc) { > >>>>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", > >>>>>> + __FUNCTION__, rc); > >>>>>> + goto out; > >>>>>> + } > >>>>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, > >>>>>> + ctxt.page_size, > >>>>>> + PROT_READ | PROT_WRITE, > >>>>>> + RSDP_ADDRESS >> ctxt.page_shift); > >>>>> I think with Anthony's on-going work you should be more flexible for all > >>>>> you tables. > >>>> Not sure I understand what you mean here. You want the address > >>>> (RSDP_ADDRESS) to be a variable as opposed to a macro? > >>>> > >>> I'm still trying to wrap my head around the possible interaction between > >>> Anthony's work and your work. > >>> > >>> Anthony's work allows dynamically loading of firmware blobs. If you use > >>> a fixed address, theoretically it can clash with firmware blobs among > >>> other things libxc can load. The high address is a safe bet so that > >>> probably won't happen in practice. > >>> > >>> Anthony's work allows loading arbitrary blobs actually. Can you take > >>> advantage of that mechanism as well? That is, to specify all your tables > >>> as modules and let libxc handle actually loading them into guest > >>> memory. > >>> > >>> Does this make sense? > >>> > >>> Also CC Anthony here. > >> > >> My understanding of Anthony's series is that its goal was to provide an > >> interface to pass DSDT (and maybe some other tables) from the toolstack > >> to hvmloader. > >> > >> Here we don't have hvmloader, we are loading the tables directly into > >> guest's memory. > >> > > Do you use the same hvm_start_info structure? I don't think that > > structure is restricted to hvmloader. > > > Yes, we do. However, in PVH(v2) case it will be seen next by the guest > who will expect the tables to already be in memory. I.e. there is no > intermediate Xen component, such as hvmloader, who can load the blobs. > Maybe you misunderstood. Anthony's work will load the blobs into guest memory -- all fields in hvm_start_info points to guest physical address IIRC. Hvmloader might want to relocate them, but that's a different matter. > Having said that, I wonder whether we (both x86 and ARM) could use > Anthony's xc_dom_image.full_acpi_module instead of acpitables_blob that > Shannon's series added. (even if we can't, I think > xc_hvm_firmware_module is the right datastructure to store the blob > since it has both toolstack's virtual and guest's physical addresses). > Yes, that's along the line I'm thinking about. Wei. > -boris >
On Mon, Jul 11, 2016 at 02:39:05PM +0100, Julien Grall wrote: > >Yes, we do. However, in PVH(v2) case it will be seen next by the guest > >who will expect the tables to already be in memory. I.e. there is no > >intermediate Xen component, such as hvmloader, who can load the blobs. > > > >Having said that, I wonder whether we (both x86 and ARM) could use > >Anthony's xc_dom_image.full_acpi_module instead of acpitables_blob that > >Shannon's series added. (even if we can't, I think > >xc_hvm_firmware_module is the right datastructure to store the blob > >since it has both toolstack's virtual and guest's physical addresses). > > In this case, xc_hvm_firmware_module would need to be renamed as ARM guests > are neither HVM nor PV. > That's trivial. It's an internal structure that we can rename at will. > FWIW, from the toolstack point of view, ARM guests is considered as PV > guest. ... while at the same time utilises HVM param... Not complaining, just this makes me chuckle a bit. :-) Wei. > > Regards, > > -- > Julien Grall
On 11/07/16 14:42, Wei Liu wrote: > On Mon, Jul 11, 2016 at 02:39:05PM +0100, Julien Grall wrote: >>> Yes, we do. However, in PVH(v2) case it will be seen next by the guest >>> who will expect the tables to already be in memory. I.e. there is no >>> intermediate Xen component, such as hvmloader, who can load the blobs. >>> >>> Having said that, I wonder whether we (both x86 and ARM) could use >>> Anthony's xc_dom_image.full_acpi_module instead of acpitables_blob that >>> Shannon's series added. (even if we can't, I think >>> xc_hvm_firmware_module is the right datastructure to store the blob >>> since it has both toolstack's virtual and guest's physical addresses). >> >> In this case, xc_hvm_firmware_module would need to be renamed as ARM guests >> are neither HVM nor PV. >> > > That's trivial. It's an internal structure that we can rename at will. > >> FWIW, from the toolstack point of view, ARM guests is considered as PV >> guest. > > ... while at the same time utilises HVM param... > > Not complaining, just this makes me chuckle a bit. :-) ARM guests is a combination of HVM and PV features. I agree it is a bit a mess, but there is code in the hypervisor/toolstack/Linux which rely on the type of guests (e.g LIBXL_DOMAIN_TYPE_* in libxl) and not a set of available features. In the hypervisor, we are trying to move towards a set of features (i.e dropping is_pv_domain/is_hvm_domain in common code) as none suit ARM guests. I think it will benefit for both ARM and x86 to move available features rather than type. However this is requiring a lot of rework which cannot be done quickly. Regards,
On Mon, Jul 11, 2016 at 09:33:21AM -0400, Boris Ostrovsky wrote: > On 07/11/2016 06:47 AM, Wei Liu wrote: > > On Fri, Jul 08, 2016 at 01:20:46PM -0400, Boris Ostrovsky wrote: > >> On 07/08/2016 12:07 PM, Wei Liu wrote: > >>> On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: > >>>> On 07/08/2016 06:55 AM, Wei Liu wrote: > >>>>>> + > >>>>>> + /* Map page that will hold RSDP */ > >>>>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; > >>>>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > >>>>>> + if (rc) { > >>>>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", > >>>>>> + __FUNCTION__, rc); > >>>>>> + goto out; > >>>>>> + } > >>>>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, > >>>>>> + ctxt.page_size, > >>>>>> + PROT_READ | PROT_WRITE, > >>>>>> + RSDP_ADDRESS >> ctxt.page_shift); > >>>>> I think with Anthony's on-going work you should be more flexible for all > >>>>> you tables. If the tool stack already knows where it can (or should) load the rsdp, there is not much reason to be flexible. > >>>> Not sure I understand what you mean here. You want the address > >>>> (RSDP_ADDRESS) to be a variable as opposed to a macro? > >>>> > >>> I'm still trying to wrap my head around the possible interaction between > >>> Anthony's work and your work. > >>> > >>> Anthony's work allows dynamically loading of firmware blobs. If you use > >>> a fixed address, theoretically it can clash with firmware blobs among > >>> other things libxc can load. The high address is a safe bet so that > >>> probably won't happen in practice. > >>> > >>> Anthony's work allows loading arbitrary blobs actually. Can you take > >>> advantage of that mechanism as well? That is, to specify all your tables > >>> as modules and let libxc handle actually loading them into guest > >>> memory. > >>> > >>> Does this make sense? > >>> > >>> Also CC Anthony here. > >> > >> My understanding of Anthony's series is that its goal was to provide an > >> interface to pass DSDT (and maybe some other tables) from the toolstack > >> to hvmloader. Not anymore. The only new functionality provided by the patch series is to load the BIOS (or OVMF) from the filesystem (instead of having this blob embedded into hvmloader). It does also change the way an extra acpi tables or a smbios is loaded into guest memory for consumption by hvmloader. But that just make the libxc code a bit cleaner. > >> Here we don't have hvmloader, we are loading the tables directly into > >> guest's memory. > >> > > Do you use the same hvm_start_info structure? I don't think that > > structure is restricted to hvmloader. > > > Yes, we do. However, in PVH(v2) case it will be seen next by the guest > who will expect the tables to already be in memory. I.e. there is no > intermediate Xen component, such as hvmloader, who can load the blobs. > > Having said that, I wonder whether we (both x86 and ARM) could use > Anthony's xc_dom_image.full_acpi_module instead of acpitables_blob that I don't have full_acpi_module anymore in my patch series. But that does not prevent you from introducing one. > Shannon's series added. (even if we can't, I think > xc_hvm_firmware_module is the right datastructure to store the blob > since it has both toolstack's virtual and guest's physical addresses).
On 07/11/2016 09:41 AM, Wei Liu wrote: > On Mon, Jul 11, 2016 at 09:33:21AM -0400, Boris Ostrovsky wrote: >> On 07/11/2016 06:47 AM, Wei Liu wrote: >>> On Fri, Jul 08, 2016 at 01:20:46PM -0400, Boris Ostrovsky wrote: >>>> On 07/08/2016 12:07 PM, Wei Liu wrote: >>>>> On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: >>>>>> On 07/08/2016 06:55 AM, Wei Liu wrote: >>>>>>>> + >>>>>>>> + /* Map page that will hold RSDP */ >>>>>>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; >>>>>>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); >>>>>>>> + if (rc) { >>>>>>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", >>>>>>>> + __FUNCTION__, rc); >>>>>>>> + goto out; >>>>>>>> + } >>>>>>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, >>>>>>>> + ctxt.page_size, >>>>>>>> + PROT_READ | PROT_WRITE, >>>>>>>> + RSDP_ADDRESS >> ctxt.page_shift); >>>>>>> I think with Anthony's on-going work you should be more flexible for all >>>>>>> you tables. >>>>>> Not sure I understand what you mean here. You want the address >>>>>> (RSDP_ADDRESS) to be a variable as opposed to a macro? >>>>>> >>>>> I'm still trying to wrap my head around the possible interaction between >>>>> Anthony's work and your work. >>>>> >>>>> Anthony's work allows dynamically loading of firmware blobs. If you use >>>>> a fixed address, theoretically it can clash with firmware blobs among >>>>> other things libxc can load. The high address is a safe bet so that >>>>> probably won't happen in practice. >>>>> >>>>> Anthony's work allows loading arbitrary blobs actually. Can you take >>>>> advantage of that mechanism as well? That is, to specify all your tables >>>>> as modules and let libxc handle actually loading them into guest >>>>> memory. >>>>> >>>>> Does this make sense? >>>>> >>>>> Also CC Anthony here. >>>> My understanding of Anthony's series is that its goal was to provide an >>>> interface to pass DSDT (and maybe some other tables) from the toolstack >>>> to hvmloader. >>>> >>>> Here we don't have hvmloader, we are loading the tables directly into >>>> guest's memory. >>>> >>> Do you use the same hvm_start_info structure? I don't think that >>> structure is restricted to hvmloader. >> >> Yes, we do. However, in PVH(v2) case it will be seen next by the guest >> who will expect the tables to already be in memory. I.e. there is no >> intermediate Xen component, such as hvmloader, who can load the blobs. >> > Maybe you misunderstood. Anthony's work will load the blobs into guest > memory -- all fields in hvm_start_info points to guest physical address > IIRC. Hvmloader might want to relocate them, but that's a different > matter. What's the status on Anthony's series? I can rebase on top of his tree (I might indeed be able to reuse some of his code) but if this is way off the dependencies become problematic (because Shannon's series would also be delayed). > >> Having said that, I wonder whether we (both x86 and ARM) could use >> Anthony's xc_dom_image.full_acpi_module instead (no full_acpi_module anymore, I was looking at an earlier series version. I guess it's system_firmware_module now) >> of acpitables_blob that >> Shannon's series added. (even if we can't, I think >> xc_hvm_firmware_module is the right datastructure to store the blob >> since it has both toolstack's virtual and guest's physical addresses). >> > Yes, that's along the line I'm thinking about. So I am confused about xc_hvm_firmware_mode: is guest_addr_out meant to be guest's physical or virtual? One one hand it looks like virtual: in https://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02901.html + module->guest_addr_out = seg.vstart; but then in https://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02902.html + modlist[index].paddr = module->guest_addr_out; -boris
On Mon, Jul 11, 2016 at 10:40:17AM -0400, Boris Ostrovsky wrote: > On 07/11/2016 09:41 AM, Wei Liu wrote: > > On Mon, Jul 11, 2016 at 09:33:21AM -0400, Boris Ostrovsky wrote: > >> On 07/11/2016 06:47 AM, Wei Liu wrote: > >>> On Fri, Jul 08, 2016 at 01:20:46PM -0400, Boris Ostrovsky wrote: > >>>> On 07/08/2016 12:07 PM, Wei Liu wrote: > >>>>> On Fri, Jul 08, 2016 at 10:48:52AM -0400, Boris Ostrovsky wrote: > >>>>>> On 07/08/2016 06:55 AM, Wei Liu wrote: > >>>>>>>> + > >>>>>>>> + /* Map page that will hold RSDP */ > >>>>>>>> + extent = RSDP_ADDRESS >> ctxt.page_shift; > >>>>>>>> + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); > >>>>>>>> + if (rc) { > >>>>>>>> + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", > >>>>>>>> + __FUNCTION__, rc); > >>>>>>>> + goto out; > >>>>>>>> + } > >>>>>>>> + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, > >>>>>>>> + ctxt.page_size, > >>>>>>>> + PROT_READ | PROT_WRITE, > >>>>>>>> + RSDP_ADDRESS >> ctxt.page_shift); > >>>>>>> I think with Anthony's on-going work you should be more flexible for all > >>>>>>> you tables. > >>>>>> Not sure I understand what you mean here. You want the address > >>>>>> (RSDP_ADDRESS) to be a variable as opposed to a macro? > >>>>>> > >>>>> I'm still trying to wrap my head around the possible interaction between > >>>>> Anthony's work and your work. > >>>>> > >>>>> Anthony's work allows dynamically loading of firmware blobs. If you use > >>>>> a fixed address, theoretically it can clash with firmware blobs among > >>>>> other things libxc can load. The high address is a safe bet so that > >>>>> probably won't happen in practice. > >>>>> > >>>>> Anthony's work allows loading arbitrary blobs actually. Can you take > >>>>> advantage of that mechanism as well? That is, to specify all your tables > >>>>> as modules and let libxc handle actually loading them into guest > >>>>> memory. > >>>>> > >>>>> Does this make sense? > >>>>> > >>>>> Also CC Anthony here. > >>>> My understanding of Anthony's series is that its goal was to provide an > >>>> interface to pass DSDT (and maybe some other tables) from the toolstack > >>>> to hvmloader. > >>>> > >>>> Here we don't have hvmloader, we are loading the tables directly into > >>>> guest's memory. > >>>> > >>> Do you use the same hvm_start_info structure? I don't think that > >>> structure is restricted to hvmloader. > >> > >> Yes, we do. However, in PVH(v2) case it will be seen next by the guest > >> who will expect the tables to already be in memory. I.e. there is no > >> intermediate Xen component, such as hvmloader, who can load the blobs. > >> > > Maybe you misunderstood. Anthony's work will load the blobs into guest > > memory -- all fields in hvm_start_info points to guest physical address > > IIRC. Hvmloader might want to relocate them, but that's a different > > matter. > > What's the status on Anthony's series? I can rebase on top of his tree > (I might indeed be able to reuse some of his code) but if this is way > off the dependencies become problematic (because Shannon's series would > also be delayed). > His series is very close to get merged. I believe toolstack in his series only require cosmetic changes and hvmloader patches are all acked. > > > >> Having said that, I wonder whether we (both x86 and ARM) could use > >> Anthony's xc_dom_image.full_acpi_module instead > > > (no full_acpi_module anymore, I was looking at an earlier series > version. I guess it's system_firmware_module now) > > > >> of acpitables_blob that > >> Shannon's series added. (even if we can't, I think > >> xc_hvm_firmware_module is the right datastructure to store the blob > >> since it has both toolstack's virtual and guest's physical addresses). > >> > > Yes, that's along the line I'm thinking about. > > So I am confused about xc_hvm_firmware_mode: is guest_addr_out meant to > be guest's physical or virtual? > > One one hand it looks like virtual: > > in > https://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02901.html > + module->guest_addr_out = seg.vstart; > > but then in > https://lists.xenproject.org/archives/html/xen-devel/2016-06/msg02902.html > > + modlist[index].paddr = module->guest_addr_out; > It should be guest physical. Wei. > > -boris > >
diff --git a/.gitignore b/.gitignore index 9dd2086..d4da37f 100644 --- a/.gitignore +++ b/.gitignore @@ -179,6 +179,10 @@ tools/libxl/testenum.c tools/libxl/tmp.* tools/libxl/_libxl.api-for-check tools/libxl/*.api-ok +tools/libxl/mk_dsdt +tools/libxl/dsdt*.c +tools/libxl/dsdt_*.asl +tools/libxl/ssdt_*.h tools/misc/cpuperf/cpuperf-perfcntr tools/misc/cpuperf/cpuperf-xen tools/misc/xc_shadow diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c index 290f005..a6ddf53 100644 --- a/tools/libacpi/build.c +++ b/tools/libacpi/build.c @@ -23,6 +23,7 @@ #include "ssdt_tpm.h" #include "ssdt_pm.h" #include "x86.h" +#include <xen/hvm/hvm_info_table.h> #include <xen/hvm/hvm_xs_strings.h> #include <xen/hvm/params.h> @@ -473,7 +474,7 @@ static int new_vm_gid(struct acpi_ctxt *ctxt, return 1; } -void acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config) +int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config) { struct acpi_20_rsdp *rsdp; struct acpi_20_rsdt *rsdt; @@ -594,11 +595,11 @@ void acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config) *(struct acpi_info *)config->ainfop = config->ainfo; - return; + return 0; oom: printf("unable to build ACPI tables: out of memory\n"); - + return -1; } /* diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h index 87a2937..791ebac 100644 --- a/tools/libacpi/libacpi.h +++ b/tools/libacpi/libacpi.h @@ -69,6 +69,15 @@ struct acpi_ctxt { void *(*alloc)(struct acpi_ctxt *ctxt, uint32_t size, uint32_t align); unsigned long (*v2p)(struct acpi_ctxt *ctxt, void *v); } mem_ops; + + unsigned int page_size; + unsigned int page_shift; + + /* Memory allocator */ + unsigned long alloc_base_paddr; + unsigned long alloc_base_vaddr; + unsigned long alloc_currp; + unsigned long alloc_end; }; struct acpi_config { @@ -98,13 +107,13 @@ struct acpi_config { * This must match the OperationRegion(BIOS, SystemMemory, ....) * definition in the DSDT */ - unsigned int ainfop; + unsigned long ainfop; /* RSDP address */ - unsigned int rsdp; + unsigned long rsdp; }; -void acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config); +int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config); #endif /* __LIBACPI_H__ */ diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index 3a2d64a..18be2e7 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -75,7 +75,20 @@ else LIBXL_OBJS-y += libxl_no_colo.o endif -LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o +ACPI_PATH = $(XEN_ROOT)/tools/libacpi +ACPI_FILES = dsdt_pvh.c build.c static_tables.c +ACPI_OBJS = $(patsubst %.c,%.o,$(ACPI_FILES)) +$(ACPI_FILES): acpi +$(ACPI_OBJS): CFLAGS += -I. -DSTDUTILS=\"$(CURDIR)/libxl_x86_acpi.h\" +vpath build.c $(ACPI_PATH)/ +vpath static_tables.c $(ACPI_PATH)/ +LIBXL_OBJS-$(CONFIG_X86) += $(ACPI_OBJS) + +.PHONY: acpi +acpi: + $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(shell pwd) + +LIBXL_OBJS-$(CONFIG_X86) += libxl_cpuid.o libxl_x86.o libxl_psr.o libxl_x86_acpi.o LIBXL_OBJS-$(CONFIG_ARM) += libxl_nocpuid.o libxl_arm.o libxl_libfdt_compat.o ifeq ($(CONFIG_NetBSD),y) @@ -166,6 +179,7 @@ $(XL_OBJS): CFLAGS += $(CFLAGS_XL) $(XL_OBJS): CFLAGS += -include $(XEN_ROOT)/tools/config.h # libxl_json.h needs it. libxl_dom.o: CFLAGS += -I$(XEN_ROOT)/tools # include libacpi/x86.h +libxl_x86_acpi.o: CFLAGS += -I$(XEN_ROOT)/tools SAVE_HELPER_OBJS = libxl_save_helper.o _libxl_save_msgs_helper.o $(SAVE_HELPER_OBJS): CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenevtchn) @@ -308,6 +322,7 @@ clean: $(RM) -f testidl.c.new testidl.c *.api-ok $(RM) -f xenlight.pc $(RM) -f xlutil.pc + $(MAKE) -C $(ACPI_PATH) ACPI_BUILD_DIR=$(shell pwd) clean distclean: clean $(RM) -f xenlight.pc.in xlutil.pc.in diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h index 34a853c..7c6536b 100644 --- a/tools/libxl/libxl_arch.h +++ b/tools/libxl/libxl_arch.h @@ -62,4 +62,7 @@ int libxl__arch_domain_construct_memmap(libxl__gc *gc, uint32_t domid, struct xc_dom_image *dom); +int libxl__dom_load_acpi(libxl__gc *gc, + libxl_domain_build_info *info, + struct xc_dom_image *dom); #endif diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index ba3472e..7e4e289 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -657,6 +657,7 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid, LOGE(ERROR, "xc_dom_build_image failed"); goto out; } + if ( (ret = xc_dom_boot_image(dom)) != 0 ) { LOGE(ERROR, "xc_dom_boot_image failed"); goto out; diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c index 32ce1d2..7201dbb 100644 --- a/tools/libxl/libxl_x86.c +++ b/tools/libxl/libxl_x86.c @@ -8,15 +8,18 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc, xc_domain_configuration_t *xc_config) { - if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM && - d_config->b_info.device_model_version != - LIBXL_DEVICE_MODEL_VERSION_NONE) { - /* HVM domains with a device model. */ - xc_config->emulation_flags = XEN_X86_EMU_ALL; - } else { - /* PV or HVM domains without a device model. */ + if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_HVM) { + if (d_config->b_info.device_model_version != + LIBXL_DEVICE_MODEL_VERSION_NONE) + xc_config->emulation_flags = XEN_X86_EMU_ALL; + else if (libxl_defbool_val(d_config->b_info.u.hvm.apic)) + /* + * HVM guests without device model may want + * to have LAPIC emulation. + */ + xc_config->emulation_flags = XEN_X86_EMU_LAPIC; + } else xc_config->emulation_flags = 0; - } return 0; } @@ -366,7 +369,15 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc, libxl_domain_build_info *info, struct xc_dom_image *dom) { - return 0; + int ret = 0; + + if ((info->type == LIBXL_DOMAIN_TYPE_HVM) && + (info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_NONE)) { + if ( (ret = libxl__dom_load_acpi(gc, info, dom)) != 0 ) + LOGE(ERROR, "libxl_dom_load_acpi failed"); + } + + return ret; } /* Return 0 on success, ERROR_* on failure. */ diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c new file mode 100644 index 0000000..144f063 --- /dev/null +++ b/tools/libxl/libxl_x86_acpi.c @@ -0,0 +1,292 @@ +/****************************************************************************** + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + */ + +#include "libxl_internal.h" +#include "libxl_arch.h" +#include <xen/hvm/hvm_info_table.h> +#include <xen/hvm/e820.h> +#include "libacpi/libacpi.h" + +#include <xc_dom.h> + +#define NUM_ACPI_PAGES 16 /* Number of pages holding ACPI tables */ +#define RSDP_ADDRESS (0xffffful - 64) /* Last doubleword of BIOS RO memory */ +#define ACPI_INFO_PHYSICAL_ADDRESS 0xfc000000 + +extern const unsigned char dsdt_pvh[]; +extern const unsigned int dsdt_pvh_len; + +/* Assumes contiguous physical space */ +static unsigned long virt_to_phys(struct acpi_ctxt *ctxt, void *v) +{ + return (((unsigned long)v - ctxt->alloc_base_vaddr) + + ctxt->alloc_base_paddr); +} + +static void *mem_alloc(struct acpi_ctxt *ctxt, uint32_t size, uint32_t align) +{ + unsigned long s, e; + + /* Align to at least 16 bytes. */ + if (align < 16) + align = 16; + + s = (ctxt->alloc_currp + align) & ~((unsigned long)align - 1); + e = s + size - 1; + + /* TODO: Reallocate memory */ + if ((e < s) || (e >= ctxt->alloc_end)) return NULL; + + while (ctxt->alloc_currp >> ctxt->page_shift != + e >> ctxt->page_shift) + ctxt->alloc_currp += ctxt->page_size; + + ctxt->alloc_currp = e; + + return (void *)s; +} + +static int init_acpi_config(libxl__gc *gc, + struct xc_dom_image *dom, + libxl_domain_build_info *b_info, + struct acpi_config *config) +{ + xc_interface *xch = dom->xch; + uint32_t domid = dom->guest_domid; + xc_dominfo_t info; + int i, rc; + + config->dsdt_anycpu = config->dsdt_15cpu = dsdt_pvh; + config->dsdt_anycpu_len = config->dsdt_15cpu_len = dsdt_pvh_len; + + rc = xc_domain_getinfo(xch, domid, 1, &info); + if (rc < 0) { + LOG(ERROR, "%s: getdomaininfo failed (rc=%d)", __FUNCTION__, rc); + return rc; + } + + config->hvminfo = libxl__zalloc(gc, sizeof(*config->hvminfo)); + + config->hvminfo->apic_mode = libxl_defbool_val(b_info->u.hvm.apic); + + if (dom->nr_vnodes) { + struct acpi_numa *numa = &config->numa; + + rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes, + &numa->nr_vmemranges, + &config->hvminfo->nr_vcpus, NULL, NULL, NULL); + if (rc) { + LOG(ERROR, "%s: xc_domain_getvnuma failed (rc=%d)", + __FUNCTION__, rc); + return rc; + } + + numa->vmemrange = libxl__zalloc(gc, dom->nr_vmemranges * + sizeof(*numa->vmemrange)); + numa->vdistance = libxl__zalloc(gc, dom->nr_vnodes * + sizeof(*numa->vdistance)); + numa->vcpu_to_vnode = libxl__zalloc(gc, config->hvminfo->nr_vcpus * + sizeof(*numa->vcpu_to_vnode)); + rc = xc_domain_getvnuma(xch, domid, &numa->nr_vnodes, + &numa->nr_vmemranges, + &config->hvminfo->nr_vcpus, numa->vmemrange, + numa->vdistance, numa->vcpu_to_vnode); + if (rc) { + LOG(ERROR, "%s: xc_domain_getvnuma failed (rc=%d)", + __FUNCTION__, rc); + return rc; + } + } + else + config->hvminfo->nr_vcpus = info.max_vcpu_id + 1; + + for (i=0; i<config->hvminfo->nr_vcpus; i++) + config->hvminfo->vcpu_online[i / 8] |= 1 << (i & 7); + + return 0; +} + +static int populate_acpi_pages(struct xc_dom_image *dom, + xen_pfn_t *extents, + unsigned int num_pages, + struct acpi_ctxt *ctxt) +{ + int rc; + xc_interface *xch = dom->xch; + uint32_t domid = dom->guest_domid; + unsigned long idx, first_high_idx = (1ull << (32 - ctxt->page_shift)); + + for (; num_pages; num_pages--, extents++) { + + if (xc_domain_populate_physmap(xch, domid, 1, 0, 0, extents) == 1) + continue; + + if (dom->highmem_end) { + idx = --dom->highmem_end; + if ( idx == first_high_idx ) + dom->highmem_end = 0; + } else + idx = --dom->lowmem_end; + + rc = xc_domain_add_to_physmap(xch, domid, + XENMAPSPACE_gmfn, + idx, *extents); + if (rc) + return rc; + } + + return 0; +} + +int libxl__dom_load_acpi(libxl__gc *gc, + libxl_domain_build_info *info, + struct xc_dom_image *dom) +{ + struct acpi_config config = {0}; + struct acpi_ctxt ctxt; + uint32_t domid = dom->guest_domid; + xc_interface *xch = dom->xch; + int rc, i, acpi_pages_num; + xen_pfn_t extent, *extents; + void *acpi_pages, *guest_acpi_pages = NULL; + unsigned long page_mask; + + if ((info->type != LIBXL_DOMAIN_TYPE_HVM) || + (info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE)) + return 0; + + ctxt.page_size = XC_DOM_PAGE_SIZE(dom); + ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom); + page_mask = (1UL << ctxt.page_shift) - 1; + + ctxt.mem_ops.alloc = mem_alloc; + ctxt.mem_ops.v2p = virt_to_phys; + + rc = init_acpi_config(gc, dom, info, &config); + if (rc) { + LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc); + return rc; + } + + /* Map page that will hold RSDP */ + extent = RSDP_ADDRESS >> ctxt.page_shift; + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); + if (rc) { + LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d", + __FUNCTION__, rc); + goto out; + } + config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid, + ctxt.page_size, + PROT_READ | PROT_WRITE, + RSDP_ADDRESS >> ctxt.page_shift); + if (!config.rsdp) { + LOG(ERROR, "%s: Can't map acpi_physical", __FUNCTION__); + rc = -1; + goto out; + } + + /* Map acpi_info page */ + extent = ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift; + rc = populate_acpi_pages(dom, &extent, 1, &ctxt); + if (rc) { + LOG(ERROR, "%s: populate_acpi_pages for acpi_info failed with %d", + __FUNCTION__, rc); + goto out; + } + + config.ainfop = (unsigned long)xc_map_foreign_range(xch, domid, + ctxt.page_size, + PROT_READ | PROT_WRITE, + ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift); + if (!config.ainfop) { + LOG(ERROR, "%s: Can't map acpi_info_page", __FUNCTION__); + rc = -1; + goto out; + } + + /* Pages to hold ACPI tables */ + acpi_pages = libxl__malloc(gc, (NUM_ACPI_PAGES + 1) * ctxt.page_size); + while ((unsigned long)acpi_pages & page_mask) + acpi_pages++; + + /* + * Set up allocator memory. + * Start next to acpi_info page to avoid fracturing e820. + */ + ctxt.alloc_base_paddr = ACPI_INFO_PHYSICAL_ADDRESS + ctxt.page_size; + ctxt.alloc_base_vaddr = ctxt.alloc_currp = (unsigned long)acpi_pages; + ctxt.alloc_end = (unsigned long)acpi_pages + + (NUM_ACPI_PAGES * ctxt.page_size); + + /* Build the tables */ + rc = acpi_build_tables(&ctxt, &config); + if (rc) { + LOG(ERROR, "%s: acpi_build_tables failed with %d", + __FUNCTION__, rc); + goto out; + } + + /* Copy ACPI tables into guest's memory */ + acpi_pages_num = + ((ctxt.alloc_currp - (unsigned long)acpi_pages) >> ctxt.page_shift) + + ((ctxt.alloc_currp & page_mask) ? 1 : 0); + extents = libxl__malloc(gc, acpi_pages_num * sizeof(*extents)); + for (i = 0; i < acpi_pages_num; i++) + extents[i] = (ctxt.alloc_base_paddr >> ctxt.page_shift) + i; + rc = populate_acpi_pages(dom, extents, acpi_pages_num, &ctxt); + if (rc) { + LOG(ERROR, "%s: populate_acpi_pages for APCI tables failed with %d", + __FUNCTION__, rc); + goto out; + } + guest_acpi_pages = xc_map_foreign_range(xch, domid, + ctxt.page_size * acpi_pages_num, + PROT_READ | PROT_WRITE, + ctxt.alloc_base_paddr >> ctxt.page_shift); + if (!guest_acpi_pages) { + LOG(ERROR, "%s Can't map guest_acpi_pages", __FUNCTION__); + rc = -1; + goto out; + } + + memcpy(guest_acpi_pages, acpi_pages, acpi_pages_num * ctxt.page_size); + + dom->acpi_pfn = ACPI_INFO_PHYSICAL_ADDRESS >> ctxt.page_shift; + dom->acpi_pages = acpi_pages_num + 1; + +out: + munmap(guest_acpi_pages, acpi_pages_num * ctxt.page_size); + munmap((void *)config.ainfop, ctxt.page_size); + munmap((void *)config.rsdp, ctxt.page_size); + + return rc; + +} +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/tools/libxl/libxl_x86_acpi.h b/tools/libxl/libxl_x86_acpi.h new file mode 100644 index 0000000..1899ec1 --- /dev/null +++ b/tools/libxl/libxl_x86_acpi.h @@ -0,0 +1,21 @@ +#ifndef LIBXL_X86_ACPI_H +#define LIBXL_X86_ACPI_H + +#include "libxl_internal.h" + +#define ASSERT(x) assert(x) + +static inline int test_bit(unsigned int b, void *p) +{ + return !!(((uint8_t *)p)[b>>3] & (1u<<(b&7))); +} + +#endif /* LIBXL_X_86_ACPI_H */ + +/* + * Local variables: + * mode: C + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- Changes in v1: * Move to libxl * Added populate_acpi_pages() * Stash location/size of tables in xc_dom_image (to be used in constructing e820 map) * Use libxl allocator * Only set XEN_X86_EMU_LAPIC flag if 'apic' option is set. * Make acpi_build_tables() return error code .gitignore | 4 + tools/libacpi/build.c | 7 +- tools/libacpi/libacpi.h | 15 ++- tools/libxl/Makefile | 17 +++- tools/libxl/libxl_arch.h | 3 + tools/libxl/libxl_dom.c | 1 + tools/libxl/libxl_x86.c | 29 +++-- tools/libxl/libxl_x86_acpi.c | 292 ++++++++++++++++++++++++++++++++++++++++++ tools/libxl/libxl_x86_acpi.h | 21 +++ 9 files changed, 373 insertions(+), 16 deletions(-) create mode 100644 tools/libxl/libxl_x86_acpi.c create mode 100644 tools/libxl/libxl_x86_acpi.h