[PATCH 0/3] pvh/dom0: switch to ACPI whitelisting

[PATCH 0/3] pvh/dom0: switch to ACPI whitelisting

[Roger Pau Monne]

Hello,

The following small series contain one cleanup, one bugfix and finally
switches PVH Dom0 from whitelisting ACPI tables instead of blacklisting
them.

The number of allowed tables ATM is fairly limited, many more could be
added if the resources described in them are properly mapped to Dom0
physmap.

Thanks, Roger.

Roger Pau Monne (3):
  pvh/dom0: init variables at declaration time
  pvh/dom0: pass address/length to pvh_acpi_table_allowed
  pvh/dom0: whitelist PVH Dom0 ACPI tables

 xen/arch/x86/hvm/dom0_build.c | 75 +++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 38 deletions(-)

-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] pvh/dom0: init variables at declaration time

[Roger Pau Monne]

Also remove a couple of newlines at the start of function
declarations.

No functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 5f4155b7ca..1c83578c5e 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -91,12 +91,11 @@ static int __init pvh_populate_memory_range(struct domain *d,
                                             unsigned long start,
                                             unsigned long nr_pages)
 {
-    unsigned int order, i = 0;
+    unsigned int order = MAX_ORDER, i = 0;
     struct page_info *page;
     int rc;
 #define MAP_MAX_ITER 64
 
-    order = MAX_ORDER;
     while ( nr_pages != 0 )
     {
         unsigned int range_order = get_order_from_pages(nr_pages + 1);
@@ -376,14 +375,12 @@ static __init void pvh_setup_e820(struct domain *d, unsigned long nr_pages)
 static int __init pvh_setup_p2m(struct domain *d)
 {
     struct vcpu *v = d->vcpu[0];
-    unsigned long nr_pages;
+    unsigned long nr_pages = dom0_compute_nr_pages(d, NULL, 0);
     unsigned int i;
     int rc;
     bool preempted;
 #define MB1_PAGES PFN_DOWN(MB(1))
 
-    nr_pages = dom0_compute_nr_pages(d, NULL, 0);
-
     pvh_setup_e820(d, nr_pages);
     do {
         preempted = false;
@@ -565,7 +562,7 @@ static int __init pvh_setup_cpus(struct domain *d, paddr_t entry,
                                  paddr_t start_info)
 {
     struct vcpu *v = d->vcpu[0];
-    unsigned int cpu, i;
+    unsigned int cpu = v->processor, i;
     int rc;
     /*
      * This sets the vCPU state according to the state described in
@@ -586,7 +583,6 @@ static int __init pvh_setup_cpus(struct domain *d, paddr_t entry,
         .cpu_regs.x86_32.tr_ar = 0x8b,
     };
 
-    cpu = v->processor;
     for ( i = 1; i < d->max_vcpus; i++ )
     {
         const struct vcpu *p = dom0_setup_vcpu(d, i, cpu);
@@ -620,7 +616,6 @@ static int __init pvh_setup_cpus(struct domain *d, paddr_t entry,
 static int __init acpi_count_intr_ovr(struct acpi_subtable_header *header,
                                      const unsigned long end)
 {
-
     acpi_intr_overrides++;
     return 0;
 }
@@ -640,7 +635,6 @@ static int __init acpi_set_intr_ovr(struct acpi_subtable_header *header,
 static int __init acpi_count_nmi_src(struct acpi_subtable_header *header,
                                      const unsigned long end)
 {
-
     acpi_nmi_sources++;
     return 0;
 }
@@ -780,10 +774,9 @@ static int __init pvh_setup_acpi_madt(struct domain *d, paddr_t *addr)
 static bool __init acpi_memory_banned(unsigned long address,
                                       unsigned long size)
 {
-    unsigned long mfn, nr_pages, i;
+    unsigned long mfn = PFN_DOWN(address);
+    unsigned long nr_pages = PFN_UP((address & ~PAGE_MASK) + size), i;
 
-    mfn = PFN_DOWN(address);
-    nr_pages = PFN_UP((address & ~PAGE_MASK) + size);
     for ( i = 0 ; i < nr_pages; i++ )
         if ( !page_is_ram_type(mfn + i, RAM_TYPE_RESERVED) &&
              !page_is_ram_type(mfn + i, RAM_TYPE_ACPI) )
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] pvh/dom0: pass address/length to pvh_acpi_table_allowed

[Roger Pau Monne]

The current usage of acpi_gbl_root_table_list inside the function is
wrong.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 1c83578c5e..830b4345cc 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -785,7 +785,9 @@ static bool __init acpi_memory_banned(unsigned long address,
     return false;
 }
 
-static bool __init pvh_acpi_table_allowed(const char *sig)
+static bool __init pvh_acpi_table_allowed(const char *sig,
+                                          unsigned long address,
+                                          unsigned long size)
 {
     static const char __initconst banned_tables[][ACPI_NAME_SIZE] = {
         ACPI_SIG_HPET, ACPI_SIG_SLIT, ACPI_SIG_SRAT, ACPI_SIG_MPST,
@@ -797,8 +799,7 @@ static bool __init pvh_acpi_table_allowed(const char *sig)
             return false;
 
     /* Make sure table doesn't reside in a RAM region. */
-    if ( acpi_memory_banned(acpi_gbl_root_table_list.tables[i].address,
-                            acpi_gbl_root_table_list.tables[i].length) )
+    if ( acpi_memory_banned(address, size) )
     {
         printk("Skipping table %.4s because resides in a non-ACPI, non-reserved region\n",
                sig);
@@ -808,13 +809,15 @@ static bool __init pvh_acpi_table_allowed(const char *sig)
     return true;
 }
 
-static bool __init pvh_acpi_xsdt_table_allowed(const char *sig)
+static bool __init pvh_acpi_xsdt_table_allowed(const char *sig,
+                                               unsigned long address,
+                                               unsigned long size)
 {
     /*
      * DSDT and FACS are pointed to from FADT and thus don't belong
      * in XSDT.
      */
-    return (pvh_acpi_table_allowed(sig) &&
+    return (pvh_acpi_table_allowed(sig, address, size) &&
             strncmp(sig, ACPI_SIG_DSDT, ACPI_NAME_SIZE) &&
             strncmp(sig, ACPI_SIG_FACS, ACPI_NAME_SIZE));
 }
@@ -825,6 +828,7 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr,
     struct acpi_table_xsdt *xsdt;
     struct acpi_table_header *table;
     struct acpi_table_rsdp *rsdp;
+    const struct acpi_table_desc *tables = acpi_gbl_root_table_list.tables;
     unsigned long size = sizeof(*xsdt);
     unsigned int i, j, num_tables = 0;
     paddr_t xsdt_paddr;
@@ -840,9 +844,8 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr,
     /* Count the number of tables that will be added to the XSDT. */
     for( i = 0; i < acpi_gbl_root_table_list.count; i++ )
     {
-        const char *sig = acpi_gbl_root_table_list.tables[i].signature.ascii;
-
-        if ( pvh_acpi_xsdt_table_allowed(sig) )
+        if ( pvh_acpi_xsdt_table_allowed(tables[i].signature.ascii,
+                                         tables[i].address, tables[i].length) )
             num_tables++;
     }
 
@@ -887,11 +890,9 @@ static int __init pvh_setup_acpi_xsdt(struct domain *d, paddr_t madt_addr,
     /* Copy the addresses of the rest of the allowed tables. */
     for( i = 0, j = 1; i < acpi_gbl_root_table_list.count; i++ )
     {
-        const char *sig = acpi_gbl_root_table_list.tables[i].signature.ascii;
-
-        if ( pvh_acpi_xsdt_table_allowed(sig) )
-            xsdt->table_offset_entry[j++] =
-                acpi_gbl_root_table_list.tables[i].address;
+        if ( pvh_acpi_xsdt_table_allowed(tables[i].signature.ascii,
+                                         tables[i].address, tables[i].length) )
+            xsdt->table_offset_entry[j++] = tables[i].address;
     }
 
     xsdt->header.revision = 1;
@@ -955,7 +956,7 @@ static int __init pvh_setup_acpi(struct domain *d, paddr_t start_info)
          * re-using MADT memory.
          */
         if ( strncmp(sig, ACPI_SIG_MADT, ACPI_NAME_SIZE)
-             ? pvh_acpi_table_allowed(sig)
+             ? pvh_acpi_table_allowed(sig, addr, size)
              : !acpi_memory_banned(addr, size) )
              pvh_add_mem_range(d, addr, addr + size, E820_ACPI);
     }
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/dom0_build.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
index 830b4345cc..82ee3fe237 100644
--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -789,24 +789,29 @@ static bool __init pvh_acpi_table_allowed(const char *sig,
                                           unsigned long address,
                                           unsigned long size)
 {
-    static const char __initconst banned_tables[][ACPI_NAME_SIZE] = {
-        ACPI_SIG_HPET, ACPI_SIG_SLIT, ACPI_SIG_SRAT, ACPI_SIG_MPST,
-        ACPI_SIG_PMTT, ACPI_SIG_MADT, ACPI_SIG_DMAR};
+    static const char __initconst allowed_tables[][ACPI_NAME_SIZE] = {
+        ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_FACS, ACPI_SIG_PSDT,
+        ACPI_SIG_SSDT, ACPI_SIG_SBST, ACPI_SIG_MCFG, ACPI_SIG_SLIC,
+        ACPI_SIG_MSDM, ACPI_SIG_WDAT, ACPI_SIG_FPDT, ACPI_SIG_S3PT,
+    };
     unsigned int i;
 
-    for ( i = 0 ; i < ARRAY_SIZE(banned_tables); i++ )
-        if ( strncmp(sig, banned_tables[i], ACPI_NAME_SIZE) == 0 )
-            return false;
-
-    /* Make sure table doesn't reside in a RAM region. */
-    if ( acpi_memory_banned(address, size) )
+    for ( i = 0 ; i < ARRAY_SIZE(allowed_tables); i++ )
     {
-        printk("Skipping table %.4s because resides in a non-ACPI, non-reserved region\n",
-               sig);
-        return false;
+        if ( strncmp(sig, allowed_tables[i], ACPI_NAME_SIZE) )
+            continue;
+
+        if ( !acpi_memory_banned(address, size) )
+            return true;
+        else
+        {
+            printk("Skipping table %.4s in non-ACPI non-reserved region\n",
+                   sig);
+            return false;
+        }
     }
 
-    return true;
+    return false;
 }
 
 static bool __init pvh_acpi_xsdt_table_allowed(const char *sig,
-- 
2.15.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0/3] pvh/dom0: switch to ACPI whitelisting

[Andrew Cooper]

On 08/02/18 12:25, Roger Pau Monne wrote:
> Hello,
>
> The following small series contain one cleanup, one bugfix and finally
> switches PVH Dom0 from whitelisting ACPI tables instead of blacklisting
> them.
>
> The number of allowed tables ATM is fairly limited, many more could be
> added if the resources described in them are properly mapped to Dom0
> physmap.
>
> Thanks, Roger.
>
> Roger Pau Monne (3):
>   pvh/dom0: init variables at declaration time
>   pvh/dom0: pass address/length to pvh_acpi_table_allowed
>   pvh/dom0: whitelist PVH Dom0 ACPI tables

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Jan Beulich]

>>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

A change like this should not come without description, providing a
reason for the change. Otherwise how will someone wanting to
understand the change in a couple of years actually be able to
make any sense of it. This is in particular because I continue to be
not fully convinced that white listing is appropriate in the Dom0
case (and for the record I'm similarly unconvinced that black listing
is the best choice, yet obviously we need to pick on of the two).

> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -789,24 +789,29 @@ static bool __init pvh_acpi_table_allowed(const char *sig,
>                                            unsigned long address,
>                                            unsigned long size)
>  {
> -    static const char __initconst banned_tables[][ACPI_NAME_SIZE] = {
> -        ACPI_SIG_HPET, ACPI_SIG_SLIT, ACPI_SIG_SRAT, ACPI_SIG_MPST,
> -        ACPI_SIG_PMTT, ACPI_SIG_MADT, ACPI_SIG_DMAR};
> +    static const char __initconst allowed_tables[][ACPI_NAME_SIZE] = {
> +        ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_FACS, ACPI_SIG_PSDT,
> +        ACPI_SIG_SSDT, ACPI_SIG_SBST, ACPI_SIG_MCFG, ACPI_SIG_SLIC,
> +        ACPI_SIG_MSDM, ACPI_SIG_WDAT, ACPI_SIG_FPDT, ACPI_SIG_S3PT,
> +    };
>      unsigned int i;
>  
> -    for ( i = 0 ; i < ARRAY_SIZE(banned_tables); i++ )
> -        if ( strncmp(sig, banned_tables[i], ACPI_NAME_SIZE) == 0 )
> -            return false;
> -
> -    /* Make sure table doesn't reside in a RAM region. */
> -    if ( acpi_memory_banned(address, size) )
> +    for ( i = 0 ; i < ARRAY_SIZE(allowed_tables); i++ )
>      {
> -        printk("Skipping table %.4s because resides in a non-ACPI, non-reserved region\n",
> -               sig);
> -        return false;
> +        if ( strncmp(sig, allowed_tables[i], ACPI_NAME_SIZE) )
> +            continue;
> +
> +        if ( !acpi_memory_banned(address, size) )
> +            return true;
> +        else

Unnecessary "else".

Jan

> +        {
> +            printk("Skipping table %.4s in non-ACPI non-reserved region\n",
> +                   sig);
> +            return false;
> +        }
>      }
>  
> -    return true;
> +    return false;
>  }


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Roger Pau Monné]

On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
> >>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> A change like this should not come without description, providing a
> reason for the change. Otherwise how will someone wanting to
> understand the change in a couple of years actually be able to
> make any sense of it. This is in particular because I continue to be
> not fully convinced that white listing is appropriate in the Dom0
> case (and for the record I'm similarly unconvinced that black listing
> is the best choice, yet obviously we need to pick on of the two).

I'm sorry, I thought we agreed at the summit to convert this to
whitelisting because it was likely better to simply not expose unknown
ACPI tables to guests.

I guess the commit message could be something like:

"The following list of whitelisted APIC tables are either known to work
or don't require any resources to be mapped in either the IO or the
memory space.

This list is expected to be expanded as more testing is performed."

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Jan Beulich]

>>> On 13.02.18 at 10:59, <roger.pau@citrix.com> wrote:
> On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
>> >>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
>> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> 
>> A change like this should not come without description, providing a
>> reason for the change. Otherwise how will someone wanting to
>> understand the change in a couple of years actually be able to
>> make any sense of it. This is in particular because I continue to be
>> not fully convinced that white listing is appropriate in the Dom0
>> case (and for the record I'm similarly unconvinced that black listing
>> is the best choice, yet obviously we need to pick on of the two).
> 
> I'm sorry, I thought we agreed at the summit to convert this to
> whitelisting because it was likely better to simply not expose unknown
> ACPI tables to guests.

"to guests" != "to Dom0".

> I guess the commit message could be something like:
> 
> "The following list of whitelisted APIC tables are either known to work
> or don't require any resources to be mapped in either the IO or the
> memory space.

Even if the white listing vs black listing question wasn't still
undecided, I think we should revert the patch in favor of one
with a description. The one above might be fine with "ACPI" in
place of "APIC" as far as tables actively white listed are
concerned, but then it still remains open why certain tables
haven't been included. I'm in particular worried about various
APEI related tables, but invisibility of e.g. an IBFT could also
lead to boot problems.

Andrew - you've acked and committed this, what is your opinion
here?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Roger Pau Monné]

On Tue, Feb 13, 2018 at 04:04:17AM -0700, Jan Beulich wrote:
> >>> On 13.02.18 at 10:59, <roger.pau@citrix.com> wrote:
> > On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
> >> >>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> 
> >> A change like this should not come without description, providing a
> >> reason for the change. Otherwise how will someone wanting to
> >> understand the change in a couple of years actually be able to
> >> make any sense of it. This is in particular because I continue to be
> >> not fully convinced that white listing is appropriate in the Dom0
> >> case (and for the record I'm similarly unconvinced that black listing
> >> is the best choice, yet obviously we need to pick on of the two).
> > 
> > I'm sorry, I thought we agreed at the summit to convert this to
> > whitelisting because it was likely better to simply not expose unknown
> > ACPI tables to guests.
> 
> "to guests" != "to Dom0".
> 
> > I guess the commit message could be something like:
> > 
> > "The following list of whitelisted APIC tables are either known to work
> > or don't require any resources to be mapped in either the IO or the
> > memory space.
> 
> Even if the white listing vs black listing question wasn't still
> undecided, I think we should revert the patch in favor of one
> with a description. The one above might be fine with "ACPI" in
> place of "APIC" as far as tables actively white listed are
> concerned, but then it still remains open why certain tables
> haven't been included. I'm in particular worried about various
> APEI related tables, but invisibility of e.g. an IBFT could also
> lead to boot problems.

Regarding APEI I think ERST, EINJ and HEST could be passed through,
BERT however requires that the BOOT Error Region is mapped into Dom0
p2m.

Since PVH Dom0 creation still ends up in a panic, I see no problem in
adding those in follow up patches.

IBFT also looks safe to pass through.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Jan Beulich]

>>> On 13.02.18 at 12:27, <roger.pau@citrix.com> wrote:
> On Tue, Feb 13, 2018 at 04:04:17AM -0700, Jan Beulich wrote:
>> >>> On 13.02.18 at 10:59, <roger.pau@citrix.com> wrote:
>> > On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
>> >> >>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
>> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> >> 
>> >> A change like this should not come without description, providing a
>> >> reason for the change. Otherwise how will someone wanting to
>> >> understand the change in a couple of years actually be able to
>> >> make any sense of it. This is in particular because I continue to be
>> >> not fully convinced that white listing is appropriate in the Dom0
>> >> case (and for the record I'm similarly unconvinced that black listing
>> >> is the best choice, yet obviously we need to pick on of the two).
>> > 
>> > I'm sorry, I thought we agreed at the summit to convert this to
>> > whitelisting because it was likely better to simply not expose unknown
>> > ACPI tables to guests.
>> 
>> "to guests" != "to Dom0".
>> 
>> > I guess the commit message could be something like:
>> > 
>> > "The following list of whitelisted APIC tables are either known to work
>> > or don't require any resources to be mapped in either the IO or the
>> > memory space.
>> 
>> Even if the white listing vs black listing question wasn't still
>> undecided, I think we should revert the patch in favor of one
>> with a description. The one above might be fine with "ACPI" in
>> place of "APIC" as far as tables actively white listed are
>> concerned, but then it still remains open why certain tables
>> haven't been included. I'm in particular worried about various
>> APEI related tables, but invisibility of e.g. an IBFT could also
>> lead to boot problems.
> 
> Regarding APEI I think ERST, EINJ and HEST could be passed through,
> BERT however requires that the BOOT Error Region is mapped into Dom0
> p2m.
> 
> Since PVH Dom0 creation still ends up in a panic, I see no problem in
> adding those in follow up patches.
> 
> IBFT also looks safe to pass through.

But you realize I've named only the few that came to mind
immediately?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Roger Pau Monné]

On Tue, Feb 13, 2018 at 06:41:14AM -0700, Jan Beulich wrote:
> >>> On 13.02.18 at 12:27, <roger.pau@citrix.com> wrote:
> > On Tue, Feb 13, 2018 at 04:04:17AM -0700, Jan Beulich wrote:
> >> >>> On 13.02.18 at 10:59, <roger.pau@citrix.com> wrote:
> >> > On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
> >> >> >>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
> >> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> >> 
> >> >> A change like this should not come without description, providing a
> >> >> reason for the change. Otherwise how will someone wanting to
> >> >> understand the change in a couple of years actually be able to
> >> >> make any sense of it. This is in particular because I continue to be
> >> >> not fully convinced that white listing is appropriate in the Dom0
> >> >> case (and for the record I'm similarly unconvinced that black listing
> >> >> is the best choice, yet obviously we need to pick on of the two).
> >> > 
> >> > I'm sorry, I thought we agreed at the summit to convert this to
> >> > whitelisting because it was likely better to simply not expose unknown
> >> > ACPI tables to guests.
> >> 
> >> "to guests" != "to Dom0".
> >> 
> >> > I guess the commit message could be something like:
> >> > 
> >> > "The following list of whitelisted APIC tables are either known to work
> >> > or don't require any resources to be mapped in either the IO or the
> >> > memory space.
> >> 
> >> Even if the white listing vs black listing question wasn't still
> >> undecided, I think we should revert the patch in favor of one
> >> with a description. The one above might be fine with "ACPI" in
> >> place of "APIC" as far as tables actively white listed are
> >> concerned, but then it still remains open why certain tables
> >> haven't been included. I'm in particular worried about various
> >> APEI related tables, but invisibility of e.g. an IBFT could also
> >> lead to boot problems.
> > 
> > Regarding APEI I think ERST, EINJ and HEST could be passed through,
> > BERT however requires that the BOOT Error Region is mapped into Dom0
> > p2m.
> > 
> > Since PVH Dom0 creation still ends up in a panic, I see no problem in
> > adding those in follow up patches.
> > 
> > IBFT also looks safe to pass through.
> 
> But you realize I've named only the few that came to mind
> immediately?

