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

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

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
>