Re: [Xen-devel] [PATCH v5 02/11] vpci: introduce basic handlers to trap accesses to the PCI config space

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

On Thu, Sep 07, 2017 at 03:06:50AM -0600, Jan Beulich wrote:
> >>> On 06.09.17 at 17:40, <roger.pau@xxxxxxxxxx> wrote:
> > On Mon, Sep 04, 2017 at 09:38:11AM -0600, Jan Beulich wrote:
> >> >>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> >> > Changes since v4:
> >> >[...]
> >> > * Hypervisor code:
> >> >[...]
> >> >  - Constify the data opaque parameter of read handlers.
> >> 
> >> Is that a good idea? Such callbacks should generally be allowed to
> >> modify their state even if the operation is just a read - think of a
> >> private lock or statistics/debugging data to be updated.
> > 
> > Right now the consistency of the opaque data is kept by the global
> > vpci lock, which I like because it makes the code simpler. If the
> > opaque data is not constified for the read handlers then I would be
> > against using a read/write lock.
> > 
> > Statistic/debug data IMHO should be kept in a separate structure with
> > it's own lock, that's referenced by the opaque data. This would allow
> > Xen to not allocate this for non-debug builds, reducing memory
> > footprint and lock contention in production.
> 
> I don't like this, as it makes adding such transiently needlessly
> hard (as one would need to drop all the const-s or cast them away).

Hm, I'm not sure I follow. I was thinking of something along the lines
of:

struct vpci_msi_stats {...};

struct vpci_msi {
    [...]
    struct vpci_msi_stats *stats;
};

Then you can easily have a handler like:

static void vpci_msi_reg(..., const void *data)
{
    const struct vpci_msi *msi = data;
    struct vpci_msi_stats *stats = msi->stats;
    [...]
}

That should work AFAICT.

> What was the reason for switching to the rwlock anyway? Did you
> measure any performance problems? Are there Dom0 kernels not
> serializing PCI config space accesses anyway?

Not that I know of, but the PCIe spec doesn't seem to require non
concurrent accesses. PCI of course must not be concurrent.

> Would it be an
> alternative to make the (spin) lock per-device rather than per-
> domain? That might also be a good idea for pass-through (as there
> Dom0 as well as the owning DomU fundamentally have access to
> the config space of a device, and they'd better be synchronized).

That would also work, then I agree it should be a spin lock and the
const from the read handlers can be dropped. Unless you say otherwise
I'm going to implement this.

Thanks, Roger.

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