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

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

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. Why should we add such a prefix?

>
[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.

> 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