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 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.

> > +/*
> > + * Merge new data into a partial result.
> > + *
> > + * Zero the bytes of 'data' from [offset, offset + size), and
> > + * merge the value found in 'new' from [0, offset) left shifted
> 
> DYM [0, size) here?

Yes, will fix.

> I also have to admit that I find it strange that
> you talk of zeroing something here - the net effect of the function
> is not producing any zeros anywhere afaict. Such a pre-function
> comment is normally describing the effect of the function as seen
> to the caller rather than its inner workings.

OK, would it be better to write it as:

/*
 * Merge new data into a partial result.
 *
 * Copy the value found in 'new' from [0, size) left shifted by
 * 'offset' into 'data'.
 */

> > + */
> > +typedef uint32_t vpci_read_t(struct pci_dev *pdev, unsigned int reg,
> > +                             const void *data);
> > +
> > +typedef void vpci_write_t(struct pci_dev *pdev, unsigned int reg,
> > +                          uint32_t val, void *data);
> 
> Do these two really need access to the struct pci_dev, rather than
> just struct vpci? And if they do, then are they really permitted to
> alter that struct pci_dev?

I'm leaning towards pdev because it already contains vpci. AFAICT it
should be fine to constify it.

Thanks, Roger.

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