From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH RFC 31/35] arm : acpi map status override table to dom0 Date: Thu, 05 Feb 2015 05:24:38 +0000 Message-ID: <54D2FE96.9090900@linaro.org> References: <1423058539-26403-1-git-send-email-parth.dixit@linaro.org> <1423058539-26403-32-git-send-email-parth.dixit@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1423058539-26403-32-git-send-email-parth.dixit@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: parth.dixit@linaro.org, xen-devel@lists.xen.org Cc: christoffer.dall@linaro.org, tim@xen.org, stefano.stabellini@citrix.com, ian.campbell@citrix.com, jbeulich@suse.com List-Id: xen-devel@lists.xenproject.org Hi Parth, You don't only map the status override table. You also create it. I would update the commit title to reflect it. On 04/02/2015 14:02, parth.dixit@linaro.org wrote: > From: Parth Dixit > > hide UART used by xen by indicating it in STAO table > and map it to dom0 > > Signed-off-by: Parth Dixit > --- > xen/arch/arm/arm64/acpi/arm-core.c | 50 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c > index a210596..9fd02f9 100644 > --- a/xen/arch/arm/arm64/acpi/arm-core.c > +++ b/xen/arch/arm/arm64/acpi/arm-core.c > @@ -256,6 +256,48 @@ static void set_psci_fadt(void) > fadt->header.checksum = (u8)( fadt->header.checksum-checksum ); > } > > +static int map_stao_table(struct domain *d, u64 *mstao) > +{ > + u64 addr; > + u64 size; > + int res; > + u8 checksum; > + struct acpi_table_stao *stao=NULL; stao = NULL > + > + stao = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_table_stao) ); No space before the last ) > + if( stao == NULL ) > + return -ENOMEM; > + > + ACPI_MEMCPY(stao->header.signature,ACPI_SIG_STAO, 4); Space after comma > + stao->header.length = sizeof(struct acpi_table_header) + 1; > + stao->header.checksum = 0; > + ACPI_MEMCPY(stao->header.oem_id, "LINARO", 6); > + ACPI_MEMCPY(stao->header.oem_table_id, "RTSMVEV8", 8); I though the plan was to use a Xen OEM ID? > + stao->header.revision = 1; > + ACPI_MEMCPY(stao->header.asl_compiler_id, "INTL", 4); > + stao->header.asl_compiler_revision = 0x20140828; Where does this revision comes from? > + stao->uart = 1; What about the devpath? > + size = sizeof(struct acpi_table_stao); > + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, stao), size); > + stao->header.checksum = (u8)( stao->header.checksum - checksum ); No space before the last ) > + *mstao = addr = virt_to_maddr(stao); > + > + res = map_ram_regions(d, > + paddr_to_pfn(addr & PAGE_MASK), > + DIV_ROUND_UP(size, PAGE_SIZE), > + paddr_to_pfn(addr & PAGE_MASK)); I'm concerned with this mapping, as long as most of the others. map_ram_regions is mapping Read/Write the region. In this case, the STAO size may not be aligned to PAGE_SIZE. So we may end up to map to DOM0 RW Xen memory. Even if DOM0 is a trusted domain, we should never let DOM0 write in Xen memory. IIRC, the plan was to map at least RO all the ACPI tables. > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to map 0x%"PRIx64 > + " - 0x%"PRIx64" in domain \n", > + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1); > + return res; > + } > + > + return 0; > +} > + > + > #define NR_NEW_XEN_TABLES 2 > > static int map_xsdt_table(struct domain *d, u64 *xsdt) > @@ -304,6 +346,14 @@ static int map_xsdt_table(struct domain *d, u64 *xsdt) > ( ( (size - sizeof(struct acpi_table_header) ) / > sizeof(acpi_native_uint) ) ); > > + /* map stao table */ > + map_stao_table(d, &addr); > + if(res) if ( ... ) > + return res; > + > + table_entry--; > + *table_entry = addr ; > + No space before ; > checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, table), table->length); > table->checksum = (u8)( table->checksum - checksum ); No space before the last ) Regards, -- Julien Grall