Re: [PATCH 1/3] sample: vfio mdev display - host device

[Gerd Hoffmann <kraxel@redhat.com>]

  Hi,

> > +/* pci ids */
> > +#define MDPY_PCI_VENDOR_ID	0x1b36 /* redhat */
> > +#define MDPY_PCI_DEVICE_ID	0x00f0
> 
> I don't see this on pci-ids, so I assume we're just squatting on an
> ID.  How do we do that without risking that we don't interfere with
> some future user?  Are we relying on this being a non-default sample
> device?  Should we just ask for an allocation?

It's grabbed from qemu id range.
Allocating one is probably a good idea even for a sample device.

> > +#define MDPY_PCI_SUBVENDOR_ID	PCI_SUBVENDOR_ID_REDHAT_QUMRANET
> > +#define MDPY_PCI_SUBDEVICE_ID	PCI_SUBDEVICE_ID_QEMU
> > +
> > +/* pci cfg space offsets for fb config (dword) */
> > +#define MDPY_FORMAT_OFFSET	0x40
> > +#define MDPY_WIDTH_OFFSET	0x44
> > +#define MDPY_HEIGHT_OFFSET	0x48
> 
> As I understand, these are just registers in PCI config space outside
> of any capabilities.  Wouldn't it be more correct to put these within a
> vendor defined capability?

Can do that.

> > +		region_info->size   = mdev_state->memsize;
> > +		region_info->flags  = (VFIO_REGION_INFO_FLAG_READ  |
> > +				       VFIO_REGION_INFO_FLAG_WRITE |
> > +				       VFIO_REGION_INFO_FLAG_MMAP);
> 
> This doesn't appear to be true, the read and write functions call the
> access function which only handles the config space region.  Are these
> really mmap-only regions?

Yes, they are mmap-only.

> read/write access support is often useful
> for tracing and debugging, QEMU will break if x-no-mmap=on is used.

Hmm, can look into adding that, should not be that difficuilt after all.

cheers,
  Gerd