Re: [Xen-devel] [PATCH RFC 32/35] arm : acpi map xen environment table to dom0

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

On 5 February 2015 at 10:59, Julien Grall <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxx wrote:
>>
>> From: Parth Dixit <parth.dixit@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> ---
>>   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

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