Re: [Xen-devel] [PATCH] qemu-xen: Intel GPU passthrough, fix OpRegion mapping. (v3)

[Jean Guyader <jean.guyader@xxxxxxxxx>]

On 12 January 2012 14:34, Stefano Stabellini
<stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Thu, 12 Jan 2012, Jean Guyader wrote:
>> On 12/01 12:41, Jean Guyader wrote:
>> > The OpRegion shouldn't be mapped 1:1 because the address in the host
>> > can't be used in the guest directly.
>> >
>> > This patch traps read and write access to the opregion of the Intel
>> > GPU config space (offset 0xfc).
>> >
>> > To work correctly this patch needs a change in hvmloader.
>> >
>> > HVMloader will allocate 2 pages for the OpRegion and write this address
>> > on the config space of the Intel GPU. Qemu will trap and map the host
>> > OpRegion to the guest. Any write to this offset after that won't have
>> > any effect. Any read of this config space offset will return the address
>> > in the guest.
>> >
>> > Signed-off-by: Jean Guyader <jean.guyader@xxxxxxxxxxxxx>
>> commit a6036bee23bb338e6cf48e9f0d75ff0845f8cfe3
>> Author: Jean Guyader <jean.guyader@xxxxxxxxxxxxx>
>> Date:   Wed Nov 23 07:53:30 2011 +0000
>>
>>     qemu-xen: Intel GPU passthrough, fix OpRegion mapping.
>>
>>     The OpRegion shouldn't be mapped 1:1 because the address in the host
>>     can't be used in the guest directly.
>>
>>     This patch traps read and write access to the opregion of the Intel
>>     GPU config space (offset 0xfc).
>>
>>     To work correctly this patch needs a change in hvmloader.
>>
>>     HVMloader will allocate 2 pages for the OpRegion and write this address
>>     on the config space of the Intel GPU. Qemu will trap and map the host
>>     OpRegion to the guest. Any write to this offset after that won't have
>>     any effect. Any read of this config space offset will return the address
>>     in the guest.
>>
>> diff --git a/hw/pass-through.c b/hw/pass-through.c
>> index dbe8804..7ee3c61 100644
>> --- a/hw/pass-through.c
>> +++ b/hw/pass-through.c
>> @@ -238,6 +238,14 @@ static int pt_bar_reg_restore(struct pt_dev *ptdev,
>>  static int pt_exp_rom_bar_reg_restore(struct pt_dev *ptdev,
>>      struct pt_reg_tbl *cfg_entry,
>>      uint32_t real_offset, uint32_t dev_value, uint32_t *value);
>> +static int pt_intel_opregion_read(struct pt_dev *ptdev,
>> +        struct pt_reg_tbl *cfg_entry,
>> +        uint32_t *value, uint32_t valid_mask);
>> +static int pt_intel_opregion_write(struct pt_dev *ptdev,
>> +        struct pt_reg_tbl *cfg_entry,
>> +        uint32_t *value, uint32_t dev_value, uint32_t valid_mask);
>> +static uint8_t pt_reg_grp_header0_size_init(struct pt_dev *ptdev,
>> +        struct pt_reg_grp_info_tbl *grp_reg, uint32_t base_offset);
>>
>>  /* pt_reg_info_tbl declaration
>>   * - only for emulated register (either a part or whole bit).
>> @@ -444,6 +452,16 @@ static struct pt_reg_info_tbl pt_emu_reg_header0_tbl[] 
>> = {
>>          .u.dw.write = pt_exp_rom_bar_reg_write,
>>          .u.dw.restore = pt_exp_rom_bar_reg_restore,
>>      },
>> +    /* Intel IGFX OpRegion reg */
>> +    {
>> +        .offset     = PCI_INTEL_OPREGION,
>> +        .size       = 4,
>> +        .init_val   = 0,
>> +        .no_wb      = 1,
>> +        .u.dw.read   = pt_intel_opregion_read,
>> +        .u.dw.write  = pt_intel_opregion_write,
>> +        .u.dw.restore  = NULL,
>> +    },
>>      {
>>          .size = 0,
>>      },
>> @@ -737,7 +755,7 @@ static const struct pt_reg_grp_info_tbl 
>> pt_emu_reg_grp_tbl[] = {
>>          .grp_id     = 0xFF,
>>          .grp_type   = GRP_TYPE_EMU,
>>          .grp_size   = 0x40,
>> -        .size_init  = pt_reg_grp_size_init,
>> +        .size_init  = pt_reg_grp_header0_size_init,
>>          .emu_reg_tbl= pt_emu_reg_header0_tbl,
>>      },
>>      /* PCI PowerManagement Capability reg group */
>> @@ -3006,6 +3024,19 @@ static uint32_t pt_msixctrl_reg_init(struct pt_dev 
>> *ptdev,
>>      return reg->init_val;
>>  }
>>
>> +static uint8_t pt_reg_grp_header0_size_init(struct pt_dev *ptdev,
>> +        struct pt_reg_grp_info_tbl *grp_reg, uint32_t base_offset)
>> +{
>> +    /*
>> +    ** By default we will trap up to 0x40 in the cfg space.
>> +    ** If an intel device is pass through we need to trap 0xfc,
>> +    ** therefore the size should be 0xff.
>> +    */
>> +    if (igd_passthru)
>> +        return 0xFF;
>> +    return grp_reg->grp_size;
>> +}
>
> Apart from the trivial code style error in the comment above, is this
> going to have the unintended side effect of initializing as 0 all the
> emulated registers between 0x40 and 0xff, that previously were probably
> passed through?
>

Based on how pt_find_reg_grp is implemented that doesn't make any difference.

Jean

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel