[lkml]   [2012]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list
    On 07/25/2012 03:49 PM, Paolo Bonzini wrote:

    > Except here the destination array has to be given to virtio, which
    > doesn't (yet) understand chaining. I'm using for_each_sg rather than a
    > simple memcpy exactly because I want to flatten the input scatterlist
    > onto consecutive scatterlist entries, which is what virtio expects (and
    > what I'll change when I get to it).
    > for_each_sg guarantees that I get non-chain scatterlists only, so it is
    > okay to value-assign them to sg[].

    So if the virtio does not understand chaining at all then surly it will
    not understand the 2-bit end marker and will get a wrong page pointer
    with the 1st bit set.

    As I said!! Since the last code did sg_set_buff() and worked then you must
    change it with sg_set_page().

    If there are *any* chaining-or-no-chaining markers errors these should
    be fixed as a separate patch!

    Please lets concentrate at the problem at hand.


    > It was _not_ properly terminated, and didn't matter because virtio
    > doesn't care about termination. Changing all the virtio devices to
    > properly terminate chains (and to use for_each_sg) is a prerequisite for
    > properly supporting long sglists).

    Fine then your code is now a crash because the terminating bit was just
    copied over, which it was not before.

    Now Back to the how to support chaining:

    Lets separate the two topics from now on. Send me one mail concerning
    the proper above patch, And a different mail for how to support chaining.

    >> In SCSI land most LLDs should support chaining just by virtu of using the
    >> for_each_sg macro. That all it takes. Your code above does support it.
    > Yes, it supports it but still has to undo them before passing to virtio.
    > What my LLD does is add a request descriptor in front of the scatterlist
    > that the LLD receives. I would like to do this with a 2-item
    > scatterlist: one for the request descriptor, and one which is a chain to
    > the original scatterlist.

    I hate that plan. Why yet override the scatter element yet again with a third
    union of a "request descriptor"

    The reason it was overloaded as a link-pointer in the first place was because
    of historical compatibility reasons and not because of a good design.

    You should have a proper "request descriptor" structure defined, pointing to
    (or followed by), an sglist-chain. And all of the above is mute.

    > Except that if I call sg_chain and my
    > architecture does not define ARCH_HAS_SG_CHAIN, it will BUG out. So I
    > need to keep all the scatterlist allocation and copying crap that I have
    > now within #ifdef, and it will bitrot.

    except that with the correct design you don't call sg_chain you just do:
    request_descriptor->sg_list = sg;

    >>> I would need to add support for the long sglists to virtio; this is not
    >>> a problem, but in the past Rusty complained that long sg-lists are not
    >>> well suited to virtio (which would like to add elements not just at the
    >>> beginning of a given sglist, but also at the end).
    >> Well that can be done as well, (If done carefully) It should be easy to add
    >> chained fragments to both the end of a given chain just as at beginning.
    >> It only means that the last element of the appended-to chain moves to
    >> the next fragment and it's place is replaced by a link.
    > But you cannot do that in constant time, can you? And that means you do
    > not enjoy any benefit in terms of cache misses etc.

    I did not understand "constant time" it is O(0) if that what you meant.

    (And surly today's code of copy the full list "cache misses")

    > Also, this assumes that I can modify the appended-to chain. I'm not
    > sure I can do this?

    Each case it's own. If the appended-to chain is const, yes you'll have
    to reallocate it and copy. Is that your case?


    >>> It seems to me that virtio would prefer to work with a struct
    >>> scatterlist ** rather than a long sglist.
    >> That's just going backwards, and lazy. As you said if you want to enjoy
    >> the better performance cake you better break some eggs ;-)
    > :)
    > Paolo

     \ /
      Last update: 2012-07-25 16:01    [W:0.064 / U:7.520 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site