diff mbox

[v2,07/30] xen/x86: split the setup of Dom0 permissions to a function

Message ID 1474991845-27962-8-git-send-email-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné Sept. 27, 2016, 3:57 p.m. UTC 
So that it can also be used by the PVH-specific domain builder. This is just
code motion, it should not introduce any 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/domain_build.c | 164 +++++++++++++++++++++++---------------------
 1 file changed, 86 insertions(+), 78 deletions(-)

Comments

Jan Beulich Sept. 29, 2016, 1:47 p.m. UTC | #1 
>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote:
> So that it can also be used by the PVH-specific domain builder. This is just
> code motion, it should not introduce any functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Looks generally okay, but please do minor style corrections as you
move code:

> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -869,6 +869,89 @@ static __init void setup_pv_physmap(struct domain *d, unsigned long pgtbl_pfn,
>      unmap_domain_page(l4start);
>  }
>  
> +static int __init setup_permissions(struct domain *d)
> +{
> +    unsigned long mfn;
> +    int i, rc = 0;

i should be unsigned int, and the initializer of rc could be avoided.

> +    /* The hardware domain is initially permitted full I/O capabilities. */
> +    rc |= ioports_permit_access(d, 0, 0xFFFF);
> +    rc |= iomem_permit_access(d, 0UL, (1UL << (paddr_bits - PAGE_SHIFT)) - 1);
> +    rc |= irqs_permit_access(d, 1, nr_irqs_gsi - 1);
> +
> +    /*
> +     * Modify I/O port access permissions.
> +     */

This is a single line comment - I understand it's trying to be more of a
separator than the others, but I'd prefer for it to do so by being
followed by a blank line.

> +    /* Master Interrupt Controller (PIC). */
> +    rc |= ioports_deny_access(d, 0x20, 0x21);
> +    /* Slave Interrupt Controller (PIC). */
> +    rc |= ioports_deny_access(d, 0xA0, 0xA1);
> +    /* Interval Timer (PIT). */
> +    rc |= ioports_deny_access(d, 0x40, 0x43);
> +    /* PIT Channel 2 / PC Speaker Control. */
> +    rc |= ioports_deny_access(d, 0x61, 0x61);
> +    /* ACPI PM Timer. */
> +    if ( pmtmr_ioport )
> +        rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
> +    /* PCI configuration space (NB. 0xcf8 has special treatment). */
> +    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
> +    /* Command-line I/O ranges. */
> +    process_dom0_ioports_disable(d);
> +
> +    /*
> +     * Modify I/O memory access permissions.
> +     */

Dito.

> -    BUG_ON(rc != 0);
> +    rc = setup_permissions(d);
> +    if ( rc != 0 )
> +        panic("Failed to setup Dom0 permissions");

To be honest, I'm not sure of this BUG_ON() -> panic() conversion.
I think I'd prefer it to stay the way it was. We're not really expecting
for any of this to fail anyway.

Jan
Roger Pau Monné Sept. 29, 2016, 3:53 p.m. UTC | #2 
On Thu, Sep 29, 2016 at 07:47:22AM -0600, Jan Beulich wrote:
> >>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote:
> > So that it can also be used by the PVH-specific domain builder. This is just
> > code motion, it should not introduce any functional change.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Looks generally okay, but please do minor style corrections as you
> move code:
> 
> > --- a/xen/arch/x86/domain_build.c
> > +++ b/xen/arch/x86/domain_build.c
> > @@ -869,6 +869,89 @@ static __init void setup_pv_physmap(struct domain *d, unsigned long pgtbl_pfn,
> >      unmap_domain_page(l4start);
> >  }
> >  
> > +static int __init setup_permissions(struct domain *d)
> > +{
> > +    unsigned long mfn;
> > +    int i, rc = 0;
> 
> i should be unsigned int, and the initializer of rc could be avoided.

Done (I've also converted the first assignment to rc below from |= to =).

> > +    /* The hardware domain is initially permitted full I/O capabilities. */
> > +    rc |= ioports_permit_access(d, 0, 0xFFFF);
> > +    rc |= iomem_permit_access(d, 0UL, (1UL << (paddr_bits - PAGE_SHIFT)) - 1);
> > +    rc |= irqs_permit_access(d, 1, nr_irqs_gsi - 1);
> > +
> > +    /*
> > +     * Modify I/O port access permissions.
> > +     */
> 
> This is a single line comment - I understand it's trying to be more of a
> separator than the others, but I'd prefer for it to do so by being
> followed by a blank line.
> 
> > +    /* Master Interrupt Controller (PIC). */
> > +    rc |= ioports_deny_access(d, 0x20, 0x21);
> > +    /* Slave Interrupt Controller (PIC). */
> > +    rc |= ioports_deny_access(d, 0xA0, 0xA1);
> > +    /* Interval Timer (PIT). */
> > +    rc |= ioports_deny_access(d, 0x40, 0x43);
> > +    /* PIT Channel 2 / PC Speaker Control. */
> > +    rc |= ioports_deny_access(d, 0x61, 0x61);
> > +    /* ACPI PM Timer. */
> > +    if ( pmtmr_ioport )
> > +        rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
> > +    /* PCI configuration space (NB. 0xcf8 has special treatment). */
> > +    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
> > +    /* Command-line I/O ranges. */
> > +    process_dom0_ioports_disable(d);
> > +
> > +    /*
> > +     * Modify I/O memory access permissions.
> > +     */
> 
> Dito.
> 
> > -    BUG_ON(rc != 0);
> > +    rc = setup_permissions(d);
> > +    if ( rc != 0 )
> > +        panic("Failed to setup Dom0 permissions");
> 
> To be honest, I'm not sure of this BUG_ON() -> panic() conversion.
> I think I'd prefer it to stay the way it was. We're not really expecting
> for any of this to fail anyway.
> 
> Jan

Done, fixed all the above, thanks.

Roger.
diff mbox

Patch

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index 04d6cb0..ffd0521 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -869,6 +869,89 @@  static __init void setup_pv_physmap(struct domain *d, unsigned long pgtbl_pfn,
     unmap_domain_page(l4start);
 }
 
+static int __init setup_permissions(struct domain *d)
+{
+    unsigned long mfn;
+    int i, rc = 0;
+
+    /* The hardware domain is initially permitted full I/O capabilities. */
+    rc |= ioports_permit_access(d, 0, 0xFFFF);
+    rc |= iomem_permit_access(d, 0UL, (1UL << (paddr_bits - PAGE_SHIFT)) - 1);
+    rc |= irqs_permit_access(d, 1, nr_irqs_gsi - 1);
+
+    /*
+     * Modify I/O port access permissions.
+     */
+    /* Master Interrupt Controller (PIC). */
+    rc |= ioports_deny_access(d, 0x20, 0x21);
+    /* Slave Interrupt Controller (PIC). */
+    rc |= ioports_deny_access(d, 0xA0, 0xA1);
+    /* Interval Timer (PIT). */
+    rc |= ioports_deny_access(d, 0x40, 0x43);
+    /* PIT Channel 2 / PC Speaker Control. */
+    rc |= ioports_deny_access(d, 0x61, 0x61);
+    /* ACPI PM Timer. */
+    if ( pmtmr_ioport )
+        rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
+    /* PCI configuration space (NB. 0xcf8 has special treatment). */
+    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
+    /* Command-line I/O ranges. */
+    process_dom0_ioports_disable(d);
+
+    /*
+     * Modify I/O memory access permissions.
+     */
+    /* Local APIC. */
+    if ( mp_lapic_addr != 0 )
+    {
+        mfn = paddr_to_pfn(mp_lapic_addr);
+        rc |= iomem_deny_access(d, mfn, mfn);
+    }
+    /* I/O APICs. */
+    for ( i = 0; i < nr_ioapics; i++ )
+    {
+        mfn = paddr_to_pfn(mp_ioapics[i].mpc_apicaddr);
+        if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) )
+            rc |= iomem_deny_access(d, mfn, mfn);
+    }
+    /* MSI range. */
+    rc |= iomem_deny_access(d, paddr_to_pfn(MSI_ADDR_BASE_LO),
+                            paddr_to_pfn(MSI_ADDR_BASE_LO +
+                                         MSI_ADDR_DEST_ID_MASK));
+    /* HyperTransport range. */
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+        rc |= iomem_deny_access(d, paddr_to_pfn(0xfdULL << 32),
+                                paddr_to_pfn((1ULL << 40) - 1));
+
+    /* Remove access to E820_UNUSABLE I/O regions above 1MB. */
+    for ( i = 0; i < e820.nr_map; i++ )
+    {
+        unsigned long sfn, efn;
+        sfn = max_t(unsigned long, paddr_to_pfn(e820.map[i].addr), 0x100ul);
+        efn = paddr_to_pfn(e820.map[i].addr + e820.map[i].size - 1);
+        if ( (e820.map[i].type == E820_UNUSABLE) &&
+             (e820.map[i].size != 0) &&
+             (sfn <= efn) )
+            rc |= iomem_deny_access(d, sfn, efn);
+    }
+
+    /* Prevent access to HPET */
+    if ( hpet_address )
+    {
+        u8 prot_flags = hpet_flags & ACPI_HPET_PAGE_PROTECT_MASK;
+
+        mfn = paddr_to_pfn(hpet_address);
+        if ( prot_flags == ACPI_HPET_PAGE_PROTECT4 )
+            rc |= iomem_deny_access(d, mfn, mfn);
+        else if ( prot_flags == ACPI_HPET_PAGE_PROTECT64 )
+            rc |= iomem_deny_access(d, mfn, mfn + 15);
+        else if ( ro_hpet )
+            rc |= rangeset_add_singleton(mmio_ro_ranges, mfn);
+    }
+
+    return rc;
+}
+
 int __init construct_dom0(
     struct domain *d,
     const module_t *image, unsigned long image_headroom,
@@ -1539,84 +1622,9 @@  int __init construct_dom0(
     if ( test_bit(XENFEAT_supervisor_mode_kernel, parms.f_required) )
         panic("Dom0 requires supervisor-mode execution");
 
-    rc = 0;
-
-    /* The hardware domain is initially permitted full I/O capabilities. */
-    rc |= ioports_permit_access(d, 0, 0xFFFF);
-    rc |= iomem_permit_access(d, 0UL, (1UL << (paddr_bits - PAGE_SHIFT)) - 1);
-    rc |= irqs_permit_access(d, 1, nr_irqs_gsi - 1);
-
-    /*
-     * Modify I/O port access permissions.
-     */
-    /* Master Interrupt Controller (PIC). */
-    rc |= ioports_deny_access(d, 0x20, 0x21);
-    /* Slave Interrupt Controller (PIC). */
-    rc |= ioports_deny_access(d, 0xA0, 0xA1);
-    /* Interval Timer (PIT). */
-    rc |= ioports_deny_access(d, 0x40, 0x43);
-    /* PIT Channel 2 / PC Speaker Control. */
-    rc |= ioports_deny_access(d, 0x61, 0x61);
-    /* ACPI PM Timer. */
-    if ( pmtmr_ioport )
-        rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
-    /* PCI configuration space (NB. 0xcf8 has special treatment). */
-    rc |= ioports_deny_access(d, 0xcfc, 0xcff);
-    /* Command-line I/O ranges. */
-    process_dom0_ioports_disable(d);
-
-    /*
-     * Modify I/O memory access permissions.
-     */
-    /* Local APIC. */
-    if ( mp_lapic_addr != 0 )
-    {
-        mfn = paddr_to_pfn(mp_lapic_addr);
-        rc |= iomem_deny_access(d, mfn, mfn);
-    }
-    /* I/O APICs. */
-    for ( i = 0; i < nr_ioapics; i++ )
-    {
-        mfn = paddr_to_pfn(mp_ioapics[i].mpc_apicaddr);
-        if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn) )
-            rc |= iomem_deny_access(d, mfn, mfn);
-    }
-    /* MSI range. */
-    rc |= iomem_deny_access(d, paddr_to_pfn(MSI_ADDR_BASE_LO),
-                            paddr_to_pfn(MSI_ADDR_BASE_LO +
-                                         MSI_ADDR_DEST_ID_MASK));
-    /* HyperTransport range. */
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
-        rc |= iomem_deny_access(d, paddr_to_pfn(0xfdULL << 32),
-                                paddr_to_pfn((1ULL << 40) - 1));
-
-    /* Remove access to E820_UNUSABLE I/O regions above 1MB. */
-    for ( i = 0; i < e820.nr_map; i++ )
-    {
-        unsigned long sfn, efn;
-        sfn = max_t(unsigned long, paddr_to_pfn(e820.map[i].addr), 0x100ul);
-        efn = paddr_to_pfn(e820.map[i].addr + e820.map[i].size - 1);
-        if ( (e820.map[i].type == E820_UNUSABLE) &&
-             (e820.map[i].size != 0) &&
-             (sfn <= efn) )
-            rc |= iomem_deny_access(d, sfn, efn);
-    }
-
-    /* Prevent access to HPET */
-    if ( hpet_address )
-    {
-        u8 prot_flags = hpet_flags & ACPI_HPET_PAGE_PROTECT_MASK;
-
-        mfn = paddr_to_pfn(hpet_address);
-        if ( prot_flags == ACPI_HPET_PAGE_PROTECT4 )
-            rc |= iomem_deny_access(d, mfn, mfn);
-        else if ( prot_flags == ACPI_HPET_PAGE_PROTECT64 )
-            rc |= iomem_deny_access(d, mfn, mfn + 15);
-        else if ( ro_hpet )
-            rc |= rangeset_add_singleton(mmio_ro_ranges, mfn);
-    }
-
-    BUG_ON(rc != 0);
+    rc = setup_permissions(d);
+    if ( rc != 0 )
+        panic("Failed to setup Dom0 permissions");
 
     if ( elf_check_broken(&elf) )
         printk(" Xen warning: dom0 kernel broken ELF: %s\n",