Re: [Qemu-devel] [SeaBIOS] [PATCH v2 03/22] virtio: add struct vp_device

[Gerd Hoffmann <kraxel@redhat.com>]

> > -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