OASIS Virtual I/O Device (VIRTIO) TC

 View Only
  • 1.  [PATCH 3/3] VIRTIO-110: ARM's feedback for MMIO chapter, clarifications

    Posted 08-04-2014 18:08
    Those changes do not add nor remove any features and constitutes only error correction and editorial changes. * Extra clarifications for QueueReady and ConfigGeneration * Re-added alignment requirement section - it existed in the original version of the spec, so no extra limitations here. Signed-off-by: Pawel Moll <pawel.moll@arm.com> --- content.tex 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/content.tex b/content.tex index 17ba3b8..769bef2 100644 --- a/content.tex +++ b/content.tex @@ -1997,8 +1997,8 @@ All register values are organized as Little Endian. } hline mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% - Writing one (0x1) to this register notifies the device that the - virtual queue is ready to be used. Reading from this register + Writing one (0x1) to this register notifies the device that it can + execute requests from this virtual queue. Reading from this register returns the last value written to it. Both read and write accesses apply to the queue selected by writing to field{QueueSel}. } @@ -2058,7 +2058,11 @@ All register values are organized as Little Endian. } hline mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ - Changes every time the configuration noticeably changes (see
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. + Reading from this register returns a value describing a version of the device-specific configuration (see field{Config}). + The driver can then access the configuration space and then read field{ConfigGeneration} again. + If no part of the configuration space has changed, field{ConfigGeneration} returns the same value. + If the value is different, the configuration space access was not atomic and the driver must perform the operation again. + See also
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. } hline mmioreg{Config}{Configuration space}{0x100+}{RW}{ @@ -2083,10 +2087,12 @@ do not represent events which took place MUST be zero. Upon reset, the device MUST clear all bits in field{InterruptStatus} and ready bits in the field{QueueReady} register for all queues in the device. -The device MUST change field{ConfigGeneration} if there is any risk of a -device seeing an inconsistent configuration state, but it MAY only change the value +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a +driver seeing an inconsistent configuration state, but it MAY only change the value after a configuration read operation. +The device MUST NOT access virtual queue contents when field{QueueReady} is zero (0x0). + drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} The driver MUST NOT access memory locations not described in the table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} @@ -2094,6 +2100,11 @@ table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register MUST NOT write to the read-only registers (direction R) and MUST NOT read from the write-only registers (direction W). +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers +described in table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. +The driver MAY use other accesses for the device-specific configuration space, +depending on the device type. + The driver MUST ignore a device with field{MagicValue} which is not 0x74726976, although it MAY report an error. -- 1.9.1


  • 2.  Re: [virtio] [PATCH 3/3] VIRTIO-110: ARM's feedback for MMIO chapter, clarifications

    Posted 08-04-2014 23:00
    On Mon, Aug 04, 2014 at 07:08:01PM +0100, Pawel Moll wrote: > Those changes do not add nor remove any features and constitutes > only error correction and editorial changes. This one seems to be borderline. > * Extra clarifications for QueueReady and ConfigGeneration > * Re-added alignment requirement section - it existed in the > original version of the spec, so no extra limitations here. What is an "original version"? I could not find this in draft 01. > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > --- > content.tex 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/content.tex b/content.tex > index 17ba3b8..769bef2 100644 > --- a/content.tex > +++ b/content.tex > @@ -1997,8 +1997,8 @@ All register values are organized as Little Endian. > } > hline > mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% > - Writing one (0x1) to this register notifies the device that the > - virtual queue is ready to be used. Reading from this register > + Writing one (0x1) to this register notifies the device that it can > + execute requests from this virtual queue. Reading from this register > returns the last value written to it. Both read and write > accesses apply to the queue selected by writing to field{QueueSel}. > } > @@ -2058,7 +2058,11 @@ All register values are organized as Little Endian. > } > hline > mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ > - Changes every time the configuration noticeably changes (see
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > + Reading from this register returns a value describing a version of the device-specific configuration (see field{Config}). > + The driver can then access the configuration space and then read field{ConfigGeneration} again. > + If no part of the configuration space has changed, field{ConfigGeneration} returns the same value. > + If the value is different, the configuration space access was not atomic and the driver must perform the operation again. > + See also
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > } > hline > mmioreg{Config}{Configuration space}{0x100+}{RW}{ > @@ -2083,10 +2087,12 @@ do not represent events which took place MUST be zero. > Upon reset, the device MUST clear all bits in field{InterruptStatus} and ready bits in the > field{QueueReady} register for all queues in the device. > > -The device MUST change field{ConfigGeneration} if there is any risk of a > -device seeing an inconsistent configuration state, but it MAY only change the value > +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a > +driver seeing an inconsistent configuration state, but it MAY only change the value > after a configuration read operation. "MAY only" is really confusing. You are using MAY here to describe a mandatory requirement. Can be re-written with "MUST NOT"? Also, surely reset changes this field to 0? > > +The device MUST NOT access virtual queue contents when field{QueueReady} is zero (0x0). > + > drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > The driver MUST NOT access memory locations not described in the > table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > @@ -2094,6 +2100,11 @@ table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register > MUST NOT write to the read-only registers (direction R) and > MUST NOT read from the write-only registers (direction W). > > +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers > +described in table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. OK this was kind of implied all the time ... you think adding this text is error correction? > +The driver MAY use other accesses for the device-specific configuration space, > +depending on the device type. > + Why are you adding this option? If there's a 4 byte register it is a pain for devices to support 1 byte accesses, and we gain nothing from it. > The driver MUST ignore a device with field{MagicValue} which is not 0x74726976, > although it MAY report an error. > > -- > 1.9.1 > > > --------------------------------------------------------------------- > To unsubscribe from this mail list, you must leave the OASIS TC that > generates this mail. Follow this link to all your TCs in OASIS at: > https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php


  • 3.  Re: [virtio] [PATCH 3/3] VIRTIO-110: ARM's feedback for MMIO chapter, clarifications

    Posted 08-05-2014 11:58
    On Tue, 2014-08-05 at 00:00 +0100, Michael S. Tsirkin wrote: > > * Re-added alignment requirement section - it existed in the > > original version of the spec, so no extra limitations here. > > What is an "original version"? I could not find this in draft 01. Damn, you're right! I was referring to the 0.9.x spec, but reading it again I can only see the R/W restriction and a somehow enigmatic note that the registers are 32-bit wide. I was sure I put it there, as this was one of the first things I did in my device implementation back there... > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > > --- > > content.tex 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/content.tex b/content.tex > > index 17ba3b8..769bef2 100644 > > --- a/content.tex > > +++ b/content.tex > > @@ -1997,8 +1997,8 @@ All register values are organized as Little Endian. > > } > > hline > > mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% > > - Writing one (0x1) to this register notifies the device that the > > - virtual queue is ready to be used. Reading from this register > > + Writing one (0x1) to this register notifies the device that it can > > + execute requests from this virtual queue. Reading from this register > > returns the last value written to it. Both read and write > > accesses apply to the queue selected by writing to field{QueueSel}. > > } > > @@ -2058,7 +2058,11 @@ All register values are organized as Little Endian. > > } > > hline > > mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ > > - Changes every time the configuration noticeably changes (see
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > + Reading from this register returns a value describing a version of the device-specific configuration (see field{Config}). > > + The driver can then access the configuration space and then read field{ConfigGeneration} again. > > + If no part of the configuration space has changed, field{ConfigGeneration} returns the same value. > > + If the value is different, the configuration space access was not atomic and the driver must perform the operation again. > > + See also
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > } > > hline > > mmioreg{Config}{Configuration space}{0x100+}{RW}{ I take that the description above is clear now? > > @@ -2083,10 +2087,12 @@ do not represent events which took place MUST be zero. > > Upon reset, the device MUST clear all bits in field{InterruptStatus} and ready bits in the > > field{QueueReady} register for all queues in the device. > > > > -The device MUST change field{ConfigGeneration} if there is any risk of a > > -device seeing an inconsistent configuration state, but it MAY only change the value > > +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a > > +driver seeing an inconsistent configuration state, but it MAY only change the value > > after a configuration read operation. > > "MAY only" is really confusing. You are using MAY here to > describe a mandatory requirement. Can be re-written with "MUST NOT"? Actually, I will remove this "MAY" part of the sentence completely. It was original Rusty's wording, but it's not really necessary here, I believe (it's also gone from PCI section, I see). > Also, surely reset changes this field to 0? Not sure about that. From the driver point of view it doesn't matter what value it is, as long as it is different or not. > > +The device MUST NOT access virtual queue contents when field{QueueReady} is zero (0x0). > > + > > drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > > The driver MUST NOT access memory locations not described in the > > table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > > @@ -2094,6 +2100,11 @@ table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register > > MUST NOT write to the read-only registers (direction R) and > > MUST NOT read from the write-only registers (direction W). > > > > +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers > > +described in table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. > > OK this was kind of implied all the time ... you think adding this text > is error correction? And this is an interesting question. As I said above, I was convinced it just got lost on the way, but I was proven wrong. As you said above, this is exactly what everyone thought and implemented. Is it a material change then? I would like to say no, but I will understand if anyone shouts that it is. > > +The driver MAY use other accesses for the device-specific configuration space, > > +depending on the device type. > > Why are you adding this option? > If there's a 4 byte register it is a pain for devices to support > 1 byte accesses, and we gain nothing from it. But 2.3 does not limit the configuration space field size nor alignment, does it? If particular, the existing drivers do memcpy-like byte-after-byte operations. I'd be more than happy to remove this exception, but I don't see how can I do that. Pawel


  • 4.  Re: [virtio] [PATCH 3/3] VIRTIO-110: ARM's feedback for MMIO chapter, clarifications

    Posted 08-05-2014 12:04
    On Tue, Aug 05, 2014 at 12:57:28PM +0100, Pawel Moll wrote: > > On Tue, 2014-08-05 at 00:00 +0100, Michael S. Tsirkin wrote: > > > * Re-added alignment requirement section - it existed in the > > > original version of the spec, so no extra limitations here. > > > > What is an "original version"? I could not find this in draft 01. > > Damn, you're right! > > I was referring to the 0.9.x spec, but reading it again I can only see > the R/W restriction and a somehow enigmatic note that the registers are > 32-bit wide. I was sure I put it there, as this was one of the first > things I did in my device implementation back there... > > > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > > > --- > > > content.tex 21 ++++++++++++++++----- > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/content.tex b/content.tex > > > index 17ba3b8..769bef2 100644 > > > --- a/content.tex > > > +++ b/content.tex > > > @@ -1997,8 +1997,8 @@ All register values are organized as Little Endian. > > > } > > > hline > > > mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% > > > - Writing one (0x1) to this register notifies the device that the > > > - virtual queue is ready to be used. Reading from this register > > > + Writing one (0x1) to this register notifies the device that it can > > > + execute requests from this virtual queue. Reading from this register > > > returns the last value written to it. Both read and write > > > accesses apply to the queue selected by writing to field{QueueSel}. > > > } > > > @@ -2058,7 +2058,11 @@ All register values are organized as Little Endian. > > > } > > > hline > > > mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ > > > - Changes every time the configuration noticeably changes (see
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > > + Reading from this register returns a value describing a version of the device-specific configuration (see field{Config}). > > > + The driver can then access the configuration space and then read field{ConfigGeneration} again. > > > + If no part of the configuration space has changed, field{ConfigGeneration} returns the same value. > > > + If the value is different, the configuration space access was not atomic and the driver must perform the operation again. > > > + See also
    ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > > } > > > hline > > > mmioreg{Config}{Configuration space}{0x100+}{RW}{ > > I take that the description above is clear now? > > > > @@ -2083,10 +2087,12 @@ do not represent events which took place MUST be zero. > > > Upon reset, the device MUST clear all bits in field{InterruptStatus} and ready bits in the > > > field{QueueReady} register for all queues in the device. > > > > > > -The device MUST change field{ConfigGeneration} if there is any risk of a > > > -device seeing an inconsistent configuration state, but it MAY only change the value > > > +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a > > > +driver seeing an inconsistent configuration state, but it MAY only change the value > > > after a configuration read operation. > > > > "MAY only" is really confusing. You are using MAY here to > > describe a mandatory requirement. Can be re-written with "MUST NOT"? > > Actually, I will remove this "MAY" part of the sentence completely. It > was original Rusty's wording, but it's not really necessary here, I > believe (it's also gone from PCI section, I see). For PCI there is instead a non-normative note suggesting how to avoid wrap-arounds. > > Also, surely reset changes this field to 0? > > Not sure about that. From the driver point of view it doesn't matter > what value it is, as long as it is different or not. Absolutely, I'm not saying we must require this, I am merely saying if reset clears this field, this violates the "may only change on read" requirement. > > > +The device MUST NOT access virtual queue contents when field{QueueReady} is zero (0x0). > > > + > > > drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > > > The driver MUST NOT access memory locations not described in the > > > table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > > > @@ -2094,6 +2100,11 @@ table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register > > > MUST NOT write to the read-only registers (direction R) and > > > MUST NOT read from the write-only registers (direction W). > > > > > > +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers > > > +described in table
    ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. > > > > OK this was kind of implied all the time ... you think adding this text > > is error correction? > > And this is an interesting question. > > As I said above, I was convinced it just got lost on the way, but I was > proven wrong. As you said above, this is exactly what everyone thought > and implemented. > > Is it a material change then? I would like to say no, but I will > understand if anyone shouts that it is. > > > > +The driver MAY use other accesses for the device-specific configuration space, > > > +depending on the device type. > > > > Why are you adding this option? > > If there's a 4 byte register it is a pain for devices to support > > 1 byte accesses, and we gain nothing from it. > > But 2.3 does not limit the configuration space field size nor alignment, > does it? No but there is no need: you look at each specific field, you see what the size and alignment is. > If particular, the existing drivers do memcpy-like > byte-after-byte operations. They are pre-1.0, we will fix them. > I'd be more than happy to remove this > exception, but I don't see how can I do that. > > Pawel Are you supporting transitional devices for MMIO? If no you don't care what happens with existing drivers. -- MST


  • 5.  Re: [virtio] [PATCH 3/3] VIRTIO-110: ARM's feedback for MMIO chapter, clarifications

    Posted 08-05-2014 12:11
    On Tue, 2014-08-05 at 13:04 +0100, Michael S. Tsirkin wrote: > > > > +The driver MAY use other accesses for the device-specific configuration space, > > > > +depending on the device type. > > > > > > Why are you adding this option? > > > If there's a 4 byte register it is a pain for devices to support > > > 1 byte accesses, and we gain nothing from it. > > > > But 2.3 does not limit the configuration space field size nor alignment, > > does it? > > No but there is no need: you look at each specific field, you see what > the size and alignment is. That's what I'm saying. The driver MAY do 8-bit load of the 8-bit field, if the field is defined as such in the device spec. Otherwise all operations on the configuration space would have to be 32-bit wide and aligned. We don't want this, do we? (or do we? :-) > Are you supporting transitional devices for MMIO? > If no you don't care what happens with existing drivers. I don't indeed. Pawel


  • 6.  Re: [virtio] [PATCH 3/3] VIRTIO-110: ARM's feedback for MMIO chapter, clarifications

    Posted 08-05-2014 12:47
    On Tue, Aug 05, 2014 at 01:10:46PM +0100, Pawel Moll wrote: > On Tue, 2014-08-05 at 13:04 +0100, Michael S. Tsirkin wrote: > > > > > +The driver MAY use other accesses for the device-specific configuration space, > > > > > +depending on the device type. > > > > > > > > Why are you adding this option? > > > > If there's a 4 byte register it is a pain for devices to support > > > > 1 byte accesses, and we gain nothing from it. > > > > > > But 2.3 does not limit the configuration space field size nor alignment, > > > does it? > > > > No but there is no need: you look at each specific field, you see what > > the size and alignment is. > > That's what I'm saying. The driver MAY do 8-bit load of the 8-bit field, > if the field is defined as such in the device spec. Otherwise all > operations on the configuration space would have to be 32-bit wide and > aligned. We don't want this, do we? (or do we? :-) Indeed. What I am saying is this from PCI part of spec: The driver MUST access each field using the ``natural'' access method, i.e. 32-bit accesses for 32-bit fields, 16-bit accesses for 16-bit fields and 8-bit accesses for 8-bit fields. Should apply. I guess when you say "depending on the device type" this is what you mean, but maybe being more explicit would be better. > > Are you supporting transitional devices for MMIO? > > If no you don't care what happens with existing drivers. > > I don't indeed. > > Pawel Right so existing drivers doing memcpy is of no concern then.