Sure, what I have in this patch is just the minimal set (plus a few
others that seem completely fine) needed in order to boot on my two
test boxes.

I know we will certainly have to expand this, but I see no issue in
adding them as we go, the more that this is all still unused.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Jan Beulich]

>>> On 13.02.18 at 16:11, <roger.pau@citrix.com> wrote:
> On Tue, Feb 13, 2018 at 06:41:14AM -0700, Jan Beulich wrote:
>> >>> On 13.02.18 at 12:27, <roger.pau@citrix.com> wrote:
>> > On Tue, Feb 13, 2018 at 04:04:17AM -0700, Jan Beulich wrote:
>> >> >>> On 13.02.18 at 10:59, <roger.pau@citrix.com> wrote:
>> >> > On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
>> >> >> >>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
>> >> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> >> >> 
>> >> >> A change like this should not come without description, providing a
>> >> >> reason for the change. Otherwise how will someone wanting to
>> >> >> understand the change in a couple of years actually be able to
>> >> >> make any sense of it. This is in particular because I continue to be
>> >> >> not fully convinced that white listing is appropriate in the Dom0
>> >> >> case (and for the record I'm similarly unconvinced that black listing
>> >> >> is the best choice, yet obviously we need to pick on of the two).
>> >> > 
>> >> > I'm sorry, I thought we agreed at the summit to convert this to
>> >> > whitelisting because it was likely better to simply not expose unknown
>> >> > ACPI tables to guests.
>> >> 
>> >> "to guests" != "to Dom0".
>> >> 
>> >> > I guess the commit message could be something like:
>> >> > 
>> >> > "The following list of whitelisted APIC tables are either known to work
>> >> > or don't require any resources to be mapped in either the IO or the
>> >> > memory space.
>> >> 
>> >> Even if the white listing vs black listing question wasn't still
>> >> undecided, I think we should revert the patch in favor of one
>> >> with a description. The one above might be fine with "ACPI" in
>> >> place of "APIC" as far as tables actively white listed are
>> >> concerned, but then it still remains open why certain tables
>> >> haven't been included. I'm in particular worried about various
>> >> APEI related tables, but invisibility of e.g. an IBFT could also
>> >> lead to boot problems.
>> > 
>> > Regarding APEI I think ERST, EINJ and HEST could be passed through,
>> > BERT however requires that the BOOT Error Region is mapped into Dom0
>> > p2m.
>> > 
>> > Since PVH Dom0 creation still ends up in a panic, I see no problem in
>> > adding those in follow up patches.
>> > 
>> > IBFT also looks safe to pass through.
>> 
>> But you realize I've named only the few that came to mind
>> immediately?
> 
> Sure, what I have in this patch is just the minimal set (plus a few
> others that seem completely fine) needed in order to boot on my two
> test boxes.
> 
> I know we will certainly have to expand this, but I see no issue in
> adding them as we go, the more that this is all still unused.

