On Mon, Jun 20, 2016 at 4:56 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 20.06.16 at 00:03, <andrey2...@gmail.com> wrote: > > Follow up on > http://www.gossamer-threads.com/lists/xen/devel/436000#436000 > > Using > http://eli.thegreenplace.net/2008/08/15/intersection-of-1d-segments as > > reference. > > > > New value > > |---------------| > > > > f1 f5 > > |---| |-----| > > f2 f4 > > |-----| f3 |-----| > > |-----| > > > > Given a new value of the type above, Current logic will not > > allow applying parts of the new value overlapping with f3 filter. > > only f2 and f4. > > I remains unclear what f1...f5 are meant to represent here. > f1-f5 would be config_field[] such as header_common in conf_space_header.c or any other config_field added (by adding quirks for example). > > > This change allows all 3 types of overlapes to be included. > > More specifically for passthrough an Industrial Ethernet Interface > > (Hilscher GmbH CIFX 50E-DP(M/S)) on a HVM DomU running the > > Xen 4.6 Hypervisor it allows to restore the LATENCY TIMER field > > given a quirk to allow read/write for that field is already in place. > > Device driver logic is such that the entire confspace is > > written in 4 byte chunks s.t. LATENCY_TIMER AND CACHE_LINE_SIZE are > > arriving together in one call to xen_pcibk_config_write. > > Tweaks to make individual devices work are dubious. Any > explanation should outline why current behavior is _generally_ > wrong. In particular with the original issue being with the > Latency Timer field, and with that field now being allowed to > be written by its entry in header_common[], ... To me current behaviour looks generally inconsistent because given a request to wrote 4 bytes starting with 0xC let's look what's happening inside xen_pcibk_config_write *[81664.632262 < 0.000035>] xen-pciback: 0000:06:00.0: write request 4 bytes at 0xc = 4000* *[81664.632264 < 0.000002>] xen-pciback: 0000:06:00.0: read 1 bytes at 0xc* *[81664.632275 < 0.000011>] xen-pciback: 0000:06:00.0: read 1 bytes at 0xc = 0 * *[81664.632282 < 0.000002>] xen-pciback: 0000:06:00.0: read 1 bytes at 0xf* *[81664.632292 < 0.000010>] xen-pciback: 0000:06:00.0: read 1 bytes at 0xf = 0* So you can see that current logic will allow to read/write 0xc which is PCI_CACHE_LINE_SIZE, skips PCI_LATENCY_TIMER also there is a quirk in place there which allows writes to this memory, skips 0xE (which is fine since this field is not allowed to be accessed) and then writes 0xf PCI_BIST So using my previous sketch adjusted for this use case |----4b write request starting at 0xc----| |--f1--| |--f2--| |--f3--| Where f1 == PCI_CACHE_LINE_SIZE f2 == PCI_LATENCY_TIMER f3 == PCI_BIST With ciurrent logic Only f1 and f3 are allowed but not f2 even when there is a field and a quirk in place allowing read write access to that memory location. To me it seems as a generally inconsistent behaviour and not specifically related to our driver. With my patch (and a fix from || to && Thanks a lot for pointing this out to me.) f1, f2 and f3 are being treated the same which IMHO is more correct. > > --- a/drivers/xen/xen-pciback/conf_space.c > > +++ b/drivers/xen/xen-pciback/conf_space.c > > @@ -230,8 +230,7 @@ int xen_pcibk_config_write(struct pci_dev *dev, int > > offset, int size, u32 value) > > field_start = OFFSET(cfg_entry); > > field_end = OFFSET(cfg_entry) + field->size; > > > > - if ((req_start >= field_start && req_start < field_end) > > - || (req_end > field_start && req_end <= field_end)) { > > + if (req_end >= field_start || field_end >= req_start) { > > tmp_val = 0; > > ... any change to the logic here which results in writes to the field > getting issued would seem wrong without even looking at the > particular nature of the field. > I am not sure I understand - please clarify. > > As to the actual change - the two _end variables hold values > pointing _past_ the region of interest, so the use of >= seems > wrong here (ought to be >). And in the end we're talking of a > classical overlap check here, which indeed can be simplified (to > just two comparisons), but such simplification mustn't result in a > change of behavior. (Such a simplification would end up being > > if (req_end > field_start && field_end > req_start) { > > afaict - note the || instead of && used in your change.) > Totally agree, than you for this insight! > > Jan > > Andrey
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
