Re: [PATCH 16/22] virtio_pci: use separate notification offsets for each vq.

["H. Peter Anvin" <hpa@zytor.com>]

I mean no offset.

"Michael S. Tsirkin" <mst@redhat.com> wrote:

>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> >On Wed, Apr 03, 2013 at 04:40:29PM +1030, Rusty Russell wrote:
>> >> "H. Peter Anvin" <hpa@zytor.com> writes:
>> >> > On 03/29/2013 08:19 PM, Rusty Russell wrote:
>> >> >>>
>> >> >>> Shift count?
>> >> >> 
>> >> >> You can only have 2^16 vqs per device.  Is it verboten to write
>> >16-bit
>> >> >> values to odd offsets?  If so, we've just dropped it to 2^15
>> >before you
>> >> >> have to do some decoding to do.  Hard to care...
>> >> >> 
>> >> >> I dislike saying "multiply offset by 2" because implementations
>> >will get
>> >> >> it wrong.  That's because 0 will work either way, and that's
>going
>> >to be
>> >> >> the common case.
>> >> >> 
>> >> >
>> >> > The main reason to use a shift count is that it lets the guest
>> >driver
>> >> > assume that the spacing is a power of two, requiring only shift,
>as
>> >> > opposed to an arbitrary number, requiring a multiply.  It seems
>> >unlikely
>> >> > that there would be a legitimate reason for a non-power-of-two
>> >spacing
>> >> > between the VQ notifiers.
>> >> >
>> >> > The other reason is that if a particular host implementation
>needs
>> >> > separate pages for each notifier, that can be a pretty large
>> >number.
>> >> 
>> >> Ah, sorry, we're talking across each other a bit.
>> >> 
>> >> Current proposal is a 16 bit 'offset' field in the queue data for
>> >each
>> >> queue, ie.
>> >>         addr = dev->notify_base + vq->notify_off;
>> >> 
>> >> You propose a per-device 'shift' field:
>> >>         addr = dev->notify_base + (vq->index <<
>dev->notify_shift);
>> >> 
>> >> Which allows greater offsets, but insists on a unique offset per
>> >queue.
>> >> Might be a fair trade-off...
>> >> 
>> >> Cheers,
>> >> Rusty.
>> >
>> >Or even
>> >       addr = dev->notify_base + (vq->notify_off <<
>dev->notify_shift);
>> >
>> >since notify_base is per capability, shift can be per capability
>too.
>> >And for IO we can allow it to be 32 to mean "always use base".
>> >
>> >This is a bit more elegant than just saying "no offsets for IO".
>> 
>
>On Wed, Apr 03, 2013 at 07:10:42AM -0700, H. Peter Anvin wrote:
>> 0 should probably mean no shift;
>
>Sure. Note no shift is not same as "no offset".
>
>> that way we explicitly prohibit odd offsets, which is a good thing,
>too.
>
>Odd offsets?
>
>> -- 
>> Sent from my mobile phone. Please excuse brevity and lack of
>formatting.

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.