| Message ID | 1423058539-26403-33-git-send-email-parth.dixit@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Parth, As for the STAO table, this is not only mapping but also creating the table. On 04/02/2015 14:02, parth.dixit@linaro.org wrote: > From: Parth Dixit <parth.dixit@linaro.org> > > xen environment table contains the grant table address,size and event , size > channel interrupt information required by dom0. > > Signed-off-by: Parth Dixit <parth.dixit@linaro.org> > --- > xen/arch/arm/arm64/acpi/arm-core.c | 52 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c > index 9fd02f9..9e9285c 100644 > --- a/xen/arch/arm/arm64/acpi/arm-core.c > +++ b/xen/arch/arm/arm64/acpi/arm-core.c > @@ -33,7 +33,6 @@ > #include <asm/cputype.h> > #include <asm/acpi.h> > #include <asm/p2m.h> > - Spurious change. > /* > * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this > * variable is still required by the ACPI core > @@ -297,6 +296,49 @@ static int map_stao_table(struct domain *d, u64 *mstao) > return 0; > } > > +static int map_xenv_table(struct domain *d, u64 *mxenv) > +{ > + u64 addr; > + u64 size; > + int res; > + u8 checksum; > + struct acpi_table_xenv *xenv=NULL; *xenv = NULL > + > + xenv = ACPI_ALLOCATE_ZEROED( sizeof(struct acpi_table_xenv) ); No space necessary after the first ( and before the last ) > + if( xenv == NULL ) > + return -ENOMEM; > + > + ACPI_MEMCPY(xenv->header.signature, ACPI_SIG_XENV, 4); > + xenv->header.length = sizeof(struct acpi_table_header)+12; Where does the 12 comes from? > + xenv->header.checksum = 0; > + ACPI_MEMCPY(xenv->header.oem_id, "XenVMM", 6); > + ACPI_MEMCPY(xenv->header.oem_table_id, "RTSMVEV8", 8); RTSMVEV8? Why? > + xenv->header.revision = 1; > + ACPI_MEMCPY(xenv->header.asl_compiler_id, "INTL", 4); > + xenv->header.asl_compiler_revision = 0x20140828; Why this compiler revision? > + xenv->gnt_start = 0x00000010000000; > + xenv->gnt_size = 0x20000; This is hardcoded. Even if you precise it in the cover letter, you should make clear in the patch that we have hardcoded. > + xenv->evt_intr = 31; > + xenv->evt_intr_flag =3; Ditto Also intr_flag = 3; and please use define rather than a value. > + size = sizeof(struct acpi_table_xenv); > + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, xenv), size); > + xenv->header.checksum = (u8)( xenv->header.checksum - checksum ); No space after the ( and before ) > + *mxenv = addr = virt_to_maddr(xenv); > + > + res = map_ram_regions(d, > + paddr_to_pfn(addr & PAGE_MASK), > + DIV_ROUND_UP(size, PAGE_SIZE), > + paddr_to_pfn(addr & PAGE_MASK)); Same remark as for the Xen Environment table (see patch #31). > + 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 > > @@ -346,6 +388,14 @@ static int map_xsdt_table(struct domain *d, u64 *xsdt) > ( ( (size - sizeof(struct acpi_table_header) ) / > sizeof(acpi_native_uint) ) ); > > + /* map xen env table */ > + res = map_xenv_table(d, &addr); > + if(res) > + return res; > + > + table_entry--; > + *table_entry = addr ; > + No space before ; > /* map stao table */ > map_stao_table(d, &addr); > if(res) > Regards,
On 5 February 2015 at 10:59, Julien Grall <julien.grall@linaro.org> wrote: > Hi Parth, > > As for the STAO table, this is not only mapping but also creating the table. > > On 04/02/2015 14:02, parth.dixit@linaro.org wrote: >> >> From: Parth Dixit <parth.dixit@linaro.org> >> >> xen environment table contains the grant table address,size and event > > > , size > >> channel interrupt information required by dom0. >> >> Signed-off-by: Parth Dixit <parth.dixit@linaro.org> >> --- >> xen/arch/arm/arm64/acpi/arm-core.c | 52 >> +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c >> b/xen/arch/arm/arm64/acpi/arm-core.c >> index 9fd02f9..9e9285c 100644 >> --- a/xen/arch/arm/arm64/acpi/arm-core.c >> +++ b/xen/arch/arm/arm64/acpi/arm-core.c >> @@ -33,7 +33,6 @@ >> #include <asm/cputype.h> >> #include <asm/acpi.h> >> #include <asm/p2m.h> >> - > > > Spurious change. > >> /* >> * We never plan to use RSDT on arm/arm64 as its deprecated in spec but >> this >> * variable is still required by the ACPI core >> @@ -297,6 +296,49 @@ static int map_stao_table(struct domain *d, u64 >> *mstao) >> return 0; >> } >> >> +static int map_xenv_table(struct domain *d, u64 *mxenv) >> +{ >> + u64 addr; >> + u64 size; >> + int res; >> + u8 checksum; >> + struct acpi_table_xenv *xenv=NULL; > > > *xenv = NULL > >> + >> + xenv = ACPI_ALLOCATE_ZEROED( sizeof(struct acpi_table_xenv) ); > > > No space necessary after the first ( and before the last ) > >> + if( xenv == NULL ) >> + return -ENOMEM; >> + >> + ACPI_MEMCPY(xenv->header.signature, ACPI_SIG_XENV, 4); >> + xenv->header.length = sizeof(struct acpi_table_header)+12; > > > Where does the 12 comes from? its total size - sizeof(struct acpi_table_header) i will remove the hardcoing, as this table does not contain variable length element i can use sizeof at compile time to determine total size. will take care in next patch >> + xenv->header.checksum = 0; >> + ACPI_MEMCPY(xenv->header.oem_id, "XenVMM", 6); >> + ACPI_MEMCPY(xenv->header.oem_table_id, "RTSMVEV8", 8); > > > RTSMVEV8? Why? Because oem id is not finalized yet >> + xenv->header.revision = 1; >> + ACPI_MEMCPY(xenv->header.asl_compiler_id, "INTL", 4); >> + xenv->header.asl_compiler_revision = 0x20140828; > > > Why this compiler revision? will remove it in next patchset, base it on the id of any existing table. >> + xenv->gnt_start = 0x00000010000000; >> + xenv->gnt_size = 0x20000; > > > This is hardcoded. Even if you precise it in the cover letter, you should > make clear in the patch that we have hardcoded. sure, will do it in next patchset >> + xenv->evt_intr = 31; >> + xenv->evt_intr_flag =3; > > > Ditto > > Also intr_flag = 3; and please use define rather than a value. > >> + size = sizeof(struct acpi_table_xenv); >> + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, xenv), size); >> + xenv->header.checksum = (u8)( xenv->header.checksum - checksum ); > > > No space after the ( and before ) > >> + *mxenv = addr = virt_to_maddr(xenv); >> + >> + res = map_ram_regions(d, >> + paddr_to_pfn(addr & PAGE_MASK), >> + DIV_ROUND_UP(size, PAGE_SIZE), >> + paddr_to_pfn(addr & PAGE_MASK)); > > > Same remark as for the Xen Environment table (see patch #31). > >> + 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 >> >> @@ -346,6 +388,14 @@ static int map_xsdt_table(struct domain *d, u64 >> *xsdt) >> ( ( (size - sizeof(struct acpi_table_header) ) / >> sizeof(acpi_native_uint) ) ); >> >> + /* map xen env table */ >> + res = map_xenv_table(d, &addr); >> + if(res) >> + return res; >> + >> + table_entry--; >> + *table_entry = addr ; >> + > > > No space before ; > >> /* map stao table */ >> map_stao_table(d, &addr); >> if(res) >> > > Regards, > > -- > Julien Grall
Hi Stefano, On 06/02/2015 01:36, Stefano Stabellini wrote: > On Wed, 4 Feb 2015, parth.dixit@linaro.org wrote: >> From: Parth Dixit <parth.dixit@linaro.org> >> >> xen environment table contains the grant table address,size and event >> channel interrupt information required by dom0. >> >> Signed-off-by: Parth Dixit <parth.dixit@linaro.org> >> --- >> xen/arch/arm/arm64/acpi/arm-core.c | 52 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c >> index 9fd02f9..9e9285c 100644 >> --- a/xen/arch/arm/arm64/acpi/arm-core.c >> +++ b/xen/arch/arm/arm64/acpi/arm-core.c >> @@ -33,7 +33,6 @@ >> #include <asm/cputype.h> >> #include <asm/acpi.h> >> #include <asm/p2m.h> >> - >> /* >> * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this >> * variable is still required by the ACPI core > > Spurious change > > >> @@ -297,6 +296,49 @@ static int map_stao_table(struct domain *d, u64 *mstao) >> return 0; >> } >> >> +static int map_xenv_table(struct domain *d, u64 *mxenv) >> +{ >> + u64 addr; >> + u64 size; >> + int res; >> + u8 checksum; >> + struct acpi_table_xenv *xenv=NULL; >> + >> + xenv = ACPI_ALLOCATE_ZEROED( sizeof(struct acpi_table_xenv) ); >> + if( xenv == NULL ) >> + return -ENOMEM; >> + >> + ACPI_MEMCPY(xenv->header.signature, ACPI_SIG_XENV, 4); >> + xenv->header.length = sizeof(struct acpi_table_header)+12; >> + xenv->header.checksum = 0; >> + ACPI_MEMCPY(xenv->header.oem_id, "XenVMM", 6); >> + ACPI_MEMCPY(xenv->header.oem_table_id, "RTSMVEV8", 8); >> + xenv->header.revision = 1; >> + ACPI_MEMCPY(xenv->header.asl_compiler_id, "INTL", 4); >> + xenv->header.asl_compiler_revision = 0x20140828; >> + xenv->gnt_start = 0x00000010000000; >> + xenv->gnt_size = 0x20000; >> + xenv->evt_intr = 31; >> + xenv->evt_intr_flag =3; >> + size = sizeof(struct acpi_table_xenv); > > As per all the other checksum calculation, I wonder whether we need a > memory barrier here. I don't see why the memory barrier is necessary, the checksum and the modification of the table is done on the same processor. But cleaning/invalidate the cache after the checksum may be required. Regards,
diff --git a/xen/arch/arm/arm64/acpi/arm-core.c b/xen/arch/arm/arm64/acpi/arm-core.c index 9fd02f9..9e9285c 100644 --- a/xen/arch/arm/arm64/acpi/arm-core.c +++ b/xen/arch/arm/arm64/acpi/arm-core.c @@ -33,7 +33,6 @@ #include <asm/cputype.h> #include <asm/acpi.h> #include <asm/p2m.h> - /* * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this * variable is still required by the ACPI core @@ -297,6 +296,49 @@ static int map_stao_table(struct domain *d, u64 *mstao) return 0; } +static int map_xenv_table(struct domain *d, u64 *mxenv) +{ + u64 addr; + u64 size; + int res; + u8 checksum; + struct acpi_table_xenv *xenv=NULL; + + xenv = ACPI_ALLOCATE_ZEROED( sizeof(struct acpi_table_xenv) ); + if( xenv == NULL ) + return -ENOMEM; + + ACPI_MEMCPY(xenv->header.signature, ACPI_SIG_XENV, 4); + xenv->header.length = sizeof(struct acpi_table_header)+12; + xenv->header.checksum = 0; + ACPI_MEMCPY(xenv->header.oem_id, "XenVMM", 6); + ACPI_MEMCPY(xenv->header.oem_table_id, "RTSMVEV8", 8); + xenv->header.revision = 1; + ACPI_MEMCPY(xenv->header.asl_compiler_id, "INTL", 4); + xenv->header.asl_compiler_revision = 0x20140828; + xenv->gnt_start = 0x00000010000000; + xenv->gnt_size = 0x20000; + xenv->evt_intr = 31; + xenv->evt_intr_flag =3; + size = sizeof(struct acpi_table_xenv); + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, xenv), size); + xenv->header.checksum = (u8)( xenv->header.checksum - checksum ); + *mxenv = addr = virt_to_maddr(xenv); + + 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 @@ -346,6 +388,14 @@ static int map_xsdt_table(struct domain *d, u64 *xsdt) ( ( (size - sizeof(struct acpi_table_header) ) / sizeof(acpi_native_uint) ) ); + /* map xen env table */ + res = map_xenv_table(d, &addr); + if(res) + return res; + + table_entry--; + *table_entry = addr ; + /* map stao table */ map_stao_table(d, &addr); if(res)