linux-kernel - Re: [PATCH 1/3] sample: vfio mdev display

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180424062919.bouvw7qxyrvi3vqc@sirius.home.kraxel.org>
Date:   Tue, 24 Apr 2018 08:29:19 +0200
From:   Gerd Hoffmann <kraxel@...hat.com>
To:     Alex Williamson <alex.williamson@...hat.com>
Cc:     kvm@...r.kernel.org, kwankhede@...dia.com,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] sample: vfio mdev display - host device

  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