[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapp
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping |
Date: |
Sun, 29 Jun 2014 14:43:58 +0300 |
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.
> >
> >
>
> [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.
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.
> >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
- Re: [Qemu-devel] [v5][PATCH 1/5] xen, gfx passthrough: basic graphics passthrough support, (continued)
- [Qemu-devel] [v5][PATCH 4/5] xen, gfx passthrough: create host bridge to passthrough, Tiejun Chen, 2014/06/24
- Re: [Qemu-devel] [v5][PATCH 4/5] xen, gfx passthrough: create host bridge to passthrough, Paolo Bonzini, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 4/5] xen, gfx passthrough: create host bridge to passthrough, Chen, Tiejun, 2014/06/27
- Re: [Qemu-devel] [v5][PATCH 4/5] xen, gfx passthrough: create host bridge to passthrough, Paolo Bonzini, 2014/06/27
- Re: [Qemu-devel] [v5][PATCH 4/5] xen, gfx passthrough: create host bridge to passthrough, Chen, Tiejun, 2014/06/29
- Re: [Qemu-devel] [v5][PATCH 4/5] xen, gfx passthrough: create host bridge to passthrough, Michael S. Tsirkin, 2014/06/29
[Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Tiejun Chen, 2014/06/24
- Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Michael S. Tsirkin, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Chen, Tiejun, 2014/06/27
- Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [v5][PATCH 5/5] xen, gfx passthrough: add opregion mapping, Chen, Tiejun, 2014/06/29
Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Paolo Bonzini, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Michael S. Tsirkin, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Chen, Tiejun, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Michael S. Tsirkin, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Paolo Bonzini, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Michael S. Tsirkin, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Paolo Bonzini, 2014/06/25
- Re: [Qemu-devel] [v5][PATCH 0/5] xen: add Intel IGD passthrough support, Michael S. Tsirkin, 2014/06/25