Just a small correction - Not if (req_end >= field_start && field_end >= req_start) But if (req_end *>* field_start && field_end *>* req_start)
On Mon, Jun 20, 2016 at 12:23 PM, Andrey Grodzovsky <andrey2...@gmail.com> wrote: > > > On Mon, Jun 20, 2016 at 11:38 AM, Jan Beulich <jbeul...@suse.com> wrote: > >> >>> On 20.06.16 at 17:15, <andrey2...@gmail.com> wrote: >> > 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. >> >> Let's leave quirks out of the picture for now. And without quirk, >> f2 is not writable (and cannot be made by adding a quirk, as >> explained before). >> >> Yes, i guess due to not allowing duplicates. > > >> > 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. >> >> Hmm, with that and the >= -> > adjustment, I can't really see >> how your variant is different from the original (other than being >> more compact). What am I overlooking here? >> >> >> > --- 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. >> >> See above - to me the original expression looks (a) correct and >> (b) identical in effect to the corrected version of yours. Hence >> if behavior changes, something must be wrong in either old or new >> code, and due to (a) I would imply it's in the new one. But as said >> above - I guess I'm missing something here. >> > > Here is printouts with applying the new logic > > [ 382.222213] xen-pciback: 0000:06:00.0: write request 4 bytes at 0xc = > 4000 > [ 382.222214] xen-pciback: 0000:06:00.0: read 1 bytes at 0xc > [ 382.222228] xen-pciback: 0000:06:00.0: read 1 bytes at 0xc = 0 > [ 382.222238] xen-pciback: 0000:06:00.0: read 1 bytes at 0xd > [ 382.222281] xen-pciback: 0000:06:00.0: read 1 bytes at 0xd = 0 > [ 382.222313] xen-pciback: 0000:06:00.0: read 1 bytes at 0xf > [ 382.222341] xen-pciback: 0000:06:00.0: read 1 bytes at 0xf = 0 > > So from prints the logic is not equivalent. 0xd is included in this case. > > In original logic field 0xd is excluded because > Not if ((req_start >= field_start && req_start < field_end) > Nor (req_end > field_start && req_end <= field_end)) evaluate to true. > Where request would be 4b segment starting with 0xc > While f (req_end >= field_start && field_end >= req_start) will evaluate > for true in this case. > > Unless I am missing something... > >> >> Jan >> > > Andrey >
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
