Re: [Xen-devel] [PATCH v2] vpci: honor read-only devices

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

On Tue, Sep 03, 2019 at 11:09:09AM +0200, Jan Beulich wrote:
> On 02.09.2019 17:30, Roger Pau Monne wrote:
> > --- a/xen/drivers/vpci/vpci.c
> > +++ b/xen/drivers/vpci/vpci.c
> > @@ -418,13 +418,21 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
> > unsigned int size,
> >          return;
> >      }
> >  
> > -    /*
> > -     * Find the PCI dev matching the address.
> > -     * Passthrough everything that's not trapped.
> > -     */
> > +    /* Find the PCI dev matching the address. */
> >      pdev = pci_get_pdev_by_domain(d, sbdf.seg, sbdf.bus, sbdf.devfn);
> >      if ( !pdev )
> >      {
> > +        const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
> > +
> > +        if ( ro_map && test_bit(sbdf.bdf, ro_map) )
> > +            /* Ignore writes to read-only devices. */
> > +            return;
> > +
> > +        /*
> > +         * Let the hardware domain access config space regions for 
> > non-existent
> > +         * devices.
> > +         * TODO: revisit for domU support.
> > +         */
> >          vpci_write_hw(sbdf, reg, size, data);
> >          return;
> >      }
> > 
> 
> In principle I'm okay with the change, but I have two more things
> to be considered:
> 
> 1) I'd prefer if the check was independent of the return value of
> pci_get_pdev_by_domain(), to be more robust against the r/o map
> having got updated but the owner still being hwdom.

So the RO check would be done ahead of the owner check?

I can do that, but it seems like a bodge for the locking issues (or
lack of it) we have in the handling of PCI devices. I assume having a
RO device assigned to a domain different than dom_xen is not possible.

> 2) Wouldn't it be better to move the check into the callers of
> vpci_write(), to avoid the duplicate lookup in the qword-MCFG-
> write case? The main questionable point here is where, for DomU
> support, the SBDF translation is going to live.

So I have a series I'm going to send quite soon in order to integrate
vPCI with ioreq, as a first step in order to make it available to
domUs.

The SBDF translation there is going to be performed by the ioreq code
(ie: hvm_select_ioreq_server), but checking against the RO map there
would be wrong, as ioreq doesn't know whether the underlying handler
is for an emulated device or for a passthrough one. I think the RO
check needs to be in the vPCI code itself.

Thanks, Roger.

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