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

[Jean Guyader <jean.guyader@xxxxxxxxxxxxx>]

On 17/01 02:51, Stefano Stabellini wrote:
> 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.

The pci pass through groups have been designed to pass through pci capabilities
from the device. You can't really create a group for something which isn't a pci
capability.

I have noted the change of behavior but that doesn't have any impact on what we
will return to the guest so I think it fine.

Jean

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