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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.09.17 at 13:30, <roger.pau@xxxxxxxxxx> wrote:
> 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.

Sure, but the structure needs allocating. Which again is something
I wouldn't want to have to worry about when adding _temporary_
debugging or statistics code. But this is all moot anyway with ...

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

... this.

Jan


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