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

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

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