[PATCH] xen/pciback: Update data filter intersection logic.

[PATCH] xen/pciback: Update data filter intersection logic.

[Andrey Grodzovsky]

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.

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.

Cc: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrey Grodzovsky <andrey2805@gmail.com>
---
 drivers/xen/xen-pciback/conf_space.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/xen/xen-pciback/conf_space.c b/drivers/xen/xen-pciback/conf_space.c
index 8e67336..317fb14 100644
--- 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;
 
 			err = xen_pcibk_config_read(dev, field_start,
-- 
2.1.4


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

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

[Jan Beulich]

>>> On 20.06.16 at 00:03, <andrey2805@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.

> 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[], ...

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

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

Jan


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

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

[Boris Ostrovsky]

On 06/20/2016 04:56 AM, Jan Beulich wrote:
>>>> On 20.06.16 at 00:03, <andrey2805@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.
>
>> 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[], ...
>
>> --- 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.
>
> 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.)

Will setting header_common[]'s PCI_CACHE_LINE_SIZE size field to 2 (and 
adding a proper comment) solve this problem?

-boris


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

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

[Jan Beulich]

>>> On 20.06.16 at 14:58, <boris.ostrovsky@oracle.com> wrote:

> 
> On 06/20/2016 04:56 AM, Jan Beulich wrote:
>>>>> On 20.06.16 at 00:03, <andrey2805@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.
>>
>>> 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[], ...
>>
>>> --- 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.
>>
>> 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.)
> 
> Will setting header_common[]'s PCI_CACHE_LINE_SIZE size field to 2 (and 
> adding a proper comment) solve this problem?

How would that work? We mean to not allow writes, or else we
could simply add a .u.b.write handler for PCI_LATENCY_TIMER.

Jan


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

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

[Boris Ostrovsky]

On 06/20/2016 09:30 AM, Jan Beulich wrote:
>>>> On 20.06.16 at 14:58, <boris.ostrovsky@oracle.com> wrote:
>> On 06/20/2016 04:56 AM, Jan Beulich wrote:
>>>>>> On 20.06.16 at 00:03, <andrey2805@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.
>>>
>>>> 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[], ...
>>>
>>>> --- 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.
>>>
>>> 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.)
>> Will setting header_common[]'s PCI_CACHE_LINE_SIZE size field to 2 (and
>> adding a proper comment) solve this problem?
> How would that work? We mean to not allow writes, or else we
> could simply add a .u.b.write handler for PCI_LATENCY_TIMER.

I thought you suggested (in another thread) to make PCI_LATENCY_TIMER 
writable, just like PCI_CACHE_LINE_SIZE?

-boris

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

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

[Jan Beulich]

>>> On 20.06.16 at 15:36, <boris.ostrovsky@oracle.com> wrote:

> 
> On 06/20/2016 09:30 AM, Jan Beulich wrote:
>>>>> On 20.06.16 at 14:58, <boris.ostrovsky@oracle.com> wrote:
>>> On 06/20/2016 04:56 AM, Jan Beulich wrote:
>>>>>>> On 20.06.16 at 00:03, <andrey2805@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.
>>>>
>>>>> 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[], ...
>>>>
>>>>> --- 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.
>>>>
>>>> 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.)
>>> Will setting header_common[]'s PCI_CACHE_LINE_SIZE size field to 2 (and
>>> adding a proper comment) solve this problem?
>> How would that work? We mean to not allow writes, or else we
>> could simply add a .u.b.write handler for PCI_LATENCY_TIMER.
> 
> I thought you suggested (in another thread) to make PCI_LATENCY_TIMER 
> writable, just like PCI_CACHE_LINE_SIZE?

Well, I did put this up for discussion (as much as I questioned
whether Cache Line Size should perhaps not be writable). And if
we wanted to make it writable, then we should do so the ordinary
way, not via some hacks. (And looking at it again I don't even
understand why Cache Line Size has an entry in header_common[]
- identical behavior would result with the entry dropped afaict.)

