Re: [Xen-devel] [PATCH v2 07/30] xen/x86: split the setup of Dom0 permissions to a function

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.16 at 17:57, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx>

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

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