Re: [RFC XEN PATCH 06/16] tools: reserve guest memory for ACPI from device model

[Haozhong Zhang <haozhong.zhang@intel.com>]

On 01/27/17 15:44 -0500, Konrad Rzeszutek Wilk wrote:
>On Mon, Oct 10, 2016 at 08:32:25AM +0800, Haozhong Zhang wrote:
>> One guest page is reserved for the device model to place guest ACPI. The
>
>guest ACPI what? ACPI SSDT? MADT?

For NVDIMM, it includes NFIT and SSDT. However, the mechanism
implemented in this and following libacpi patches can be a generic one
to pass guest ACPI from device model. A simple conflict detection is
implemented in patch 11.

>
>Also why one page? What if there is a need for more than one page?
>
>You add  HVM_XS_DM_ACPI_LENGTH which makes me think this is accounted
>for?
>

One page is enough for NVDIMM (NFIT + SSDT). I don't see fundamental
restriction to not allow more one page, so I use HVM_XS_DM_ACPI_LENGTH
to pass the reserved size to allow changing the size in future.

>> base address and size of the reserved area are passed to the device
>> model via XenStore keys hvmloader/dm-acpi/{address, length}.
>>
>> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> ---
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/libxc/include/xc_dom.h            |  1 +
>>  tools/libxc/xc_dom_x86.c                |  7 +++++++
>>  tools/libxl/libxl_dom.c                 | 25 +++++++++++++++++++++++++
>>  xen/include/public/hvm/hvm_xs_strings.h | 11 +++++++++++
>>  4 files changed, 44 insertions(+)
>>
>> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
>> index 608cbc2..19d65cd 100644
>> --- a/tools/libxc/include/xc_dom.h
>> +++ b/tools/libxc/include/xc_dom.h
>> @@ -98,6 +98,7 @@ struct xc_dom_image {
>>      xen_pfn_t xenstore_pfn;
>>      xen_pfn_t shared_info_pfn;
>>      xen_pfn_t bootstack_pfn;
>> +    xen_pfn_t dm_acpi_pfn;
>
>Perhaps an pointer to an variable size array?
>
> xen_pfn_t *dm_acpi_pfns;
> unsigned int dm_apci_nr;
>
>?

dm_acpi_pfn is passed to QEMU via xenstore. Though passing an array of
pfns via xenstore is also doable, a pair of base pfn and length is
simpler.

>>      xen_pfn_t pfn_alloc_end;
>>      xen_vaddr_t virt_alloc_end;
>>      xen_vaddr_t bsd_symtab_start;
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 0eab8a7..47f14a1 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -674,6 +674,13 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>                           ioreq_server_pfn(0));
>>          xc_hvm_param_set(xch, domid, HVM_PARAM_NR_IOREQ_SERVER_PAGES,
>>                           NR_IOREQ_SERVER_PAGES);
>> +
>> +        dom->dm_acpi_pfn = xc_dom_alloc_page(dom, "DM ACPI");
>> +        if ( dom->dm_acpi_pfn == INVALID_PFN )
>> +        {
>> +            DOMPRINTF("Could not allocate page for device model ACPI.");
>> +            goto error_out;
>> +        }
>>      }
>>
>>      rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
>> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
>> index d519c8d..f0a1d97 100644
>> --- a/tools/libxl/libxl_dom.c
>> +++ b/tools/libxl/libxl_dom.c
>> @@ -865,6 +865,31 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
>>              goto err;
>>      }
>>
>> +    if (dom->dm_acpi_pfn) {
>> +        uint64_t guest_addr_out = dom->dm_acpi_pfn * XC_DOM_PAGE_SIZE(dom);
>> +
>> +        if (guest_addr_out >= 0x100000000ULL) {
>> +            LOG(ERROR,
>> +                "Guest address of DM ACPI is 0x%"PRIx64", but expected below 4G",
>> +                guest_addr_out);
>> +            goto err;
>> +        }
>> +
>> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_DM_ACPI_ADDRESS, domid);
>> +
>> +        ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%"PRIx64,
>> +                               guest_addr_out);
>> +        if (ret)
>> +            goto err;
>> +
>> +        path = GCSPRINTF("/local/domain/%d/"HVM_XS_DM_ACPI_LENGTH, domid);
>> +
>> +        ret = libxl__xs_printf(gc, XBT_NULL, path, "0x%"PRIx64,
>> +                               (uint64_t) XC_DOM_PAGE_SIZE(dom));
>I don't think you need the space here:      ^

will remove the space here and in other patches

Thanks,
Haozhong

>> +        if (ret)
>> +            goto err;
>> +    }
>> +
>>      return 0;
>>
>>  err:
>> diff --git a/xen/include/public/hvm/hvm_xs_strings.h b/xen/include/public/hvm/hvm_xs_strings.h
>> index 146b0b0..f44f71f 100644
>> --- a/xen/include/public/hvm/hvm_xs_strings.h
>> +++ b/xen/include/public/hvm/hvm_xs_strings.h
>> @@ -79,4 +79,15 @@
>>   */
>>  #define HVM_XS_OEM_STRINGS             "bios-strings/oem-%d"
>>
>> +/* Follows are XenStore keys for DM ACPI (ACPI built by device model,
>> + * e.g. QEMU).
>> + *
>> + * A reserved area of guest physical memory is used to pass DM
>> + * ACPI. Values of following two keys specify the base address and
>> + * length (in bytes) of the reserved area.
>> + */
>> +#define HVM_XS_DM_ACPI_ROOT              "hvmloader/dm-acpi"
>> +#define HVM_XS_DM_ACPI_ADDRESS           HVM_XS_DM_ACPI_ROOT"/address"
>> +#define HVM_XS_DM_ACPI_LENGTH            HVM_XS_DM_ACPI_ROOT"/length"
>> +
>>  #endif /* __XEN_PUBLIC_HVM_HVM_XS_STRINGS_H__ */
>> --
>> 2.10.1
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> https://lists.xen.org/xen-devel

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