Re: [Xen-devel] [PATCH RFC 31/35] arm : acpi map status override table to dom0

[Parth Dixit <parth.dixit@xxxxxxxxxx>]

On 5 February 2015 at 10:54, Julien Grall <julien.grall@xxxxxxxxxx> 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@xxxxxxxxxx wrote:
>>
>> From: Parth Dixit <parth.dixit@xxxxxxxxxx>
>>
>> hide UART used by xen by indicating it in STAO table
>> and map it to dom0
>>
>> Signed-off-by: Parth Dixit <parth.dixit@xxxxxxxxxx>
>> ---
>>   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

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