[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] comment on pci section
"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 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.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]