On Wed, Sep 19, 2018 at 01:52:19PM -0600, Alex Williamson wrote:
> On Tue, 18 Sep 2018 15:38:12 +0200
> Gerd Hoffmann <kraxel@xxxxxxxxxx> wrote:
>
> No empty commit logs please. There must be something to say about the
> goal or motivation beyond the subject.
>
> > Signed-off-by: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> > ---
> > include/uapi/linux/vfio.h | 39 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 1aa7b82e81..78e5a37d83 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -301,6 +301,45 @@ struct vfio_region_info_cap_type {
> > #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG (2)
> > #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG (3)
> >
> > +#define VFIO_REGION_TYPE_PCI_GFX (1)
>
> nit, what's the PCI dependency?
None I guess, just copy&paste from the (pci) vendor cap above ...
Will rename it.
> > + * The guest should be notified about edid changes, for example by
> > + * setting the link status to down temporarely (emulate monitor
> > + * hotplug).
>
> Who is responsible for this notification, the user interacting with
> this region or the driver providing the region when a new edid is
> provided? This comment needs to state the expected API as clearly as
> possible.
> > + * @link_state:
> > + * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on.
> > + * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off.
> > + *
> > + * @edid_size: Size of the edid data blob.
> > + * @edid_blob: The actual edid data.
>
> What signals that the user edid_blob update is complete? Should the
> size be written before or after the blob? Is the user required to
> update the entire blob in a single write or can it be written
> incrementally?
My qemu test code first sets link-state = down, then updates edid, then
sets link-state = up. I can document that as official update protocol.
Drivers should be able to map it do emulation events pretty straight
forward. Except maybe for the final down-up transition, drivers might
have to delay this to make sure the guest has seen the up-down
transition.
> It might also be worth defining access widths, I see that you use
> memcpy to support any width in mbochs, but we could define only native
> field accesses for discrete registers if it makes the implementation
> easier.
Makes sense for the u32 registers (especially the writable ones).
For mbochs the virtual hardware has no link state notification, so I can
just store whatever I get, then check link-state when the guest attempts
to read the edid.
When actually implementing link-state notification it is probably useful
to not have to deal with a half-written link-state field ...
> > + */
> > +struct vfio_region_gfx_edid {
> > + /* device capability hints (read only) */
> > + __u32 max_xres;
> > + __u32 max_yres;
> > + __u32 __reserved1[6];
>
> Is the plan to use the version field within vfio_info_cap_header to
> make use of these reserved fields later, ie. version 2 might define a
> field from this reserved block?
Yes.
> > + /* device state (read/write) */
> > + __u32 link_state;
> > +#define VFIO_DEVICE_GFX_LINK_STATE_UP 1
> > +#define VFIO_DEVICE_GFX_LINK_STATE_DOWN 2
> > + __u32 edid_size;
> > + __u32 __reserved2[6];
> > +
> > + /* edid blob (read/write) */
> > + __u8 edid_blob[512];
>
> It seems the placement of this blob is what makes us feel like we need
> to introduce reserved fields for later use, but we could instead define
> an edid_offset read-only field so that the blob is always at the end of
> whatever discrete fields we define.
Yes, sounds sensible.
> Perhaps then we wouldn't even need a read-only vs read-write section,
> simply define it per virtual register.
I'll look into this.
> Overall, I prefer this approach rather than adding yet more ioctls for
> every feature and extension we add, thanks for implementing it. What's
> your impression vs ioctls?
I'd still prefer ioctls, but this is workable too. So if you like it
better this way lets go for it.
cheers,
Gerd
