Xen project Mailing List

On Mon, Jun 20, 2016 at 11:38 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 20.06.16 at 17:15, <andrey2805@xxxxxxxxx> wrote:
> 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.

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

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