Re: [PATCH-for-4.9 v1 4/8] dm_op: convert HVMOP_set_pci_intx_level, HVMOP_set_isa_irq_level, and...

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 25 November 2016 11:50
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; xen-
> devel@lists.xenproject.org; Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Subject: Re: [Xen-devel] [PATCH-for-4.9 v1 4/8] dm_op: convert
> HVMOP_set_pci_intx_level, HVMOP_set_isa_irq_level, and...
> 
> >>> On 18.11.16 at 18:14, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/dm.c
> > +++ b/xen/arch/x86/hvm/dm.c
> > @@ -105,6 +105,60 @@ static int dm_op_track_dirty_vram(struct domain
> *d,
> >          hap_track_dirty_vram(d, first_pfn, nr, buf.h);
> >  }
> >
> > +static int dm_op_set_pci_intx_level(struct domain *d, uint8_t domain,
> > +                                    uint8_t bus, uint8_t device,
> > +                                    uint8_t intx, uint8_t level)
> 
> Btw., none of these static helper functions have any need to have
> a dm_op_ prefix. Such disambiguation is needed only for non-
> static ones.

It make it easier to find them quickly with cscope, but I can drop the prefix if you prefer.

> 
> > +static int dm_op_set_pci_link_route(struct domain *d, uint8_t link,
> > +                                    uint8_t isa_irq)
> > +{
> > +    if ( link > 3 || isa_irq > 15 )
> > +        return -EINVAL;
> > +
> > +    hvm_set_pci_link_route(d, link, isa_irq);
> > +
> > +    return 0;
> > +}
> 
> In the hvmctl series I did specifically avoid to create this trivial a
> wrapper for a function with no other callers. Simply move the
> argument range checks there.
> 

Ok, I'll inline in the switch if you prefer that style.

> > +struct xen_dm_op_set_pci_intx_level {
> > +    /* IN - PCI INTx identification (domain:bus:device:intx) */
> > +    uint8_t  domain, bus, device, intx;
> 
> I've just now noticed that I did overlook this in the hvmctl series
> too: domain should be uint16_t.
> 

Indeed it should.

  Paul

> Jan


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