Jan


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

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

[Boris Ostrovsky]

On 06/20/2016 09:55 AM, Jan Beulich wrote:
>>>> On 20.06.16 at 15:36, <boris.ostrovsky@oracle.com> wrote:
>> On 06/20/2016 09:30 AM, Jan Beulich wrote:
>>>>>> On 20.06.16 at 14:58, <boris.ostrovsky@oracle.com> wrote:
>>>> On 06/20/2016 04:56 AM, Jan Beulich wrote:
>>>>>>>> On 20.06.16 at 00:03, <andrey2805@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.
>>>>>
>>>>>> 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[], ...
>>>>>
>>>>>> --- 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.
>>>>>
>>>>> 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.)
>>>> Will setting header_common[]'s PCI_CACHE_LINE_SIZE size field to 2 (and
>>>> adding a proper comment) solve this problem?
>>> How would that work? We mean to not allow writes, or else we
>>> could simply add a .u.b.write handler for PCI_LATENCY_TIMER.
>> I thought you suggested (in another thread) to make PCI_LATENCY_TIMER 
>> writable, just like PCI_CACHE_LINE_SIZE?
> Well, I did put this up for discussion (as much as I questioned
> whether Cache Line Size should perhaps not be writable). And if
> we wanted to make it writable, then we should do so the ordinary
> way, not via some hacks. 

That depends on how we want to view header_common's offset field:
whether it's address of a specific register (which is what it is now) or
it's just an offset and length of an access within config space (which
AFAIUI is how the driver in question wants to treat config space) and
it's up to read/write ops to sort out specifics if necessary.

> (And looking at it again I don't even
> understand why Cache Line Size has an entry in header_common[]
> - identical behavior would result with the entry dropped afaict.)

I think so too.

-boris
>



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

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

[Jan Beulich]

>>> On 20.06.16 at 17:01, <boris.ostrovsky@oracle.com> wrote:
> On 06/20/2016 09:55 AM, Jan Beulich wrote:
>>>>> On 20.06.16 at 15:36, <boris.ostrovsky@oracle.com> wrote:
>>> On 06/20/2016 09:30 AM, Jan Beulich wrote:
>>>>>>> On 20.06.16 at 14:58, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 06/20/2016 04:56 AM, Jan Beulich wrote:
>>>>>>>>> On 20.06.16 at 00:03, <andrey2805@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.
>>>>>>
>>>>>>> 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[], ...
>>>>>>
>>>>>>> --- 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.
>>>>>>
>>>>>> 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.)
>>>>> Will setting header_common[]'s PCI_CACHE_LINE_SIZE size field to 2 (and
>>>>> adding a proper comment) solve this problem?
>>>> How would that work? We mean to not allow writes, or else we
>>>> could simply add a .u.b.write handler for PCI_LATENCY_TIMER.
>>> I thought you suggested (in another thread) to make PCI_LATENCY_TIMER 
>>> writable, just like PCI_CACHE_LINE_SIZE?
>> Well, I did put this up for discussion (as much as I questioned
>> whether Cache Line Size should perhaps not be writable). And if
>> we wanted to make it writable, then we should do so the ordinary
>> way, not via some hacks. 
> 
> That depends on how we want to view header_common's offset field:
> whether it's address of a specific register (which is what it is now) or
> it's just an offset and length of an access within config space (which
> AFAIUI is how the driver in question wants to treat config space) and
> it's up to read/write ops to sort out specifics if necessary.

I don't see how this matters: The merge logic should be taking care
of these aspects when wider width writes get used than what an
individual register covers. Which isn't to say that I'm excluding an
issue in that merge logic.

Jan

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

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

[Boris Ostrovsky]

