"Michael S. Tsirkin" <> writes:
> On Mon, Feb 03, 2014 at 11:36:20AM +1030, Rusty Russell wrote:
>> "Michael S. Tsirkin" <> 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
have access to a coffee machine!
>> 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
>> same effect as reads.
>
> 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.
