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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/20/2016 11:14 AM, Jan Beulich wrote:
>>>> On 20.06.16 at 17:01, <boris.ostrovsky@xxxxxxxxxx> wrote:
>> On 06/20/2016 09:55 AM, Jan Beulich wrote:
>>>>>> On 20.06.16 at 15:36, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>> On 06/20/2016 09:30 AM, Jan Beulich wrote:
>>>>>>>> On 20.06.16 at 14:58, <boris.ostrovsky@xxxxxxxxxx> wrote:
>>>>>> On 06/20/2016 04:56 AM, Jan Beulich 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.
>>>>>>>
>>>>>>>> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel