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

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

On Tue, Sep 03, 2019 at 11:48:19AM +0200, Jan Beulich wrote:
> On 03.09.2019 11:28, Roger Pau Monné  wrote:
> > 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.
> 
> It ought not be possible. Hence me saying "more robust" (i.e. in
> case the "ought not" somehow gets broken). And no, the comment
> wasn't really related to the (lack of) locking here - that's an
> orthogonal issue.

Ack, I have to send v3 anyway since I was missing some changes to the
vPCI test harness anyway.

> >> 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.
> 
> Oh, sure. The question then simply converts to "Where can it be done
> the earliest?" I.e. when/where do we have the physical SBDF in our
> hands?

I'm going to introduce a vPCI ioreq handler that will forward accesses
to vpci_{read/write}, but that's not here yet, and in any case it's
not going to make much of a difference IMO.

I think at least ATM the best place to put the check is in vpci_write,
later on we can see about moving it, there's a TODO next to it which
will help identify this.

Thanks, Roger.

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