Re: [virtio] [PATCH] pci: missing documentation for dealing with 64 bit config fields

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Apr 12, 2015 at 03:57:07PM +0000, James Bottomley wrote:
> On Sun, 2015-04-12 at 17:53 +0200, Michael S. Tsirkin wrote:
> > On Sun, Apr 12, 2015 at 03:38:07PM +0000, James Bottomley wrote:
> > > On Sun, 2015-04-12 at 17:28 +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Apr 07, 2015 at 10:40:50PM +0000, James Bottomley wrote:
> > > > > On Tue, 2015-04-07 at 12:02 +0200, Michael S. Tsirkin wrote:
> > > > > > On Tue, Apr 07, 2015 at 11:19:08AM +0200, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Apr 06, 2015 at 06:17:56PM +0000, James Bottomley wrote:
> > > > > > > > On Thu, 2015-04-02 at 12:23 +0200, Cornelia Huck wrote:
> > > > > > > > > On Thu, 2 Apr 2015 12:06:41 +0200
> > > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > > > 
> > > > > > > > > > On Thu, Apr 02, 2015 at 11:01:45AM +0200, Cornelia Huck wrote:
> > > > > > > > > > > On Wed, 1 Apr 2015 20:42:13 +0200
> > > > > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > > > 
> > > > > > > > > > > > +For device configuration access, the driver MUST use 8-bit wide
> > > > > > > > > > > > +accesses for 8-bit wide fields, 16-bit wide and aligned accesses
> > > > > > > > > > > > +for 16-bit wide fields and 32-bit wide and aligned accesses for
> > > > > > > > > > > > +32-bit and 64-bit wide fields. For 64-bit fields, the driver MAY
> > > > > > > > > > > > +access each of the high and low 32-bit parts of the field
> > > > > > > > > > > > +independently.
> > > > > > > > > > > 
> > > > > > > > > > > Doesn't the last sentence follow from "use 32-bit wide accesses"
> > > > > > > > > > > already?
> > > > > > > > > > 
> > > > > > > > > > Someone might assume that driver must always access low part,
> > > > > > > > > > then high part. Last sentence says it does not have to.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > OK, that clears it up then.
> > > > > > > > 
> > > > > > > > Not entirely.  There's a hidden gotcha here in terms of fields which
> > > > > > > > perform actions.  For instance the status register is used to transmit
> > > > > > > > actions and information from the guest to the host.  Problems happen if
> > > > > > > > any action register is wider than 32 bits, because a 32 bit OS cannot
> > > > > > > > now set it atomically.
> > > > > > > > 
> > > > > > > > There a various ways around this: we could say no action register may
> > > > > > > > ever be wider than 32 bits (easy) or we could prescribe a mechanism for
> > > > > > > > using 64 bit action registers on 32 bits (locking + defined initiation,
> > > > > > > > say writing to the lower 32 bits triggers the action, so on 32 bit
> > > > > > > > architectures, you lock, write upper, then write lower, then unlock).
> > > > > > > > 
> > > > > > > > I'd actually say it's useful clarification to code the former into the
> > > > > > > > Spec until such time as someone finds a need for a 64 bit action
> > > > > > > > register.
> > > > > > > > 
> > > > > > > > James
> > > > > > > > 
> > > > > > > 
> > > > > > > This seems to belong to appendix B: creating new device types.
> > > > > > > We already have this text there:
> > > > > > > 	Remember that configuration fields over 32 bits wide might not be
> > > > > > > 	atomically writable by the driver.
> > > > > > > 
> > > > > > > Is this sufficient? If not, we can add
> > > > > > > 	Therefore, it is generally not a good idea to have
> > > > > > > 	such writeable fields, and especially to have
> > > > > > > 	driver writes into such fields trigger device actions".
> > > > > > > 
> > > > > > > Is this what you had in mind?
> > > > > > 
> > > > > > If yes I'd prefer that you change your vote to yes,
> > > > > > with a comment, and open another issue to add a note.
> > > > > 
> > > > > Well, I'd prefer the updates went in atomically, but I think this will
> > > > > work as a compromise.
> > > > > 
> > > > > James
> > > > > 
> > > > 
> > > > Created VIRTIO-140 for this.
> > > 
> > > Will post a patch when I can post to the list (still having trouble with
> > > our email rebranding).
> > > 
> > > James
> > > 
> > 
> > Since you are a member of the TC, I don't think there are  any IPR
> > issues if you just send any email to me, I'll forward.
> > OTOH if you just want that text added, I can send that patch myself.
> 
> Well, you got the patch, but only to the virtio list ... I've got my
> email there changed, but not, apparently, on virtio-dev.
> 
> James
> 

I see it on virtio dev now.
issue updated - let's give others a bit of time to review,
then I'll start a ballot.