* [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
@ 2016-11-29 2:59 Shanker Donthineni
2016-11-29 10:40 ` Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Shanker Donthineni @ 2016-11-29 2:59 UTC (permalink / raw)
To: xen-devel, Julien Grall, Stefano Stabellini
Cc: Shanker Donthineni, Vikram Sethi, Campbell Sean
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@codeaurora.org>
---
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));
+
+ gtdt = (struct acpi_table_gtdt *)base_ptr;
+ gtdt->header.length = table_size;
+ gtdt->platform_timer_count = 0;
+ gtdt->platform_timer_offset = table_size;
+ checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, gtdt), table_size);
+ gtdt->header.checksum -= checksum;
+
+ tbl_add[TBL_GTDT].start = d->arch.efi_acpi_gpa + offset;
+ tbl_add[TBL_GTDT].size = table_size;
+
+ return 0;
+}
+
static int acpi_create_madt(struct domain *d, struct membank tbl_add[])
{
struct acpi_table_header *table = NULL;
@@ -1909,6 +1946,10 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
if ( rc != 0 )
return rc;
+ rc = acpi_create_gtdt(d, tbl_add);
+ if ( rc != 0 )
+ return rc;
+
rc = acpi_create_xsdt(d, tbl_add);
if ( rc != 0 )
return rc;
diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
index 9f954d3..214511c 100644
--- a/xen/include/asm-arm/acpi.h
+++ b/xen/include/asm-arm/acpi.h
@@ -36,6 +36,7 @@ typedef enum {
TBL_FADT,
TBL_MADT,
TBL_STAO,
+ TBL_GTDT,
TBL_XSDT,
TBL_RSDP,
TBL_EFIT,
--
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 2:59 [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0 Shanker Donthineni
@ 2016-11-29 10:40 ` Jan Beulich
2016-11-29 11:38 ` Roger Pau Monne
2016-11-29 19:08 ` Stefano Stabellini
2016-11-30 10:29 ` Julien Grall
2 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2016-11-29 10:40 UTC (permalink / raw)
To: Julien Grall, Roger Pau Monne, Shanker Donthineni, Stefano Stabellini
Cc: xen-devel, Vikram Sethi, Campbell Sean
>>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
The mere need for a patch like this is, to me at least, a pretty
clear indication that such black listing models don't work well
(and using white listing instead would likely be too restrictive).
As the same is about to become an issue on x86 too (with
PVHv2) I think there's a need to reconsider how to deal with
ACPI with other than a traditional PV Dom0, namely whether
(a) it wouldn't make sense to expect a little more awareness
by the Dom0 kernel, and
(b) whether we shouldn't unconditionally hand everything to
Dom0 which Xen doesn't explicitly make use of itself (implying
that MMIO regions get suitably prepared either during boot
or on demand).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 10:40 ` Jan Beulich
@ 2016-11-29 11:38 ` Roger Pau Monne
2016-11-29 11:44 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monne @ 2016-11-29 11:38 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Julien Grall,
xen-devel, Shanker Donthineni
On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
> >>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
>
> The mere need for a patch like this is, to me at least, a pretty
> clear indication that such black listing models don't work well
> (and using white listing instead would likely be too restrictive).
> As the same is about to become an issue on x86 too (with
> PVHv2) I think there's a need to reconsider how to deal with
> ACPI with other than a traditional PV Dom0, namely whether
> (a) it wouldn't make sense to expect a little more awareness
> by the Dom0 kernel, and
> (b) whether we shouldn't unconditionally hand everything to
> Dom0 which Xen doesn't explicitly make use of itself (implying
> that MMIO regions get suitably prepared either during boot
> or on demand).
I cannot speak about ARM, but given that support for ACPI was added in the
previous release, and is AFAIK still marked as experimental, I expect such fixes
to be normal.
Then, speaking about the general picture and approach, (a) is mostly true on x86
PVHv2 Dom0, while I don't see the point in also doing (b). Regarding (a), it's
quite clear that some awareness by Dom0 is going to be needed when running as
Dom0 (report MCFG regions and the poweroff sequence at least) as much as I
would like to avoid that. Regarding (b), there are tables that we already know
are completely useless to Dom0 ATM, like DMAR, SRAT, SLIT... IMHO, those should
not be presented to Dom0 at all, because if they are presented now they should
be ignored by Dom0, and if we ever want to somehow for example report NUMA
topology to Dom0, we would be screwed.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 11:38 ` Roger Pau Monne
@ 2016-11-29 11:44 ` Jan Beulich
2016-11-29 12:09 ` Roger Pau Monne
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2016-11-29 11:44 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Julien Grall,
xen-devel, Shanker Donthineni
>>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
> On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
>> >>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
>>
>> The mere need for a patch like this is, to me at least, a pretty
>> clear indication that such black listing models don't work well
>> (and using white listing instead would likely be too restrictive).
>> As the same is about to become an issue on x86 too (with
>> PVHv2) I think there's a need to reconsider how to deal with
>> ACPI with other than a traditional PV Dom0, namely whether
>> (a) it wouldn't make sense to expect a little more awareness
>> by the Dom0 kernel, and
>> (b) whether we shouldn't unconditionally hand everything to
>> Dom0 which Xen doesn't explicitly make use of itself (implying
>> that MMIO regions get suitably prepared either during boot
>> or on demand).
>
> I cannot speak about ARM, but given that support for ACPI was added in the
> previous release, and is AFAIK still marked as experimental, I expect such fixes
> to be normal.
Yes and no. My issue here is that the need for such fixes may arise
with new additions to the ACPI spec, and that would easily end up
in a maintenance nightmare.
> Then, speaking about the general picture and approach, (a) is mostly true on x86
> PVHv2 Dom0, while I don't see the point in also doing (b). Regarding (a), it's
> quite clear that some awareness by Dom0 is going to be needed when running as
> Dom0 (report MCFG regions and the poweroff sequence at least) as much as I
> would like to avoid that. Regarding (b), there are tables that we already know
> are completely useless to Dom0 ATM, like DMAR, SRAT, SLIT... IMHO, those should
> not be presented to Dom0 at all, because if they are presented now they should
> be ignored by Dom0, and if we ever want to somehow for example report NUMA
> topology to Dom0, we would be screwed.
The tables you name really fall into the "used by Xen" category.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 11:44 ` Jan Beulich
@ 2016-11-29 12:09 ` Roger Pau Monne
2016-11-29 13:07 ` Jan Beulich
2016-11-29 20:19 ` Andrew Cooper
0 siblings, 2 replies; 21+ messages in thread
From: Roger Pau Monne @ 2016-11-29 12:09 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Julien Grall,
xen-devel, Shanker Donthineni
On Tue, Nov 29, 2016 at 04:44:18AM -0700, Jan Beulich wrote:
> >>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
> > On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
> >> >>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
> >>
> >> The mere need for a patch like this is, to me at least, a pretty
> >> clear indication that such black listing models don't work well
> >> (and using white listing instead would likely be too restrictive).
> >> As the same is about to become an issue on x86 too (with
> >> PVHv2) I think there's a need to reconsider how to deal with
> >> ACPI with other than a traditional PV Dom0, namely whether
> >> (a) it wouldn't make sense to expect a little more awareness
> >> by the Dom0 kernel, and
> >> (b) whether we shouldn't unconditionally hand everything to
> >> Dom0 which Xen doesn't explicitly make use of itself (implying
> >> that MMIO regions get suitably prepared either during boot
> >> or on demand).
> >
> > I cannot speak about ARM, but given that support for ACPI was added in the
> > previous release, and is AFAIK still marked as experimental, I expect such fixes
> > to be normal.
>
> Yes and no. My issue here is that the need for such fixes may arise
> with new additions to the ACPI spec, and that would easily end up
> in a maintenance nightmare.
Well, if we simply pass everything to Dom0, then Dom0 would need to know what it
can use and what it cannot use, so you are just moving the problem away from Xen
and into every Dom0 OS, in which case it's worse because we then need to fixup
all the Dom0 OSes that we support.
Some entity either Dom0 or Xen needs to know which tables it can use and which
tables it cannot use, and if we do this in Xen we avoid having to put all this
logic in every Dom0 kernel.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 12:09 ` Roger Pau Monne
@ 2016-11-29 13:07 ` Jan Beulich
2016-11-29 14:28 ` Roger Pau Monne
2016-11-29 20:19 ` Andrew Cooper
1 sibling, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2016-11-29 13:07 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Julien Grall,
xen-devel, Shanker Donthineni
>>> On 29.11.16 at 13:09, <roger.pau@citrix.com> wrote:
> On Tue, Nov 29, 2016 at 04:44:18AM -0700, Jan Beulich wrote:
>> >>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
>> > On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
>> >> >>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
>> >>
>> >> The mere need for a patch like this is, to me at least, a pretty
>> >> clear indication that such black listing models don't work well
>> >> (and using white listing instead would likely be too restrictive).
>> >> As the same is about to become an issue on x86 too (with
>> >> PVHv2) I think there's a need to reconsider how to deal with
>> >> ACPI with other than a traditional PV Dom0, namely whether
>> >> (a) it wouldn't make sense to expect a little more awareness
>> >> by the Dom0 kernel, and
>> >> (b) whether we shouldn't unconditionally hand everything to
>> >> Dom0 which Xen doesn't explicitly make use of itself (implying
>> >> that MMIO regions get suitably prepared either during boot
>> >> or on demand).
>> >
>> > I cannot speak about ARM, but given that support for ACPI was added in the
>> > previous release, and is AFAIK still marked as experimental, I expect such fixes
>> > to be normal.
>>
>> Yes and no. My issue here is that the need for such fixes may arise
>> with new additions to the ACPI spec, and that would easily end up
>> in a maintenance nightmare.
>
> Well, if we simply pass everything to Dom0, then Dom0 would need to know what it
> can use and what it cannot use, so you are just moving the problem away from Xen
> and into every Dom0 OS, in which case it's worse because we then need to fixup
> all the Dom0 OSes that we support.
>
> Some entity either Dom0 or Xen needs to know which tables it can use and which
> tables it cannot use, and if we do this in Xen we avoid having to put all this
> logic in every Dom0 kernel.
Well, that's how it is supposed to work anyway, the more that we're
not talking about functionality at table granularity: The patch here
is about just some piece of a table, and if you think about e.g. PM
timer, that's also something which doesn't come with its own table,
yet Dom0 has to keep its hands off. Hence I think this is better
viewed the other way around: Dom0 should not use what isn't
explicitly or from an abstract perspective fine to use. Arguably
things like watchdog timers may be a gray area, as they might be
useful to both, and it might also be possible for Dom0 to use one if
Xen (perhaps dynamically, e.g. due to some command line option
decided not use it. Yet in such a case Dom0 should ask for Xen's
permission rather than blindly using it.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 13:07 ` Jan Beulich
@ 2016-11-29 14:28 ` Roger Pau Monne
2016-11-29 18:57 ` Stefano Stabellini
0 siblings, 1 reply; 21+ messages in thread
From: Roger Pau Monne @ 2016-11-29 14:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Julien Grall,
xen-devel, Shanker Donthineni
On Tue, Nov 29, 2016 at 06:07:36AM -0700, Jan Beulich wrote:
> >>> On 29.11.16 at 13:09, <roger.pau@citrix.com> wrote:
> > On Tue, Nov 29, 2016 at 04:44:18AM -0700, Jan Beulich wrote:
> >> >>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
> >> > On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
> >> >> >>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
> >> >>
> >> >> The mere need for a patch like this is, to me at least, a pretty
> >> >> clear indication that such black listing models don't work well
> >> >> (and using white listing instead would likely be too restrictive).
> >> >> As the same is about to become an issue on x86 too (with
> >> >> PVHv2) I think there's a need to reconsider how to deal with
> >> >> ACPI with other than a traditional PV Dom0, namely whether
> >> >> (a) it wouldn't make sense to expect a little more awareness
> >> >> by the Dom0 kernel, and
> >> >> (b) whether we shouldn't unconditionally hand everything to
> >> >> Dom0 which Xen doesn't explicitly make use of itself (implying
> >> >> that MMIO regions get suitably prepared either during boot
> >> >> or on demand).
> >> >
> >> > I cannot speak about ARM, but given that support for ACPI was added in the
> >> > previous release, and is AFAIK still marked as experimental, I expect such fixes
> >> > to be normal.
> >>
> >> Yes and no. My issue here is that the need for such fixes may arise
> >> with new additions to the ACPI spec, and that would easily end up
> >> in a maintenance nightmare.
> >
> > Well, if we simply pass everything to Dom0, then Dom0 would need to know what it
> > can use and what it cannot use, so you are just moving the problem away from Xen
> > and into every Dom0 OS, in which case it's worse because we then need to fixup
> > all the Dom0 OSes that we support.
> >
> > Some entity either Dom0 or Xen needs to know which tables it can use and which
> > tables it cannot use, and if we do this in Xen we avoid having to put all this
> > logic in every Dom0 kernel.
>
> Well, that's how it is supposed to work anyway, the more that we're
> not talking about functionality at table granularity: The patch here
> is about just some piece of a table, and if you think about e.g. PM
> timer, that's also something which doesn't come with its own table,
> yet Dom0 has to keep its hands off.
I would say that everything that Xen can fix from static tables, Xen should do
it. Then there are pieces that simply Xen cannot get it's hands on, because the
description is in dynamic tables, and there we have to play tricks either with
Dom0 or with ACPI overwrites like the STAO (which is under our control and we
can modify it's specification at will).
> Hence I think this is better
> viewed the other way around: Dom0 should not use what isn't
> explicitly or from an abstract perspective fine to use. Arguably
> things like watchdog timers may be a gray area, as they might be
> useful to both, and it might also be possible for Dom0 to use one if
> Xen (perhaps dynamically, e.g. due to some command line option
> decided not use it. Yet in such a case Dom0 should ask for Xen's
> permission rather than blindly using it.
IMHO, the best way to ask for such permissions is to fixup the tables so that
Dom0 knows whether those devices/functionality can be used or not. Creating an
out-of-band method to delivery this information when there's already a native
way to do it (and can be implemented in Xen) is sub-optimal.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 14:28 ` Roger Pau Monne
@ 2016-11-29 18:57 ` Stefano Stabellini
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Stabellini @ 2016-11-29 18:57 UTC (permalink / raw)
To: Roger Pau Monne
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Julien Grall,
Jan Beulich, xen-devel, Shanker Donthineni
On Tue, 29 Nov 2016, Roger Pau Monne wrote:
> On Tue, Nov 29, 2016 at 06:07:36AM -0700, Jan Beulich wrote:
> > >>> On 29.11.16 at 13:09, <roger.pau@citrix.com> wrote:
> > > On Tue, Nov 29, 2016 at 04:44:18AM -0700, Jan Beulich wrote:
> > >> >>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
> > >> > On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
> > >> >> >>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
> > >> >>
> > >> >> The mere need for a patch like this is, to me at least, a pretty
> > >> >> clear indication that such black listing models don't work well
> > >> >> (and using white listing instead would likely be too restrictive).
> > >> >> As the same is about to become an issue on x86 too (with
> > >> >> PVHv2) I think there's a need to reconsider how to deal with
> > >> >> ACPI with other than a traditional PV Dom0, namely whether
> > >> >> (a) it wouldn't make sense to expect a little more awareness
> > >> >> by the Dom0 kernel, and
> > >> >> (b) whether we shouldn't unconditionally hand everything to
> > >> >> Dom0 which Xen doesn't explicitly make use of itself (implying
> > >> >> that MMIO regions get suitably prepared either during boot
> > >> >> or on demand).
> > >> >
> > >> > I cannot speak about ARM, but given that support for ACPI was added in the
> > >> > previous release, and is AFAIK still marked as experimental, I expect such fixes
> > >> > to be normal.
> > >>
> > >> Yes and no. My issue here is that the need for such fixes may arise
> > >> with new additions to the ACPI spec, and that would easily end up
> > >> in a maintenance nightmare.
We do need to keep an eye on ASWG activity going forward. Fortunately
some of the people in the Xen community work for companies that are
members and can do it.
> > > Well, if we simply pass everything to Dom0, then Dom0 would need to know what it
> > > can use and what it cannot use, so you are just moving the problem away from Xen
> > > and into every Dom0 OS, in which case it's worse because we then need to fixup
> > > all the Dom0 OSes that we support.
> > >
> > > Some entity either Dom0 or Xen needs to know which tables it can use and which
> > > tables it cannot use, and if we do this in Xen we avoid having to put all this
> > > logic in every Dom0 kernel.
> >
> > Well, that's how it is supposed to work anyway, the more that we're
> > not talking about functionality at table granularity: The patch here
> > is about just some piece of a table, and if you think about e.g. PM
> > timer, that's also something which doesn't come with its own table,
> > yet Dom0 has to keep its hands off.
>
> I would say that everything that Xen can fix from static tables, Xen should do
> it. Then there are pieces that simply Xen cannot get it's hands on, because the
> description is in dynamic tables, and there we have to play tricks either with
> Dom0 or with ACPI overwrites like the STAO (which is under our control and we
> can modify it's specification at will).
That's exactly right.
> > Hence I think this is better
> > viewed the other way around: Dom0 should not use what isn't
> > explicitly or from an abstract perspective fine to use. Arguably
> > things like watchdog timers may be a gray area, as they might be
> > useful to both, and it might also be possible for Dom0 to use one if
> > Xen (perhaps dynamically, e.g. due to some command line option
> > decided not use it. Yet in such a case Dom0 should ask for Xen's
> > permission rather than blindly using it.
>
> IMHO, the best way to ask for such permissions is to fixup the tables so that
> Dom0 knows whether those devices/functionality can be used or not. Creating an
> out-of-band method to delivery this information when there's already a native
> way to do it (and can be implemented in Xen) is sub-optimal.
I completely agree.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 2:59 [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0 Shanker Donthineni
2016-11-29 10:40 ` Jan Beulich
@ 2016-11-29 19:08 ` Stefano Stabellini
2016-11-30 9:47 ` Julien Grall
2016-11-30 10:29 ` Julien Grall
2 siblings, 1 reply; 21+ messages in thread
From: Stefano Stabellini @ 2016-11-29 19:08 UTC (permalink / raw)
To: Shanker Donthineni
Cc: Julien Grall, xen-devel, Stefano Stabellini, Vikram Sethi, Campbell Sean
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@codeaurora.org>
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?
> + checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, gtdt), table_size);
> + gtdt->header.checksum -= checksum;
> +
> + tbl_add[TBL_GTDT].start = d->arch.efi_acpi_gpa + offset;
> + tbl_add[TBL_GTDT].size = table_size;
> +
> + return 0;
> +}
> +
> static int acpi_create_madt(struct domain *d, struct membank tbl_add[])
> {
> struct acpi_table_header *table = NULL;
> @@ -1909,6 +1946,10 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
> if ( rc != 0 )
> return rc;
>
> + rc = acpi_create_gtdt(d, tbl_add);
> + if ( rc != 0 )
> + return rc;
> +
> rc = acpi_create_xsdt(d, tbl_add);
> if ( rc != 0 )
> return rc;
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index 9f954d3..214511c 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -36,6 +36,7 @@ typedef enum {
> TBL_FADT,
> TBL_MADT,
> TBL_STAO,
> + TBL_GTDT,
> TBL_XSDT,
> TBL_RSDP,
> TBL_EFIT,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 12:09 ` Roger Pau Monne
2016-11-29 13:07 ` Jan Beulich
@ 2016-11-29 20:19 ` Andrew Cooper
2016-11-30 8:54 ` Jan Beulich
1 sibling, 1 reply; 21+ messages in thread
From: Andrew Cooper @ 2016-11-29 20:19 UTC (permalink / raw)
To: Roger Pau Monne, Jan Beulich
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Julien Grall,
xen-devel, Shanker Donthineni
On 29/11/16 12:09, Roger Pau Monne wrote:
> On Tue, Nov 29, 2016 at 04:44:18AM -0700, Jan Beulich wrote:
>>>>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
>>> On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
>>>>>>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
>>>> The mere need for a patch like this is, to me at least, a pretty
>>>> clear indication that such black listing models don't work well
>>>> (and using white listing instead would likely be too restrictive).
>>>> As the same is about to become an issue on x86 too (with
>>>> PVHv2) I think there's a need to reconsider how to deal with
>>>> ACPI with other than a traditional PV Dom0, namely whether
>>>> (a) it wouldn't make sense to expect a little more awareness
>>>> by the Dom0 kernel, and
>>>> (b) whether we shouldn't unconditionally hand everything to
>>>> Dom0 which Xen doesn't explicitly make use of itself (implying
>>>> that MMIO regions get suitably prepared either during boot
>>>> or on demand).
>>> I cannot speak about ARM, but given that support for ACPI was added in the
>>> previous release, and is AFAIK still marked as experimental, I expect such fixes
>>> to be normal.
>> Yes and no. My issue here is that the need for such fixes may arise
>> with new additions to the ACPI spec, and that would easily end up
>> in a maintenance nightmare.
> Well, if we simply pass everything to Dom0, then Dom0 would need to know what it
> can use and what it cannot use, so you are just moving the problem away from Xen
> and into every Dom0 OS, in which case it's worse because we then need to fixup
> all the Dom0 OSes that we support.
>
> Some entity either Dom0 or Xen needs to know which tables it can use and which
> tables it cannot use, and if we do this in Xen we avoid having to put all this
> logic in every Dom0 kernel.
Dom0 should have nothing which isn't explicitly whitelisted as ok by Xen.
The culture of the blacklist model which has existing for much of Xen's
history is a broken concept.
I have spent a long time trying to undo the damage caused by this
culture with the CPUID levelling work, and there is a lot more to do
(e.g. MSR levelling) which needs to be done at some point. While these
are examples which typically crash guests, the same whitelist/blacklist
arguments apply to situations like this.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 20:19 ` Andrew Cooper
@ 2016-11-30 8:54 ` Jan Beulich
2016-11-30 10:14 ` Roger Pau Monne
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2016-11-30 8:54 UTC (permalink / raw)
To: Andrew Cooper
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, JulienGrall,
xen-devel, Shanker Donthineni, Roger Pau Monne
>>> On 29.11.16 at 21:19, <andrew.cooper3@citrix.com> wrote:
> On 29/11/16 12:09, Roger Pau Monne wrote:
>> On Tue, Nov 29, 2016 at 04:44:18AM -0700, Jan Beulich wrote:
>>>>>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
>>>> On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
>>>>>>>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
>>>>> The mere need for a patch like this is, to me at least, a pretty
>>>>> clear indication that such black listing models don't work well
>>>>> (and using white listing instead would likely be too restrictive).
>>>>> As the same is about to become an issue on x86 too (with
>>>>> PVHv2) I think there's a need to reconsider how to deal with
>>>>> ACPI with other than a traditional PV Dom0, namely whether
>>>>> (a) it wouldn't make sense to expect a little more awareness
>>>>> by the Dom0 kernel, and
>>>>> (b) whether we shouldn't unconditionally hand everything to
>>>>> Dom0 which Xen doesn't explicitly make use of itself (implying
>>>>> that MMIO regions get suitably prepared either during boot
>>>>> or on demand).
>>>> I cannot speak about ARM, but given that support for ACPI was added in the
>>>> previous release, and is AFAIK still marked as experimental, I expect such fixes
>>>> to be normal.
>>> Yes and no. My issue here is that the need for such fixes may arise
>>> with new additions to the ACPI spec, and that would easily end up
>>> in a maintenance nightmare.
>> Well, if we simply pass everything to Dom0, then Dom0 would need to know what it
>> can use and what it cannot use, so you are just moving the problem away from Xen
>> and into every Dom0 OS, in which case it's worse because we then need to fixup
>> all the Dom0 OSes that we support.
>>
>> Some entity either Dom0 or Xen needs to know which tables it can use and which
>> tables it cannot use, and if we do this in Xen we avoid having to put all this
>> logic in every Dom0 kernel.
>
> Dom0 should have nothing which isn't explicitly whitelisted as ok by Xen.
>
> The culture of the blacklist model which has existing for much of Xen's
> history is a broken concept.
>
> I have spent a long time trying to undo the damage caused by this
> culture with the CPUID levelling work, and there is a lot more to do
> (e.g. MSR levelling) which needs to be done at some point. While these
> are examples which typically crash guests, the same whitelist/blacklist
> arguments apply to situations like this.
All understood. Still I'm afraid using white listing for ACPI tables is
going to be functionally more limiting than doing do for e.g. CPUID,
as it's going to be hard to ever get a complete picture of all tables,
namely including OEM ones. Not to speak of possible references
between tables (or from AML to static tables, an example of which
we even have in the ACPI tables we build for HVM guests, where
the processor objects reference MADT).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 19:08 ` Stefano Stabellini
@ 2016-11-30 9:47 ` Julien Grall
2016-11-30 18:44 ` Stefano Stabellini
0 siblings, 1 reply; 21+ messages in thread
From: Julien Grall @ 2016-11-30 9:47 UTC (permalink / raw)
To: Stefano Stabellini, Shanker Donthineni
Cc: xen-devel, Vikram Sethi, Campbell Sean
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@codeaurora.org>
>
> 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@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 8:54 ` Jan Beulich
@ 2016-11-30 10:14 ` Roger Pau Monne
0 siblings, 0 replies; 21+ messages in thread
From: Roger Pau Monne @ 2016-11-30 10:14 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Vikram Sethi, Campbell Sean, Andrew Cooper,
JulienGrall, xen-devel, Shanker Donthineni
On Wed, Nov 30, 2016 at 01:54:00AM -0700, Jan Beulich wrote:
> >>> On 29.11.16 at 21:19, <andrew.cooper3@citrix.com> wrote:
> > On 29/11/16 12:09, Roger Pau Monne wrote:
> >> On Tue, Nov 29, 2016 at 04:44:18AM -0700, Jan Beulich wrote:
> >>>>>> On 29.11.16 at 12:38, <roger.pau@citrix.com> wrote:
> >>>> On Tue, Nov 29, 2016 at 03:40:37AM -0700, Jan Beulich wrote:
> >>>>>>>> On 29.11.16 at 03:59, <shankerd@codeaurora.org> 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@codeaurora.org>
> >>>>> The mere need for a patch like this is, to me at least, a pretty
> >>>>> clear indication that such black listing models don't work well
> >>>>> (and using white listing instead would likely be too restrictive).
> >>>>> As the same is about to become an issue on x86 too (with
> >>>>> PVHv2) I think there's a need to reconsider how to deal with
> >>>>> ACPI with other than a traditional PV Dom0, namely whether
> >>>>> (a) it wouldn't make sense to expect a little more awareness
> >>>>> by the Dom0 kernel, and
> >>>>> (b) whether we shouldn't unconditionally hand everything to
> >>>>> Dom0 which Xen doesn't explicitly make use of itself (implying
> >>>>> that MMIO regions get suitably prepared either during boot
> >>>>> or on demand).
> >>>> I cannot speak about ARM, but given that support for ACPI was added in the
> >>>> previous release, and is AFAIK still marked as experimental, I expect such fixes
> >>>> to be normal.
> >>> Yes and no. My issue here is that the need for such fixes may arise
> >>> with new additions to the ACPI spec, and that would easily end up
> >>> in a maintenance nightmare.
> >> Well, if we simply pass everything to Dom0, then Dom0 would need to know what it
> >> can use and what it cannot use, so you are just moving the problem away from Xen
> >> and into every Dom0 OS, in which case it's worse because we then need to fixup
> >> all the Dom0 OSes that we support.
> >>
> >> Some entity either Dom0 or Xen needs to know which tables it can use and which
> >> tables it cannot use, and if we do this in Xen we avoid having to put all this
> >> logic in every Dom0 kernel.
> >
> > Dom0 should have nothing which isn't explicitly whitelisted as ok by Xen.
> >
> > The culture of the blacklist model which has existing for much of Xen's
> > history is a broken concept.
> >
> > I have spent a long time trying to undo the damage caused by this
> > culture with the CPUID levelling work, and there is a lot more to do
> > (e.g. MSR levelling) which needs to be done at some point. While these
> > are examples which typically crash guests, the same whitelist/blacklist
> > arguments apply to situations like this.
>
> All understood. Still I'm afraid using white listing for ACPI tables is
> going to be functionally more limiting than doing do for e.g. CPUID,
> as it's going to be hard to ever get a complete picture of all tables,
> namely including OEM ones. Not to speak of possible references
> between tables (or from AML to static tables, an example of which
> we even have in the ACPI tables we build for HVM guests, where
> the processor objects reference MADT).
How this is implemented now on x86 allows to turn it into a whitelist with a
couple of line changes.
Now, I don't really have a strong opinion either way, both implementations have
it's downsides. With blacklisting we risk passing tables to the guest that
contain information that breaks the hardware description (either because the
hardware is not available or not properly passed-through by Xen). While with
whitelisting, we risk missing some OEM tables or tables referenced from AML. I
guess we can move this discussion to the PVHv2 Dom0 thread.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-29 2:59 [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0 Shanker Donthineni
2016-11-29 10:40 ` Jan Beulich
2016-11-29 19:08 ` Stefano Stabellini
@ 2016-11-30 10:29 ` Julien Grall
2016-11-30 14:31 ` Shanker Donthineni
2 siblings, 1 reply; 21+ messages in thread
From: Julien Grall @ 2016-11-30 10:29 UTC (permalink / raw)
To: Shanker Donthineni, xen-devel, Stefano Stabellini
Cc: Vikram Sethi, Campbell Sean
Hi Shanker,
On 29/11/2016 02:59, 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.
IHMO, the patch description is not really accurate. You are removing the
platform timer array that contains watchdog but also Block Timer.
Whilst you mention watchdog, you don't have a word on the Block Timer.
Taking a step back, DOM0 is not able to use it because it does not
request to map the memory region (this is the behavior expected for PCI
and AMBA devices). So this is a bug in the kernel for me.
Assuming this would be fixed, what would be the drawback to give access
to dom0 to the watchdogs?
My worry with that change is what if in the future we decide to expose
watchdog to DOM0? Linux will still not be ready, unless we have Xen to
map those regions at DOM0 build time. That would break the design we
have for ACPI.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 10:29 ` Julien Grall
@ 2016-11-30 14:31 ` Shanker Donthineni
2016-11-30 14:43 ` Julien Grall
2016-11-30 14:43 ` Shanker Donthineni
0 siblings, 2 replies; 21+ messages in thread
From: Shanker Donthineni @ 2016-11-30 14:31 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Vikram Sethi, Campbell Sean
Hi Julien,
On 11/30/2016 04:29 AM, Julien Grall wrote:
> Hi Shanker,
>
> On 29/11/2016 02:59, 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.
>
> IHMO, the patch description is not really accurate. You are removing
> the platform timer array that contains watchdog but also Block Timer.
>
> Whilst you mention watchdog, you don't have a word on the Block Timer.
>
Sure, I'll include detailed description that has word 'Block Timer'.
> Taking a step back, DOM0 is not able to use it because it does not
> request to map the memory region (this is the behavior expected for
> PCI and AMBA devices). So this is a bug in the kernel for me.
>
Not all the drivers/modules in the Linux kernel are bounded to a device
object 'struct device'. The watchdog timer driver is one of this
category, neither a PCIe nor a AMBA device.
You can find GTDT watchdog timer code at https://lkml.org/lkml/2016/7/25/34.
> Assuming this would be fixed, what would be the drawback to give
> access to dom0 to the watchdogs?
>
Absolutely no problem.
> My worry with that change is what if in the future we decide to expose
> watchdog to DOM0? Linux will still not be ready, unless we have Xen to
> map those regions at DOM0 build time. That would break the design we
> have for ACPI.
>
Agree, I was also thinking in the same direction. Let me create a v2
patch that does stage-2 map of MMIO regions at the time of building the
dom0 domain to fix the problem.
> Regards,
>
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 14:31 ` Shanker Donthineni
@ 2016-11-30 14:43 ` Julien Grall
2016-11-30 14:43 ` Shanker Donthineni
1 sibling, 0 replies; 21+ messages in thread
From: Julien Grall @ 2016-11-30 14:43 UTC (permalink / raw)
To: shankerd, xen-devel, Stefano Stabellini; +Cc: Vikram Sethi, Campbell Sean
On 30/11/16 14:31, Shanker Donthineni wrote:
> Hi Julien,
Hi Shanker,
>
> On 11/30/2016 04:29 AM, Julien Grall wrote:
>> Hi Shanker,
>>
>> On 29/11/2016 02:59, 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.
>>
>> IHMO, the patch description is not really accurate. You are removing
>> the platform timer array that contains watchdog but also Block Timer.
>>
>> Whilst you mention watchdog, you don't have a word on the Block Timer.
>>
>
> Sure, I'll include detailed description that has word 'Block Timer'.
>
>> Taking a step back, DOM0 is not able to use it because it does not
>> request to map the memory region (this is the behavior expected for
>> PCI and AMBA devices). So this is a bug in the kernel for me.
>>
> Not all the drivers/modules in the Linux kernel are bounded to a device
> object 'struct device'. The watchdog timer driver is one of this
> category, neither a PCIe nor a AMBA device.
>
> You can find GTDT watchdog timer code at
> https://lkml.org/lkml/2016/7/25/34.
This link points to an i2c driver. Did you intend to post a different link?
>
>> Assuming this would be fixed, what would be the drawback to give
>> access to dom0 to the watchdogs?
>>
> Absolutely no problem.
>
>> My worry with that change is what if in the future we decide to expose
>> watchdog to DOM0? Linux will still not be ready, unless we have Xen to
>> map those regions at DOM0 build time. That would break the design we
>> have for ACPI.
>>
>
> Agree, I was also thinking in the same direction. Let me create a v2
> patch that does stage-2 map of MMIO regions at the time of building the
> dom0 domain to fix the problem.
You didn't understand my point here. The design we agreed for ACPI
support is DOM0 should request the mapping before using any device. This
is because Xen cannot know in advance the memory attribute to use.
So for me this solution is just a workaround. It does not address the
real problem that Linux does not request the mapping of the memory
region before using it. So we will end up with the same problem later if
a device is neither AMBA, nor PCI.
This is easy to solve because watchdog are described the static tables.
What if the device is described in ASL?
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 14:31 ` Shanker Donthineni
2016-11-30 14:43 ` Julien Grall
@ 2016-11-30 14:43 ` Shanker Donthineni
2016-11-30 15:00 ` Julien Grall
1 sibling, 1 reply; 21+ messages in thread
From: Shanker Donthineni @ 2016-11-30 14:43 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Vikram Sethi, Campbell Sean
Hi Julien,
On 11/30/2016 08:31 AM, Shanker Donthineni wrote:
> Hi Julien,
>
>
> On 11/30/2016 04:29 AM, Julien Grall wrote:
>> Hi Shanker,
>>
>> On 29/11/2016 02:59, 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.
>>
>> IHMO, the patch description is not really accurate. You are removing
>> the platform timer array that contains watchdog but also Block Timer.
>>
>> Whilst you mention watchdog, you don't have a word on the Block Timer.
>>
>
> Sure, I'll include detailed description that has word 'Block Timer'
> in v2 patch.
>
>> Taking a step back, DOM0 is not able to use it because it does not
>> request to map the memory region (this is the behavior expected for
>> PCI and AMBA devices). So this is a bug in the kernel for me.
>>
> Not all the drivers/modules in the Linux kernel are bounded to a device
> object 'struct device'. The watchdog timer driver is one of this
> category, neither a PCIe nor a AMBA device.
>
> You can find GTDT watchdog timer code at
> https://lkml.org/lkml/2016/7/25/34.
>
Sorry, right link is https://lkml.org/lkml/2016/7/25/345
>> Assuming this would be fixed, what would be the drawback to give
>> access to dom0 to the watchdogs?
>>
> Absolutely no problem.
>
>> My worry with that change is what if in the future we decide to expose
>> watchdog to DOM0? Linux will still not be ready, unless we have Xen to
>> map those regions at DOM0 build time. That would break the design we
>> have for ACPI.
>>
>
> Agree, I was also thinking in the same direction. Let me create a v2
> patch that does stage-2 map of MMIO regions at the time of building the
> dom0 domain to fix the problem.
>
>> Regards,
>>
>
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 14:43 ` Shanker Donthineni
@ 2016-11-30 15:00 ` Julien Grall
2016-11-30 15:35 ` Shanker Donthineni
0 siblings, 1 reply; 21+ messages in thread
From: Julien Grall @ 2016-11-30 15:00 UTC (permalink / raw)
To: shankerd, xen-devel, Stefano Stabellini; +Cc: Vikram Sethi, Campbell Sean
On 30/11/16 14:43, Shanker Donthineni wrote:
> Hi Julien,
Hi Shanker,
> On 11/30/2016 08:31 AM, Shanker Donthineni wrote:
>> On 11/30/2016 04:29 AM, Julien Grall wrote:
>>> Hi Shanker,
>>>
>>> On 29/11/2016 02:59, 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.
>>>
>>> IHMO, the patch description is not really accurate. You are removing
>>> the platform timer array that contains watchdog but also Block Timer.
>>>
>>> Whilst you mention watchdog, you don't have a word on the Block Timer.
>>>
>>
>> Sure, I'll include detailed description that has word 'Block Timer'
>> in v2 patch.
>>
>>> Taking a step back, DOM0 is not able to use it because it does not
>>> request to map the memory region (this is the behavior expected for
>>> PCI and AMBA devices). So this is a bug in the kernel for me.
>>>
>> Not all the drivers/modules in the Linux kernel are bounded to a device
>> object 'struct device'. The watchdog timer driver is one of this
>> category, neither a PCIe nor a AMBA device.
>>
>> You can find GTDT watchdog timer code at
>> https://lkml.org/lkml/2016/7/25/34.
>>
>
> Sorry, right link is https://lkml.org/lkml/2016/7/25/345
Thank you. Looking at patch #9 [1] that add the watchdog driver. A new
platform device is created (see the call to
platform_device_register_simple).
We already have a platform bus notifier for Xen in linux (see
drivers/xen/arm-device.c). So I am not sure to understand what is the
problem here as the region should be mapped by the notifier. Could you
give a bit more details?
Regards,
[1] https://lkml.org/lkml/2016/7/25/353
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 15:00 ` Julien Grall
@ 2016-11-30 15:35 ` Shanker Donthineni
2016-11-30 19:07 ` Stefano Stabellini
0 siblings, 1 reply; 21+ messages in thread
From: Shanker Donthineni @ 2016-11-30 15:35 UTC (permalink / raw)
To: Julien Grall, xen-devel, Stefano Stabellini; +Cc: Vikram Sethi, Campbell Sean
Hi Julien,
We are using Fu's [v5] patch series
https://patchwork.codeaurora.org/patch/20325/ in our testing. We thought
system crash in xen was related to watchdog timer driver, so removed the
watchdog timer sections including GT blocks in GTDT to fix the crash.
Let me root cause the issue and update the results to you by end of this
week.
Thanks,
Shanker
--
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 9:47 ` Julien Grall
@ 2016-11-30 18:44 ` Stefano Stabellini
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Stabellini @ 2016-11-30 18:44 UTC (permalink / raw)
To: Julien Grall
Cc: Vikram Sethi, Stefano Stabellini, xen-devel, Shanker Donthineni,
Campbell Sean
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@codeaurora.org>
> >
> > 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@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0
2016-11-30 15:35 ` Shanker Donthineni
@ 2016-11-30 19:07 ` Stefano Stabellini
0 siblings, 0 replies; 21+ messages in thread
From: Stefano Stabellini @ 2016-11-30 19:07 UTC (permalink / raw)
To: Shanker Donthineni
Cc: Julien Grall, xen-devel, Stefano Stabellini, Vikram Sethi, Campbell Sean
On Wed, 30 Nov 2016, Shanker Donthineni wrote:
> Hi Julien,
>
> We are using Fu's [v5] patch series
> https://patchwork.codeaurora.org/patch/20325/ in our testing. We thought
> system crash in xen was related to watchdog timer driver, so removed the
> watchdog timer sections including GT blocks in GTDT to fix the crash. Let me
> root cause the issue and update the results to you by end of this week.
FYI the Linux notifier is
drivers/xen/arm-device.c:xen_platform_notifier. If it doesn't get called
for some reason, it would be useful to know why and fix the problem.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2016-11-30 19:07 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-29 2:59 [PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0 Shanker Donthineni
2016-11-29 10:40 ` Jan Beulich
2016-11-29 11:38 ` Roger Pau Monne
2016-11-29 11:44 ` Jan Beulich
2016-11-29 12:09 ` Roger Pau Monne
2016-11-29 13:07 ` Jan Beulich
2016-11-29 14:28 ` Roger Pau Monne
2016-11-29 18:57 ` Stefano Stabellini
2016-11-29 20:19 ` Andrew Cooper
2016-11-30 8:54 ` Jan Beulich
2016-11-30 10:14 ` Roger Pau Monne
2016-11-29 19:08 ` Stefano Stabellini
2016-11-30 9:47 ` Julien Grall
2016-11-30 18:44 ` Stefano Stabellini
2016-11-30 10:29 ` Julien Grall
2016-11-30 14:31 ` Shanker Donthineni
2016-11-30 14:43 ` Julien Grall
2016-11-30 14:43 ` Shanker Donthineni
2016-11-30 15:00 ` Julien Grall
2016-11-30 15:35 ` Shanker Donthineni
2016-11-30 19:07 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.