On Mon, Sep 04, 2017 at 09:38:11AM -0600, Jan Beulich wrote: > >>> On 14.08.17 at 16:28, <roger....@citrix.com> 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@lists.xen.org https://lists.xen.org/xen-devel