On 06/20/2016 11:14 AM, Jan Beulich wrote:
>>>> On 20.06.16 at 17:01, <boris.ostrovsky@oracle.com> wrote:
>> On 06/20/2016 09:55 AM, Jan Beulich wrote:
>>>>>> On 20.06.16 at 15:36, <boris.ostrovsky@oracle.com> wrote:
>>>> On 06/20/2016 09:30 AM, Jan Beulich wrote:
>>>>>>>> On 20.06.16 at 14:58, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 06/20/2016 04:56 AM, Jan Beulich wrote:
>>>>>>>>>> On 20.06.16 at 00:03, <andrey2805@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.
>>>>>>>
>>>>>>>> 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[], ...
>>>>>>>
>>>>>>>> --- 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.
>>>>>>>
>>>>>>> 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.)
>>>>>> Will setting header_common[]'s PCI_CACHE_LINE_SIZE size field to 2 (and
>>>>>> adding a proper comment) solve this problem?
>>>>> How would that work? We mean to not allow writes, or else we
>>>>> could simply add a .u.b.write handler for PCI_LATENCY_TIMER.
>>>> I thought you suggested (in another thread) to make PCI_LATENCY_TIMER 
>>>> writable, just like PCI_CACHE_LINE_SIZE?
>>> Well, I did put this up for discussion (as much as I questioned
>>> whether Cache Line Size should perhaps not be writable). And if
>>> we wanted to make it writable, then we should do so the ordinary
>>> way, not via some hacks. 
>> That depends on how we want to view header_common's offset field:
>> whether it's address of a specific register (which is what it is now) or
>> it's just an offset and length of an access within config space (which
>> AFAIUI is how the driver in question wants to treat config space) and
>> it's up to read/write ops to sort out specifics if necessary.
> I don't see how this matters: The merge logic should be taking care
> of these aspects when wider width writes get used than what an
> individual register covers. Which isn't to say that I'm excluding an
> issue in that merge logic.

Yes, I was assuming the list_for_each_entry() loop breaks as soon as it
finds a match. It doesn't, so what I suggested is not especially useful
(if a write op is added to PCI_LATENCY TIMER)


-boris


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

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

[Andrey Grodzovsky]

