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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

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