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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>> 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'.
>  */

Yes.

>> > + */
>> > +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.

Well, I certainly guessed that to be the reason for the way things
are now. But what data in the physical device structure would such
a handler possibly require? Oh, looking at later patches, you take
SBDF from there. That's certainly a good enough reason then.

> AFAICT it should be fine to constify it.

In which case please do.

Jan


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