Re: [Xen-devel] [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@xxxxxxxxxxxxx]
> Sent: Wednesday, May 28, 2014 8:24 PM
> To: Chen, Tiejun
> Cc: Stefano Stabellini; anthony.perard@xxxxxxxxxx; mst@xxxxxxxxxx;
> Kelly.Zytaruk@xxxxxxx; qemu-devel@xxxxxxxxxx;
> xen-devel@xxxxxxxxxxxxxxxxxxx; peter.maydell@xxxxxxxxxx;
> anthony@xxxxxxxxxxxxx; Kay, Allen M; Zhang, Yang Z
> Subject: RE: [v3][PATCH 5/5] xen, gfx passthrough: add opregion mapping
> 
> On Wed, 28 May 2014, Chen, Tiejun wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:stefano.stabellini@xxxxxxxxxxxxx]
> > > Sent: Wednesday, May 28, 2014 1:36 AM
> > > To: Chen, Tiejun
> > > Cc: anthony.perard@xxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> > > mst@xxxxxxxxxx; Kelly.Zytaruk@xxxxxxx; qemu-devel@xxxxxxxxxx;
> > > xen-devel@xxxxxxxxxxxxxxxxxxx; peter.maydell@xxxxxxxxxx;
> > > anthony@xxxxxxxxxxxxx; Kay, Allen M; Zhang, Yang Z
> > > Subject: Re: [v3][PATCH 5/5] xen, gfx passthrough: add opregion
> > > mapping
> > >

[snip]

> > > > +
> > > > +uint32_t igd_read_opregion(XenPCIPassthroughState *s) {
> > > > +    uint32_t val = 0;
> > > > +
> > > > +    if (igd_guest_opregion == 0) {
> > > > +        return val;
> > >
> > > Sorry for not having spotted it before, but isn't this supposed to return 
> > > an
> error?
> > > The old code returns -1 in this case.
> >
> > I already commented this in v2 log above.
> >
> > We shouldn't return "-1" to indicate an invalid address since we often think
> "!0" is a valid address value.
> >
> > In Linux instance,
> >
> > drivers/gpu/drm/i915/intel_opregion.c:
> >
> > int intel_opregion_setup(struct drm_device *dev) {
> >     ...
> >     pci_read_config_dword(dev->pdev, PCI_ASLS, &asls);
> >     DRM_DEBUG_DRIVER("graphic opregion physical addr: 0x%x\n", asls);
> >     if (asls == 0) {
> >         DRM_DEBUG_DRIVER("ACPI OpRegion not supported!\n");
> >         return -ENOTSUPP;
> >     }
> >     ...
> 
> Ops, you are right! igd_read_opregion returns an address not an error code.
> In that case, given that xen_pt_intel_opregion_read can return an error code
> as well as an address, maybe it makes sense to do the same in
> igd_read_opregion and allow the function to return an error code and set the
> value by pointer.
> 

I don't think so.

As I understand, we does check if that return value is valid, but not check if 
this read behavior is good. This pci read transmit should be guaranteed by the 
hardware, and this should be transparent to driver. 

In qemu emulator, qemu should do this check on behavior of the hardware so we 
need an error code when use xen_pt_intel_oprerion() as a wrapper of 
igd_read_opregion(). But that return value we're talking about should be 
delivered directly if this pci read behavior is fine.

But even if this read is failed, we should do something in pci host controller 
of qemu to further emulate this issue. But as emulator, I think qemu often 
never do this thing just specific to a normal pci read if you have no any 
obvious reason.   

Thanks
Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel