-u16 vp_init_simple(u16 bdf) +struct vp_device *vp_init_simple(u16 bdf) {
- u16 ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) &
- struct vp_device *vp = malloc_high(sizeof(*vp));
- vp->ioaddr = pci_config_readl(bdf, PCI_BASE_ADDRESS_0) & PCI_BASE_ADDRESS_IO_MASK;
The malloc_high() call can fail - so the code needs to check if it returns NULL. (It really can fail in practice and if code writes to NULL it will corrupt the 16bit isr table, which is painful to debug.)
Why not pass in a vp_device* to vp_init_simple() and have vp_init_simple() fill it. Then init_virtio_scsi() can stack allocate a temporary vp_device and virtio_scsi_add_lun() can memcpy it to virtio_lun_s.
Moved allocation to callers now. virtio-blk embeds the struct, so there is no extra malloc needed. virtio-scsi continues to allocate it separately and stick a pointer to each device, which makes sense IMHO considering that the struct will grow with the following patches.
+static inline u32 vp_get_features(struct vp_device *vp) {
- return inl(ioaddr + VIRTIO_PCI_HOST_FEATURES);
- return inl(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_HOST_FEATURES);
}
These GET_LOWFLAT() calls are confusing (as they are only valid for memory allocated with malloc_low() ). Granted, they are no-ops in 32bit mode, but they are still confusing.
Leftover oddity from the initial revision of the patch, which was ordered before the 32bit switchover patch. The following patches which switch over all the vp_*() functions to use vp_{read,write) clean that up.
cheers, Gerd