Unused - sure. But how will we learn which ones we need to add?
Surely waiting for problem reports from the field is not an
acceptable model.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Roger Pau Monné]

On Tue, Feb 13, 2018 at 08:22:33AM -0700, Jan Beulich wrote:
> >>> On 13.02.18 at 16:11, <roger.pau@citrix.com> wrote:
> > On Tue, Feb 13, 2018 at 06:41:14AM -0700, Jan Beulich wrote:
> >> >>> On 13.02.18 at 12:27, <roger.pau@citrix.com> wrote:
> >> > On Tue, Feb 13, 2018 at 04:04:17AM -0700, Jan Beulich wrote:
> >> >> >>> On 13.02.18 at 10:59, <roger.pau@citrix.com> wrote:
> >> >> > On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
> >> >> >> >>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
> >> >> >> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> >> >> 
> >> >> >> A change like this should not come without description, providing a
> >> >> >> reason for the change. Otherwise how will someone wanting to
> >> >> >> understand the change in a couple of years actually be able to
> >> >> >> make any sense of it. This is in particular because I continue to be
> >> >> >> not fully convinced that white listing is appropriate in the Dom0
> >> >> >> case (and for the record I'm similarly unconvinced that black listing
> >> >> >> is the best choice, yet obviously we need to pick on of the two).
> >> >> > 
> >> >> > I'm sorry, I thought we agreed at the summit to convert this to
> >> >> > whitelisting because it was likely better to simply not expose unknown
> >> >> > ACPI tables to guests.
> >> >> 
> >> >> "to guests" != "to Dom0".
> >> >> 
> >> >> > I guess the commit message could be something like:
> >> >> > 
> >> >> > "The following list of whitelisted APIC tables are either known to work
> >> >> > or don't require any resources to be mapped in either the IO or the
> >> >> > memory space.
> >> >> 
> >> >> Even if the white listing vs black listing question wasn't still
> >> >> undecided, I think we should revert the patch in favor of one
> >> >> with a description. The one above might be fine with "ACPI" in
> >> >> place of "APIC" as far as tables actively white listed are
> >> >> concerned, but then it still remains open why certain tables
> >> >> haven't been included. I'm in particular worried about various
> >> >> APEI related tables, but invisibility of e.g. an IBFT could also
> >> >> lead to boot problems.
> >> > 
> >> > Regarding APEI I think ERST, EINJ and HEST could be passed through,
> >> > BERT however requires that the BOOT Error Region is mapped into Dom0
> >> > p2m.
> >> > 
> >> > Since PVH Dom0 creation still ends up in a panic, I see no problem in
> >> > adding those in follow up patches.
> >> > 
> >> > IBFT also looks safe to pass through.
> >> 
> >> But you realize I've named only the few that came to mind
> >> immediately?
> > 
> > Sure, what I have in this patch is just the minimal set (plus a few
> > others that seem completely fine) needed in order to boot on my two
> > test boxes.
> > 
> > I know we will certainly have to expand this, but I see no issue in
> > adding them as we go, the more that this is all still unused.
> 
> Unused - sure. But how will we learn which ones we need to add?

