[PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0

[PATCH] arm/acpi: hide watchdog timer in GTDT table for dom0

[Shanker Donthineni]

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

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

[Jan Beulich]

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

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

[Roger Pau Monne]

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

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

[Jan Beulich]

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

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

[Roger Pau Monne]

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

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

[Jan Beulich]

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

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

[Roger Pau Monne]

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

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

[Stefano Stabellini]

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

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

[Andrew Cooper]

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

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

[Jan Beulich]

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

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

[Roger Pau Monne]

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

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

[Stefano Stabellini]

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

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

[Julien Grall]

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

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

[Stefano Stabellini]

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

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

[Julien Grall]

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

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

[Shanker Donthineni]

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

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

[Julien Grall]

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

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

[Shanker Donthineni]

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

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

[Julien Grall]

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

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

[Shanker Donthineni]

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

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

[Stefano Stabellini]

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