| Message ID | 1423058539-26403-32-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 <parth.dixit@linaro.org> > > hide UART used by xen by indicating it in STAO table > and map it to dom0 > > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > --- > 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,
On 5 February 2015 at 10:54, Julien Grall <julien.grall@linaro.org> wrote: > Hi Parth, > > You don't only map the status override table. You also create it. > I would update the commit title to reflect it. > Sure, will take care in next patchset > On 04/02/2015 14:02, parth.dixit@linaro.org wrote: >> >> From: Parth Dixit <parth.dixit@linaro.org> >> >> hide UART used by xen by indicating it in STAO table >> and map it to dom0 >> >> Signed-off-by: Parth Dixit <parth.dixit@linaro.org> >> --- >> 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? yes, but its not clear what should be used as xen oem id is not finalized yet. >> + 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? from the spec >> + stao->uart = 1; > > > What about the devpath? there is no table for devpath yet, it would require table specific handling (mostly parsing) and it can then be updated in it, or maybe i can create separate structure which can be used here but element would be added at runtime for each table. what do you think? >> + 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. Sure, i'll map them to RO in next patchset. >> + 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
Hi Parth, On 05/02/2015 18:57, Parth Dixit wrote: > On 5 February 2015 at 10:54, Julien Grall <julien.grall@linaro.org> wrote: >> On 04/02/2015 14:02, parth.dixit@linaro.org wrote: >>> + 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? > yes, but its not clear what should be used as xen oem id is not finalized yet. I though we decided a name on the email, what is missing? >>> + 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? > from the spec What do you mean? I didn't know that the spec requires a specific compiler version... IHMO, this would be wrong. >>> + stao->uart = 1; >> >> >> What about the devpath? > there is no table for devpath yet, it would require table specific handling > (mostly parsing) and it can then be updated in it, or maybe i can > create separate structure > which can be used here but element would be added at runtime for each table. > what do you think? The devpath is a list of device blacklisted by path, right? If so, the comment on the field devpath is wrong. Also, it's defined as u8[1], which is very confusing. >>> + 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. > Sure, i'll map them to RO in next patchset. I didn't say this is the right solution ;). It was something I recall from a discussion we had few months ago. So before using this solution, can anyone (re-)confirm me that the ACPI tables should not be modified by the guest? If so, this should also be written somewhere for documentation purpose. It may save time in the future :). Regards,
On 06/02/2015 01:39, Stefano Stabellini wrote: > On Thu, 5 Feb 2015, Julien Grall wrote: >> Hi Parth, >> >> On 05/02/2015 18:57, Parth Dixit wrote: >>> On 5 February 2015 at 10:54, Julien Grall <julien.grall@linaro.org> wrote: >>>> On 04/02/2015 14:02, parth.dixit@linaro.org wrote: >>>>> + 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? >>> yes, but its not clear what should be used as xen oem id is not finalized >>> yet. >> >> I though we decided a name on the email, what is missing? >> >>>>> + 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? >>> from the spec >> >> What do you mean? I didn't know that the spec requires a specific compiler >> version... IHMO, this would be wrong. >> >>>>> + stao->uart = 1; >>>> >>>> >>>> What about the devpath? >>> there is no table for devpath yet, it would require table specific handling >>> (mostly parsing) and it can then be updated in it, or maybe i can >>> create separate structure >>> which can be used here but element would be added at runtime for each table. >>> what do you think? >> >> The devpath is a list of device blacklisted by path, right? If so, the comment >> on the field devpath is wrong. Also, it's defined as u8[1], which is very >> confusing. >> >>>>> + 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. >>> Sure, i'll map them to RO in next patchset. >> >> I didn't say this is the right solution ;). It was something I recall from a >> discussion we had few months ago. >> >> So before using this solution, can anyone (re-)confirm me that the ACPI tables >> should not be modified by the guest? If so, this should also be written >> somewhere for documentation purpose. It may save time in the future :). > > At this point we are completely trusting dom0 with the ACPI tables, I am > not sure how much we would gain by mapping the tables RO. I agree that we trust DOM0... but in this specific case, because of the page-alignment requirement, we may expose Xen memory/Guest data. The Read-Only solution would avoid DOM0 to write in the such zone and mess up the hypervisor by mistake. FYI, we had a thread about it a couple of months ago. And it was confirmed that ACPI is RO at least from guest POV. (I could re-forward you the mail if necessary). So it's better to map Read-Only just in case. Regards,
Hi Ian, On 05/02/2015 19:47, Ian Campbell wrote: > On Thu, 2015-02-05 at 16:27 +0530, Parth Dixit wrote: >>>> + 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? >> yes, but its not clear what should be used as xen oem id is not finalized yet. > > Are these IDs the ones defined for x86 in > tools/firmware/hvmloader/acpi/acpi2_0.h: > #define ACPI_OEM_ID "Xen" > #define ACPI_OEM_TABLE_ID "HVM" > #define ACPI_OEM_REVISION 0 > > #define ACPI_CREATOR_ID ASCII32('H','V','M','L') /* HVMLoader */ > #define ACPI_CREATOR_REVISION 0 > > ? If so we should reuse them, although maybe not OEM_TABLE_ID and > CREATOR_ID since those are x86/HVM specific. I didn't know that HVMLoader was using one. "XenVMM" was decided for ARM (see see http://wiki.xenproject.org/mediawiki/images/c/c4/Xen-environment-table.pdf). Although, it would be good to have a single OEM ID for Xen project. > What is the process for assigning those? Given our unique OEM_ID are we > allowed to just declare them ourselves? Stefano sent an email to the ACPI guys to know the process. I guess the x86 one has not been declared? Regards,
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 = ACPI_ALLOCATE_ZEROED(sizeof(struct acpi_table_stao) ); + if( stao == NULL ) + return -ENOMEM; + + ACPI_MEMCPY(stao->header.signature,ACPI_SIG_STAO, 4); + 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); + stao->header.revision = 1; + ACPI_MEMCPY(stao->header.asl_compiler_id, "INTL", 4); + stao->header.asl_compiler_revision = 0x20140828; + stao->uart = 1; + size = sizeof(struct acpi_table_stao); + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, stao), size); + stao->header.checksum = (u8)( stao->header.checksum - checksum ); + *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)); + 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) + return res; + + table_entry--; + *table_entry = addr ; + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, table), table->length); table->checksum = (u8)( table->checksum - checksum ); return 0;