OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

# virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-comment] comment on pci section

• From: Rusty Russell <rusty@au1.ibm.com>
• To: "Michael S. Tsirkin" <mst@redhat.com>
• Date: Tue, 04 Feb 2014 10:01:52 +1030

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Feb 03, 2014 at 11:36:20AM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > I'd like to explain.
>> > What we are trying to implement is PCI bus master bit.
>> > Rules:
>> > - It must be set before DRIVER_OK.
>> > - If it set/cleared any number of time before DRIVER_OK, device will be
>> >   livea after DRIVER_OK
>> > - If it is cleared when driver sets DRIVER_OK, or if
>> >   it is cleared after driver sets DRIVER_OK, device stops
>> >   being live, you must reset to make it live again
>>
>> I'm not sure what the semantics of the PCI bus master bit are on normal
>> devices.  I assume if you turn it off, the device stops working?
>
> Stop being a bus master: doing writes (including msi) and reads.
>
>>  Why
>> would we insist on a reset to set it again?
>>
>> Should we leave this interaction as undefined?
>
> I don't exactly understand.
>
> OS-es clear bus master to disable devices in
> a device independent way.
> Afterwards, reset must bring the device back
> to a functional state (with bus master disabled)
> and allow driver to re-attach.
> It does not have to be the only way but in practice it is.

OK, so in effect this provides a PCI-specific way of deactivating the
device.

>> > - It must be set before DRIVER_OK.
>> > - If it set/cleared any number of time before DRIVER_OK, device will be
>> >   livea after DRIVER_OK
>> > - If it is cleared when driver sets DRIVER_OK, or if
>> >   it is cleared after driver sets DRIVER_OK, device stops
>> >   being live, you must reset to make it live again

Why allow DRIVER_OK on a non-bus-master at all?

On reset, the device MUST have the PCI bus master bit clear.  The driver
MUST set the bus master bit prior to setting DRIVER_OK.   The driver MAY
clear the bus master bit, at which point all the device's virtqueues are
no longer live, and the driver MUST reset the device to operate it
again.

>> >> BTW, the struct virtio_pci_cfg_cap seems weird: why do you trigger on
>> >> offset read/write, rather than triggering on the actual pci_cfg_data?
>> >> Or am I misreading the spec?
>> >>
>> >> Thanks,
>> >> Rusty.
>> >
>> > This is not what was intended.
>> > I see what gives you this impression:
>> >
>> > After this field is written by driver, the first length bytes in
>> > pci_cfg_data are written at the selected offset
>> > in the selected BAR.
>> > When this field is read by driver, length bytes at the selected offset
>> > in the selected BAR are read into
>> > pci_cfg_data.
>> >
>> > "this" should be replaced by pci_cfg_data.
>>
>> Much better, it's just a simple window.
>
> Not exactly. There are corner cases, see below:
>
>> How's this?
>>
>> ==To access a device region, the driver writes into the capability
>> structure (ie. within the PCI configuration space) as follows:
>>
>> \begin{itemize}
>> \item The driver sets the BAR to access by writing to the cap.bar field.
>>
>> \item The driver sets the size of the access by writing 1, 2 or 4 to
>>   the cap.length field.
>>
>> \item The driver sets the offset within the BAR by writing to the
>>   cap.offset field.  The driver MUST NOT write an offset which is not
>>   a multiple of cap.length (ie. all accesses must be aligned).
>> \end{itemsize}
>
> s/itemsize/itemize/ :)

Wow, I'm not doing well this week!  This is what happens when I don't

>> At that point, the pci_cfg_data field will provide a window of size
>> cap.length into the given cap.bar at offset cap.offset: writes will
>> have the same effect as writes into the BAR, and reads will have the
>
> better "same effect and return same value as reads from the BAR"

OK, changed.

>> The driver MUST perform reads/writes from/to pci_cfg_data of the same
>> width as given by cap.length.
>
> I strongly think we should allow reading a different number of bytes,
> for debugging.
>
> For example if you read some register with setpci
> it's nice not to have the device crash because access width
> did not match length : )
> cat /sys/bus/pci/devices/0000\:00\:00.0/config
> is also a nice troubleshooting tool, and that
> one might read configuration byte by byte.

I don't think you can do that, because you have to set cap.length
correctly.  And we already specify that accesses must be "natural
length".

> Also, I started thinking we should specify whether this gateway access
> to a disabled BAR should be allowed.  It would allow using device before
> bus enumeration, so useful if we ever want a system device like a pci
> host based on virtio.  If yes that would mean access would have a
> different effect from BAR access.

I'm not sure I understand the use case for this, so I'm happy to leave
it until someone has a case :)

Cheers,
Rusty.



[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]