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

[Julien Grall <julien.grall@xxxxxxx>]

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.

Regards,

--
Julien Grall

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