Re: [Xen-devel] [PATCH v2 10/11] ioreq: split the code to detect PCI config space accesses

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Sep 10, 2019 at 04:06:20PM +0200, Paul Durrant wrote:
> > -----Original Message-----
> > From: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> > Sent: 03 September 2019 17:14
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>; Paul Durrant 
> > <Paul.Durrant@xxxxxxxxxx>; Jan Beulich
> > <jbeulich@xxxxxxxx>; Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu 
> > <wl@xxxxxxx>
> > Subject: [PATCH v2 10/11] ioreq: split the code to detect PCI config space 
> > accesses
> > 
> > Place the code that converts a PIO/COPY ioreq into a PCI_CONFIG one
> > into a separate function, and adjust the code to make use of this
> > newly introduced function.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > Changes since v1:
> >  - New in this version.
> > ---
> >  xen/arch/x86/hvm/ioreq.c | 111 +++++++++++++++++++++++----------------
> >  1 file changed, 67 insertions(+), 44 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> > index fecdc2786f..33c56b880c 100644
> > --- a/xen/arch/x86/hvm/ioreq.c
> > +++ b/xen/arch/x86/hvm/ioreq.c
> > @@ -183,6 +183,54 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu *sv, 
> > ioreq_t *p)
> >      return true;
> >  }
> > 
> > +static void convert_pci_ioreq(struct domain *d, ioreq_t *p)
> > +{
> > +    const struct hvm_mmcfg *mmcfg;
> > +    uint32_t cf8 = d->arch.hvm.pci_cf8;
> > +
> > +    if ( p->type != IOREQ_TYPE_PIO && p->type != IOREQ_TYPE_COPY )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        return;
> > +    }
> > +
> > +    read_lock(&d->arch.hvm.mmcfg_lock);
> 
> Actually, looking at this... can you not restrict holding the mmcfg_lock...
> 
> > +    if ( (p->type == IOREQ_TYPE_PIO &&
> > +          (p->addr & ~3) == 0xcfc &&
> > +          CF8_ENABLED(cf8)) ||
> > +         (p->type == IOREQ_TYPE_COPY &&
> > +          (mmcfg = hvm_mmcfg_find(d, p->addr)) != NULL) )
> > +    {
> > +        uint32_t x86_fam;
> > +        pci_sbdf_t sbdf;
> > +        unsigned int reg;
> > +
> > +        reg = p->type == IOREQ_TYPE_PIO ? hvm_pci_decode_addr(cf8, p->addr,
> > +                                                              &sbdf)
> > +                                        : hvm_mmcfg_decode_addr(mmcfg, 
> > p->addr,
> > +                                                                &sbdf);
> 
> ... to within hvm_mmcfg_decode_addr()?

Hm, not really. There's a call to hvm_mmcfg_find in the if condition
which needs the lock to be held, and then breaking this into two
different lock sections (pick lock, get mmcfg, unlock, pick lock,
decode, unlock) could lead to the mmcfg region being freed under our
feet.

I think the locking needs to stay as-is unless we switch to a
different locking model for the mmcfg regions. Note it's a read lock,
so it shouldn't have any contention since modifying the mmcfg region
list is very rare.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel