From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH RFC 31/35] arm : acpi map status override table to dom0 Date: Fri, 06 Feb 2015 08:54:24 +0800 Message-ID: <54D410C0.8060603@linaro.org> References: <1423058539-26403-1-git-send-email-parth.dixit@linaro.org> <1423058539-26403-32-git-send-email-parth.dixit@linaro.org> <54D2FE96.9090900@linaro.org> <54D380A6.5030308@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: Ian Campbell , tim@xen.org, xen-devel , Stefano Stabellini , Jan Beulich , Parth Dixit , Christoffer Dall List-Id: xen-devel@lists.xenproject.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 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