[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] xen/pciback: Update data filter intersection logic.




On Mon, Jun 20, 2016 at 4:56 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>> On 20.06.16 at 00:03, <andrey2805@xxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.