From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH RESEND 05/14] libxl/arm: Construct ACPI GTDT table Date: Mon, 6 Jun 2016 15:50:23 +0100 (BST) Message-ID: References: <1464670986-10256-1-git-send-email-zhaoshenglong@huawei.com> <1464670986-10256-6-git-send-email-zhaoshenglong@huawei.com> <5755641A.9000206@arm.com> <57558950.7060603@linaro.org> Mime-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="8323329-1265440533-1465224627=:6721" Return-path: In-Reply-To: <57558950.7060603@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Shannon Zhao Cc: Stefano Stabellini , wei.liu2@citrix.com, ian.jackson@eu.citrix.com, peter.huangpeng@huawei.com, xen-devel@lists.xen.org, Julien Grall , Shannon Zhao List-Id: xen-devel@lists.xenproject.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-1265440533-1465224627=:6721 Content-Type: TEXT/PLAIN; charset=UTF-8 Content-Transfer-Encoding: 8BIT On Mon, 6 Jun 2016, Shannon Zhao wrote: > On 2016年06月06日 20:04, Stefano Stabellini wrote: > > On Mon, 6 Jun 2016, Julien Grall wrote: > >> > Hello, > >> > > >> > On 06/06/16 12:40, Stefano Stabellini wrote: > >>> > > On Tue, 31 May 2016, Shannon Zhao wrote: > >>>> > > > From: Shannon Zhao > >>>> > > > > >>>> > > > Construct GTDT table with the interrupt information of timers. > >>>> > > > > >>>> > > > Signed-off-by: Shannon Zhao > >>>> > > > --- > >>>> > > > tools/libxl/libxl_arm.c | 75 > >>>> > > > ++++++++++++++++++++++++++++++++++++++++++++++++- > >>>> > > > 1 file changed, 74 insertions(+), 1 deletion(-) > >>>> > > > > >>>> > > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c > >>>> > > > index 9e99159..0fb4f69 100644 > >>>> > > > --- a/tools/libxl/libxl_arm.c > >>>> > > > +++ b/tools/libxl/libxl_arm.c > >>>> > > > @@ -3,6 +3,7 @@ > >>>> > > > #include "libxl_libfdt_compat.h" > >>>> > > > > >>>> > > > #include > >>>> > > > +#include > >>>> > > > #include > >>>> > > > #include > >>>> > > > #include > >>>> > > > @@ -880,13 +881,85 @@ out: > >>>> > > > return rc; > >>>> > > > } > >>>> > > > > >>>> > > > +static void make_acpi_header(struct acpi_table_header *h, const char > >>>> > > > *sig, > >>>> > > > + int len, uint8_t rev) > >>>> > > > +{ > >>>> > > > + memcpy(&h->signature, sig, 4); > >>>> > > > + h->length = len; > >>>> > > > + h->revision = rev; > >>>> > > > + memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6); > >>>> > > > + memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4); > >>>> > > > + memcpy(h->oem_table_id + 4, sig, 4); > >>>> > > > + h->oem_revision = 1; > >>>> > > > + memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4); > >>>> > > > + h->asl_compiler_revision = 1; > >>>> > > > + h->checksum = 0; > >>>> > > > +} > >>>> > > > + > >>>> > > > +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom) > >>>> > > > +{ > >>>> > > > + struct acpi_gtdt_descriptor *gtdt; > >>>> > > > + > >>>> > > > + gtdt = libxl__zalloc(gc, sizeof(*gtdt)); > >>>> > > > + > >>>> > > > + gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI; > >>>> > > > + gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE << > >>>> > > > ACPI_GTDT_INTERRUPT_MODE) > >>>> > > > + |(ACPI_ACTIVE_LOW << > >>>> > > > ACPI_GTDT_INTERRUPT_POLARITY); > >>>> > > > + gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI; > >>>> > > > + gtdt->non_secure_el1_flags = > >>>> > > > + (ACPI_LEVEL_SENSITIVE << > >>>> > > > ACPI_GTDT_INTERRUPT_MODE) > >>>> > > > + |(ACPI_ACTIVE_LOW << > >>>> > > > ACPI_GTDT_INTERRUPT_POLARITY); > >>>> > > > + gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI; > >>>> > > > + gtdt->virtual_timer_flags = > >>>> > > > + (ACPI_LEVEL_SENSITIVE << > >>>> > > > ACPI_GTDT_INTERRUPT_MODE) > >>>> > > > + |(ACPI_ACTIVE_LOW << > >>>> > > > ACPI_GTDT_INTERRUPT_POLARITY); > >>>> > > > + > >>>> > > > + make_acpi_header(>dt->header, "GTDT", sizeof(*gtdt), 2); > >>>> > > > + > >>>> > > > + dom->acpitable_blob->gtdt.table = (void *)gtdt; > >>>> > > > + /* Align to 64bit. */ > >>>> > > > + dom->acpitable_blob->gtdt.size = sizeof(*gtdt); > >>>> > > > + dom->acpitable_size += dom->acpitable_blob->gtdt.size; > >>>> > > > +} > >>>> > > > + > >>>> > > > +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info, > >>>> > > > + libxl__domain_build_state *state, > >>>> > > > + struct xc_dom_image *dom) > >>>> > > > +{ > >>>> > > > + const libxl_version_info *vers; > >>>> > > > + > >>>> > > > + /* convenience aliases */ > >>>> > > > + xc_domain_configuration_t *xc_config = &state->config; > >>>> > > > + > >>>> > > > + vers = libxl_get_version_info(CTX); > >>>> > > > + if (vers == NULL) > >>>> > > > + return ERROR_FAIL; > >>>> > > > + > >>>> > > > + LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest", > >>>> > > > + vers->xen_version_major, vers->xen_version_minor); > >>>> > > > + > >>>> > > > + /* Alloc memory for ACPI blob placeholders. */ > >>>> > > > + dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct > >>>> > > > acpitable_blob)); > >>>> > > > + dom->acpitable_size = 0; > >>>> > > > + > >>>> > > > + make_acpi_gtdt(gc, dom); > >>>> > > > + > >>>> > > > + return 0; > >>>> > > > +} > >>>> > > > + > >>>> > > > int libxl__arch_domain_init_hw_description(libxl__gc *gc, > >>>> > > > libxl_domain_build_info > >>>> > > > *info, > >>>> > > > libxl__domain_build_state > >>>> > > > *state, > >>>> > > > struct xc_dom_image *dom) > >>>> > > > { > >>>> > > > + int rc; > >>>> > > > + > >>>> > > > assert(info->type == LIBXL_DOMAIN_TYPE_PV); > >>>> > > > - return prepare_dtb(gc, info, state, dom); > >>>> > > > + rc = prepare_dtb(gc, info, state, dom); > >>>> > > > + if (rc) > >>>> > > > + return rc; > >>>> > > > + > >>>> > > > + return prepare_acpi(gc, info, state, dom); > >>>> > > > } > >>> > > > >>> > > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM > >>> > > config file enables them, with default off. > >> > > >> > The VM system specification for ARM [1] mandates that both ACPI and DT should > >> > be provided and described the entire VM and its peripheral (see the section > >> > "Hardware Description"). > >> > > >> > Furthermore, the user may not know if the guest OS will use ACPI or DT For > >> > instance Redhat is using ACPI whilst Debian is using DT. > >> > > >> > So we have to provide both by default. However, 32-bit domain should only have > >> > Device-Tree table created. > >> > > >> > Anyway, the reason should have been described in the commit message. I would > >> > split this patch in two: introducing prepare ACPI and then GTDT table. So we > >> > can provide details in the commit message. > > All right, let me rephrase then: we should have a VMSPEC=on or off to > > enable or disable compliance with the VM system specification for ARM. > > (The good thing about specifications is that there are so many to choose > > from.) With compliance disabled, we can avoid introducing ACPI tables > > for the guest. > > > > Given that "VMSPEC" is cumbersome, I suggest to introduce a simpler and > > more meaningful alias: "ACPI" :-) > > > > I am open to discussion on what the default should be, but there needs > > to be a way to disable ACPI. > I don't know why we need to disable ACPI because we can provide ACPI > tables but guest could choose to not use it. And for ARM32 domain, since > the linux guest kernel doesn't support ACPI, even we provide ACPI > tables, it can't use it, anyway. Memory usage. Simplicity: if you know you are not going to use ACPI, you might as well disable it to have one less moving piece (every line of code is potential for a bug). Guest configuration: if your guest operating system supports both ACPI and Device Tree and you want to be sure that Device Tree is the one that gets used, then you can do it by disabling ACPI at the VM level. Linux offers a command line option to do that, but other OSes might not and could choose ACPI by default. Debugging: it might be helpful to be able to enable/disable ACPI at the VM level just to test different code paths in the guest. These are just the first few reasons that come to mind. --8323329-1265440533-1465224627=:6721 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --8323329-1265440533-1465224627=:6721--