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

[Julien Grall <julien.grall@linaro.org>]

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,

-- 
Julien Grall