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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 6 Feb 2015, Julien Grall wrote:
> 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@xxxxxxxxxx>
> > > > wrote:
> > > > > On 04/02/2015 14:02, parth.dixit@xxxxxxxxxx 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.

This would be terrible!
I was assuming that all ACPI tables addresses are page aligned and sizes
are page aligned too. Is that true?
If not, is unacceptable to map them the entire page RW in Dom0.


> The Read-Only solution would avoid DOM0 to write in the such zone and mess up
> the hypervisor by mistake.

It is the minimum that we can do.


> 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.

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