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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 16 Jan 2012, Jean Guyader wrote:
> 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.
 
actually there is a small change in behaviour: before your patch
pt_find_reg_grp would return NULL for any cfg register between 0x40 and
0xff. Now if igd_passthru is set pt_find_reg_grp would return the
reg_grp_entry corresponding to "Header Type0 reg group" and then
pt_find_reg would return NULL.
This case seems to be handled correctly and bring to the same results
in both pt_pci_write_config and pt_pci_read_config.

In any case PCI_INTEL_OPREGION should be part of "Header Type0 reg
group" only it if really is part of this group otherwise it should be in
its own separate group.

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