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

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

On 2014/6/29 19:43, Michael S. Tsirkin wrote:
> On Fri, Jun 27, 2014 at 05:22:18PM +0800, Chen, Tiejun wrote:
> > On 2014/6/25 15:13, Michael S. Tsirkin wrote:
> > > On Wed, Jun 25, 2014 at 10:17:21AM +0800, Tiejun Chen wrote:
> > [snip]
> >
> > > > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > > > index 507165c..25147cf 100644
> > > > --- a/hw/xen/xen_pt.h
> > > > +++ b/hw/xen/xen_pt.h
> > > > @@ -63,7 +63,7 @@ typedef int (*xen_pt_conf_byte_read)
> > > >   #define XEN_PT_BAR_UNMAPPED (-1)
> > > >
> > > >   #define PCI_CAP_MAX 48
> > > > -
> > > > +#define PCI_INTEL_OPREGION 0xfc
> > > XEN_.... please
> > >
> > > PCI_CAP_MAX should be fixed too.
> > They are specific to PCI, not XEN.
> They are?  Where in the PCI spec does it say 48?
> Same for PCI_INTEL_OPREGION.
>
> > Why should we add such a prefix?
> So that people working on core pci do not have to worry about breaking
> your devices by adding a symbol in the global header.
Okay.

> > >
> > [snip]
> >
> > > > +    if (igd_guest_opregion) {
> > > > +        ret = xc_domain_memory_mapping(xen_xc, xen_domid,
> > > > +                (unsigned long)(igd_guest_opregion >> XC_PAGE_SHIFT),
> > > > +                (unsigned long)(igd_host_opregion >> XC_PAGE_SHIFT),
> > > don't spread casts all around.
> > > Should be a last resort.
> > Okay.
> >
> > > > +                3,
> > > > +                DPCI_REMOVE_MAPPING);
> > > > +        if (ret) {
> > > > +            return ret;
> > > > +        }
> > > > +    }
> > > > +
> > > >       return 0;
> > > >   }
> > > >
> > > > @@ -447,3 +462,52 @@ err_out:
> > > >       XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
> > > >       return -1;
> > > >   }
> > > > +
> > > > +uint32_t igd_read_opregion(XenPCIPassthroughState *s)
> > > > +{
> > > > +    uint32_t val = 0;
> > > > +
> > > > +    if (igd_guest_opregion == 0) {
> > > !igd_guest_opregion is shorter and does the same,
> > Okay.
> >
> > > > +        return val;
> > > > +    }
> > > > +
> > > > +    val = igd_guest_opregion;
> > > > +
> > > > +    XEN_PT_LOG(&s->dev, "Read opregion val=%x\n", val);
> > > > +    return val;
> > > > +}
> > > > +
> > > > +void igd_write_opregion(XenPCIPassthroughState *s, uint32_t val)
> > > > +{
> > > > +    int ret;
> > > > +
> > > > +    if (igd_guest_opregion) {
> > > > +        XEN_PT_LOG(&s->dev, "opregion register already been set, ignoring 
> > > > %x\n",
> > > > +                   val);
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION,
> > > > +            (uint8_t *)&igd_host_opregion, 4);
> > > > +    igd_guest_opregion = (unsigned long)(val & ~0xfff)
> > > > +                            | (igd_host_opregion & 0xfff);
> > > > +
> > > Clearly broken on BE.
> > I still can't understand why we need to address this in BE case.
> So code is clean and reusable. Copy and paste is a fact of life,
> you don't want people to inherit bugs.
Understood.

> If some code absolutely must be LE specific,
> it needs a comment that explains this and cautions
> people against trying to use it elsewhere in QEMU.
I think its fine enough to add a comment.

Thanks
Tiejun

> > > Maybe not important here but writing clean code is
> > > just as easy.
> > > uint8_t igd_host_opregion[4];
> > >
> > > ...
> > >
> > >      xen_host_pci_get_block(&s->real_device, PCI_INTEL_OPREGION,
> > >                            igd_host_opregion, sizeof igd_host_opregion);
> > >
> > >      igd_guest_opregion = (val & ~0xfff) |
> > >             (pci_get_word(igd_host_opregion) & 0xfff);
> > >
> > > 0xfff should be a macro too to avoid duplication.
> > Okay.
> >
> > Thanks
> > Tiejun
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel