Re: [Xen-devel] [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 30 Nov 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 29/11/2016 19:08, Stefano Stabellini wrote:
> > On Mon, 28 Nov 2016, Shanker Donthineni wrote:
> > > Either we have to hide the watchdog timer section in GTDT or emulate
> > > watchdog timer block for dom0. Otherwise, system gets panic when
> > > dom0 accesses its MMIO registers. The current XEN doesn't support
> > > virtualization of watchdog timer, so hide the watchdog timer section
> > > for dom0.
> > > 
> > > Signed-off-by: Shanker Donthineni <shankerd@xxxxxxxxxxxxxx>
> > 
> > Thanks for the patch, it looks good. Just a couple of questions below.
> > 
> > 
> > >  xen/arch/arm/domain_build.c | 41
> > > +++++++++++++++++++++++++++++++++++++++++
> > >  xen/include/asm-arm/acpi.h  |  1 +
> > >  2 files changed, 42 insertions(+)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index e8a400c..611c803 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -1668,6 +1668,8 @@ static int acpi_create_xsdt(struct domain *d, struct
> > > membank tbl_add[])
> > >                             ACPI_SIG_FADT, tbl_add[TBL_FADT].start);
> > >      acpi_xsdt_modify_entry(xsdt->table_offset_entry, entry_count,
> > >                             ACPI_SIG_MADT, tbl_add[TBL_MADT].start);
> > > +    acpi_xsdt_modify_entry(xsdt->table_offset_entry, entry_count,
> > > +                           ACPI_SIG_GTDT, tbl_add[TBL_GTDT].start);
> > >      xsdt->table_offset_entry[entry_count] = tbl_add[TBL_STAO].start;
> > > 
> > >      xsdt->header.length = table_size;
> > > @@ -1718,6 +1720,41 @@ static int acpi_create_stao(struct domain *d,
> > > struct membank tbl_add[])
> > >      return 0;
> > >  }
> > > 
> > > +static int acpi_create_gtdt(struct domain *d, struct membank tbl_add[])
> > > +{
> > > +    struct acpi_table_header *table = NULL;
> > > +    struct acpi_table_gtdt *gtdt = NULL;
> > > +    u32 table_size = sizeof(struct acpi_table_gtdt);
> > > +    u32 offset = acpi_get_table_offset(tbl_add, TBL_GTDT);
> > > +    acpi_status status;
> > > +    u8 *base_ptr, checksum;
> > > +
> > > +    status = acpi_get_table(ACPI_SIG_GTDT, 0, &table);
> > > +
> > > +    if ( ACPI_FAILURE(status) )
> > > +    {
> > > +        const char *msg = acpi_format_exception(status);
> > > +
> > > +        printk("Failed to get GTDT table, %s\n", msg);
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    base_ptr = d->arch.efi_acpi_table + offset;
> > > +    ACPI_MEMCPY(base_ptr, table, sizeof(struct acpi_table_gtdt));
> > 
> > Use table_size
> > 
> > 
> > > +    gtdt = (struct acpi_table_gtdt *)base_ptr;
> > > +    gtdt->header.length = table_size;
> > > +    gtdt->platform_timer_count = 0;
> > > +    gtdt->platform_timer_offset = table_size;
> > 
> > Why table_size instead of 0? Is that the expected values when the array
> > is empty?
> 
> platform_timer_offset contains the offset to the start of the array from the
> beginning of the table. So I don't think it matters here.
> 
> Actually I would even avoid to update this parameter.

I understand that it shouldn't matter, but this kind of parameters
usually have a default value regardless. However in this instance it is
not in the spec so I guess anything should be OK.

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