I already have a kind of drafted list, of which ones could be added,
which ones need some handlers in order to make sure relevant areas are
mapped and finally a list of tables that will never be exposed to
Dom0.

This is based on the tables currently known to Xen from the actblX.h
headers, there are probably more in the wild, even ones not documented
in http://www.uefi.org/acpi at all.

I can cleanup and send that list.

> Surely waiting for problem reports from the field is not an
> acceptable model.

That's last resort of course, but given that PVH Dom0 doesn't exist
yet I don't see it as bad to receive reports of missing tables (or
things not working as expected) during the experimental version of
it.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] pvh/dom0: whitelist PVH Dom0 ACPI tables

[Andrew Cooper]

On 13/02/18 15:48, Roger Pau Monné wrote:
> On Tue, Feb 13, 2018 at 08:22:33AM -0700, Jan Beulich wrote:
>>>>> On 13.02.18 at 16:11, <roger.pau@citrix.com> wrote:
>>> On Tue, Feb 13, 2018 at 06:41:14AM -0700, Jan Beulich wrote:
>>>>>>> On 13.02.18 at 12:27, <roger.pau@citrix.com> wrote:
>>>>> On Tue, Feb 13, 2018 at 04:04:17AM -0700, Jan Beulich wrote:
>>>>>>>>> On 13.02.18 at 10:59, <roger.pau@citrix.com> wrote:
>>>>>>> On Tue, Feb 13, 2018 at 02:29:08AM -0700, Jan Beulich wrote:
>>>>>>>>>>> On 08.02.18 at 13:25, <roger.pau@citrix.com> wrote:
>>>>>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>>>>> A change like this should not come without description, providing a
>>>>>>>> reason for the change. Otherwise how will someone wanting to
>>>>>>>> understand the change in a couple of years actually be able to
>>>>>>>> make any sense of it. This is in particular because I continue to be
>>>>>>>> not fully convinced that white listing is appropriate in the Dom0
>>>>>>>> case (and for the record I'm similarly unconvinced that black listing
>>>>>>>> is the best choice, yet obviously we need to pick on of the two).
>>>>>>> I'm sorry, I thought we agreed at the summit to convert this to
>>>>>>> whitelisting because it was likely better to simply not expose unknown
>>>>>>> ACPI tables to guests.
>>>>>> "to guests" != "to Dom0".
>>>>>>
>>>>>>> I guess the commit message could be something like:
>>>>>>>
>>>>>>> "The following list of whitelisted APIC tables are either known to work
>>>>>>> or don't require any resources to be mapped in either the IO or the
>>>>>>> memory space.
>>>>>> Even if the white listing vs black listing question wasn't still
>>>>>> undecided, I think we should revert the patch in favor of one
>>>>>> with a description. The one above might be fine with "ACPI" in
>>>>>> place of "APIC" as far as tables actively white listed are
>>>>>> concerned, but then it still remains open why certain tables
>>>>>> haven't been included. I'm in particular worried about various
>>>>>> APEI related tables, but invisibility of e.g. an IBFT could also
>>>>>> lead to boot problems.
>>>>> Regarding APEI I think ERST, EINJ and HEST could be passed through,
>>>>> BERT however requires that the BOOT Error Region is mapped into Dom0
>>>>> p2m.
>>>>>
>>>>> Since PVH Dom0 creation still ends up in a panic, I see no problem in
>>>>> adding those in follow up patches.
>>>>>
>>>>> IBFT also looks safe to pass through.
>>>> But you realize I've named only the few that came to mind
>>>> immediately?
>>> Sure, what I have in this patch is just the minimal set (plus a few
>>> others that seem completely fine) needed in order to boot on my two
>>> test boxes.
>>>
>>> I know we will certainly have to expand this, but I see no issue in
>>> adding them as we go, the more that this is all still unused.
>> Unused - sure. But how will we learn which ones we need to add?
> I already have a kind of drafted list, of which ones could be added,
> which ones need some handlers in order to make sure relevant areas are
> mapped and finally a list of tables that will never be exposed to
> Dom0.
>
> This is based on the tables currently known to Xen from the actblX.h
> headers, there are probably more in the wild, even ones not documented
> in http://www.uefi.org/acpi at all.
>
> I can cleanup and send that list.
>
>> Surely waiting for problem reports from the field is not an
>> acceptable model.

APCI tables are no different to CPUID values, or MSRs, etc.  Such
reports from the field would be missing features, not bugs.  (TBF, I
wouldn't even put IBFT in to begin with, because I'm not convinced the
IOMMU logic is good enough to work in the common case.  We still don't
account for ACS/ARI errata in the PLX bridges, and iommu=dom0-strict
mode breaks horribly on all hardware I've ever tried using it on.)

Dom0 should not be treated specially.  It just has a bit more hardware
and permissions by default.  The current PV interactions, and especially
the workarounds we've had to maintain the hypervisor, demonstrate
precisely why a whitelist approach is better than a blacklist.

This is all completely brand new work, so we've got the opportunity to
do things correctly from the beginning.  I'd much rather it takes longer
to do properly, than inheriting the same mistakes and problems that PV
dom0 has.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel