Re: [Xen-devel] [PATCH v2 19/30] xen/dcpi: add a dpci passthrough handler for hardware domain

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Mon, Oct 03, 2016 at 10:02:27AM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne [mailto:roger.pau@xxxxxxxxxx]
> > Sent: 27 September 2016 16:57
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: konrad.wilk@xxxxxxxxxx; boris.ostrovsky@xxxxxxxxxx; Roger Pau Monne
> > <roger.pau@xxxxxxxxxx>; Paul Durrant <Paul.Durrant@xxxxxxxxxx>; Jan
> > Beulich <jbeulich@xxxxxxxx>; Andrew Cooper
> > <Andrew.Cooper3@xxxxxxxxxx>
> > Subject: [PATCH v2 19/30] xen/dcpi: add a dpci passthrough handler for
> > hardware domain
> > 
> > This is very similar to the PCI trap used for the traditional PV(H) Dom0.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > Cc: Jan Beulich <jbeulich@xxxxxxxx>
> > Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/io.c         | 72
> > ++++++++++++++++++++++++++++++++++++++++++-
> >  xen/arch/x86/traps.c          | 39 -----------------------
> >  xen/drivers/passthrough/pci.c | 39 +++++++++++++++++++++++
> >  xen/include/xen/pci.h         |  2 ++
> >  4 files changed, 112 insertions(+), 40 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index
> > 1e7a5f9..31d54dc 100644
> > --- a/xen/arch/x86/hvm/io.c
> > +++ b/xen/arch/x86/hvm/io.c
> > @@ -247,12 +247,79 @@ static int dpci_portio_write(const struct
> > hvm_io_handler *handler,
> >      return X86EMUL_OKAY;
> >  }
> > 
> > +static bool_t hw_dpci_portio_accept(const struct hvm_io_handler
> > *handler,
> > +                                    const ioreq_t *p) {
> > +    if ( (p->addr == 0xcf8 && p->size == 4) || (p->addr & 0xfffc) == 0xcfc)
> > +    {
> > +        return 1;
> > +    }
> > +
> > +    return 0;
> 
> Why not just return the value of the boolean expression?

Thanks, fixed.
 
> > +}
> > +
> > +static int hw_dpci_portio_read(const struct hvm_io_handler *handler,
> > +                            uint64_t addr,
> > +                            uint32_t size,
> > +                            uint64_t *data) {
> > +    struct domain *currd = current->domain;
> > +
> > +    if ( addr == 0xcf8 )
> > +    {
> > +        ASSERT(size == 4);
> > +        *data = currd->arch.pci_cf8;
> > +        return X86EMUL_OKAY;
> > +    }
> > +
> > +    ASSERT((addr & 0xfffc) == 0xcfc);
> 
> You could do 'addr &= 3' and simplify the expressions below.

Fixed.
 
> > +    size = min(size, 4 - ((uint32_t)addr & 3));
> > +    if ( size == 3 )
> > +        size = 2;
> > +    if ( pci_cfg_ok(currd, addr & 3, size, NULL) )
> 
> Is this the right place to do the check. Can this not be done in the accept 
> op?

In the intercept op we don't know if the operation is a read or a write, and 
pci_cfg_ok seems to do more than a check, it calls pci_conf_write_intercept. 

> > +        *data = pci_conf_read(currd->arch.pci_cf8, addr & 3, size);
> > +
> > +    return X86EMUL_OKAY;
> > +}
> > +
> > +static int hw_dpci_portio_write(const struct hvm_io_handler *handler,
> > +                                uint64_t addr,
> > +                                uint32_t size,
> > +                                uint64_t data) {
> > +    struct domain *currd = current->domain;
> > +    uint32_t data32;
> > +
> > +    if ( addr == 0xcf8 )
> > +    {
> > +            ASSERT(size == 4);
> > +            currd->arch.pci_cf8 = data;
> > +            return X86EMUL_OKAY;
> > +    }
> > +
> > +    ASSERT((addr & 0xfffc) == 0xcfc);
> 
> 'addr &= 3' again here.

Thanks.

Roger.

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