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

[Roger Pau Monne <roger.pau@citrix.com>]

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.

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