[-- Attachment #1.1: Type: text/plain, Size: 4661 bytes --]

On Mon, Jun 20, 2016 at 4:56 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 20.06.16 at 00:03, <andrey2805@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

[-- Attachment #1.2: Type: text/html, Size: 7959 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 20.06.16 at 17:15, <andrey2805@gmail.com> wrote:
> On Mon, Jun 20, 2016 at 4:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 20.06.16 at 00:03, <andrey2805@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).

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

Jan

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

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

[Andrey Grodzovsky]

[-- Attachment #1.1: Type: text/plain, Size: 6226 bytes --]

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

> >>> On 20.06.16 at 17:15, <andrey2805@gmail.com> wrote:
> > On Mon, Jun 20, 2016 at 4:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >
> >> >>> On 20.06.16 at 00:03, <andrey2805@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

[-- Attachment #1.2: Type: text/html, Size: 9001 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Andrey Grodzovsky]

[-- Attachment #1.1: Type: text/plain, Size: 6668 bytes --]

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 <andrey2805@gmail.com>
wrote:

>
>
> On Mon, Jun 20, 2016 at 11:38 AM, Jan Beulich <JBeulich@suse.com> wrote:
>
>> >>> On 20.06.16 at 17:15, <andrey2805@gmail.com> wrote:
>> > On Mon, Jun 20, 2016 at 4:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> >
>> >> >>> On 20.06.16 at 00:03, <andrey2805@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
>

[-- Attachment #1.2: Type: text/html, Size: 9851 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 20.06.16 at 18:23, <andrey2805@gmail.com> wrote:
> 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

Oh, I see - the current expression is screwed in two ways: For one
it has req_* and field_* the wrong way round (or should I better
say their uses are not symmetric, which for any kind of overlap
check they of course should be), and then it uses || instead of &&
(i.e. kind of, but not really checking that req is contained in field,
whereas for the purpose here we'd need the other direction). So
yes, your change is not just a simplification, but a correction.

But then on top of the previously mentioned change to your patch
you should also fix the read path in a similar manner. And the
description should of course accurately reflect the change (i.e.
no talk of quirks and overlapping filters, and a proper explanation
of what's wrong with the current expression).

Jan


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

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

[Jan Beulich]

>>> On 21.06.16 at 11:04, <JBeulich@suse.com> wrote:
>>>> On 20.06.16 at 18:23, <andrey2805@gmail.com> wrote:
>> 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
> 
> Oh, I see - the current expression is screwed in two ways: For one
> it has req_* and field_* the wrong way round (or should I better
> say their uses are not symmetric, which for any kind of overlap
> check they of course should be), and then it uses || instead of &&
> (i.e. kind of, but not really checking that req is contained in field,
> whereas for the purpose here we'd need the other direction). So
> yes, your change is not just a simplification, but a correction.
> 
> But then on top of the previously mentioned change to your patch
> you should also fix the read path in a similar manner. And the
> description should of course accurately reflect the change (i.e.
> no talk of quirks and overlapping filters, and a proper explanation
> of what's wrong with the current expression).

Yet then what I don't understand: How does this change help you?
There's still not going to be any actual write to the Latency Timer
register, as conf_space_write() - even if now getting called - won't
do anything for it.

Jan


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

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

[Andrey Grodzovsky]

[-- Attachment #1.1: Type: text/plain, Size: 2324 bytes --]

On Tue, Jun 21, 2016 at 5:23 AM, Jan Beulich <JBeulich@suse.com> wrote:

> >>> On 21.06.16 at 11:04, <JBeulich@suse.com> wrote:
> >>>> On 20.06.16 at 18:23, <andrey2805@gmail.com> wrote:
> >> 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
> >
> > Oh, I see - the current expression is screwed in two ways: For one
> > it has req_* and field_* the wrong way round (or should I better
> > say their uses are not symmetric, which for any kind of overlap
> > check they of course should be), and then it uses || instead of &&
> > (i.e. kind of, but not really checking that req is contained in field,
> > whereas for the purpose here we'd need the other direction). So
> > yes, your change is not just a simplification, but a correction.
> >
> > But then on top of the previously mentioned change to your patch
> > you should also fix the read path in a similar manner. And the
> > description should of course accurately reflect the change (i.e.
> > no talk of quirks and overlapping filters, and a proper explanation
> > of what's wrong with the current expression).
>
> Sent updated patch for review.


> Yet then what I don't understand: How does this change help you?
> There's still not going to be any actual write to the Latency Timer
> register, as conf_space_write() - even if now getting called - won't
> do anything for it.
>


>
> Jan
>
> Not sure, seems to me sometime the reset sequence in the net driver
doesn't
actually erase the conf space so it masks the issue

[-- Attachment #1.2: Type: text/html, Size: 3509 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Jan Beulich]

>>> On 21.06.16 at 16:56, <andrey2805@gmail.com> wrote:
> On Tue, Jun 21, 2016 at 5:23 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Yet then what I don't understand: How does this change help you?
>> There's still not going to be any actual write to the Latency Timer
>> register, as conf_space_write() - even if now getting called - won't
>> do anything for it.
> 
> Not sure, seems to me sometime the reset sequence in the net driver
> doesn't
> actually erase the conf space so it masks the issue

Well, that contradicts earlier statements (by Jürgen, I think). Also,
I don't think "sometimes" would really help you. I guess there's
more to be understood here.

As a side note - can you please get your reply quoting sorted? I've
removed the stray > in the quote above, but if you look back at
your reply you'll see that these extra > prefixes make it hard to
tell at the first glance what part of the mail is actually your reply.

Jan

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

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

[Jan Beulich]

>>> On 20.06.16 at 17:15, <andrey2805@gmail.com> wrote:

Argh - I had started a second reply. This was actually meant to be
part of it.

> On Mon, Jun 20, 2016 at 4:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>> >>> On 20.06.16 at 00:03, <andrey2805@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).

Ah, but these are intended to not overlap.

Jan


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