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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

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